Fix GraphQL parser to handle renamed objects correctly#2670
Fix GraphQL parser to handle renamed objects correctly#2670koxudaxi merged 10 commits intokoxudaxi:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2670 +/- ##
==========================================
- Coverage 99.61% 99.50% -0.11%
==========================================
Files 76 76
Lines 10648 10728 +80
Branches 1300 1314 +14
==========================================
+ Hits 10607 10675 +68
- Misses 21 32 +11
- Partials 20 21 +1
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 #2670 will not alter performanceComparing Summary
Footnotes
|
| self.references[type_.name] = Reference( | ||
| path=f"{paths!s}/{resolved_type.value}/{type_.name}", | ||
| name=type_.name, | ||
| name=self.model_resolver.get_class_name(type_.name, unique=False).name, |
There was a problem hiding this comment.
Could you elaborate:
- the rationale of this change
- Whether
unique=Falseis safe or may impact the code generation
Thanks!
There was a problem hiding this comment.
I just realized this change is not necessary to fix my issue. The rationale was to move the GraphQL parser behaviour more in line with the jsonschema one. I was comparing the field names in the data models after calling parse_raw using the GraphQL parser and the jsonschema parser respectively. I noticed that the field names differed for equivalent schemas, and that the name used in the JSON Schema parser came from this line:
The reason this change is not required is that Parser.__replace_duplicate_name_in_module is called after parsing, which fixes the names anyway. I will undo the change for now; let me know if this is actually desirable.
The unique flag actually doesn't do anything because there are no references at the model_resolver level.
WalkthroughParser change: GraphQL field parsing now resolves named types by consulting the internal reference registry and sets a plain type name when no reference exists. Several GraphQL test fixtures and expected generated outputs were added, removed, or reordered to reflect the resolution and forward-ref updates. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)src/datamodel_code_generator/parser/graphql.py (3)
⏰ 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). (14)
🔇 Additional comments (1)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/datamodel_code_generator/parser/graphql.py(2 hunks)tests/data/expected/main/graphql/casing.py(1 hunks)tests/data/expected/main/graphql/enum_casing.py(0 hunks)tests/data/expected/main/graphql/simple_star_wars.py(1 hunks)tests/data/expected/main/graphql/simple_star_wars_extra_fields_allow.py(1 hunks)tests/data/expected/main/graphql/type_alias.py(1 hunks)tests/data/expected/main/graphql/type_alias_py312.py(1 hunks)tests/data/expected/parser/graphql/union-aliased-bug.py(1 hunks)tests/data/expected/parser/graphql/union-commented.py(1 hunks)tests/data/graphql/casing.graphql(1 hunks)tests/data/graphql/enum-casing.graphql(0 hunks)tests/main/graphql/test_main_graphql.py(1 hunks)
💤 Files with no reviewable changes (2)
- tests/data/graphql/enum-casing.graphql
- tests/data/expected/main/graphql/enum_casing.py
🧰 Additional context used
🧬 Code graph analysis (8)
tests/main/graphql/test_main_graphql.py (2)
tests/main/conftest.py (2)
output_file(94-96)run_main_and_assert(196-352)tests/test_main_kr.py (1)
output_file(37-39)
tests/data/expected/parser/graphql/union-commented.py (1)
tests/data/expected/parser/graphql/union-aliased-bug.py (2)
Resource(44-46)UserMetadata(31-35)
tests/data/expected/main/graphql/type_alias.py (1)
tests/data/expected/main/graphql/type_alias_py312.py (1)
ModelWithTypeAliasField(50-56)
src/datamodel_code_generator/parser/graphql.py (2)
src/datamodel_code_generator/model/base.py (1)
name(583-585)src/datamodel_code_generator/reference.py (3)
get_class_name(860-895)reference(77-79)get(843-845)
tests/data/expected/main/graphql/simple_star_wars.py (1)
tests/data/expected/main/graphql/simple_star_wars_dataclass.py (2)
Film(37-55)Person(59-79)
tests/data/expected/main/graphql/simple_star_wars_extra_fields_allow.py (4)
tests/data/expected/main/graphql/simple_star_wars.py (2)
Film(36-54)Person(57-77)tests/data/expected/main/graphql/simple_star_wars_dataclass.py (2)
Film(37-55)Person(59-79)tests/data/expected/main/graphql/type_alias.py (1)
Person(33-36)tests/data/expected/main/graphql/type_alias_py312.py (1)
Person(32-35)
tests/data/expected/main/graphql/casing.py (1)
src/datamodel_code_generator/model/type_alias.py (1)
TypeAlias(37-42)
tests/data/expected/parser/graphql/union-aliased-bug.py (1)
tests/data/expected/parser/graphql/union-commented.py (2)
Resource(64-66)UserMetadata(35-45)
⏰ 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). (14)
- GitHub Check: 3.9 on Ubuntu
- GitHub Check: 3.10 on Windows
- GitHub Check: 3.10 on Ubuntu
- GitHub Check: 3.11 on Windows
- GitHub Check: py312-black22 on Ubuntu
- GitHub Check: 3.9 on Windows
- GitHub Check: 3.10 on macOS
- GitHub Check: py312-isort7 on Ubuntu
- GitHub Check: py312-isort6 on Ubuntu
- GitHub Check: 3.12 on Windows
- GitHub Check: 3.9 on macOS
- GitHub Check: 3.11 on macOS
- GitHub Check: benchmarks
- GitHub Check: Analyze (python)
🔇 Additional comments (10)
tests/data/expected/parser/graphql/union-commented.py (1)
62-66: LGTM—Resource class correctly placed after its dependencies.The Resource class is now generated after UserMetadata (which it depends on), aligning with the PR's improvement to dependency ordering. The class definition itself is unchanged.
src/datamodel_code_generator/parser/graphql.py (2)
330-330: Core fix: Use resolved class names for references.This change addresses the reported bug where renamed GraphQL types weren't properly referenced. By using
model_resolver.get_class_name(type_.name, unique=False).name, the reference now stores the resolved Python class name (e.g., "Lowercasetype") instead of the raw GraphQL name (e.g., "lowercasetype").Regarding
unique=False: This is safe here because we're establishing the initial canonical name during reference registration in_resolve_types. Theunique=Falseparameter tells the resolver to apply naming conventions (PascalCase, etc.) without enforcing global uniqueness yet—uniqueness will be enforced later when actual model instances are created and added to the results. This avoids premature collisions during the initial type discovery phase.Response to past review comment: The
unique=Falseparameter is appropriate because this is the initial reference registration phase. Uniqueness enforcement happens later when models are instantiated via_create_data_model, where the reference is already established with the correctly cased name.
489-490: Essential fix: Wire up existing references before defaulting to primitives.This change ensures that non-enum fields correctly use references from the
self.referencesregistry (populated during_resolve_types) before falling back to treating the type as a primitive. Without this, field annotations would use raw GraphQL type names even when renamed class references exist, causing the undefined type errors described in issue #2669.tests/data/expected/parser/graphql/union-aliased-bug.py (1)
42-46: LGTM—Improved dependency ordering.The Resource class is now generated after its dependency (UserMetadata), consistent with the PR's dependency ordering improvements.
tests/data/expected/main/graphql/type_alias.py (1)
49-57: LGTM—ModelWithTypeAliasField correctly ordered after its dependencies.The class is now generated after the type aliases (SimpleString, String, UnionType) it depends on, consistent with improved dependency ordering.
tests/data/expected/main/graphql/type_alias_py312.py (1)
48-56: LGTM—Consistent with type_alias.py relocation.Same dependency ordering improvement as the non-py312 variant.
tests/data/graphql/casing.graphql (1)
1-24: Excellent test coverage for the casing bug.This schema comprehensively tests the scenarios addressed by the PR:
- Lowercase enum and type names requiring capitalization (lines 1-7)
- Name collision handling with "conflict" vs "Conflict" (lines 9-17)
- References to renamed types (lines 19-24)
This test case directly validates that the parser correctly generates renamed class names in field annotations, which was the core issue in #2669.
tests/data/expected/main/graphql/casing.py (1)
54-59: Critical validation: Field annotations correctly use renamed class names.This Ref class demonstrates the fix is working correctly:
- Line 55:
bar: Lowercase(notlowercase)- Line 56:
baz: Lowercasetype(notlowercasetype)- Line 58:
spam: ConflictModel(notconflict)Before the fix, these fields would reference the raw GraphQL names (e.g.,
lowercasetype), causing undefined type errors. The fix ensures fields use the actual Python class names generated by the parser.tests/data/expected/main/graphql/simple_star_wars_extra_fields_allow.py (1)
174-177: LGTM—Explicit forward reference resolution for circular dependencies.Adding
update_forward_refs()calls for Film and Person is appropriate since they have circular references (Film → List[Person] and Person → List[Film]). This ensures Pydantic properly resolves the forward-referenced types at runtime.tests/data/expected/main/graphql/simple_star_wars.py (1)
156-159: LGTM!The
update_forward_refs()calls correctly resolve the circular dependencies betweenFilmandPersonclasses. This is required for Pydantic models with forward references and aligns with the pattern used in related test expectations.
|
I further changed the references logic to remove the special handling for enums as now every type gets a reference. This causes the coverage to drop because the "else" branch is now unreachable in all tests. Should I exclude that branch from testing or create a test case with a schema like this? type Query {
foo: bar!
}
type bar {
baz: Query!
}This will hit the else branch but this kind of schema is currently not handled correctly. (Query will be undefined) |
|
@siminn-arnorgj |
3 participants
Summary
graphql.pyto use model_resolver.get_class_name rather than the naive type name. This aligns more closely with how the type name is set on the references in the model_resolver and fixes the bug. (Looking more closely at the code, I think graphql.py should be using the reference system on the model_resolver like the other parsers, but I'm not familiar enough with the codebase to be sure that is the right approach).parse_field, non-enums now pick up their reference if one exists, so field annotations use the renamed class names instead of using the raw GraphQL name.Fixes: #2669
Summary by CodeRabbit
Bug Fixes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.