-
-
Notifications
You must be signed in to change notification settings - Fork 435
Fix indefinite hang on OpenAPI schemas with cyclic model dependencies #3078
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 7 commits
ae7ba7a
dff13a4
b4e675c
7eaf6ab
7786cf9
8d3ee39
bb037b0
db5f26a
6b51fbe
207dda3
6cc7834
f9da08d
670f35d
7deb47d
c380a72
582a495
45f0208
b3a3e3e
064c620
d10ed4a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| # generated by datamodel-codegen: | ||
| # filename: cyclic-mutual-types.graphql | ||
| # timestamp: 2019-07-26T00:00:00+00:00 | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from typing import Literal | ||
|
|
||
| from pydantic import BaseModel, Field | ||
| from typing_extensions import TypeAliasType | ||
|
|
||
| Boolean = TypeAliasType("Boolean", bool) | ||
| """ | ||
| The `Boolean` scalar type represents `true` or `false`. | ||
| """ | ||
|
|
||
|
|
||
| String = TypeAliasType("String", str) | ||
| """ | ||
| The `String` scalar type represents textual data, represented as UTF-8 character sequences. The String type is most often used by GraphQL to represent free-form human-readable text. | ||
| """ | ||
|
|
||
|
|
||
| class Model(BaseModel): | ||
| name: String | None = None | ||
| typename__: Literal['Model'] | None = Field('Model', alias='__typename') | ||
|
|
||
|
|
||
| class Team(Person, Model): | ||
| name: String | None = None | ||
| typename__: Literal['Team'] | None = Field('Team', alias='__typename') | ||
|
|
||
|
|
||
| class Person(Team, Model): | ||
| name: String | None = None | ||
| typename__: Literal['Person'] | None = Field('Person', alias='__typename') |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| # generated by datamodel-codegen: | ||
| # filename: cyclic_mutual_defs.json | ||
| # timestamp: 2019-07-26T00:00:00+00:00 | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from pydantic import BaseModel | ||
|
|
||
|
|
||
| class Model(BaseModel): | ||
| pass | ||
|
|
||
|
|
||
| class Person(Team, Model): | ||
| name: str | None = None | ||
|
|
||
|
|
||
| class Team(Person, Model): | ||
| name: str | None = None | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| # generated by datamodel-codegen: | ||
| # filename: cyclic_component_refs.yaml | ||
| # timestamp: 2019-07-26T00:00:00+00:00 | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from pydantic import BaseModel | ||
|
|
||
|
|
||
| class Model(BaseModel): | ||
| pass | ||
|
|
||
|
|
||
| class Person(Team, Model): | ||
| name: str | None = None | ||
|
|
||
|
|
||
| class Team(Person, Model): | ||
| name: str | None = None | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
python - <<'PY'
src = """
from __future__ import annotations
class BaseModel: pass
class Model(BaseModel): pass
class Person(Team, Model): pass
class Team(Person, Model): pass
"""
try:
exec(src, {})
except Exception as exc:
print(f"{type(exc).__name__}: {exc}")
PYRepository: koxudaxi/datamodel-code-generator Length of output: 114 🏁 Script executed: cat -n tests/data/expected/main/openapi/cyclic_component_refs.pyRepository: koxudaxi/datamodel-code-generator Length of output: 543 Fix the cyclic inheritance in the expected fixture—this code cannot be imported. The generated 🧰 Tools🪛 Ruff (0.15.9)[error] 14-14: Undefined name (F821) 🤖 Prompt for AI Agents
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll see if I can create better test data. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Option A — Cyclic field references instead of cyclic inheritance If your input schema uses # generated by datamodel-codegen:
# filename: cyclic_component_refs.yaml
# timestamp: 2019-07-26T00:00:00+00:00
from __future__ import annotations
from typing import Optional
from pydantic import BaseModel
class Person(BaseModel):
name: str | None = None
team: Optional[Team] = None # forward ref resolved by `from __future__ import annotations`
class Team(BaseModel):
name: str | None = None
member: Optional[Person] = NoneThis is valid Python (the Option B — Non-cyclic inheritance with a shared base If the test is specifically meant to exercise how the generator handles class Model(BaseModel):
pass
class Person(Model):
name: str | None = None
team: Team | None = None # field ref, not inheritance
class Team(Model):
name: str | None = NoneOption C — Adjust the input schema The root cause is the input YAML/JSON/GraphQL schema itself encoding a pattern (mutual Which approach fits your schema best? Happy to help refine the expected fixture once you decide on a direction. 🧠 Learnings used
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kevin-paulson-mindbridge-ai since you said those examples won't generate any output before1, and now they generate this output, I think this may been introduced by your bugfix. That's an interesting corner-case. I think we should look into that before merging. @kevin-paulson-mindbridge-ai IMHO if you can't fix that, consider adding an error to communicate to the user this is an (yet) unsupported corner-case, in order to avoid generating broken code. We may track that in another issue then and fix later. Finding the edges of the problem is the first step to solve it, after all. CC: @koxudaxi WDYT? 🙂 FootnotesThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I no longer think that this test data is fully reproducing the original issue and I don't think the fix is quite it either. What caused me to look at this is we have a somewhat large schema available at test.mindbridge.ai/api/docs/mindbridge-openapi.json which produces with this change an about 20,000 line (valid) python file. Although with this change it will now generate the python file instead of never terminating:
I will plan to continue reviewing this PR later this week to see what I can discover. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| type Query { | ||
| entry: Team | ||
| } | ||
|
|
||
| # GraphQL interfaces must declare fields; OpenAPI `Model` is an empty object, so `name` | ||
| # is the shared optional field carried by Team/Person (mirrors OpenAPI `name` on each). | ||
| interface Model { | ||
| name: String | ||
| } | ||
|
|
||
| # Mirrors allOf [Model, Person] + properties { name }. | ||
| interface Team implements Model & Person { | ||
| name: String | ||
| } | ||
|
|
||
| # Mirrors allOf [Model, Team] + properties { name }. | ||
| interface Person implements Model & Team { | ||
| name: String | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| { | ||
| "$schema": "http://json-schema.org/draft-07/schema#", | ||
| "type": "object", | ||
| "properties": {}, | ||
| "definitions": { | ||
| "Model": { | ||
| "title": "Model", | ||
| "type": "object" | ||
| }, | ||
| "Team": { | ||
| "title": "Team", | ||
| "allOf": [ | ||
| { "$ref": "#/definitions/Model" }, | ||
| { "$ref": "#/definitions/Person" } | ||
| ], | ||
| "type": "object", | ||
| "properties": { | ||
| "name": { "type": "string" } | ||
| } | ||
| }, | ||
| "Person": { | ||
| "title": "Person", | ||
| "allOf": [ | ||
| { "$ref": "#/definitions/Model" }, | ||
| { "$ref": "#/definitions/Team" } | ||
| ], | ||
| "type": "object", | ||
| "properties": { | ||
| "name": { "type": "string" } | ||
| } | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| openapi: 3.0.0 | ||
| info: | ||
| title: Cyclic component $ref | ||
| version: "1.0" | ||
| paths: {} | ||
| components: | ||
| schemas: | ||
| Model: | ||
| type: object | ||
| Team: | ||
| allOf: | ||
| - $ref: "#/components/schemas/Model" | ||
| - $ref: "#/components/schemas/Person" | ||
| type: object | ||
| properties: | ||
| name: | ||
| type: string | ||
| Person: | ||
| allOf: | ||
| - $ref: "#/components/schemas/Model" | ||
| - $ref: "#/components/schemas/Team" | ||
| type: object | ||
| properties: | ||
| name: | ||
| type: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: koxudaxi/datamodel-code-generator
Length of output: 132
🏁 Script executed:
# Check the full content of the fixture file cat -n tests/data/expected/main/jsonschema/cyclic_mutual_defs.pyRepository: koxudaxi/datamodel-code-generator
Length of output: 540
🏁 Script executed:
# Look for related test files that might explain the purpose fd -i cyclic tests/Repository: koxudaxi/datamodel-code-generator
Length of output: 495
🏁 Script executed:
# Check for input schema files fd cyclic_mutual tests/ -type fRepository: koxudaxi/datamodel-code-generator
Length of output: 247
🏁 Script executed:
# Read the input schema file cat -n tests/data/jsonschema/cyclic_mutual_defs.jsonRepository: koxudaxi/datamodel-code-generator
Length of output: 994
🏁 Script executed:
# Search for test code that validates this fixture rg -l cyclic_mutual_defs tests/ -t pyRepository: koxudaxi/datamodel-code-generator
Length of output: 180
🏁 Script executed:
# Find and read the test file that uses this fixture rg -B 5 -A 10 cyclic_mutual_defs tests/main/jsonschema/test_main_jsonschema.pyRepository: koxudaxi/datamodel-code-generator
Length of output: 714
🏁 Script executed:
# Check the other cyclic test case (OpenAPI) cat -n tests/data/expected/main/openapi/cyclic_component_refs.pyRepository: koxudaxi/datamodel-code-generator
Length of output: 543
🏁 Script executed:
# Check the GraphQL cyclic test case cat -n tests/data/expected/main/graphql/cyclic_mutual_types.pyRepository: koxudaxi/datamodel-code-generator
Length of output: 1337
🏁 Script executed:
Repository: koxudaxi/datamodel-code-generator
Length of output: 18508
🏁 Script executed:
Repository: koxudaxi/datamodel-code-generator
Length of output: 142
🏁 Script executed:
Repository: koxudaxi/datamodel-code-generator
Length of output: 701
🏁 Script executed:
Repository: koxudaxi/datamodel-code-generator
Length of output: 108
🏁 Script executed:
Repository: koxudaxi/datamodel-code-generator
Length of output: 59
🏁 Script executed:
Repository: koxudaxi/datamodel-code-generator
Length of output: 3872
🏁 Script executed:
Repository: koxudaxi/datamodel-code-generator
Length of output: 526
🏁 Script executed:
Repository: koxudaxi/datamodel-code-generator
Length of output: 8419
🏁 Script executed:
# Examine the SCC (Strongly Connected Components) module for cycle handling cat src/datamodel_code_generator/parser/_scc.pyRepository: koxudaxi/datamodel-code-generator
Length of output: 5865
🏁 Script executed:
Repository: koxudaxi/datamodel-code-generator
Length of output: 3802
🏁 Script executed:
Repository: koxudaxi/datamodel-code-generator
Length of output: 4456
🏁 Script executed:
# Check the inheritance_forward_ref fixture to see how it handles forward references cat -n tests/data/expected/main/jsonschema/inheritance_forward_ref.pyRepository: koxudaxi/datamodel-code-generator
Length of output: 1616
🏁 Script executed:
# Also check the input schema for inheritance_forward_ref cat tests/data/jsonschema/inheritance_forward_ref.jsonRepository: koxudaxi/datamodel-code-generator
Length of output: 2386
🏁 Script executed:
# Check if cyclic_mutual_defs.py has model_rebuild calls tail -20 tests/data/expected/main/jsonschema/cyclic_mutual_defs.pyRepository: koxudaxi/datamodel-code-generator
Length of output: 407
🏁 Script executed:
Repository: koxudaxi/datamodel-code-generator
Length of output: 1180
🏁 Script executed:
Repository: koxudaxi/datamodel-code-generator
Length of output: 149
🏁 Script executed:
Repository: koxudaxi/datamodel-code-generator
Length of output: 197
🏁 Script executed:
Repository: koxudaxi/datamodel-code-generator
Length of output: 123
Invalid Python output: forward base reference and mutual inheritance cycle
Person(Team, Model)at Line 14 referencesTeambefore it exists, andPerson/Teamform an impossible direct inheritance cycle. The output fails at runtime withNameError: name 'Team' is not defined. Even withfrom __future__ import annotations, base class names are resolved eagerly at class definition time, so forward references do not help. Themodel_rebuild()pattern used in other forward-reference fixtures does not apply here because mutual class inheritance cannot be resolved post-hoc.Consider generating valid output by removing the mutual inheritance:
Proposed fixture correction
🧰 Tools
🪛 Ruff (0.15.9)
[error] 14-14: Undefined name
Team(F821)
🤖 Prompt for AI Agents