Add --field-type-collision-strategy option#2733
Conversation
|
Warning Rate limit exceeded@koxudaxi has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 12 minutes and 4 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughIntroduces Changes
Sequence Diagram(s)sequenceDiagram
participant User as CLI/User
participant Config as Config (cli args)
participant Generate as generate()
participant Parser as Parser (JsonSchema/OpenAPI/GraphQL)
participant Codegen as Code Generator / Emitter
User->>Config: provide --field-type-collision-strategy
Config->>Generate: call generate(..., field_type_collision_strategy)
Generate->>Parser: instantiate Parser(..., field_type_collision_strategy)
Parser->>Parser: parse schemas, detect field/type name collision
alt strategy == RenameType
Parser->>Parser: rename type (append underscore)
else strategy == RenameField or None
Parser->>Parser: rename field (append suffix/alias)
end
Parser->>Codegen: emit models with resolved names
Codegen->>User: write generated files
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
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 |
🤖 Generated by GitHub Actions
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2733 +/- ##
=======================================
Coverage 99.36% 99.36%
=======================================
Files 83 83
Lines 12068 12087 +19
Branches 1456 1457 +1
=======================================
+ Hits 11991 12010 +19
Misses 45 45
Partials 32 32
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:
|
CodSpeed Performance ReportMerging #2733 will not alter performanceComparing Summary
Footnotes
|
There was a problem hiding this comment.
Actionable comments posted: 1
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)
1843-1877: Rename‑type strategy mutates types without cross‑module filtering, breaking importsThe
rename_typecollision strategy in__change_field_nameunconditionally mutatessource.class_namewhen a field name collides with a referenced type's short name, but it does not check whether the colliding type lives in the same module as the field.Risk: When a field in module A collides with a type from module B:
- Imports for module A are generated before
__change_field_nameruns- The rename-type logic then mutates the type's class name in module B to add a
_suffix- Module A's imports reference the old name, but module B now defines the suffixed name
- This causes NameError at runtime
The codebase already uses
source.module_path != model.module_pathchecks in__apply_discriminator_typefor exactly this purpose, but__change_field_nameomits this guard.Additional concern: Appending
"_"without validation against existing types can silently create duplicate class names if the schema already defines a type with that suffix.Recommended fix:
- Guard the
colliding_referenceassignment with a module-path check: only set it whensource.module_path == model.module_path- For cross-module collisions, fall back to field renaming (the else branch), which doesn't mutate types
- Consider using
ModelResolverto generate unique names instead of hard-coding the suffixTests needed:
- Cross-module collision scenario: type and field in different modules with
--field-type-collision-strategy rename-type- Existing
_-suffixed type: verify no silent name collisions when schema already definesFruit_
🧹 Nitpick comments (6)
docs/cli-reference/field-customization.md (1)
15-15: Update table description to match improved documentation.The table entry uses the same test-focused language. After updating the main documentation section, this should be updated to match.
Suggested description for the table:
| [`--field-type-collision-strategy`](#field-type-collision-strategy) | Configure how to resolve naming conflicts between field names and generated type names. |src/datamodel_code_generator/__main__.py (1)
474-474: Remove unusednoqadirective.Static analysis correctly identifies that the
UP045code in thenoqacomment is not needed, as it refers to a rule that isn't being violated here.🔎 Proposed fix
- field_type_collision_strategy: Optional[FieldTypeCollisionStrategy] = None # noqa: UP045 + field_type_collision_strategy: Optional[FieldTypeCollisionStrategy] = Nonedocs/cli-reference/index.md (1)
77-77: Update description after fixing field-customization.md.This entry links to the field-customization page and should be updated once the documentation there is improved (as noted in the review comment for field-customization.md).
docs/cli-reference/quick-reference.md (1)
59-59: Update descriptions after fixing field-customization.md.Both entries (table at line 59 and alphabetical index at line 208) use the test-focused description "Test field-type collision with rename-type strategy." These should be updated to match the improved user-facing documentation suggested in the review comment for field-customization.md.
Also applies to: 208-208
src/datamodel_code_generator/arguments.py (1)
598-605: Keep help text and core default for collision strategy in syncThe help string states that
'rename-field'is the default strategy, while the CLI argument itself defaults toNone(with the actual default presumably applied in core logic). That’s fine, but it does create a small coupling: if the core’s default ever changes, this help text will also need updating to avoid drift.tests/main/jsonschema/test_main_jsonschema.py (1)
3575-3612: Rename‑type field/type collision tests are well‑targetedBoth tests correctly exercise
--field-type-collision-strategy rename-typefor single‑ and multi‑model JSON Schema inputs against the new golden files, matching the intended behavior (renaming the enum type while preserving the field name). The cli_doc metadata also looks consistent; just ensure thatgolden_output="field_type_collision_rename_type.py"matches the path conventions your CLI docs generator expects.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
docs/cli-reference/field-customization.mddocs/cli-reference/index.mddocs/cli-reference/quick-reference.mdsrc/datamodel_code_generator/__init__.pysrc/datamodel_code_generator/__main__.pysrc/datamodel_code_generator/arguments.pysrc/datamodel_code_generator/cli_options.pysrc/datamodel_code_generator/parser/base.pysrc/datamodel_code_generator/parser/graphql.pysrc/datamodel_code_generator/parser/jsonschema.pysrc/datamodel_code_generator/parser/openapi.pytests/data/expected/main/jsonschema/field_type_collision_rename_type.pytests/data/expected/main/jsonschema/field_type_collision_rename_type_multi_model.pytests/data/jsonschema/field_type_collision_rename_type.jsontests/data/jsonschema/field_type_collision_rename_type_multi_model.jsontests/main/jsonschema/test_main_jsonschema.py
🧰 Additional context used
🧬 Code graph analysis (7)
src/datamodel_code_generator/parser/graphql.py (1)
src/datamodel_code_generator/__init__.py (1)
FieldTypeCollisionStrategy(312-320)
src/datamodel_code_generator/parser/jsonschema.py (1)
src/datamodel_code_generator/__init__.py (1)
FieldTypeCollisionStrategy(312-320)
tests/data/expected/main/jsonschema/field_type_collision_rename_type.py (2)
src/datamodel_code_generator/model/enum.py (1)
Enum(39-121)tests/data/expected/main/jsonschema/field_type_collision_rename_type_multi_model.py (1)
Fruit_(12-14)
tests/data/expected/main/jsonschema/field_type_collision_rename_type_multi_model.py (2)
src/datamodel_code_generator/model/enum.py (1)
Enum(39-121)tests/data/expected/main/jsonschema/field_type_collision_rename_type.py (1)
Fruit_(12-14)
src/datamodel_code_generator/parser/openapi.py (1)
src/datamodel_code_generator/__init__.py (1)
FieldTypeCollisionStrategy(312-320)
src/datamodel_code_generator/__main__.py (1)
src/datamodel_code_generator/__init__.py (1)
FieldTypeCollisionStrategy(312-320)
src/datamodel_code_generator/arguments.py (1)
src/datamodel_code_generator/__init__.py (1)
FieldTypeCollisionStrategy(312-320)
🪛 Ruff (0.14.8)
src/datamodel_code_generator/__main__.py
474-474: Unused noqa directive (non-enabled: UP045)
Remove unused noqa directive
(RUF100)
⏰ 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: py312-black22 on Ubuntu
- GitHub Check: 3.10 on Ubuntu
- GitHub Check: 3.11 on Windows
- GitHub Check: 3.14 on Windows
- GitHub Check: py312-isort7 on Ubuntu
- GitHub Check: 3.12 on Windows
- GitHub Check: py312-pydantic1 on Ubuntu
- GitHub Check: py312-black24 on Ubuntu
- GitHub Check: 3.13 on Windows
- GitHub Check: 3.10 on Windows
- GitHub Check: Analyze (python)
- GitHub Check: benchmarks
🔇 Additional comments (22)
tests/data/jsonschema/field_type_collision_rename_type.json (1)
1-12: LGTM! Well-designed test case.This JSON schema effectively tests the field-type collision scenario by creating a property named "Fruit" with enum values that would generate a type also named "Fruit", triggering the collision resolution logic.
src/datamodel_code_generator/cli_options.py (1)
137-139: LGTM! Consistent with existing option metadata.The CLI option metadata entry follows the established pattern and is correctly categorized under
OptionCategory.FIELD.src/datamodel_code_generator/__main__.py (2)
31-31: LGTM! Import added correctly.The
FieldTypeCollisionStrategyimport is properly added alongside other similar enums and configuration types.
781-781: LGTM! Parameter correctly threaded through to generation.The
field_type_collision_strategyis properly passed from the config to thegenerate()function, following the same pattern as other configuration options.tests/data/jsonschema/field_type_collision_rename_type_multi_model.json (1)
1-27: LGTM! Comprehensive multi-model test case.This schema effectively tests the collision resolution strategy across multiple models (Parent and Child) that both reference the same enum definition, ensuring the
rename-typestrategy works correctly when the same type is shared.docs/cli-reference/index.md (1)
13-13: LGTM! Count correctly updated.The Field Customization option count is properly incremented from 20 to 21 to reflect the new
--field-type-collision-strategyoption.tests/data/expected/main/jsonschema/field_type_collision_rename_type.py (1)
12-18: LGTM! Expected output correctly demonstrates rename-type strategy.The generated code correctly shows:
- The enum type renamed to
Fruit_(with underscore suffix) to avoid collision- The field keeps its original name
Fruit(no alias needed)This demonstrates that the
rename-typestrategy successfully resolves the naming conflict by renaming the generated type rather than the field, which aligns with the PR's objective to address issue #2364.tests/data/expected/main/jsonschema/field_type_collision_rename_type_multi_model.py (1)
1-23: Expected model layout matches rename‑type semanticsEnum
Fruit_reused by bothChild.FruitandParent.Fruitwith optionalFruit_ | None = Nonematches the intended rename‑type strategy and is consistent with existing fixtures. No changes needed.src/datamodel_code_generator/arguments.py (1)
17-30: CLI wiring for--field-type-collision-strategylooks correctImporting
FieldTypeCollisionStrategyand usingchoices=[s.value for s in FieldTypeCollisionStrategy]integrates the new option cleanly into the existing field customization group while remaining backward compatible (default=None). No functional issues spotted.Also applies to: 598-605
src/datamodel_code_generator/parser/openapi.py (1)
20-35: OpenAPI parser correctly threads field/type collision strategy to base classImporting
FieldTypeCollisionStrategy, addingfield_type_collision_strategyas the last__init__parameter, and forwarding it viasuper().__init__(..., field_type_collision_strategy=field_type_collision_strategy)cleanly exposes the new configuration without breaking existing call sites.Also applies to: 181-282, 284-379
src/datamodel_code_generator/parser/graphql.py (1)
16-28: GraphQL parser API stays backward compatible while exposing new strategyThe new
field_type_collision_strategyparameter is appended at the end ofGraphQLParser.__init__and forwarded to the baseParser, matching the pattern used in other parsers. This provides consistent configurability without impacting existing usages.Also applies to: 98-197, 199-295
src/datamodel_code_generator/parser/jsonschema.py (3)
24-37: ImportingFieldTypeCollisionStrategyhere is appropriateWiring the enum in at the parser layer via this import matches its usage in the base
Parser; no issues from a dependency or layering perspective.
512-609: JSONSchemaParser: newfield_type_collision_strategyarg is correctly threadedThe new parameter is optional, at the end of the signature, and passed through to
Parser.__init__, so it’s backwards compatible for existing callers and keeps behavior unchanged when left asNone.Please double‑check that every other
Parsersubclass (OpenAPIParser,GraphQLParser, etc.) also accepts and forwardsfield_type_collision_strategyso thatgenerate(..., field_type_collision_strategy=...)never hits an unexpected‑keyword error.
612-707: Super call propagation offield_type_collision_strategylooks correctPassing
field_type_collision_strategy=field_type_collision_strategyintosuper().__init__is consistent with the base signature and is the only wiring needed here; no additional logic is required in this class.src/datamodel_code_generator/__init__.py (4)
312-321: FieldTypeCollisionStrategy enum definition is clear and well‑scopedThe enum values (
"rename-field","rename-type") and docstring clearly document the behavior; exposing this at the top level is appropriate for both CLI and library users.
393-500:generate()parameter addition is API‑compatibleAdding
field_type_collision_strategy: FieldTypeCollisionStrategy | None = Noneat the end ofgenerate()’s parameter list preserves all existing call patterns and keeps default behavior unchanged whenNone.If any external integrations call
generate()with**kwargsbuilt dynamically, ensure they either tolerate or explicitly set this new key when running against older versions for backwards‑compat.
646-745: Correctly propagatingfield_type_collision_strategyinto parser constructionPassing
field_type_collision_strategy=field_type_collision_strategyintoparser_class(...)ensures the strategy flows through to all parsers without impacting users who don’t opt in.
882-898: Re‑exporting FieldTypeCollisionStrategy via__all__is appropriateAdding
FieldTypeCollisionStrategyto__all__makes the new enum part of the public API surface, which aligns with its use in CLI and library code.src/datamodel_code_generator/parser/base.py (4)
25-35: Base parser import of FieldTypeCollisionStrategy is fineImporting
FieldTypeCollisionStrategyalongside the other configuration enums keeps configuration concerns centralized in this module.
677-775: Parser.init: new strategy parameter is wired but unused beyond storageAccepting
field_type_collision_strategyhere and storing it onselfis the right place for parser‑wide configuration; there’s no extra logic required at construction time.
929-930: Storingfield_type_collision_strategyon the instance is appropriateThis keeps the strategy available for downstream methods like
__change_field_namewithout affecting existing behavior when left asNone.
2727-2730: Ordering: field/type renaming after imports is subtle
__change_field_name(models)is invoked after__change_from_import(...)within_process_single_module. This ordering is fine for the existing field‑rename behavior, but it’s exactly why the rename‑type strategy can desync type names from already‑computed imports in cross‑module scenarios (see earlier comment on__change_field_name).If you keep rename‑type, please ensure either:
- Type renames happen before import computation, or
- They’re constrained to same‑module types so imports remain valid.
🤖 Generated by GitHub Actions
…ategy' into feature/field-type-collision-strategy
🤖 Generated by GitHub Actions
Fixes: #2364
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.