Fix MRO ordering for multiple inheritance in GraphQL and JSON Schema/OpenAPI#2941
Fix MRO ordering for multiple inheritance in GraphQL and JSON Schema/OpenAPI#2941
Conversation
|
Warning Rate limit exceeded@koxudaxi has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 24 minutes and 8 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 (1)
📝 WalkthroughWalkthroughAdds MRO-safe base-class ordering: interface sorting in GraphQL parser and a general base-class sorting pass; updates code generation to use sorted interfaces; adds expected fixtures and tests (some duplicated) for GraphQL interface MRO and union class-name-prefix, plus related JSON Schema fixtures and a new test. Changes
Sequence Diagram(s)(Skipped — changes are internal sorting and ordering logic without a multi-component sequential interaction requiring diagramming.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
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 |
|
📚 Docs Preview: https://pr-2941.datamodel-code-generator.pages.dev |
CodSpeed Performance ReportMerging #2941 will not alter performanceComparing
|
![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
🤖 Fix all issues with AI Agents
In @tests/main/graphql/test_main_graphql.py:
- Around line 817-839: The test test_main_graphql_interface_mro references a
missing input GRAPHQL file (GRAPHQL_DATA_PATH / "interface_mro.graphql"); add
that file under tests/data/graphql/interface_mro.graphql containing a GraphQL
schema that defines the interface inheritance used by the expected output (e.g.,
interfaces Node and Notification where Notification extends Node and a type that
implements both) so the generator can produce the expected interface MRO
ordering; ensure the schema names match those used in tests and the expected
file interface_mro.py.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
tests/data/graphql/interface_mro.graphqlis excluded by!tests/data/**/*.graphqland included by nonetests/data/graphql/union_class_name_prefix.graphqlis excluded by!tests/data/**/*.graphqland included by none
📒 Files selected for processing (4)
src/datamodel_code_generator/parser/graphql.pytests/data/expected/main/graphql/interface_mro.pytests/data/expected/main/graphql/union_class_name_prefix.pytests/main/graphql/test_main_graphql.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/main/graphql/test_main_graphql.py (2)
tests/main/conftest.py (2)
output_file(99-101)run_main_and_assert(245-409)tests/test_main_kr.py (1)
output_file(44-46)
🔇 Additional comments (5)
src/datamodel_code_generator/parser/graphql.py (2)
424-466: Well-implemented MRO sorting algorithm.The topological sort approach is correct for ensuring valid Python MRO. The algorithm properly:
- Returns early for trivial cases (0-1 interfaces)
- Builds ancestor relationships considering only interfaces in the current list
- Sorts so that interfaces with more "descendants" (interfaces that inherit from them) come later
One minor observation: the nested
get_ancestorsfunction uses BFS-style traversal which correctly handles multi-level inheritance hierarchies.
493-496: Clean integration of MRO sorting.The sorting is correctly applied before building the
base_classeslist, ensuring the reference order matches the sorted interface order.tests/data/expected/main/graphql/union_class_name_prefix.py (1)
1-67: Valid expected output for union class name prefix test.The generated file correctly demonstrates that the
--class-name-prefix Foooption is applied to:
- Scalar type aliases (
FooBoolean,FooID, etc.)- Interface/base class (
FooIResource)- Concrete types (
FooCar,FooEmployee)- Union type (
FooResource) and its member referencestests/data/expected/main/graphql/interface_mro.py (2)
50-56: Correct MRO ordering for multiple interface inheritance.The class declaration
CustomerNeedNotification(Entity, Notification, Node)demonstrates the fix working correctly:
NotificationextendsNode(line 42), soNotificationmust appear beforeNodein the base class listEntityis independent (extends onlyBaseModel), so its position is flexible- This ordering produces a valid Python MRO, fixing the original issue #2938
1-56: Well-structured expected output file.The generated models correctly demonstrate:
- Scalar type aliases with docstrings
- Independent interface (
Entity)- Interface hierarchy (
Node→Notification)- Concrete type with multiple interface inheritance using correct MRO ordering
koxudaxi
force-pushed
the
fix/graphql-mro-sorting
branch
from
a49a698 to
67e563b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/datamodel_code_generator/parser/graphql.py (1)
424-424: Remove unusednoqadirective.The static analysis indicates that
PLR6301is not enabled in your linter configuration, making thisnoqacomment unnecessary.🔎 Suggested fix
- def _sort_interfaces_for_mro( # noqa: PLR6301 + def _sort_interfaces_for_mro( self, interfaces: list[graphql.GraphQLInterfaceType], ) -> list[graphql.GraphQLInterfaceType]:
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
tests/data/graphql/interface_mro.graphqlis excluded by!tests/data/**/*.graphqland included by nonetests/data/graphql/union_class_name_prefix.graphqlis excluded by!tests/data/**/*.graphqland included by none
📒 Files selected for processing (4)
src/datamodel_code_generator/parser/graphql.pytests/data/expected/main/graphql/interface_mro.pytests/data/expected/main/graphql/union_class_name_prefix.pytests/main/graphql/test_main_graphql.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/main/graphql/test_main_graphql.py
🧰 Additional context used
🪛 Ruff (0.14.10)
src/datamodel_code_generator/parser/graphql.py
424-424: Unused noqa directive (non-enabled: PLR6301)
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). (16)
- GitHub Check: py312-isort7 on Ubuntu
- GitHub Check: py312-isort6 on Ubuntu
- GitHub Check: 3.10 on Windows
- GitHub Check: 3.11 on macOS
- GitHub Check: 3.11 on Windows
- GitHub Check: py312-black23 on Ubuntu
- GitHub Check: py312-black24 on Ubuntu
- GitHub Check: 3.12 on macOS
- GitHub Check: 3.12 on Windows
- GitHub Check: 3.10 on Ubuntu
- GitHub Check: 3.13 on Ubuntu
- GitHub Check: 3.14 on Windows
- GitHub Check: 3.11 on Ubuntu
- GitHub Check: 3.13 on Windows
- GitHub Check: Analyze (python)
- GitHub Check: benchmarks
🔇 Additional comments (5)
tests/data/expected/main/graphql/union_class_name_prefix.py (1)
1-67: LGTM!This test fixture correctly demonstrates the union class name prefix feature. The single inheritance hierarchy (
FooIResource→FooCar/FooEmployee) is straightforward with no MRO concerns. Field aliases properly map GraphQL camelCase to Python snake_case conventions.src/datamodel_code_generator/parser/graphql.py (2)
424-466: Well-designed MRO sorting algorithm.The algorithm correctly ensures subclass interfaces precede parent interfaces by counting ancestor relationships. The approach of sorting by "how many others have this as an ancestor" elegantly handles the MRO constraint. Using name as a tie-breaker ensures deterministic output.
493-496: LGTM!Clean integration of the MRO sorting. Converting interfaces to a list and applying the sort before building base class references correctly addresses the MRO issue with minimal code change.
tests/data/expected/main/graphql/interface_mro.py (2)
50-56: LGTM - Correct MRO ordering demonstrated.The base class order
(Entity, Notification, Node)is valid for Python's MRO.Notificationcorrectly precedesNode(its parent), which is exactly what the_sort_interfaces_for_mrofix ensures. WhileNodeappears redundant sinceNotificationalready extends it, this reflects the GraphQL schema where all interfaces are explicitly listed, and Python handles it correctly.
1-48: LGTM!The scalar definitions and interface hierarchy (
Entity,Node,Notification) correctly set up the test case for validating MRO ordering. The inheritance chainNotification(Node)is the key relationship that exercises the sorting algorithm.
koxudaxi
force-pushed
the
fix/graphql-mro-sorting
branch
3 times, most recently
from
180ba04 to
a0a27c4
Compare
koxudaxi
force-pushed
the
fix/graphql-mro-sorting
branch
from
a0a27c4 to
76b9070
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/datamodel_code_generator/parser/graphql.py (1)
424-424: Remove unusednoqadirective.The
PLR6301rule is not enabled in your linting configuration.🔎 Proposed fix
- def _sort_interfaces_for_mro( # noqa: PLR6301 + def _sort_interfaces_for_mro( self, interfaces: list[graphql.GraphQLInterfaceType], ) -> list[graphql.GraphQLInterfaceType]:
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
tests/data/graphql/interface_mro.graphqlis excluded by!tests/data/**/*.graphqland included by nonetests/data/graphql/union_class_name_prefix.graphqlis excluded by!tests/data/**/*.graphqland included by none
📒 Files selected for processing (4)
src/datamodel_code_generator/parser/graphql.pytests/data/expected/main/graphql/interface_mro.pytests/data/expected/main/graphql/union_class_name_prefix.pytests/main/graphql/test_main_graphql.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/main/graphql/test_main_graphql.py
- tests/data/expected/main/graphql/union_class_name_prefix.py
🧰 Additional context used
🪛 Ruff (0.14.10)
src/datamodel_code_generator/parser/graphql.py
424-424: Unused noqa directive (non-enabled: PLR6301)
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). (11)
- GitHub Check: py312-isort7 on Ubuntu
- GitHub Check: 3.14 on Windows
- GitHub Check: 3.11 on Windows
- GitHub Check: 3.12 on macOS
- GitHub Check: 3.10 on Windows
- GitHub Check: 3.12 on Windows
- GitHub Check: 3.11 on macOS
- GitHub Check: 3.13 on Windows
- GitHub Check: build-deploy
- GitHub Check: Analyze (python)
- GitHub Check: benchmarks
🔇 Additional comments (6)
src/datamodel_code_generator/parser/graphql.py (2)
424-466: LGTM! Solid MRO sorting implementation.The ancestor-based sorting correctly ensures subclasses precede their parent interfaces:
- Interfaces that are ancestors of more others receive higher
ancestor_countvalues and appear later in the sorted list- This produces valid Python MRO for multiple inheritance scenarios like
class CustomerNeedNotification(Entity, Notification, Node)whereNotification extends NodeThe algorithm handles diamond inheritance and uses stable sorting with a name tiebreaker for deterministic output.
495-496: Remove the verification suggestion; the code already guards against missinginterfaceswithhasattr().The code at lines 495-496 is executed only within the
if hasattr(obj, "interfaces"):guard clause on line 494, which safely handles compatibility across graphql-core versions. No further verification is needed.Likely an incorrect or invalid review comment.
tests/data/expected/main/graphql/interface_mro.py (4)
88-94: LGTM! Correctly ordered bases for MRO.The base class order
(Entity, Notification, Node)correctly places theNotificationsubclass before its parentNode, preventing the "Cannot create a consistent method resolution order" TypeError from issue #2938.
97-104: LGTM! Diamond inheritance correctly resolved.The base class order
(LeftBranch, RightBranch, BaseInterface)properly handles the diamond inheritance pattern where both branches extend the base, placing the common ancestor last.
107-111: LGTM! Linear inheritance chain correctly ordered.The base class order
(Parent, GrandParent)correctly places the subclass before its parent.
114-121: LGTM! Multi-level inheritance correctly ordered.The base class order
(Child, Parent, GrandParent)correctly maintains the inheritance hierarchy with each subclass preceding its parent.
koxudaxi
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/datamodel_code_generator/parser/graphql.py (1)
424-424: Remove unused noqa directive.The
# noqa: PLR6301directive is not needed since PLR6301 is not enabled in the static analysis configuration.🔎 Proposed fix
- def _sort_interfaces_for_mro( # noqa: PLR6301 + def _sort_interfaces_for_mro( self, interfaces: list[graphql.GraphQLInterfaceType], ) -> list[graphql.GraphQLInterfaceType]:
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
tests/data/graphql/interface_mro.graphqlis excluded by!tests/data/**/*.graphqland included by nonetests/data/graphql/union_class_name_prefix.graphqlis excluded by!tests/data/**/*.graphqland included by nonetests/data/jsonschema/allof_mro.jsonis excluded by!tests/data/**/*.jsonand included by nonetests/data/jsonschema/unknown_format.jsonis excluded by!tests/data/**/*.jsonand included by none
📒 Files selected for processing (8)
src/datamodel_code_generator/parser/base.pysrc/datamodel_code_generator/parser/graphql.pytests/data/expected/main/graphql/interface_mro.pytests/data/expected/main/graphql/union_class_name_prefix.pytests/data/expected/main/jsonschema/allof_mro.pytests/data/expected/main/jsonschema/unknown_format.pytests/main/graphql/test_main_graphql.pytests/main/jsonschema/test_main_jsonschema.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/main/graphql/test_main_graphql.py
- tests/data/expected/main/graphql/interface_mro.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:
tests/data/expected/main/jsonschema/unknown_format.py
🧬 Code graph analysis (1)
src/datamodel_code_generator/parser/base.py (2)
src/datamodel_code_generator/reference.py (3)
reference(77-79)get(1019-1021)add(931-1017)src/datamodel_code_generator/model/base.py (2)
path(920-922)BaseClassDataType(615-616)
🪛 Ruff (0.14.10)
src/datamodel_code_generator/parser/graphql.py
424-424: Unused noqa directive (non-enabled: PLR6301)
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-isort5 on Ubuntu
- GitHub Check: py312-isort7 on Ubuntu
- GitHub Check: py312-black24 on Ubuntu
- GitHub Check: 3.13 on Windows
- GitHub Check: 3.12 on Windows
- GitHub Check: 3.14 on Windows
- GitHub Check: 3.11 on Windows
- GitHub Check: 3.10 on Windows
- GitHub Check: 3.14 on macOS
- GitHub Check: build-deploy
- GitHub Check: benchmarks
- GitHub Check: Analyze (python)
🔇 Additional comments (6)
src/datamodel_code_generator/parser/base.py (2)
488-548: LGTM! MRO sorting logic is correct.The implementation correctly reorders base classes to satisfy Python's C3 linearization. The algorithm counts how many other base classes have each class as an ancestor, placing classes with higher counts (more ancestors) later in the list. This ensures subclasses precede their parents in the inheritance list, preventing "Cannot create a consistent method resolution order" errors.
The use of stable sorting preserves the original order when counts are equal, and the BFS traversal correctly identifies all ancestors within the base class set.
3227-3227: Correct placement of MRO sorting.The call to
sort_base_classes_for_mrois correctly positioned after dependency sorting and before module structure building, ensuring all base classes are properly ordered before code generation begins.src/datamodel_code_generator/parser/graphql.py (2)
424-459: LGTM! Interface sorting correctly implements MRO ordering.The method properly sorts GraphQL interfaces so subclasses precede their parents, preventing MRO errors. Key implementation details:
- Uses BFS to find all ancestor interfaces within the provided set
- Counts how many other interfaces have each interface as an ancestor
- Sorts with
(ancestor_count, name)tuple for deterministic ordering- Returns sorted list that can be directly used for base_classes
This complements the general
sort_base_classes_for_mroin base.py by handling the GraphQL-specific interface ordering during parsing.
488-489: Correct integration of interface sorting.The sorted interfaces are properly used to build the
base_classeslist, ensuring that the inheritance order respects Python's MRO requirements before the models are generated.tests/main/jsonschema/test_main_jsonschema.py (1)
8069-8080: New allOf MRO regression test is correctly wired and aligned with fixture
- Input path, expected filename, and
--use-schema-descriptionflag correctly target the newallof_mro.json/allof_mro.pypair.- This will lock in the desired base-class ordering for the diamond allOf case via golden-file comparison and fits existing benchmark test patterns.
tests/data/expected/main/jsonschema/allof_mro.py (1)
1-31: Expected allOf diamond fixture correctly encodes MRO-safe inheritance
- DiamondType lists the two branch bases before the shared base, giving a consistent Python MRO for the diamond pattern.
- The embedded docstring matches the “diamond inheritance” description the test asserts via
--use-schema-description.- Overall structure and typing are consistent with other JSON Schema expected fixtures.
Breaking Change AnalysisResult: No breaking changes detected Reasoning: This PR fixes a bug where generated code for schemas with multiple inheritance (diamond patterns) would produce invalid Python code that fails with MRO errors. The change reorders base classes to satisfy Python's C3 linearization requirements. While the generated output changes (base class order), this is a bug fix that makes previously broken code work correctly. The old generated code would fail at runtime with "TypeError: Cannot create a consistent method resolution order", so users couldn't use it anyway. No templates, CLI options, APIs, defaults, Python versions, or error handling were changed. 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. |
1 participant
Fixes: #2938
Summary by CodeRabbit
Bug Fixes
Tests
Examples
✏️ Tip: You can customize this high-level summary in your review settings.