Fix indefinite hang on OpenAPI schemas with cyclic model dependencies#3078
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 (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughBug fixes in the parser's base module prevent infinite loops when sorting circular base-class dependencies and add a guard to skip discriminator base classes without references. Two new tests validate stable sorting and safe discriminator handling without dereferencing missing references. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 improve performance by 14.9%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | WallTime | test_perf_aws_style_openapi_pydantic_v2 |
1.9 s | 1.7 s | +11.38% |
| ⚡ | WallTime | test_perf_all_options_enabled |
6.7 s | 5.9 s | +12.08% |
| ⚡ | WallTime | test_perf_graphql_style_pydantic_v2 |
837 ms | 756.3 ms | +10.68% |
| ⚡ | WallTime | test_perf_deep_nested |
6.2 s | 5.5 s | +12.99% |
| ⚡ | WallTime | test_perf_complex_refs |
2.2 s | 1.9 s | +14.02% |
| ⚡ | WallTime | test_perf_duplicate_names |
1,062.9 ms | 946.1 ms | +12.34% |
| ⚡ | WallTime | test_perf_large_models_pydantic_v2 |
3.8 s | 3.3 s | +14.9% |
| ⚡ | WallTime | test_perf_multiple_files_input |
3.8 s | 3.4 s | +12.38% |
| ⚡ | WallTime | test_perf_kubernetes_style_pydantic_v2 |
2.7 s | 2.4 s | +10.25% |
| ⚡ | WallTime | test_perf_openapi_large |
3 s | 2.7 s | +10.42% |
Comparing kevin-paulson-mindbridge-ai:slow_schema_generation_fix (7eaf6ab) with main (05901ff)
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 18247 18290 +43
Branches 2087 2089 +2
=========================================
+ Hits 18247 18290 +43
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 🙂
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