Fix required field default rendering and --use-default nullable types#3054
Fix required field default rendering and --use-default nullable types#3054butvinm wants to merge 4 commits intokoxudaxi:mainfrom
Conversation
__set_validate_default_on_fields set validate_default=True on fields with model references without checking field.required, causing required model-ref fields to render defaults while required scalar fields didn't. Skip required fields (unless use_default_with_required) so all required fields consistently have no defaults rendered. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
--use-default previously skipped setting field.required=True, which made fields non-required and therefore nullable (e.g. str | None = 'foo'). Now fields stay required=True with a new use_default_with_required flag that allows defaults to render without changing the type. Produces `status: str = 'foo'` instead of `status: str | None = 'foo'`. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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 ignored due to path filters (6)
📒 Files selected for processing (16)
📝 WalkthroughWalkthroughThe PR introduces a new Changes
Sequence DiagramsequenceDiagram
participant Parser as Parser<br/>(jsonschema/graphql/openapi)
participant Compute as Flag Computation
participant Field as DataModelFieldBase
participant Serializer as Field Serializer<br/>(dataclass/msgspec/pydantic)
participant Output as Generated Code
Parser->>Compute: required, has_default, apply_defaults config
Compute-->>Parser: use_default_with_required = required && has_default && config
Parser->>Field: create(required=True, use_default_with_required=True, default=value)
Field->>Serializer: serialize field with use_default_with_required flag
alt use_default_with_required is True
Serializer->>Output: include default_factory/default keys
else use_default_with_required is False
Serializer->>Output: omit default keys for required fields
end
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🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can customize the tone of the review comments and chat replies.Configure the |
Merging this PR will not alter performance
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3054 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 85 86 +1
Lines 17911 18020 +109
Branches 2074 2075 +1
==========================================
+ Hits 17911 18020 +109
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:
|
Cover the previously uncovered code paths: - allOf with $ref + sub-schema required fields + --use-default - allOf with outer required fields + --force-optional Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3f51eb9 to
bcef9e0
Compare
|
Thanks for the PR! |
|
@ilovelinux If you have time, could you share your thoughts on this PR, the related issues, and the approach? I feel like the PR needs to be split since it addresses two separate problems. |
|
@koxudaxi I'll give a look ASAP 🙂 |
|
@ilovelinux Thanks! I haven't gone through all the related issues yet and this looks like it could have a wide impact, so I want to take my time. I'll be back tonight, no rush at all. |
ilovelinux
left a comment
There was a problem hiding this comment.
Hi @koxudaxi, @butvinm. Sorry for the late answer. Last week has been tough.
Note
I splitted this comment in multiple paragraph. I though I wouldn't have much to say, but I was wrong. I hope they are easier to read.
About the changes
Note
If you have time, could you share your thoughts on this PR, the related issues, and the approach?
I admit reviewing changes hasn't been easy. I feel like we should reduce code complexity to increase code maintainability. However it kinda makes sense to me. I'll try to do a more in-depth review after this comment to double-check the logic.
I appreciate @butvinm work and agree with the fixes introduced, because:
- The first change allow a coherent usage of default values.
- The second change fix
Nonetype hint for required properties with default value defined (which can't beNonebecause they are required).
IMHO both changes make sense and reflect the expected user experience.
I also found an useful description in the OpenAPI docs1 (emphasis mine):
The default value to use for substitution, which SHALL be sent if an alternate value is not supplied.
E.g. We have a Python client and a Python server which both uses datamodel-code-generator. The server shouldn't populate ANY property with default values, instead the client should use default values and send them explicitly.
More generally, this adhere to the DRY principle: one side populate the data, the other side validates the data. Double-validation and, more important, double-population of default values, is error-prone.
As now, it isn't possible to adhere to the DRY principle using models generated by datamodel-code-generator. This PR make default values coherent avoiding double-population, and removes wrong None type hint improving data validation, allowing to apply DRY principle.
About the multiple-responsibilities
It looks like the PR is trying to fix multiple problems at once, so I want to check the related issues for each of them before reviewing.
I agree the two changes solve two different problems. However:
- I'm not sure if they are also independent of each other.
- If they are interdependent, splitting would make things more complex since one of them would depend on the other.
- They are both breaking changes about the same topic and I feel like they should be released together to reduce the end-user impact.
Despite single-responsibility PRs are cleaner and easier to review, I think we could avoid splitting this PR because the two changes are small and share the same surface impact (= default values).
About the breaking changes
I don't know how could a migration period for the users looks like for a change like this. I am not sure it would be useful because there's no way to avoid the breaking change. E.g.: changing the default formatter is a breaking change, but the migration period allow the users to acknowledge that and pin the old ones in the configuration file; this, instead, changes the way default values are rendered. There's no way to avoid that for the user and there's nothing the user can do to mitigate the breaking change. This looks more like an "incompatible version migration".
What if, instead, we communicate to the user what did change as soon as the user run the new version? A warning about breaking changes would:
- Reduce the cognitive effort to keep track of the breaking changes.
- Increases the chances the user understand and acknowledge what and why that happened.
- Encourage the user to open an issue if something is broken because of the breaking change (e.g. a corner case that is no longer handled).
In this case, I think the users would appreciate this PR since it's a bugfix and I don't think many people were relying on either the wrong generated type hints or the default values only for model-ref fields. I think, instead, many other people may be having some hard time because of this.
I'm not approving this PR because I'm taking some time to think about the impact, what @koxudaxi said, and all I have written. In the meanwhile, I'd like to hear what do you all think! 🙂
Also, a hot pizza is waiting for me! 🍕 Can't ignore Italian priorities 😆
Footnotes
|
@ilovelinux thanks for the thorough review! I agree on all points - especially about not splitting and treating this as a bugfix rather than a breaking change. Building on that, I think we could go even further. Since this PR already introduces breaking changes, we could address the broader nullable/required fields problem rather than patching it incrementally. Required fields have their defaults dropped by default. The sole purpose of If there's no evidence of use cases that depend on dropping defaults from required fields, we could always render defaults - removing the special case entirely and deprecating I understand this is a bigger breaking change, so I'm fine keeping this PR as-is and exploring that direction separately. |
Deprecating However, since I think the end user expects that default values are used to populate fields "by default", we may consider making
Yes. Feel free to open an issue about that so we can explore this proposal deeply without polluting this PR. 🙂 |
|
@ilovelinux agreed, will file a separate issue. Worth noting that currently |
|
Thanks for the detailed discussion.
For this PR, I would prefer to limit the scope to the first one.
From the behavior/correctness point of view, I think these changes are closer to bug fixes. So I think the safest way to handle this is:
For the broader design questions around
I think it would be better to open a separate issue and discuss them there. Does this direction sound good to both of you?
|
Fixes #3048
Two changes:
1. Consistent required field defaults (fixes the reported inconsistency)
__set_validate_default_on_fieldsnow skips required fields, so model-ref fields no longer get defaults while scalar fields don't. All required fields behave the same — no defaults rendered without--use-default.2.
--use-defaultno longer makes fields nullable (breaking change)Previously
--use-defaultturnedstatus: str | None = 'active'— making required fields optional and nullable. Now it producesstatus: str = 'active'— default is rendered but the type stays non-nullable. I consider the previous behavior a bug: the schema says the type isstring, notstring | null, and--use-defaultshouldn't change that.Before (bug)
Using the schema from #3048, without
--use-default:Scalar required fields (
status,quantity, etc.) have defaults dropped, but model-ref required fields (shipping_address, etc.) render defaults — inconsistent.After (fix)
Without
--use-default— consistent, all required fields have no defaults:With
--use-default— all defaults rendered, types stay non-nullable:Summary by CodeRabbit
Release Notes
New Features
Tests