fix #3034 and #3035 by introducing ValidatedDefault#3040
fix #3034 and #3035 by introducing ValidatedDefault#3040koxudaxi merged 15 commits intokoxudaxi:mainfrom
ValidatedDefault#3040Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new ValidatedDefault wrapper and wiring so parser wraps certain complex/type-alias defaults; codegen for Pydantic v2 emits those as TypeAdapter(...).validate_python(...) expressions. Many tests and expected outputs for type-alias default scenarios were added. Changes
Sequence Diagram(s)sequenceDiagram
participant Parser
participant Codegen
participant Runtime
Parser->>Parser: analyze field type & default\n(_needs_validated_default + helpers)
alt default requires validation
Parser->>Parser: wrap default -> ValidatedDefault(type_name, value)
end
Parser->>Codegen: produce model AST containing ValidatedDefault
Codegen->>Codegen: add TypeAdapter import when needed
Codegen->>Runtime: emit default as\nTypeAdapter(type_name).validate_python(value)
Note right of Runtime: TypeAdapter validates and constructs\nruntime default instance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca51faaaef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/datamodel_code_generator/model/base.py (1)
28-41:⚠️ Potential issue | 🟠 MajorDon't narrow
model.baseexports with this new__all__.
__all__ = ["ValidatedDefault", "WrappedDefault"]turnsfrom datamodel_code_generator.model.base import *into a two-symbol export, which is a breaking API change for any existing consumer that relied on the module's other public names. If the goal is just to re-export the new wrapper types, avoid adding__all__here or include the full intended public surface.Suggested fix
from datamodel_code_generator.model._types import ValidatedDefault, WrappedDefault - -__all__ = ["ValidatedDefault", "WrappedDefault"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/datamodel_code_generator/model/base.py` around lines 28 - 41, The new module-level __all__ currently restricts exports to only ValidatedDefault and WrappedDefault, breaking existing consumers of other public symbols from model.base; either remove the __all__ declaration entirely or expand it to include the full public surface (e.g., Reference, _BaseModel, ANY, NONE, OPTIONAL_PREFIX, UNION_PREFIX, DataType, Nullable, chain_as_tuple, get_optional_type along with ValidatedDefault and WrappedDefault) so that from datamodel_code_generator.model.base import * continues to export the original API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/datamodel_code_generator/parser/base.py`:
- Around line 480-484: The branch filtering uses data_type.data_types which can
be empty for aliases; change the non-none branch computation in the
resolved.is_union case to iterate over resolved.data_types (not
data_type.data_types) so it correctly builds non_none_branches, e.g. update the
tuple comprehension that defines non_none_branches to use resolved.data_types;
keep the subsequent checks that call
_uses_existing_model_factory_path(non_none_branches[0]) and the return that
calls _default_matches_plain_container_branch on resolved.data_types unchanged.
In `@tests/data/expected/main/jsonschema/type_alias_recursive_default_list.py`:
- Line 9: The fixture file containing the PEP 695 syntax (the line "type Alias =
list[Alias] | int") should be renamed from type_alias_recursive_default_list.py
to type_alias_recursive_default_list_py312.py to follow the repository
convention for Python 3.12-only fixtures; update any tests or references that
import or reference the old filename to the new filename
(type_alias_recursive_default_list_py312.py) so imports/fixtures point to the
renamed file.
---
Outside diff comments:
In `@src/datamodel_code_generator/model/base.py`:
- Around line 28-41: The new module-level __all__ currently restricts exports to
only ValidatedDefault and WrappedDefault, breaking existing consumers of other
public symbols from model.base; either remove the __all__ declaration entirely
or expand it to include the full public surface (e.g., Reference, _BaseModel,
ANY, NONE, OPTIONAL_PREFIX, UNION_PREFIX, DataType, Nullable, chain_as_tuple,
get_optional_type along with ValidatedDefault and WrappedDefault) so that from
datamodel_code_generator.model.base import * continues to export the original
API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1e835353-fc2e-4e36-9d7a-5b3d7738fbbe
⛔ Files ignored due to path filters (16)
tests/data/jsonschema/type_alias_chain_model_default_object_ref.jsonis excluded by!tests/data/**/*.jsonand included by nonetests/data/jsonschema/type_alias_chain_union_default_object_ref.jsonis excluded by!tests/data/**/*.jsonand included by nonetests/data/jsonschema/type_alias_dict_union_default_object_ref.jsonis excluded by!tests/data/**/*.jsonand included by nonetests/data/jsonschema/type_alias_inline_dict_union_default_object.jsonis excluded by!tests/data/**/*.jsonand included by nonetests/data/jsonschema/type_alias_inline_list_union_default_object.jsonis excluded by!tests/data/**/*.jsonand included by nonetests/data/jsonschema/type_alias_inline_union_default_object.jsonis excluded by!tests/data/**/*.jsonand included by nonetests/data/jsonschema/type_alias_inline_union_default_object_one_of.jsonis excluded by!tests/data/**/*.jsonand included by nonetests/data/jsonschema/type_alias_inline_union_default_object_silent_wrong_branch.jsonis excluded by!tests/data/**/*.jsonand included by nonetests/data/jsonschema/type_alias_list_model_default_object_ref.jsonis excluded by!tests/data/**/*.jsonand included by nonetests/data/jsonschema/type_alias_list_union_default_object_ref.jsonis excluded by!tests/data/**/*.jsonand included by nonetests/data/jsonschema/type_alias_model_default_object_ref.jsonis excluded by!tests/data/**/*.jsonand included by nonetests/data/jsonschema/type_alias_recursive_default_list.jsonis excluded by!tests/data/**/*.jsonand included by nonetests/data/jsonschema/type_alias_union_default_object_ref.jsonis excluded by!tests/data/**/*.jsonand included by nonetests/data/jsonschema/type_alias_union_default_object_ref_any_of.jsonis excluded by!tests/data/**/*.jsonand included by nonetests/data/jsonschema/type_alias_union_default_object_ref_mixed_scalar.jsonis excluded by!tests/data/**/*.jsonand included by nonetests/data/jsonschema/type_alias_union_default_object_ref_one_of.jsonis excluded by!tests/data/**/*.jsonand included by none
📒 Files selected for processing (22)
src/datamodel_code_generator/model/_types.pysrc/datamodel_code_generator/model/base.pysrc/datamodel_code_generator/model/pydantic_base.pysrc/datamodel_code_generator/model/pydantic_v2/base_model.pysrc/datamodel_code_generator/parser/base.pytests/data/expected/main/jsonschema/type_alias_chain_model_default_object_ref.pytests/data/expected/main/jsonschema/type_alias_chain_union_default_object_ref.pytests/data/expected/main/jsonschema/type_alias_dict_union_default_object_ref.pytests/data/expected/main/jsonschema/type_alias_inline_dict_union_default_object.pytests/data/expected/main/jsonschema/type_alias_inline_list_union_default_object.pytests/data/expected/main/jsonschema/type_alias_inline_union_default_object.pytests/data/expected/main/jsonschema/type_alias_inline_union_default_object_one_of.pytests/data/expected/main/jsonschema/type_alias_inline_union_default_object_silent_wrong_branch.pytests/data/expected/main/jsonschema/type_alias_list_model_default_object_ref.pytests/data/expected/main/jsonschema/type_alias_list_union_default_object_ref.pytests/data/expected/main/jsonschema/type_alias_model_default_object_ref.pytests/data/expected/main/jsonschema/type_alias_recursive_default_list.pytests/data/expected/main/jsonschema/type_alias_union_default_object_ref.pytests/data/expected/main/jsonschema/type_alias_union_default_object_ref_any_of.pytests/data/expected/main/jsonschema/type_alias_union_default_object_ref_mixed_scalar.pytests/data/expected/main/jsonschema/type_alias_union_default_object_ref_one_of.pytests/main/jsonschema/test_main_jsonschema.py
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Merging this PR will not alter performance
|
|
@keyz Could you make one small design cleanup in this PR before merge? The fix looks good overall, but I think the new validated-default logic in parser/base.py should use a model-level capability instead of checking concrete Pydantic v2 classes directly. That pattern already exists in this codebase ( Also, if the existing bot comments are valid, please address those here as well. In particular, the alias-union branch check (resolved.data_types vs data_type.data_types) looks worth fixing, and the comment about capturing |
|
I think That runs before later steps like type overrides, so the generated |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/datamodel_code_generator/parser/base.py (1)
3286-3295:⚠️ Potential issue | 🟠 MajorWrap validated defaults after type finalization.
Line 3287 freezes
ValidatedDefault.type_namebefore__apply_type_overrides()and__update_type_aliases(). If either pass rewrites the field type, the generatedTypeAdapter(...)call points at the old symbol.Suggested fix
self.__set_default_enum_member(models) self.__wrap_root_model_default_values(models) - self.__wrap_validated_default_values(models) self.__sort_models(models, imports, use_deferred_annotations=config.use_deferred_annotations) self.__change_field_name(models) self.__apply_discriminator_type(models, imports) self.__set_one_literal_on_default(models) self.__fix_dataclass_field_ordering(models) models = self.__remove_overridden_models(models) self.__apply_type_overrides(models) self.__update_type_aliases(models) + self.__wrap_validated_default_values(models)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/datamodel_code_generator/parser/base.py` around lines 3286 - 3295, The ValidatedDefault.type_name is being finalized too early by calling __wrap_validated_default_values(models) before type-finalizing passes; move the call (or re-run wrapping) so that __wrap_validated_default_values(models) is executed after both __apply_type_overrides(models) and __update_type_aliases(models) (i.e., ensure __apply_type_overrides and __update_type_aliases run before __wrap_validated_default_values) so that the TypeAdapter(...) generation uses the final, possibly rewritten field type names.
🧹 Nitpick comments (1)
src/datamodel_code_generator/parser/base.py (1)
415-456: Use model capabilities here instead of concretepydantic_v2classes.
_contains_model_reference()and_uses_existing_model_factory_path()hard-codepydantic_model_v2.BaseModel/RootModel, which pulls backend-specific knowledge back intoparser/base.py. The rest of this file already relies on model capability flags for this kind of branching, so these helpers should do the same to keep the parser backend-agnostic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/datamodel_code_generator/parser/base.py`:
- Around line 3286-3295: The ValidatedDefault.type_name is being finalized too
early by calling __wrap_validated_default_values(models) before type-finalizing
passes; move the call (or re-run wrapping) so that
__wrap_validated_default_values(models) is executed after both
__apply_type_overrides(models) and __update_type_aliases(models) (i.e., ensure
__apply_type_overrides and __update_type_aliases run before
__wrap_validated_default_values) so that the TypeAdapter(...) generation uses
the final, possibly rewritten field type names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c8917c83-c3e0-48f9-a0da-f0403b1fee83
📒 Files selected for processing (1)
src/datamodel_code_generator/parser/base.py
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/datamodel_code_generator/parser/base.py (1)
415-456: Use model-level capability flags instead of concrete class checks.Per maintainer feedback,
_contains_model_referenceand_uses_existing_model_factory_pathcheck concrete Pydantic v2 classes directly:isinstance(resolved.reference.source, (pydantic_model_v2.BaseModel, pydantic_model_v2.RootModel))This pattern differs from the codebase convention of using model-level capability flags (like
SUPPORTS_DISCRIMINATOR,SUPPORTS_WRAPPED_DEFAULT). Consider introducing a capability check on the source model (e.g., checking for aSUPPORTS_VALIDATED_DEFAULTflag or a similar marker) to maintain consistency and allow other model types to opt-in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/datamodel_code_generator/parser/base.py` around lines 415 - 456, Replace the concrete Pydantic class checks in _contains_model_reference and _uses_existing_model_factory_path with a model-level capability flag check on the referenced model (e.g., test reference.source for a boolean marker like SUPPORTS_VALIDATED_DEFAULT or another agreed-upon capability constant) instead of isinstance(..., (pydantic_model_v2.BaseModel, pydantic_model_v2.RootModel)); update both functions to call the same capability predicate (e.g., hasattr(reference.source, "SUPPORTS_VALIDATED_DEFAULT") and truthiness) when examining reference.source and reference of nested/data_types items so other model types can opt-in without relying on concrete classes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/datamodel_code_generator/parser/base.py`:
- Around line 2240-2260: The bug is that __wrap_validated_default_values
computes ValidatedDefault.type_name using
_get_validated_default_type_name(model_field.data_type) before
__apply_type_overrides runs, producing wrong TypeAdapter names; fix by either
calling __wrap_validated_default_values after __apply_type_overrides (move its
invocation to after __apply_type_overrides) or change ValidatedDefault to defer
type_name resolution (store the original data_type or a callable/lazy resolver
instead of a concrete type_name and compute it during render), updating places
that read ValidatedDefault.type_name to call the resolver.
In `@tests/main/jsonschema/test_main_jsonschema.py`:
- Around line 5722-5990: Add a new regression test that exercises
post-finalization type rewrites by creating a test function (e.g.,
test_main_jsonschema_type_alias_post_finalization_rewrite) that calls
run_main_and_assert with an input JSON schema designed to change the rendered
field type after wrapping (e.g., JSON_SCHEMA_DATA_PATH /
"type_alias_post_finalization_rewrite.json") and an expected output file that
asserts the generated TypeAdapter uses the final type hint (e.g.,
"type_alias_post_finalization_rewrite.py"); use the same flags as the other
tests (--use-type-alias, --use-annotated, --target-python-version 3.12,
--output-model-type pydantic_v2.BaseModel, --target-pydantic-version 2.11) so
the test verifies the adapter string reflects the finalized type rather than a
stale name, and add the test next to the other type-alias tests using
run_main_and_assert and assert_file_content.
- Around line 5704-5719: The test function
test_main_jsonschema_type_alias_inline_union_default_object is missing the
Black-version skip guard; wrap or annotate this test with the same
pytest.mark.skipif used in sibling tests (checking
int(black.__version__.split(".")[0]) < 23) so it is skipped when Black major
version is less than 23, ensuring it only runs in environments that support the
new 'type' syntax generated by --use-type-alias.
---
Nitpick comments:
In `@src/datamodel_code_generator/parser/base.py`:
- Around line 415-456: Replace the concrete Pydantic class checks in
_contains_model_reference and _uses_existing_model_factory_path with a
model-level capability flag check on the referenced model (e.g., test
reference.source for a boolean marker like SUPPORTS_VALIDATED_DEFAULT or another
agreed-upon capability constant) instead of isinstance(...,
(pydantic_model_v2.BaseModel, pydantic_model_v2.RootModel)); update both
functions to call the same capability predicate (e.g., hasattr(reference.source,
"SUPPORTS_VALIDATED_DEFAULT") and truthiness) when examining reference.source
and reference of nested/data_types items so other model types can opt-in without
relying on concrete classes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bb5436bd-8ba5-4be1-83b2-b354e066f7f0
📒 Files selected for processing (3)
src/datamodel_code_generator/model/base.pysrc/datamodel_code_generator/parser/base.pytests/main/jsonschema/test_main_jsonschema.py
|
thanks @koxudaxi! i pushed some new fixtures for |
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/main/jsonschema/test_main_jsonschema.py (1)
5594-6052: Add one non-Pydantic regression for the wrapped-default capability gate.This block only exercises
pydantic_v2.BaseModel. The PR objective also calls out moving wrapped-default support behind a model capability flag, so an unsupported backend could start emittingValidatedDefault/TypeAdapter(...)without this suite noticing. Please add one companion case for a backend that should not support wrapped defaults and assert it keeps that backend’s native default emission.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/main/jsonschema/test_main_jsonschema.py` around lines 5594 - 6052, Add a new test function alongside the existing ones that mirrors test_main_jsonschema_type_alias_union_default_object_ref_* but targets a non-Pydantic backend (call it e.g. test_main_jsonschema_type_alias_union_default_object_ref_non_pydantic_backend) that calls run_main_and_assert with the same input JSON (type_alias_union_default_object_ref.json), but sets extra_args to include "--use-type-alias", "--target-python-version", "3.12" and "--output-model-type" set to a non-pydantic backend (e.g. "dataclasses.dataclass"); set expected_file to a new back-end-specific expected output (e.g. "type_alias_union_default_object_ref_dataclasses.py") and assert_file_content so the test verifies the backend emits its native default (not ValidatedDefault/TypeAdapter).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/datamodel_code_generator/parser/base.py`:
- Around line 440-451: The helper _uses_existing_model_factory_path is
inspecting union branch wrappers directly and must first unwrap any alias-backed
branches before checking .reference, .is_list, or .data_types; modify
_uses_existing_model_factory_path to, for each candidate, follow/resolve an
alias chain to the underlying/base DataType (the same resolution logic used by
_needs_validated_default) and then perform the existing checks (reference +
_supports_validated_default_model and list-with-single-branch checks) against
that resolved type; apply the identical alias-unwrapping change to the other
helper checks noted in the comment so all places treat alias->dict/list/Model
branches the same way.
---
Nitpick comments:
In `@tests/main/jsonschema/test_main_jsonschema.py`:
- Around line 5594-6052: Add a new test function alongside the existing ones
that mirrors test_main_jsonschema_type_alias_union_default_object_ref_* but
targets a non-Pydantic backend (call it e.g.
test_main_jsonschema_type_alias_union_default_object_ref_non_pydantic_backend)
that calls run_main_and_assert with the same input JSON
(type_alias_union_default_object_ref.json), but sets extra_args to include
"--use-type-alias", "--target-python-version", "3.12" and "--output-model-type"
set to a non-pydantic backend (e.g. "dataclasses.dataclass"); set expected_file
to a new back-end-specific expected output (e.g.
"type_alias_union_default_object_ref_dataclasses.py") and assert_file_content so
the test verifies the backend emits its native default (not
ValidatedDefault/TypeAdapter).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1f120291-e38e-4d6c-8bac-9bce3ac77911
⛔ Files ignored due to path filters (2)
tests/data/jsonschema/type_alias_inline_union_default_object_import_collision.jsonis excluded by!tests/data/**/*.jsonand included by nonetests/data/jsonschema/type_alias_inline_union_default_object_type_override.jsonis excluded by!tests/data/**/*.jsonand included by none
📒 Files selected for processing (4)
src/datamodel_code_generator/parser/base.pytests/data/expected/main/jsonschema/type_alias_inline_union_default_object_import_collision.pytests/data/expected/main/jsonschema/type_alias_inline_union_default_object_type_override.pytests/main/jsonschema/test_main_jsonschema.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54b947c421
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/data/expected/main/jsonschema/type_alias_nullable_model_default_object_ref.py (1)
16-21: Same redundant| Nonepattern as in other fixtures.
AliasisA | None, sox: Alias | Noneis redundant. This appears to be a consistent pattern in generated fixtures. If intentional for schema fidelity, consider documenting this; otherwise, the generator could simplify tox: Alias.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/data/expected/main/jsonschema/type_alias_nullable_model_default_object_ref.py` around lines 16 - 21, The type annotation for field x redundantly adds "| None" on top of Alias which is already A | None; update the generator so it does not duplicate optionality by removing the extra "| None" when Alias is already nullable — i.e., change the produced annotation from "x: Alias | None" to "x: Alias" (leaving default_factory and usage of TypeAdapter(Alias) unchanged) so Model.x reflects Alias's nullability only once.tests/data/expected/main/jsonschema/type_alias_nullable_list_model_default_object_ref.py (1)
16-21: Redundant| Nonein field type declaration.
Aliasis defined aslist[A] | None, so declaringx: Alias | Noneresults inlist[A] | None | None, which is semantically redundant. If the generator intentionally preserves this for schema fidelity, this may be acceptable, but otherwise consider emitting justx: Alias.Note: The Ruff warning about
typestatement syntax is expected since this fixture targets Python 3.12 as indicated in the PR objectives.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/data/expected/main/jsonschema/type_alias_nullable_list_model_default_object_ref.py` around lines 16 - 21, The field annotation on Model is redundant: Alias is already defined as "list[A] | None", so change the Model.x annotation from "Alias | None" to just "Alias" (keep the existing Field(default_factory=...) and the TypeAdapter(Alias).validate_python([...]) call) so the type resolves to "list[A] | None" without duplicating the optional; update the annotation on the Model class accordingly.
🤖 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/main/jsonschema/test_main_jsonschema.py`:
- Around line 5774-6123: The tests exercising “relevant flags” are missing the
--use-default-kwarg path; update each test function in this diff (e.g.,
test_main_jsonschema_type_alias_union_default_object_ref_any_of_relevant_flags,
test_main_jsonschema_type_alias_chain_union_default_object_ref_relevant_flags,
test_main_jsonschema_type_alias_union_default_object_ref_one_of_relevant_flags,
test_main_jsonschema_type_alias_inline_union_default_object_one_of_relevant_flags,
test_main_jsonschema_type_alias_inline_union_default_object_import_collision_relevant_flags,
test_main_jsonschema_type_alias_inline_union_default_object_type_override_relevant_flags,
test_main_jsonschema_type_alias_inline_union_default_object_silent_wrong_branch_relevant_flags,
test_main_jsonschema_type_alias_inline_list_union_default_object_relevant_flags,
test_main_jsonschema_type_alias_list_union_default_object_ref_relevant_flags,
test_main_jsonschema_type_alias_inline_dict_union_default_object_relevant_flags,
test_main_jsonschema_type_alias_dict_union_default_object_ref_relevant_flags,
test_main_jsonschema_type_alias_union_default_object_ref_dict_alias_branch_relevant_flags,
test_main_jsonschema_type_alias_union_default_object_ref_mixed_scalar_relevant_flags)
to include "--use-default-kwarg" in their extra_args list so the suite covers
the Field(default=...) rendering path.
---
Nitpick comments:
In
`@tests/data/expected/main/jsonschema/type_alias_nullable_list_model_default_object_ref.py`:
- Around line 16-21: The field annotation on Model is redundant: Alias is
already defined as "list[A] | None", so change the Model.x annotation from
"Alias | None" to just "Alias" (keep the existing Field(default_factory=...) and
the TypeAdapter(Alias).validate_python([...]) call) so the type resolves to
"list[A] | None" without duplicating the optional; update the annotation on the
Model class accordingly.
In
`@tests/data/expected/main/jsonschema/type_alias_nullable_model_default_object_ref.py`:
- Around line 16-21: The type annotation for field x redundantly adds "| None"
on top of Alias which is already A | None; update the generator so it does not
duplicate optionality by removing the extra "| None" when Alias is already
nullable — i.e., change the produced annotation from "x: Alias | None" to "x:
Alias" (leaving default_factory and usage of TypeAdapter(Alias) unchanged) so
Model.x reflects Alias's nullability only once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8b549caf-4202-4fb9-87ba-cc869cd4c80b
⛔ Files ignored due to path filters (3)
tests/data/jsonschema/type_alias_nullable_list_model_default_object_ref.jsonis excluded by!tests/data/**/*.jsonand included by nonetests/data/jsonschema/type_alias_nullable_model_default_object_ref.jsonis excluded by!tests/data/**/*.jsonand included by nonetests/data/jsonschema/type_alias_union_default_object_ref_dict_alias_branch.jsonis excluded by!tests/data/**/*.jsonand included by none
📒 Files selected for processing (5)
src/datamodel_code_generator/parser/base.pytests/data/expected/main/jsonschema/type_alias_nullable_list_model_default_object_ref.pytests/data/expected/main/jsonschema/type_alias_nullable_model_default_object_ref.pytests/data/expected/main/jsonschema/type_alias_union_default_object_ref_dict_alias_branch.pytests/main/jsonschema/test_main_jsonschema.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3040 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 85 85
Lines 17692 17883 +191
Branches 2068 2091 +23
==========================================
+ Hits 17692 17883 +191
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:
|
Breaking Change AnalysisResult: No breaking changes detected Reasoning: This PR is a bug fix for issues #3034 and #3035. It introduces This analysis was performed by Claude Code Action |
|
🎉 Released in 0.55.0 This PR is now available in the latest release. See the release notes for details. |
…tructured defaults Replace the ValidatedDefault/TypeAdapter mechanism from koxudaxi#3040 and the original model_validate() codegen with Pydantic's native validate_default=True. Instead of generating complex default_factory lambdas like: default_factory=lambda: A.model_validate({'type': 'b'}) default_factory=lambda: TypeAdapter(A | B).validate_python({'type': 'b'}) Now generates: Field({'type': 'b'}, validate_default=True) This lets Pydantic handle union branch selection, dict-to-model coercion, and RootModel wrapping at runtime. Fixes koxudaxi#3034, fixes koxudaxi#3035. Removed: - ValidatedDefault class and SUPPORTS_VALIDATED_DEFAULT flag - TypeAdapter import in generated code - 70-line branch-matching loop in _get_default_as_pydantic_model - Helper functions: _supports_validated_default_model, _uses_existing_model_factory_path, _default_matches_plain_container_branch, _get_validated_default_type_name Added: - _needs_validate_default: simple check (is default dict/list/scalar and does the type contain a model reference?) - __set_validate_default_on_fields: sets extras["validate_default"] = True Co-Authored-By: Claude Opus 4.6 <[email protected]>
* Use validate_default=True instead of model_validate/TypeAdapter for structured defaults Replace the ValidatedDefault/TypeAdapter mechanism from #3040 and the original model_validate() codegen with Pydantic's native validate_default=True. Instead of generating complex default_factory lambdas like: default_factory=lambda: A.model_validate({'type': 'b'}) default_factory=lambda: TypeAdapter(A | B).validate_python({'type': 'b'}) Now generates: Field({'type': 'b'}, validate_default=True) This lets Pydantic handle union branch selection, dict-to-model coercion, and RootModel wrapping at runtime. Fixes #3034, fixes #3035. Removed: - ValidatedDefault class and SUPPORTS_VALIDATED_DEFAULT flag - TypeAdapter import in generated code - 70-line branch-matching loop in _get_default_as_pydantic_model - Helper functions: _supports_validated_default_model, _uses_existing_model_factory_path, _default_matches_plain_container_branch, _get_validated_default_type_name Added: - _needs_validate_default: simple check (is default dict/list/scalar and does the type contain a model reference?) - __set_validate_default_on_fields: sets extras["validate_default"] = True Co-Authored-By: Claude Opus 4.6 <[email protected]> * Remove dead None guard from _needs_validate_default to fix coverage The caller already filters None/UNDEFINED defaults before calling this function, so the None check was unreachable. Removed the unused `default` parameter entirely. Co-Authored-By: Claude Opus 4.6 <[email protected]> * Remove WrappedDefault in favor of validate_default=True for RootModel defaults WrappedDefault wrapped RootModel scalar defaults (e.g., CountType(10)) in default_factory lambdas. Since validate_default=True handles this natively, the entire mechanism is redundant. Removed: WrappedDefault class, _types.py module, SUPPORTS_WRAPPED_DEFAULT flag, __wrap_root_model_default_values method, _get_default_as_pydantic_model method. Co-Authored-By: Claude Opus 4.6 <[email protected]> * Make GraphQL scalar types extend TypeAliasBase _DataTypeScalarBase generated the same type alias syntax as TypeAliasBase but didn't inherit from it, causing _unwrap_type_alias to not see through scalar references. This led to unnecessary validate_default=True on fields like `String | None = 'default_user'`. Co-Authored-By: Claude Opus 4.6 <[email protected]> * Fix required fields with validate_default losing their default value When a required field had validate_default=True set (e.g., empty list default on list[Container]), the rendering logic prepended ... instead of the actual default value. Now skips the ... override when validate_default is set, since the default is needed for Pydantic to validate at runtime. Co-Authored-By: Claude Opus 4.6 <[email protected]> * Fix inaccurate docstring in test_needs_validate_default test Co-Authored-By: Claude Opus 4.6 <[email protected]> --------- Co-authored-by: Claude Opus 4.6 <[email protected]>
|
📢 Related PR Released: 0.56.0 PR #3050, which references this PR, has been released. See the release notes for details. |
3 participants
this is an attempt to fix #3034 and #3035 via introducing
ValidatedDefaultfor structured defaults (that must be validated through the declared field type)before this change, some union and type-alias defaults were emitted either as raw dict/list literals or validated through the wrong union branch
overall I think the fixture coverage is good, but there may be a cleaner fix than
ValidatedDefault. please feel free to edit this PR or open a separate PR if you see a better solution!as extra context: here are the CLI flags I use. I use the library to translate codex app-server json schema to pydantic models:
Summary by CodeRabbit
New Features
Tests