Correctly handle nested fragment spreads on interface fields #1735
Conversation
This reverts commit 9be6418.
Summary of ChangesHello @enisdenjo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where nested fragment spreads on interface fields could lead to the incorrect stripping of required base fields due to overly strict isolation logic. The change modifies the field filtering mechanism to be more inclusive, ensuring that essential fields are retained across complex interface and object type combinations, thereby improving data propagation correctness. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The change from every to some in filterBaseSubschema for both objectFieldFilter and interfaceFieldFilter is a logical correction for how isolated fields are detected within type hierarchies. This ensures that a field is identified as 'isolated' if it's defined as computed on any relevant type (e.g., an object type or any of its interfaces), which seems to be the intended behavior for correctly handling computed fields on interfaces. My main concern is the lack of tests, which is acknowledged in the pull request description. It is critical to add comprehensive tests for this logic change to ensure it behaves as expected across different schema configurations and doesn't introduce any regressions, especially for the scenarios with nested fragments and interfaces mentioned in the title.
| const isIsolatedFieldName = allTypes.some((implementingTypeName) => | ||
| isIsolatedField(implementingTypeName, fieldName, isolatedSchemaTypes), | ||
| ); |
There was a problem hiding this comment.
This logic change from every to some is crucial for correctly identifying isolated fields across type hierarchies. As noted in the PR description, this change is not currently covered by tests. Please add tests to verify this new behavior and prevent future regressions, especially for cases involving nested fragments and interfaces.
| const isIsolatedFieldName = allTypes.some((implementingTypeName) => | ||
| isIsolatedField(implementingTypeName, fieldName, isolatedSchemaTypes), | ||
| ); |
There was a problem hiding this comment.
Similar to the objectFieldFilter, this logic change from every to some is crucial. It's important to add tests that validate this new filtering logic for interface fields to ensure correctness and prevent regressions.
🚀 Snapshot Release (
|
| Package | Version | Info |
|---|---|---|
@graphql-tools/batch-delegate |
10.0.6-alpha-a5e9ca8108a6c2bc82240bca7f1c84a85505b01f |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/delegate |
12.0.0-alpha-a5e9ca8108a6c2bc82240bca7f1c84a85505b01f |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/federation |
4.2.4-alpha-a5e9ca8108a6c2bc82240bca7f1c84a85505b01f |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/fusion-runtime |
1.6.0-alpha-a5e9ca8108a6c2bc82240bca7f1c84a85505b01f |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/gateway |
2.1.20-alpha-a5e9ca8108a6c2bc82240bca7f1c84a85505b01f |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/nestjs |
2.0.25-alpha-a5e9ca8108a6c2bc82240bca7f1c84a85505b01f |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/plugin-aws-sigv4 |
2.0.18-alpha-a5e9ca8108a6c2bc82240bca7f1c84a85505b01f |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/plugin-opentelemetry |
1.2.2-alpha-a5e9ca8108a6c2bc82240bca7f1c84a85505b01f |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-prometheus |
2.1.6-alpha-a5e9ca8108a6c2bc82240bca7f1c84a85505b01f |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/router-runtime |
1.1.0-alpha-a5e9ca8108a6c2bc82240bca7f1c84a85505b01f |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/gateway-runtime |
2.3.6-alpha-a5e9ca8108a6c2bc82240bca7f1c84a85505b01f |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/stitch |
10.1.4-alpha-a5e9ca8108a6c2bc82240bca7f1c84a85505b01f |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/stitching-directives |
4.0.6-alpha-a5e9ca8108a6c2bc82240bca7f1c84a85505b01f |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/gateway-testing |
2.0.6-alpha-a5e9ca8108a6c2bc82240bca7f1c84a85505b01f |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/wrap |
11.1.0-alpha-a5e9ca8108a6c2bc82240bca7f1c84a85505b01f |
npm ↗︎ unpkg ↗︎ |
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Bun Docker Image)The latest changes of this PR are available as image on GitHub Container Registry (based on the declared |
🚀 Snapshot Release (Node Docker Image)The latest changes of this PR are available as image on GitHub Container Registry (based on the declared |
2 participants
Adjusted filtering logic to ensure key fields propagate correctly for interface/object combinations, preventing computed-field isolation from stripping required base fields.
TODO