Fix indefinite hang on OpenAPI schemas with cyclic model dependencies#3078
Fix indefinite hang on OpenAPI schemas with cyclic model dependencies#3078kevin-paulson-mindbridge-ai wants to merge 26 commits intokoxudaxi:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR stabilizes dependency ordering in the parser to avoid non-terminating permutations for circular discriminated unions and adds a guard to skip discriminator validation when a base-class reference is missing. It also adds regression tests (JSON Schema and OpenAPI) and deduplicates union type-hint parts. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Merging this PR will degrade performance by 12.89%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | WallTime | test_perf_graphql_style_pydantic_v2 |
751.9 ms | 842.8 ms | -10.79% |
| ❌ | WallTime | test_perf_aws_style_openapi_pydantic_v2 |
1.7 s | 1.9 s | -11.68% |
| ❌ | WallTime | test_perf_kubernetes_style_pydantic_v2 |
2.4 s | 2.7 s | -10.92% |
| ❌ | WallTime | test_perf_large_models_pydantic_v2 |
3.3 s | 3.8 s | -12.89% |
| ❌ | WallTime | test_perf_stripe_style_pydantic_v2 |
1.8 s | 2 s | -10.57% |
| ❌ | WallTime | test_perf_complex_refs |
2 s | 2.2 s | -10.36% |
| ❌ | WallTime | test_perf_multiple_files_input |
3.4 s | 3.8 s | -10.76% |
| ❌ | WallTime | test_perf_openapi_large |
2.6 s | 3 s | -11.62% |
Comparing kevin-paulson-mindbridge-ai:slow_schema_generation_fix (9eaaaf9) with main (92bdc27)
Footnotes
-
98 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/parser/test_base.py (1)
270-303: Consider asserting a postcondition, not just “no exception.”This catches the crash, but it would be a bit stronger if it also pinned one observable outcome after
__apply_discriminator_type()runs, so a future silent no-op doesn’t still pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/parser/test_base.py` around lines 270 - 303, The test currently only ensures no exception is raised; add a concrete postcondition after calling parser._Parser__apply_discriminator_type to ensure the union was not silently modified by the discriminator logic: e.g., assert that union_inner still contains exactly the two members referencing ref_pet and ref_other (check identities of union_inner.data_types entries or their .reference attributes) and that pet_model.base_classes remains unchanged — locate this in test_apply_discriminator_type_skips_base_class_without_reference and add the assertion(s) after the call to parser._Parser__apply_discriminator_type([root], Imports()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/parser/test_base.py`:
- Around line 270-303: The test currently only ensures no exception is raised;
add a concrete postcondition after calling
parser._Parser__apply_discriminator_type to ensure the union was not silently
modified by the discriminator logic: e.g., assert that union_inner still
contains exactly the two members referencing ref_pet and ref_other (check
identities of union_inner.data_types entries or their .reference attributes) and
that pet_model.base_classes remains unchanged — locate this in
test_apply_discriminator_type_skips_base_class_without_reference and add the
assertion(s) after the call to parser._Parser__apply_discriminator_type([root],
Imports()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: af656bb8-01d5-4471-9440-e33695a9c92c
📒 Files selected for processing (2)
src/datamodel_code_generator/parser/base.pytests/parser/test_base.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3078 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 87 87
Lines 18306 18320 +14
Branches 2091 2093 +2
=========================================
+ Hits 18306 18320 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ilovelinux
left a comment
ilovelinux
left a comment
There was a problem hiding this comment.
This PR implements cycle detection as it is usually done in graph algorithms (BFS, DFS, etc...).
LGTM! Thank you @kevin-paulson-mindbridge-ai 🙂
ilovelinux
left a comment
ilovelinux
left a comment
There was a problem hiding this comment.
Hi @kevin-paulson-mindbridge-ai, I realized it would be great to add some generation test for schemas with cyclic references in order to track the generated output of this corner-case.
See tests/main/{openapi,jsonschema,graphql}/test_main_*.py for examples. 🙂
…to track the generated output
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/main/openapi/test_main_openapi.py (1)
4756-4761: Add an explicit timeout to this hang-regression test.The test for cyclic component reference handling (which previously caused indefinite hangs) has no timeout protection. If this regression returns, CI will stall until a global job timeout. Since
run_main_and_assert()does not enforce a per-test timeout, add one directly to this test using@pytest.mark.timeout()to fast-fail if the code hangs again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/main/openapi/test_main_openapi.py` around lines 4756 - 4761, This test calling run_main_and_assert(...) for "cyclic_component_refs.yaml" lacks a per-test timeout and can hang CI; add a pytest timeout decorator (use `@pytest.mark.timeout`(seconds)) to the test function that invokes run_main_and_assert for cyclic_component_refs.yaml so the test fails fast if it hangs—place the decorator immediately above the test definition and choose a reasonable timeout (e.g., 10–30s) to prevent indefinite hangs while allowing normal runs.tests/main/graphql/test_main_graphql.py (1)
924-929: Make the expected snapshot explicit in this regression test.On Line 924, this relies on implicit
run_main_and_assertbehavior for expected output resolution. Please passexpected_file="cyclic_mutual_types.py"explicitly to keep the regression assertion unambiguous and resilient to helper changes.Proposed patch
def test_main_cyclic_mutual_types(output_file: Path) -> None: """Mutual object-type cycle (A <-> B) in GraphQL.""" run_main_and_assert( input_path=GRAPHQL_DATA_PATH / "cyclic-mutual-types.graphql", output_path=output_file, input_file_type="graphql", assert_func=assert_file_content, + expected_file="cyclic_mutual_types.py", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/main/graphql/test_main_graphql.py` around lines 924 - 929, Update the test call to run_main_and_assert to pass the explicit expected snapshot filename so the regression is unambiguous: modify the invocation of run_main_and_assert (the one using input_path=GRAPHQL_DATA_PATH / "cyclic-mutual-types.graphql", output_path=output_file, input_file_type="graphql", assert_func=assert_file_content) to include expected_file="cyclic_mutual_types.py" as an additional argument.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/main/graphql/test_main_graphql.py`:
- Around line 924-929: Update the test call to run_main_and_assert to pass the
explicit expected snapshot filename so the regression is unambiguous: modify the
invocation of run_main_and_assert (the one using input_path=GRAPHQL_DATA_PATH /
"cyclic-mutual-types.graphql", output_path=output_file,
input_file_type="graphql", assert_func=assert_file_content) to include
expected_file="cyclic_mutual_types.py" as an additional argument.
In `@tests/main/openapi/test_main_openapi.py`:
- Around line 4756-4761: This test calling run_main_and_assert(...) for
"cyclic_component_refs.yaml" lacks a per-test timeout and can hang CI; add a
pytest timeout decorator (use `@pytest.mark.timeout`(seconds)) to the test
function that invokes run_main_and_assert for cyclic_component_refs.yaml so the
test fails fast if it hangs—place the decorator immediately above the test
definition and choose a reasonable timeout (e.g., 10–30s) to prevent indefinite
hangs while allowing normal runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6a41a0a5-bdda-41a2-953a-aae47badee83
⛔ Files ignored due to path filters (3)
tests/data/graphql/cyclic-mutual-types.graphqlis excluded by!tests/data/**/*.graphqland included by nonetests/data/jsonschema/cyclic_mutual_defs.jsonis excluded by!tests/data/**/*.jsonand included by nonetests/data/openapi/cyclic_component_refs.yamlis excluded by!tests/data/**/*.yamland included by none
📒 Files selected for processing (6)
tests/data/expected/main/graphql/cyclic_mutual_types.pytests/data/expected/main/jsonschema/cyclic_mutual_defs.pytests/data/expected/main/openapi/cyclic_component_refs.pytests/main/graphql/test_main_graphql.pytests/main/jsonschema/test_main_jsonschema.pytests/main/openapi/test_main_openapi.py
✅ Files skipped from review due to trivial changes (3)
- tests/data/expected/main/openapi/cyclic_component_refs.py
- tests/data/expected/main/jsonschema/cyclic_mutual_defs.py
- tests/data/expected/main/graphql/cyclic_mutual_types.py
ilovelinux
left a comment
ilovelinux
left a comment
There was a problem hiding this comment.
I locally cherry-picked the last two commits (the ones that introduce the generation tests) to main. I expect them to fail (or hang) but they succeed instead.
Given remote:
$ git remote add kevin-paulson-mindbridge-ai https://github.com/kevin-paulson-mindbridge-ai/datamodel-code-generator.git
$ git fetch kevin-paulson-mindbridge-aiTo replicate. exec from main branch:
$ git cherry-pick 7786cf90867db72f716f1796ab071fefcffe4dc5
$ git cherry-pick 8d3ee3958ce8d9c595fdaa3d46a73bba2b54fa23
$ tox -e py314-parallel -- -k cyclicThis means that the cyclic model dependency added as test was already handled by the tool.
In fact, also the other tests (which implements the same cyclic model dependency corner-case) works on main.
$ git switch --detach kevin-paulson-mindbridge-ai/slow_schema_generation_fix
$ git restore src/datamodel_code_generator/parser/base.py --source main
$ tox -e py314-parallel -- -k cyclic@kevin-paulson-mindbridge-ai could you double-check the corner-case you have in your OpenAPI schema? As now, it looks like the one you proposed was already supported, therefore we cannot reproduce the issue, validate the fix & add regression tests 🙁
…ctually reproduce the issue (don't complete in a reasonable time)
My apologies, I neglected to validate that the new tests reproduced the issue before the change. I have updated the 3 tests such that they reproduce the issue before the change (they each ran at least 30 seconds before I killed them) and after the change they pass quickly (less than a second each). They look OK to me, but I am most familiar with OpenAPI and I am very unfamiliar with graphql. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/data/expected/main/jsonschema/cyclic_mutual_defs.py`:
- Around line 14-19: The generated classes Person and Team create an impossible
mutual inheritance cycle (Person(Team, Model) and Team(Person, Model)) causing a
runtime NameError; replace this mutual inheritance with a valid hierarchy by
removing the forward base reference—e.g., have both classes inherit from Model
only or make one inherit from the other (but not both) so that Person and Team
do not form a direct cycle; update the class headers for Person and Team (the
class definitions named Person and Team) accordingly and ensure any
tests/fixtures referencing Model still import/define Model as before.
In `@tests/data/expected/main/openapi/cyclic_component_refs.py`:
- Around line 14-19: The expected fixture currently creates an unimportable
cyclic base-class relationship between Person and Team (Person(Team, Model) and
Team(Person, Model)); fix it by removing the cyclic inheritance and making both
classes inherit only from Model (e.g., class Person(Model): and class
Team(Model):) and, if a cross-reference is needed, add attributes that reference
the other type as a forward annotation (e.g., person has team: "Team" | None =
None and team has member: "Person" | None = None) so the file can be imported
successfully while preserving the relationship between Person and Team.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c31b332b-2832-4806-b2b4-639415011af1
⛔ Files ignored due to path filters (3)
tests/data/graphql/cyclic-mutual-types.graphqlis excluded by!tests/data/**/*.graphqland included by nonetests/data/jsonschema/cyclic_mutual_defs.jsonis excluded by!tests/data/**/*.jsonand included by nonetests/data/openapi/cyclic_component_refs.yamlis excluded by!tests/data/**/*.yamland included by none
📒 Files selected for processing (3)
tests/data/expected/main/graphql/cyclic_mutual_types.pytests/data/expected/main/jsonschema/cyclic_mutual_defs.pytests/data/expected/main/openapi/cyclic_component_refs.py
| class Person(Team, Model): | ||
| name: str | None = None | ||
|
|
||
|
|
||
| class Team(Person, Model): | ||
| name: str | None = None |
There was a problem hiding this comment.
🧩 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 Person and Team classes form an unimportable cycle. Line 14 references Team before it is defined, which raises NameError at class definition time. The from __future__ import annotations pragma only defers annotations, not base-class lookup. Either reject this schema in the generator or adjust the expected output to produce valid Python.
🧰 Tools
🪛 Ruff (0.15.9)
[error] 14-14: Undefined name Team
(F821)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/data/expected/main/openapi/cyclic_component_refs.py` around lines 14 -
19, The expected fixture currently creates an unimportable cyclic base-class
relationship between Person and Team (Person(Team, Model) and Team(Person,
Model)); fix it by removing the cyclic inheritance and making both classes
inherit only from Model (e.g., class Person(Model): and class Team(Model):) and,
if a cross-reference is needed, add attributes that reference the other type as
a forward annotation (e.g., person has team: "Team" | None = None and team has
member: "Person" | None = None) so the file can be imported successfully while
preserving the relationship between Person and Team.
There was a problem hiding this comment.
I'll see if I can create better test data.
There was a problem hiding this comment.
@kevin-paulson-mindbridge-ai, sounds good! Here are a few concrete directions you could take:
Option A — Cyclic field references instead of cyclic inheritance
If your input schema uses allOf to pull in referenced types as base classes, consider restructuring it so each type holds the other as an optional field (forward-referenced). This produces importable Python:
# 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 from __future__ import annotations does help for field annotations).
Option B — Non-cyclic inheritance with a shared base
If the test is specifically meant to exercise how the generator handles allOf with a common parent, restructure so the cycle is not in the base-class chain:
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 allOf inheritance) that has no valid Python mapping. You could adjust the input schema to use composition via properties instead of allOf-based inheritance, which would make the expected output naturally importable.
Which approach fits your schema best? Happy to help refine the expected fixture once you decide on a direction.
🧠 Learnings used
Learnt from: koxudaxi
Repo: koxudaxi/datamodel-code-generator PR: 2890
File: tests/data/expected/main/jsonschema/ref_nullable_with_constraint.py:14-15
Timestamp: 2026-01-02T08:25:22.111Z
Learning: The datamodel-code-generator currently generates RootModel subclasses with an explicit `root` field annotation (e.g., `class StringType(RootModel[str]): root: str`). This is existing behavior of the code generator and should not be flagged as an issue introduced by new changes.
There was a problem hiding this comment.
@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? 🙂
Footnotes
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
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:
- The generated python file produces duplicate union members, this was not noticed as they were hidden by
ruff check --fix - I'm having a lot of difficulty determining a test that reproduces the issue and still generates the valid Python expected.
I will plan to continue reviewing this PR later this week to see what I can discover.
That's not a problem, I appreciate your effort. Reviews & double-checks are useful because of that 🙂
I'll review it as soon as I'll get some free time 🙂
Could you add a timeout as suggested by the first nitpick comment here: #3078 (review)? |
1) Avoid infinite loop due to cyclic inheritance with discriminators 2) De-duplicate union members (but keep same order)
|
I think I've got things much closer now. I'll plan to come back to this to:
|
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
This is now ready for review. Note that I don't believe graphql has the same issue due to the code paths followed and the nature of graphql. |
2 participants
We've been generating a model with the tool based on our OpenAPI schema, however since version 0.37.0 we've been unsuccessful in doing so. In the latest release (0.56.0) the generation was revealed to be stuck in
sort_data_models. I waited up to an hour, I don't think it was ever going to complete. With this change it now works as expected.I tried to follow https://datamodel-code-generator.koxudaxi.dev/development-contributing/ as best as I can, but let me know if you would like me to make any changes or if I should create an issue or anything else. Thanks!
Summary by CodeRabbit
Bug Fixes
Tests
Chores