Fix discriminator with allOf without Literal type for Pydantic v2#2722
Fix discriminator with allOf without Literal type for Pydantic v2#2722
Conversation
WalkthroughAdded helpers to normalize discriminator mapping values to fully-qualified component refs and store normalized discriminators on parsed fields. Enhanced union type resolution to fall back to normalized discriminator mappings when no allOf subtypes are found. Added OpenAPI fixtures and tests for short mapping names and no-mapping scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/datamodel_code_generator/parser/openapi.py (2)
398-418: Remove unusednoqadirective.The static analysis tool reports that the
noqa: PLR6301directive on line 398 is unused, as the code doesn't trigger the PLR6301 rule (method could be a static/class method). Since the docstring already explains why this isn't a staticmethod (due to the@snooper_to_methods()decorator), the noqa directive can be safely removed.🔎 Proposed fix
- def _normalize_discriminator_mapping_ref(self, mapping_value: str) -> str: # noqa: PLR6301 + def _normalize_discriminator_mapping_ref(self, mapping_value: str) -> str: """Normalize a discriminator mapping value to a full $ref path.
477-483: Remove unusednoqadirective.The static analysis tool reports that the
noqa: PLW2901directive on line 479 is unused. The code doesn't trigger a loop variable reassignment warning, so this directive can be removed.🔎 Proposed fix
if (discriminator := self._discriminator_schemas.get(field.ref)) ): new_field_type = self._get_discriminator_union_type(field.ref) or field_obj.data_type normalized_discriminator = self._normalize_discriminator(discriminator) - field_obj = self.data_model_field_type(**{ # noqa: PLW2901 + field_obj = self.data_model_field_type(**{ **field_obj.__dict__, "data_type": new_field_type, "extras": {**field_obj.extras, "discriminator": normalized_discriminator},
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/datamodel_code_generator/parser/openapi.py(2 hunks)tests/data/expected/main/openapi/discriminator/allof_no_subtypes.py(2 hunks)tests/data/expected/main/openapi/discriminator/short_mapping_names.py(1 hunks)tests/data/openapi/discriminator_short_mapping_names.yaml(1 hunks)tests/main/openapi/test_main_openapi.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/datamodel_code_generator/parser/openapi.py (3)
src/datamodel_code_generator/__main__.py (1)
get(112-114)src/datamodel_code_generator/reference.py (1)
get(831-833)src/datamodel_code_generator/types.py (1)
DataType(285-619)
tests/main/openapi/test_main_openapi.py (2)
tests/test_main_kr.py (1)
output_file(44-46)tests/main/conftest.py (2)
output_file(94-96)run_main_and_assert(196-352)
tests/data/expected/main/openapi/discriminator/short_mapping_names.py (1)
tests/data/expected/main/openapi/discriminator/allof_no_subtypes.py (4)
BaseItem(12-13)FooItem(16-18)BarItem(21-23)ItemContainer(26-27)
🪛 Ruff (0.14.8)
src/datamodel_code_generator/parser/openapi.py
398-398: Unused noqa directive (non-enabled: PLR6301)
Remove unused noqa directive
(RUF100)
479-479: Unused noqa directive (non-enabled: PLW2901)
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). (10)
- GitHub Check: py312-pydantic1 on Ubuntu
- GitHub Check: 3.10 on Windows
- GitHub Check: 3.11 on Windows
- GitHub Check: 3.12 on macOS
- GitHub Check: 3.11 on macOS
- GitHub Check: 3.12 on Windows
- GitHub Check: 3.14 on Windows
- GitHub Check: 3.13 on Windows
- GitHub Check: Analyze (python)
- GitHub Check: benchmarks
🔇 Additional comments (6)
src/datamodel_code_generator/parser/openapi.py (2)
420-428: LGTM! Clean discriminator normalization.The method correctly creates a copy of the discriminator dictionary and normalizes all string mapping values to full references. The
isinstance(v, str)check appropriately filters mapping values, as per the OpenAPI specification where discriminator mappings should always contain string references.
430-449: Excellent enhancement for discriminator union resolution.The fallback to discriminator mappings when no
allOfsubtypes are found elegantly handles the case where schemas define discriminators with explicit mappings but don't use inheritance. The normalization of mapping references ensures compatibility with both short names (FooItem) and full refs (#/components/schemas/FooItem).This directly addresses the PR objective of fixing discriminators with
allOfwithout Literal type for Pydantic v2.tests/data/openapi/discriminator_short_mapping_names.yaml (1)
1-50: LGTM! Well-structured test fixture for short mapping names.This test fixture properly exercises the discriminator normalization functionality by using short mapping names (
foo: FooItem,bar: BarItem) instead of full references. The structure withBaseItem,FooItem,BarItem, andItemContainerprovides comprehensive coverage for the discriminator resolution with mapping-based subtypes.tests/main/openapi/test_main_openapi.py (1)
274-291: LGTM! Comprehensive test for discriminator short mapping names.The test properly validates the new discriminator normalization functionality with short mapping names. The docstring clearly explains the scenario being tested, and the test follows the established patterns in the test suite. Using
pydantic_v2.BaseModelas the output model type is appropriate for testing Pydantic v2 discriminator support.tests/data/expected/main/openapi/discriminator/allof_no_subtypes.py (1)
7-27: LGTM! Expected output correctly reflects discriminated union pattern.The generated code properly uses:
Literaltypes for discriminator fields onFooItemandBarItem- Union type
FooItem | BarItemfor theItemContainer.itemfieldField(..., discriminator='itemType')to wire up the discriminatorThis output demonstrates that the discriminator normalization and union type resolution are working correctly for schemas without
allOfinheritance.tests/data/expected/main/openapi/discriminator/short_mapping_names.py (1)
1-27: LGTM! New expected output correctly validates short mapping name handling.The expected output properly demonstrates that short discriminator mapping names (
foo: FooItem,bar: BarItem) are correctly:
- Normalized to full references during parsing
- Resolved to create the discriminated union type
FooItem | BarItem- Generated with proper
Literaldiscriminator fields andField(..., discriminator='itemType')This validates the entire normalization and union resolution pipeline introduced in this PR.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2722 +/- ##
=======================================
Coverage 99.35% 99.35%
=======================================
Files 83 83
Lines 11722 11744 +22
Branches 1412 1416 +4
=======================================
+ Hits 11646 11668 +22
Misses 45 45
Partials 31 31
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 #2722 will not alter performanceComparing Summary
Footnotes
|
koxudaxi
force-pushed
the
fix/discriminator-allof-without-literal-type
branch
4 times, most recently
from
c70ff8c to
d79de04
Compare
koxudaxi
force-pushed
the
fix/discriminator-allof-without-literal-type
branch
from
d79de04 to
e468f2d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/datamodel_code_generator/parser/openapi.py (2)
398-418: Remove unusednoqadirective.The
# noqa: PLR6301directive on line 398 is not needed as the rule is not enabled in your configuration.🔎 Proposed fix
- def _normalize_discriminator_mapping_ref(self, mapping_value: str) -> str: # noqa: PLR6301 + def _normalize_discriminator_mapping_ref(self, mapping_value: str) -> str:
477-481: Remove unusednoqadirective.The
# noqa: PLW2901directive on line 478 is not needed as the rule is not enabled in your configuration.🔎 Proposed fix
field_obj = self.data_model_field_type(**{ # noqa: PLW2901 + field_obj = self.data_model_field_type(**{
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/datamodel_code_generator/parser/openapi.py(2 hunks)tests/data/expected/main/openapi/discriminator/allof_no_subtypes.py(2 hunks)tests/data/expected/main/openapi/discriminator/no_mapping.py(1 hunks)tests/data/expected/main/openapi/discriminator/no_mapping_no_subtypes.py(1 hunks)tests/data/expected/main/openapi/discriminator/short_mapping_names.py(1 hunks)tests/data/openapi/discriminator_no_mapping.yaml(1 hunks)tests/data/openapi/discriminator_no_mapping_no_subtypes.yaml(1 hunks)tests/data/openapi/discriminator_short_mapping_names.yaml(1 hunks)tests/main/openapi/test_main_openapi.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/data/openapi/discriminator_short_mapping_names.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/main/openapi/test_main_openapi.py
- tests/data/expected/main/openapi/discriminator/allof_no_subtypes.py
🧰 Additional context used
🧬 Code graph analysis (3)
src/datamodel_code_generator/parser/openapi.py (3)
src/datamodel_code_generator/__main__.py (1)
get(112-114)src/datamodel_code_generator/reference.py (1)
get(831-833)src/datamodel_code_generator/types.py (1)
DataType(285-619)
tests/data/expected/main/openapi/discriminator/no_mapping.py (1)
tests/data/expected/main/openapi/discriminator/no_mapping_no_subtypes.py (3)
BaseItem(10-11)FooItem(14-15)ItemContainer(18-19)
tests/data/expected/main/openapi/discriminator/short_mapping_names.py (3)
tests/data/expected/main/openapi/discriminator/allof_no_subtypes.py (4)
BaseItem(12-13)FooItem(16-18)BarItem(21-23)ItemContainer(26-27)tests/data/expected/main/openapi/discriminator/no_mapping.py (4)
BaseItem(12-13)FooItem(16-18)BarItem(21-23)ItemContainer(26-27)tests/data/expected/main/openapi/discriminator/no_mapping_no_subtypes.py (3)
BaseItem(10-11)FooItem(14-15)ItemContainer(18-19)
🪛 Checkov (3.2.334)
tests/data/openapi/discriminator_no_mapping_no_subtypes.yaml
[high] 1-40: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-40: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
tests/data/openapi/discriminator_no_mapping.yaml
[high] 1-49: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-49: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
🪛 Ruff (0.14.8)
src/datamodel_code_generator/parser/openapi.py
398-398: Unused noqa directive (non-enabled: PLR6301)
Remove unused noqa directive
(RUF100)
478-478: Unused noqa directive (non-enabled: PLW2901)
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). (8)
- GitHub Check: py312-isort7 on Ubuntu
- GitHub Check: 3.11 on Windows
- GitHub Check: 3.14 on Ubuntu
- GitHub Check: 3.12 on Windows
- GitHub Check: 3.10 on Windows
- GitHub Check: 3.11 on macOS
- GitHub Check: Analyze (python)
- GitHub Check: benchmarks
🔇 Additional comments (9)
tests/data/openapi/discriminator_no_mapping.yaml (1)
1-47: LGTM! Test fixture is well-structured.This OpenAPI fixture correctly defines a discriminator scenario without explicit mapping, which tests the parser's ability to infer subtypes from allOf references. The structure is appropriate for testing discriminator normalization logic.
Note: The static analysis security warnings (CKV_OPENAPI_4, CKV_OPENAPI_5) are false positives for test fixtures, as these are not production API specifications.
tests/data/openapi/discriminator_no_mapping_no_subtypes.yaml (1)
1-38: LGTM! Appropriate test fixture for edge case.This fixture correctly tests the scenario where a discriminator is defined but no allOf subtypes exist. FooItem is defined as a standalone object without extending BaseItem via allOf, which exercises the fallback logic in
_get_discriminator_union_type.Note: The static analysis security warnings are false positives for test fixtures.
tests/data/expected/main/openapi/discriminator/no_mapping_no_subtypes.py (1)
1-19: LGTM! Generated output correctly represents the fixture.The generated models appropriately reflect the
no_mapping_no_subtypes.yamlfixture structure where FooItem is defined independently (not via allOf), and the discriminator is applied to the field in ItemContainer. This is the expected behavior for Pydantic v2 discriminated unions.src/datamodel_code_generator/parser/openapi.py (4)
398-418: Verify the edge case for dotted filenames without path separators.The docstring states that bare file references like
"other.yaml"will be treated as schema names and require"./other.yaml"format for file references. This could be counterintuitive since.yamlsuggests a file. Consider whether this behavior might catch users by surprise.However, the logic is well-documented and consistent with treating any value without "/" or "#" as a short schema name.
Do you want to add a test case covering the edge case where a mapping value like
"Pet.V1"or"other.yaml"is used to ensure the normalization behaves as expected?
420-428: LGTM! Normalization preserves the original discriminator structure.The method correctly creates a copy of the discriminator dict and only normalizes string mapping values, preserving all other fields intact.
430-448: Excellent fallback logic for discriminators without allOf subtypes.The enhancement to fall back to discriminator mapping values when no allOf subtypes are found addresses the core issue in the PR. This ensures that discriminators work correctly even when schemas don't explicitly use allOf inheritance but have explicit discriminator mappings.
The logic properly:
- Tries allOf subtypes first
- Falls back to normalized mapping values if no subtypes exist
- Returns None if neither approach yields subtypes
477-481: LGTM! Discriminator normalization correctly applied to field extras.The change to store the normalized discriminator (via
_normalize_discriminator) in field extras ensures that short mapping names are converted to full refs before being used by Pydantic. This is critical for correct discriminated union behavior.tests/data/expected/main/openapi/discriminator/short_mapping_names.py (1)
1-27: LGTM! Correctly generated discriminated union with short mapping names.The generated models properly represent a discriminated union where:
- FooItem and BarItem use Literal types for the discriminator field (
itemType)- ItemContainer's
itemfield uses a union type with the discriminator attribute- The structure aligns with Pydantic v2's discriminated union pattern
This validates that the short mapping name normalization (e.g.,
"foo"→"#/components/schemas/FooItem") is working correctly.tests/data/expected/main/openapi/discriminator/no_mapping.py (1)
1-27: LGTM! Correctly generated discriminated union with inheritance.The generated models appropriately use inheritance (FooItem and BarItem extend BaseItem) to reflect the allOf structure in the
discriminator_no_mapping.yamlfixture. The Literal types use the class names ('FooItem','BarItem') as discriminator values, which is the expected behavior when no explicit mapping is provided.This validates that the discriminator inference from allOf references is working correctly.
1 participant
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.