feat(composition): support @provides on fields returning a Union type#3026
Open
Aenimus wants to merge 1 commit into
Open
feat(composition): support @provides on fields returning a Union type#3026Aenimus wants to merge 1 commit into
Aenimus wants to merge 1 commit into
Conversation
Contributor
WalkthroughThis PR adds ChangesProvides validation update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (72.97%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #3026 +/- ##
==========================================
- Coverage 46.53% 42.97% -3.56%
==========================================
Files 1120 861 -259
Lines 154966 124930 -30036
Branches 10060 10063 +3
==========================================
- Hits 72107 53692 -18415
+ Misses 81025 70893 -10132
+ Partials 1834 345 -1489
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
composition/src/v1/normalization/normalization-factory.ts (1)
2061-2122: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftValidate inline fragments by possible-type overlap
composition/src/v1/normalization/normalization-factory.ts:2061-2122The kind-based checks here miss GraphQL’s overlap rule: interface fragments on a union parent are rejected even when they share concrete members, and union type conditions are accepted unconditionally even when they are disjoint. Replace the shortcut logic with a possible-type intersection check for the parent and fragment type condition.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@composition/src/v1/normalization/normalization-factory.ts` around lines 2061 - 2122, The inline fragment validation in normalization-factory is using kind-specific shortcuts instead of GraphQL possible-type overlap, so update the logic around the fragmentNamedTypeData switch to compare the parent and typeCondition by intersecting their possible concrete types. In the fragment handling path near the existing parentDatas/shouldDefineSelectionSet flow, keep the existing error helpers but replace the interface/union/object acceptance rules with a shared overlap check for parentData and fragmentNamedTypeData, and only allow the fragment when their possible types intersect; otherwise fall through to invalidInlineFragmentTypeConditionErrorMessage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@composition/src/errors/errors.ts`:
- Around line 779-781: Update the incompatible `@provides` error message in the
error helper that builds the response-type validation error so it includes Union
as a valid kind alongside Object and Interface, matching the new
VALID_PROVIDES_PARENT_KINDS behavior. While touching that return in errors.ts,
collapse the message construction into a single template literal instead of
concatenating strings, keeping the same fieldCoords, subgraphName, and
responseType placeholders.
In `@composition/tests/v1/directives/provides.test.ts`:
- Around line 1242-1268: The union-returning `@provides` tests only verify that
warnings are empty, so they do not prove the directive is preserved in the
composed result. Update the relevant tests around the
createSubgraph/federateSubgraphsSuccess flow to assert the composed output for
the union parent path, especially subgraphConfigBySubgraphName and the
conditional-field data produced for the `@provides` selection. Keep the existing
no-warning check, but add assertions that the composed config still contains the
union-field `@provides` behavior for the valid union cases.
---
Outside diff comments:
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Around line 2061-2122: The inline fragment validation in normalization-factory
is using kind-specific shortcuts instead of GraphQL possible-type overlap, so
update the logic around the fragmentNamedTypeData switch to compare the parent
and typeCondition by intersecting their possible concrete types. In the fragment
handling path near the existing parentDatas/shouldDefineSelectionSet flow, keep
the existing error helpers but replace the interface/union/object acceptance
rules with a shared overlap check for parentData and fragmentNamedTypeData, and
only allow the fragment when their possible types intersect; otherwise fall
through to invalidInlineFragmentTypeConditionErrorMessage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 73b11a7c-85b6-4368-8689-78f7025b2741
📒 Files selected for processing (8)
composition/src/ast/utils.tscomposition/src/errors/errors.tscomposition/src/schema-building/types/types.tscomposition/src/utils/string-constants.tscomposition/src/v1/normalization/normalization-factory.tscomposition/src/v1/normalization/types/results.tscomposition/src/v1/normalization/types/types.tscomposition/tests/v1/directives/provides.test.ts
Comment on lines
+779
to
+781
| return new Error( | ||
| ` A "@provides" directive is declared on field "${fieldCoords}" in subgraph "${subgraphName}".\n` + | ||
| ` However, the response type "${responseType}" is not an Object nor Interface.` | ||
| ` However, the response type "${responseType}" is not an Object nor Interface.`, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Update the incompatible @provides error text for union support.
This helper still tells users that only Object and Interface return types are valid, but the new VALID_PROVIDES_PARENT_KINDS path now accepts Union as well. Folding this into one template literal also matches the TS guideline. As per coding guidelines, "Use template literals instead of string concatenation."
Suggested fix
return new Error(
- ` A "`@provides`" directive is declared on field "${fieldCoords}" in subgraph "${subgraphName}".\n` +
- ` However, the response type "${responseType}" is not an Object nor Interface.`,
+ ` A "`@provides`" directive is declared on field "${fieldCoords}" in subgraph "${subgraphName}".\n` +
+ ` However, the response type "${responseType}" is not an Object, Interface, or Union.`,
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return new Error( | |
| ` A "@provides" directive is declared on field "${fieldCoords}" in subgraph "${subgraphName}".\n` + | |
| ` However, the response type "${responseType}" is not an Object nor Interface.` | |
| ` However, the response type "${responseType}" is not an Object nor Interface.`, | |
| return new Error( | |
| ` A "`@provides`" directive is declared on field "${fieldCoords}" in subgraph "${subgraphName}".\n` + | |
| ` However, the response type "${responseType}" is not an Object, Interface, or Union.`, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@composition/src/errors/errors.ts` around lines 779 - 781, Update the
incompatible `@provides` error message in the error helper that builds the
response-type validation error so it includes Union as a valid kind alongside
Object and Interface, matching the new VALID_PROVIDES_PARENT_KINDS behavior.
While touching that return in errors.ts, collapse the message construction into
a single template literal instead of concatenating strings, keeping the same
fieldCoords, subgraphName, and responseType placeholders.
Source: Coding guidelines
Comment on lines
+1242
to
+1268
| test('that provides on a field that returns a Union is valid #1', () => { | ||
| const subgraphA = createSubgraph( | ||
| 'a', | ||
| ` | ||
| type Query { | ||
| union: Union @provides(fields: "... on Entity { name }") | ||
| } | ||
|
|
||
| type Entity @key(fields: "id") { | ||
| id: ID! | ||
| name: String! @external @shareable | ||
| } | ||
|
|
||
| union Union = Entity | ||
| `, | ||
| ); | ||
| const subgraphB = createSubgraph( | ||
| 'b', | ||
| ` | ||
| type Entity @key(fields: "id") { | ||
| id: ID! | ||
| name: String! @shareable | ||
| } | ||
| `, | ||
| ); | ||
| const { warnings } = federateSubgraphsSuccess([subgraphA, subgraphB], ROUTER_COMPATIBILITY_VERSION_ONE); | ||
| expect(warnings).toHaveLength(0); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert the composed output, not just “no warnings.”
Both new “valid” union cases only check warnings.length === 0. These tests would still pass if the new union-parent path merely stopped rejecting @provides but then dropped it from the composed config. Please assert the relevant subgraphConfigBySubgraphName / conditional-field data so the PR actually locks in union-returning @provides behavior.
Also applies to: 1271-1345
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@composition/tests/v1/directives/provides.test.ts` around lines 1242 - 1268,
The union-returning `@provides` tests only verify that warnings are empty, so they
do not prove the directive is preserved in the composed result. Update the
relevant tests around the createSubgraph/federateSubgraphsSuccess flow to assert
the composed output for the union parent path, especially
subgraphConfigBySubgraphName and the conditional-field data produced for the
`@provides` selection. Keep the existing no-warning check, but add assertions that
the composed config still contains the union-field `@provides` behavior for the
valid union cases.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
@providesscenarios, including clearer errors for invalid field sets and unsupported parent types.@providescases involving inline fragments, non-member types, and non-external fields.Tests
@providesbehavior across valid and invalid schemas.Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.