-
Notifications
You must be signed in to change notification settings - Fork 127
Fixes issue with multiple nested fragments on interface and union #501
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request fixes an issue with nested fragments on interfaces in GraphQL code generation. The fix ensures that nested interface implementations are correctly identified and that specialized type fragments are properly generated from superclass selections rather than the entire hierarchy.
Key changes:
- Added validation to check if nested interface names make sense by verifying containment
- Changed loop iteration from hierarchy keys to superclass selection keys to prevent incorrect fragment generation
- Added comprehensive test coverage for nested fragments on interfaces with new GraphQL schema types (Author, Person, Company, Group, Book, Textbook, ColoringBook)
Reviewed Changes
Copilot reviewed 5 out of 19 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
codegen/gql_code_builder/lib/src/utils/field_utils.dart |
Added validation check to ensure nested interface names contain the superName before creating interface implementations |
codegen/gql_code_builder/lib/src/inline_fragment_classes.dart |
Fixed loop to iterate over superclassSelections.keys instead of context.hierarchy.keys to prevent incorrect fragment generation |
codegen/end_to_end_test/test/operation/nested_fragments_on_interface_test.dart |
Added comprehensive test coverage for nested fragment deserialization and serialization |
codegen/end_to_end_test/lib/graphql/schema.graphql |
Extended schema with Author/Book interfaces and their implementations |
| Multiple generated files | Auto-generated code from the new GraphQL schema and fragments |
.gitignore |
Added iOS ephemeral directory to gitignore |
Files not reviewed (11)
- codegen/end_to_end_test/lib/fragments/generated/hero_with_interface_subtyped_fragments.data.gql.dart: Language not supported
- codegen/end_to_end_test/lib/fragments/generated/hero_with_interface_subtyped_fragments.data.gql.g.dart: Language not supported
- codegen/end_to_end_test/lib/fragments/generated/nested_fragments_on_interface.ast.gql.dart: Language not supported
- codegen/end_to_end_test/lib/fragments/generated/nested_fragments_on_interface.req.gql.dart: Language not supported
- codegen/end_to_end_test/lib/fragments/generated/nested_fragments_on_interface.req.gql.g.dart: Language not supported
- codegen/end_to_end_test/lib/fragments/generated/nested_fragments_on_interface.var.gql.dart: Language not supported
- codegen/end_to_end_test/lib/fragments/generated/nested_fragments_on_interface.var.gql.g.dart: Language not supported
- codegen/end_to_end_test/lib/graphql/generated/schema.ast.gql.dart: Language not supported
- codegen/end_to_end_test/lib/graphql/generated/schema.schema.gql.dart: Language not supported
- codegen/end_to_end_test/lib/graphql/generated/serializers.gql.dart: Language not supported
- codegen/end_to_end_test/lib/graphql/generated/serializers.gql.g.dart: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks so much for this! I am currently trying to understand why this would work ;) Oh... void addInterface(String name) {
final parts = name.split("__"); // Split on "__"
final base = parts[0]; // e.g., "GHeroData_hero"
hierarchy.putIfAbsent(base, () => {}); // Create entry for base
if (parts.length > 1) { // If specialized version
hierarchy[base]!.add(name); // Add to base's set
}
}I think the culprit is just the naive split on the first |
|
Hello, I don't have the time right now to explain a little further, but this fix introduced a regression when we use a named Fragment on a specific union type, which select a field which is an interface. I didn't have a simple repro case for now, but I wanted to warn you about this. I didn't look at the |
|
Thanks! I already stumbled upon this or something similar when trying to write more comprehensive regression tests for this - which is why I did not merge this yet. did not have time to look into it further yet |
This PR aims to fix gql-dart/ferry#610.
I don't fully understand the code, so I'm not sure why I needed to use
superclassSelectionsinstead ofcontext.hierarchyininline_fragment_classes.dart, but it seems to work. I've tested it in our codebase and so far I don't see any regression and we can now use the kind of fragments discussed in the issue 🥳 .