-
-
Notifications
You must be signed in to change notification settings - Fork 426
Fix required list fields ignoring empty default values #2958
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
Conversation
📝 WalkthroughWalkthroughTightened DataModelField stringification: default_factory is preserved when a required field has an explicit default, and the ellipsis ("...") for required fields is only emitted when the field is required and no default_factory is present. Changes
Sequence Diagram(s)(Skipped — changes are focused on generator logic and test fixtures; no multi-component runtime flow to visualize.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)tests/data/expected/main/jsonschema/has_default_value_pydantic_v2.py (1)
⏰ 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). (14)
🔇 Additional comments (6)
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-2958.datamodel-code-generator.pages.dev |
CodSpeed Performance ReportMerging this PR will not alter performanceComparing
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2958 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 94 94
Lines 17738 17739 +1
Branches 2038 2038
=========================================
+ Hits 17738 17739 +1
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:
|
a68f28b to
ad58a89
Compare
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @tests/main/jsonschema/test_main_jsonschema.py:
- Around line 3257-3273: The parametrized case for "pydantic_v2.BaseModel" in
test_main_jsonschema_has_default_value is not guarded and will run in Pydantic
v1 CI; update the param list to replace the plain tuple for the v2 case with
pytest.param("pydantic_v2.BaseModel", "has_default_value_pydantic_v2.py",
marks=PYDANTIC_V2_SKIP) (you can also add ids for clarity), keeping the
pydantic.BaseModel entry unchanged and leaving the test function
test_main_jsonschema_has_default_value and run_main_and_assert usage as-is.
🧹 Nitpick comments (1)
tests/data/expected/main/jsonschema/has_default_value_pydantic_v2.py (1)
45-46: Consider generating enum member references instead of string literals.The enum defaults use string literals (
'Department') rather than enum member references (TeamType.Department). While Pydantic coerces these at runtime, static type checkers may flag this as a type mismatch.This appears to be existing generator behavior and is out of scope for this PR, but could be addressed in a follow-up.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/data/jsonschema/has_default_value.jsonis excluded by!tests/data/**/*.jsonand included by none
📒 Files selected for processing (4)
src/datamodel_code_generator/model/pydantic/base_model.pytests/data/expected/main/jsonschema/has_default_value.pytests/data/expected/main/jsonschema/has_default_value_pydantic_v2.pytests/main/jsonschema/test_main_jsonschema.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/data/expected/main/jsonschema/has_default_value.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-02T08:25:22.111Z
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.
Applied to files:
src/datamodel_code_generator/model/pydantic/base_model.py
🧬 Code graph analysis (3)
src/datamodel_code_generator/model/pydantic/base_model.py (1)
src/datamodel_code_generator/parser/jsonschema.py (1)
has_default(430-432)
tests/main/jsonschema/test_main_jsonschema.py (1)
tests/main/conftest.py (2)
output_file(99-101)run_main_and_assert(245-409)
tests/data/expected/main/jsonschema/has_default_value_pydantic_v2.py (2)
src/datamodel_code_generator/model/enum.py (1)
Enum(39-121)src/datamodel_code_generator/util.py (1)
model_validate(302-306)
⏰ 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). (16)
- GitHub Check: py312-pydantic1 on Ubuntu
- GitHub Check: py312-black22 on Ubuntu
- GitHub Check: 3.10 on macOS
- GitHub Check: 3.14 on Windows
- GitHub Check: 3.10 on Windows
- GitHub Check: 3.14 on macOS
- GitHub Check: 3.10 on Ubuntu
- GitHub Check: 3.11 on Ubuntu
- GitHub Check: 3.11 on macOS
- GitHub Check: 3.11 on Windows
- GitHub Check: 3.12 on Windows
- GitHub Check: 3.12 on macOS
- GitHub Check: 3.13 on Windows
- GitHub Check: 3.13 on macOS
- GitHub Check: Analyze (python)
- GitHub Check: benchmarks
🔇 Additional comments (3)
src/datamodel_code_generator/model/pydantic/base_model.py (2)
252-256: Correct handling of ellipsis for required fields with default_factory.The additional
and not default_factorycondition ensures thatField(...)is only emitted when there's truly no default — preventing incorrect output likeField(..., default_factory=list)for fields that have computed default factories.
223-228: LGTM! Logic correctly preserves default_factory for required fields with explicit defaults.The condition change from
if self.required:toif self.required and not self.has_default:fixes the core issue — required fields with explicit defaults (e.g., empty lists[]) will no longer have theirdefault_factorynullified, allowing the computed factory from_get_default_as_pydantic_model()to be used. Thehas_defaultattribute is inherited fromDataModelFieldBaseand is properly accessible here.tests/data/expected/main/jsonschema/has_default_value_pydantic_v2.py (1)
27-38: These patterns correctly demonstrate the fix for empty list defaults.The
FamilyandFamilyPetsmodels now properly usedefault_factory=lambda: [...]instead of the problematicdefault_factory=lambda: [Model.model_validate(v) for v in []]pattern. This aligns with the PR objective of generating simple empty-list defaults for optional list fields with[]specified.
ad58a89 to
85fa705
Compare
Breaking Change AnalysisResult: Breaking changes detected Reasoning: This PR modifies the code generation logic to properly handle required fields that also have default values. Previously, the condition Content for Release NotesCode Generation Changes
This analysis was performed by Claude Code Action |
|
🎉 Released in 0.53.0 This PR is now available in the latest release. See the release notes for details. |
Fixes: #2947
Summary by CodeRabbit
Bug Fixes
New Examples
Tests
✏️ Tip: You can customize this high-level summary in your review settings.