Fix duplicate model generation for $ref with nullable#2890
Conversation
📝 WalkthroughWalkthroughAdds a cached property Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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 |
|
📚 Docs Preview: https://pr-2890.datamodel-code-generator.pages.dev |
CodSpeed Performance ReportMerging #2890 will not alter performanceComparing
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/main/jsonschema/test_main_jsonschema.py (1)
7410-7492: New$ref + nullabletests accurately capture the intended merge vs non-merge behaviorThe five tests cleanly cover:
- nullable-only refs (with and without
--strict-nullable) avoiding duplicate models,- metadata-only additions (
title/description) still reusing the referenced type, and- constraint / schema-affecting extras (
minLength,if/then/else) correctly forcing a merged model.Names, docstrings, and
expected_file/extra_argscombinations all line up with the PR objective of preventing spurious duplicate models while still merging when the schema truly changes. If you ever want to reduce repetition, these could be parameterized over(input, expected, extra_args), but that’s purely optional given the clarity of the current tests.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
tests/data/jsonschema/ref_nullable_only.yamlis excluded by!tests/data/**/*.yamland included by nonetests/data/jsonschema/ref_nullable_with_constraint.yamlis excluded by!tests/data/**/*.yamland included by nonetests/data/jsonschema/ref_nullable_with_extra.yamlis excluded by!tests/data/**/*.yamland included by nonetests/data/jsonschema/ref_nullable_with_metadata.yamlis excluded by!tests/data/**/*.yamland included by none
📒 Files selected for processing (7)
src/datamodel_code_generator/parser/jsonschema.pytests/data/expected/main/jsonschema/ref_nullable_only.pytests/data/expected/main/jsonschema/ref_nullable_only_strict.pytests/data/expected/main/jsonschema/ref_nullable_with_constraint.pytests/data/expected/main/jsonschema/ref_nullable_with_extra.pytests/data/expected/main/jsonschema/ref_nullable_with_metadata.pytests/main/jsonschema/test_main_jsonschema.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/data/expected/main/jsonschema/ref_nullable_with_metadata.py (1)
tests/data/expected/main/jsonschema/ref_nullable_with_extra.py (2)
User(18-19)Model(14-15)
tests/data/expected/main/jsonschema/ref_nullable_with_extra.py (2)
src/datamodel_code_generator/model/base.py (1)
name(827-829)tests/data/expected/main/jsonschema/ref_nullable_with_metadata.py (2)
Model(14-19)User(10-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: 3.10 on macOS
- GitHub Check: 3.14 on Windows
- GitHub Check: 3.10 on Windows
- GitHub Check: 3.11 on macOS
- GitHub Check: 3.11 on Windows
- GitHub Check: 3.12 on Windows
- GitHub Check: 3.13 on Windows
- GitHub Check: Analyze (python)
- GitHub Check: benchmarks
🔇 Additional comments (8)
src/datamodel_code_generator/parser/jsonschema.py (3)
480-500: LGTM! Well-designed property to detect nullable-only refs.The
is_ref_with_nullable_onlyproperty correctly identifies when a $ref is combined exclusively withnullable: true, excluding metadata-only fields and extension fields (x-*). This enables the parser to avoid unnecessary schema merging and duplicate model generation in such cases.
2724-2729: LGTM! Early resolution for nullable-only refs prevents duplicate models.The early return correctly handles
$refwith onlynullable: trueby directly resolving the reference and optionally wrapping it asOptionalwhenstrict_nullableis enabled. This bypasses unnecessary schema merging and avoids creating duplicate models.
3553-3554: LGTM! Skip merge for nullable-only refs.The modified condition correctly skips schema merging when
is_ref_with_nullable_onlyis true, delegating to the direct reference handling inparse_item. This is consistent with the overall approach to avoid duplicate model generation.tests/data/expected/main/jsonschema/ref_nullable_with_metadata.py (1)
1-19: LGTM! Generated output correctly handles $ref with nullable and metadata.The generated code demonstrates the fix working as intended:
- The
Usermodel is defined once (no duplicate)- The
Model.user_with_titlefield correctly usesUser | Noneas the type- Metadata fields (description, title) are preserved in the
Fieldannotation- This confirms that metadata-only fields don't trigger unnecessary schema merging
tests/data/expected/main/jsonschema/ref_nullable_only.py (1)
1-17: LGTM! Generated output correctly handles multiple nullable refs without duplication.The generated code demonstrates the core fix:
- The
Usermodel is defined once- Multiple nullable references (
user_a,user_b,user_c) all useUser | None- No duplicate models are created for each nullable reference
- This validates the primary objective of the PR
tests/data/expected/main/jsonschema/ref_nullable_with_constraint.py (1)
1-15: LGTM! Generated output correctly creates separate model for nullable ref with constraint.The generated code validates that constraints are treated as schema-affecting keywords:
StringTypeis created as a separateRootModelbecause the ref has a constraint (min_length=5)- The
Model.constrained_stringfield correctly usesconstr(min_length=5) | None- This demonstrates that nullable-only logic doesn't apply when constraints are present
tests/data/expected/main/jsonschema/ref_nullable_with_extra.py (1)
1-19: LGTM! Generated output correctly creates separate models for nullable refs with extras.The generated code validates that non-metadata extra fields trigger model creation:
UserWithExtraandUserare created as separate models- The
Model.user_with_extrafield usesUserWithExtra | None- This demonstrates that the nullable-only logic correctly excludes refs that have schema-affecting extras
tests/data/expected/main/jsonschema/ref_nullable_only_strict.py (1)
1-17: Strict-nullable expected fixture is consistent and correctThe generated models and nullable
User | None = Nonefields match the described behavior for$ref + nullableunder--strict-nullable; nothing to fix here.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2890 +/- ##
=======================================
Coverage 99.37% 99.37%
=======================================
Files 92 92
Lines 16152 16179 +27
Branches 1906 1912 +6
=======================================
+ Hits 16051 16078 +27
Misses 52 52
Partials 49 49
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:
|
3220f8a to
0e08667
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
tests/data/jsonschema/ref_nullable_only.yamlis excluded by!tests/data/**/*.yamland included by nonetests/data/jsonschema/ref_nullable_with_constraint.yamlis excluded by!tests/data/**/*.yamland included by nonetests/data/jsonschema/ref_nullable_with_extra.yamlis excluded by!tests/data/**/*.yamland included by nonetests/data/jsonschema/ref_nullable_with_metadata.yamlis excluded by!tests/data/**/*.yamland included by none
📒 Files selected for processing (7)
src/datamodel_code_generator/parser/jsonschema.pytests/data/expected/main/jsonschema/ref_nullable_only.pytests/data/expected/main/jsonschema/ref_nullable_only_strict.pytests/data/expected/main/jsonschema/ref_nullable_with_constraint.pytests/data/expected/main/jsonschema/ref_nullable_with_extra.pytests/data/expected/main/jsonschema/ref_nullable_with_metadata.pytests/main/jsonschema/test_main_jsonschema.py
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/data/expected/main/jsonschema/ref_nullable_with_metadata.py
- src/datamodel_code_generator/parser/jsonschema.py
- tests/main/jsonschema/test_main_jsonschema.py
- tests/data/expected/main/jsonschema/ref_nullable_with_extra.py
- tests/data/expected/main/jsonschema/ref_nullable_only.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/data/expected/main/jsonschema/ref_nullable_only_strict.py (1)
tests/data/expected/main/jsonschema/ref_nullable_only.py (2)
User(10-11)Model(14-17)
tests/data/expected/main/jsonschema/ref_nullable_with_constraint.py (4)
tests/data/expected/main/jsonschema/ref_nullable_only.py (1)
Model(14-17)tests/data/expected/main/jsonschema/ref_nullable_only_strict.py (1)
Model(14-17)tests/data/expected/main/jsonschema/ref_nullable_with_extra.py (1)
Model(14-15)tests/data/expected/main/jsonschema/ref_nullable_with_metadata.py (1)
Model(14-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: 3.12 on Windows
- GitHub Check: 3.10 on Ubuntu
- GitHub Check: 3.10 on Windows
- GitHub Check: py312-isort7 on Ubuntu
- GitHub Check: 3.11 on Windows
- GitHub Check: 3.11 on Ubuntu
- GitHub Check: 3.12 on Ubuntu
- GitHub Check: 3.14 on macOS
- GitHub Check: 3.13 on Windows
- GitHub Check: 3.14 on Windows
- GitHub Check: Analyze (python)
- GitHub Check: benchmarks
🔇 Additional comments (3)
tests/data/expected/main/jsonschema/ref_nullable_only_strict.py (1)
1-17: LGTM! Test expectation correctly demonstrates the fix.This expected output file correctly validates the fix for duplicate model generation with
$ref+nullable. Key observations:
- Only one
Usermodel is generated (lines 10-11), demonstrating that the fix prevents duplicates- All three nullable references (
user_a,user_b,user_c) correctly useUser | None = Nonewith Pydantic v2 syntax- The
from __future__ import annotationsimport (line 5) properly enables the modern union syntax- Structure matches the related non-strict version in
ref_nullable_only.pyThe file correctly represents the expected behavior after applying the
is_ref_with_nullable_onlyoptimization mentioned in the AI summary.tests/data/expected/main/jsonschema/ref_nullable_with_constraint.py (2)
5-7: LGTM!The imports are correctly set up for Pydantic v2, with future annotations enabled for modern union syntax.
10-11: LGTM!The Model class correctly demonstrates the combination of a constrained type (
constr(min_length=5)) with nullable union syntax, which is the key scenario this PR addresses.
Breaking Change AnalysisResult: Breaking changes detected Reasoning: This PR changes the generated code output for a specific pattern: when a JSON Schema uses While this is a bug fix that produces cleaner output, it is a breaking change because:
The change only affects the Content for Release NotesCode Generation Changes
This analysis was performed by Claude Code Action |
|
🎉 Released in 0.52.0 This PR is now available in the latest release. See the release notes for details. |
Fixes: #1792
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.