fix(composition): change how interface field selections are validated…#3027
fix(composition): change how interface field selections are validated…#3027Aenimus wants to merge 8 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a new ChangesInterface field provides validation
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3027 +/- ##
===========================================
- Coverage 65.50% 35.95% -29.55%
===========================================
Files 335 808 +473
Lines 48753 108084 +59331
Branches 5427 6697 +1270
===========================================
+ Hits 31935 38864 +6929
- Misses 16792 68877 +52085
- Partials 26 343 +317 🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (2)
composition/src/v1/normalization/normalization-factory.ts (1)
1898-1905: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale naming from the method rename.
The comment and surrounding style still read as if this is a boolean-only predicate (
isAnyImplementationFieldExternal), matching the still-unrenamedIsAnyImplementationFieldExternalParamstype intypes/params.ts. Since the method now also has the side effect of populatingconditionalFieldDataByCoords, consider aligning the comment/type name withhandleConditionalImplementationField.🤖 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 1898 - 1905, The naming here is stale and still suggests a pure boolean predicate even though handleConditionalImplementationField now also updates conditionalFieldDataByCoords. Update the nearby comment and the IsAnyImplementationFieldExternalParams type name in types/params.ts to match the renamed behavior, and make sure any references to the old isAnyImplementationFieldExternal wording are aligned with handleConditionalImplementationField.composition/src/v1/normalization/types/params.ts (1)
61-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType name no longer matches its consumer.
IsAnyImplementationFieldExternalParamsreads like a boolean predicate name, but it now backsNormalizationFactory#handleConditionalImplementationField(seenormalization-factory.ts), which has side effects (records conditionalprovidedBydata) beyond just testing externality. Consider renaming to something likeHandleConditionalImplementationFieldParamsfor consistency with the renamed method.🤖 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/types/params.ts` around lines 61 - 67, The parameter type name is outdated and no longer matches its consumer: IsAnyImplementationFieldExternalParams is used by NormalizationFactory#handleConditionalImplementationField, which does more than check externality. Rename the type to something aligned with the new method name, update the corresponding import/usage sites in normalization-factory.ts, and keep the naming consistent across the related normalization params definitions.
🤖 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/v1/normalization/normalization-factory.ts`:
- Around line 1899-1943: The conditional field handling in
handleConditionalImplementationField still adds providedBy entries for
implementation fields marked `@external` even when they are already
unconditionally provided. Update the logic in this method to skip those fields
by checking isUnconditionallyProvided before pushing to
conditionalFieldDataByCoords, so interface fields do not get a spurious
conditional/external ancestor.
In `@composition/tests/v1/directives/provides.test.ts`:
- Around line 1791-2586: Remove the duplicated federation test block that starts
with the Union “provides” cases in provides.test.ts and keep only one copy of
each test. The copied tests with names like “that provides on a field that
returns a Union is valid `#1/`#2” and the related Union error cases are stale
leftovers and should be deleted, since the original versions already exist and
correctly assert the warning behavior. Preserve the newer Interface-related
tests that follow, and verify the remaining tests still reference the same
helpers such as federateSubgraphsSuccess, federateSubgraphsFailure, and
subgraphValidationError.
---
Nitpick comments:
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Around line 1898-1905: The naming here is stale and still suggests a pure
boolean predicate even though handleConditionalImplementationField now also
updates conditionalFieldDataByCoords. Update the nearby comment and the
IsAnyImplementationFieldExternalParams type name in types/params.ts to match the
renamed behavior, and make sure any references to the old
isAnyImplementationFieldExternal wording are aligned with
handleConditionalImplementationField.
In `@composition/src/v1/normalization/types/params.ts`:
- Around line 61-67: The parameter type name is outdated and no longer matches
its consumer: IsAnyImplementationFieldExternalParams is used by
NormalizationFactory#handleConditionalImplementationField, which does more than
check externality. Rename the type to something aligned with the new method
name, update the corresponding import/usage sites in normalization-factory.ts,
and keep the naming consistent across the related normalization params
definitions.
🪄 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: c7fe6dfa-bf4b-48f7-9f6a-ad04ce9bbb81
📒 Files selected for processing (8)
composition/src/errors/errors.tscomposition/src/errors/types/params.tscomposition/src/types/results.tscomposition/src/v1/normalization/normalization-factory.tscomposition/src/v1/normalization/types/params.tscomposition/src/v1/normalization/types/results.tscomposition/src/v1/normalization/types/types.tscomposition/tests/v1/directives/provides.test.ts
| handleConditionalImplementationField({ | ||
| fieldCoordsPath, | ||
| fieldName, | ||
| fieldPath, | ||
| interfaceTypeName, | ||
| isProvides, | ||
| }: IsAnyImplementationFieldExternalParams): boolean { | ||
| const implementationTypeNames = this.concreteTypeNamesByAbstractTypeName.get(interfaceTypeName); | ||
| if (!implementationTypeNames) { | ||
| return false; | ||
| } | ||
|
|
||
| let hasExternalField = false; | ||
| for (const typeName of implementationTypeNames) { | ||
| const data = this.parentDefinitionDataByTypeName.get(typeName); | ||
| if (!isObjectDefinitionData(data)) { | ||
| continue; | ||
| } | ||
|
|
||
| const fieldData = data.fieldDataByName.get(fieldName); | ||
| if (!fieldData) { | ||
| continue; | ||
| } | ||
|
|
||
| if (fieldData.directivesByName.has(EXTERNAL)) { | ||
| if (!isProvides) { | ||
| return true; | ||
| } | ||
|
|
||
| getValueOrDefault( | ||
| this.conditionalFieldDataByCoords, | ||
| `${typeName}.${fieldName}`, | ||
| newConditionalFieldData, | ||
| ).providedBy.push( | ||
| newFieldSetConditionData({ | ||
| fieldCoordinatesPath: [...fieldCoordsPath], | ||
| fieldPath: [...fieldPath], | ||
| }), | ||
| ); | ||
| hasExternalField = true; | ||
| } | ||
| } | ||
|
|
||
| return hasExternalField; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether externalFieldDataBySubgraphName / isUnconditionallyProvided is consulted anywhere
# in handleConditionalImplementationField or its call sites for interface-implementation fields.
rg -n -A5 -B5 'isUnconditionallyProvided' composition/src/v1/normalization/normalization-factory.tsRepository: wundergraph/cosmo
Length of output: 3457
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the function body and its callers to see how `isUnconditionallyProvided`
# is threaded through the normalization flow.
sed -n '1880,1955p' composition/src/v1/normalization/normalization-factory.ts
printf '\n---\n'
sed -n '2010,2085p' composition/src/v1/normalization/normalization-factory.ts
printf '\n---\n'
sed -n '3995,4085p' composition/src/v1/normalization/normalization-factory.ts
printf '\n---\n'
rg -n -A4 -B4 'handleConditionalImplementationField\(' composition/src/v1/normalization/normalization-factory.tsRepository: wundergraph/cosmo
Length of output: 11854
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the second call site and the conditional-field data structures
# to see whether unconditionally-provided fields are excluded elsewhere.
sed -n '2115,2155p' composition/src/v1/normalization/normalization-factory.ts
printf '\n---\n'
rg -n -A4 -B4 'newConditionalFieldData|providedBy|conditionalFieldDataByCoords' composition/src/v1/normalization/normalization-factory.tsRepository: wundergraph/cosmo
Length of output: 4599
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the data model for external field metadata and all places where
# `isUnconditionallyProvided` is read or written.
rg -n -A3 -B3 'isUnconditionallyProvided|externalFieldDataBySubgraphName|newExternalFieldData' composition/src/v1
printf '\n---\n'
sed -n '1,220p' composition/src/v1/schema-building/utils.tsRepository: wundergraph/cosmo
Length of output: 14712
Skip isUnconditionallyProvided here too. handleConditionalImplementationField still records providedBy for implementation fields that are @external but already unconditionally provided, which can leave a spurious conditional/external ancestor on interface fields.
🤖 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 1899
- 1943, The conditional field handling in handleConditionalImplementationField
still adds providedBy entries for implementation fields marked `@external` even
when they are already unconditionally provided. Update the logic in this method
to skip those fields by checking isUnconditionallyProvided before pushing to
conditionalFieldDataByCoords, so interface fields do not get a spurious
conditional/external ancestor.
| 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); | ||
| }); | ||
|
|
||
| test('that provides on a field that returns a Union is valid #2', () => { | ||
| const subgraphA = createSubgraph( | ||
| 'a', | ||
| ` | ||
| extend schema | ||
| @link( | ||
| url: "https://specs.apollo.dev/federation/v2.3" | ||
| import: ["@key", "@shareable", "@external"] | ||
| ) | ||
|
|
||
| type Query { | ||
| media: [Media] @shareable | ||
| } | ||
|
|
||
| union Media = Book | Movie | ||
|
|
||
| type Book @key(fields: "id") { | ||
| id: ID! | ||
| } | ||
|
|
||
| type Movie @key(fields: "id") { | ||
| id: ID! | ||
| } | ||
| `, | ||
| ); | ||
| const subgraphB = createSubgraph( | ||
| 'b', | ||
| ` | ||
| extend schema | ||
| @link( | ||
| url: "https://specs.apollo.dev/federation/v2.3" | ||
| import: ["@key", "@shareable", "@provides", "@external"] | ||
| ) | ||
|
|
||
| type Query { | ||
| media: [Media] @shareable @provides(fields: "... on Book { title }") | ||
| } | ||
|
|
||
| union Media = Book | Movie | ||
|
|
||
| type Book @key(fields: "id") { | ||
| id: ID! | ||
| title: String @external | ||
| } | ||
|
|
||
| type Movie @key(fields: "id") { | ||
| id: ID! | ||
| } | ||
| `, | ||
| ); | ||
| const subgraphC = createSubgraph( | ||
| 'c', | ||
| ` | ||
| extend schema | ||
| @link( | ||
| url: "https://specs.apollo.dev/federation/v2.3" | ||
| import: ["@key", "@shareable"] | ||
| ) | ||
|
|
||
| type Book @key(fields: "id") { | ||
| id: ID! | ||
| title: String @shareable | ||
| } | ||
|
|
||
| type Movie @key(fields: "id") { | ||
| id: ID! | ||
| title: String @shareable | ||
| } | ||
| `, | ||
| ); | ||
| const { subgraphConfigBySubgraphName, warnings } = federateSubgraphsSuccess( | ||
| [subgraphA, subgraphB, subgraphC], | ||
| ROUTER_COMPATIBILITY_VERSION_ONE, | ||
| ); | ||
| expect(warnings).toHaveLength(0); | ||
| }); | ||
|
|
||
| test('that an error is returned if a non-external field is provided through a Union type fragment', () => { | ||
| const subgraphA = createSubgraph( | ||
| 'a', | ||
| ` | ||
| type Query { | ||
| union: Union @provides(fields: "... on Entity { name }") | ||
| } | ||
|
|
||
| type Entity @key(fields: "id") { | ||
| id: ID! | ||
| name: String! @shareable | ||
| } | ||
|
|
||
| union Union = Entity | ||
| `, | ||
| ); | ||
| const subgraphB = createSubgraph( | ||
| 'b', | ||
| ` | ||
| type Entity @key(fields: "id") { | ||
| id: ID! | ||
| name: String! @shareable | ||
| } | ||
| `, | ||
| ); | ||
| const { errors, warnings } = federateSubgraphsFailure([subgraphA, subgraphB], ROUTER_COMPATIBILITY_VERSION_ONE); | ||
| expect(errors).toHaveLength(1); | ||
| expect(errors).toStrictEqual([ | ||
| subgraphValidationError(subgraphA.name, [ | ||
| nonExternalConditionalFieldError({ | ||
| directiveCoords: 'Query.union', | ||
| fieldSet: `... on Entity { name }`, | ||
| directiveName: PROVIDES, | ||
| subgraphName: subgraphA.name, | ||
| targetCoords: 'Entity.name', | ||
| }), | ||
| ]), | ||
| ]); | ||
| expect(warnings).toHaveLength(0); | ||
| }); | ||
|
|
||
| test('that an error is returned if a Union fieldset defines an unknown type fragment', () => { | ||
| const subgraphA = createSubgraph( | ||
| 'a', | ||
| ` | ||
| type Query { | ||
| union: Union @provides(fields: "... on Unknown { name }") | ||
| } | ||
|
|
||
| type Entity @key(fields: "id") { | ||
| id: ID! | ||
| name: String! @shareable | ||
| } | ||
|
|
||
| union Union = Entity | ||
| `, | ||
| ); | ||
| const subgraphB = createSubgraph( | ||
| 'b', | ||
| ` | ||
| type Entity @key(fields: "id") { | ||
| id: ID! | ||
| name: String! @shareable | ||
| } | ||
| `, | ||
| ); | ||
| const { errors, warnings } = federateSubgraphsFailure([subgraphA, subgraphB], ROUTER_COMPATIBILITY_VERSION_ONE); | ||
| expect(errors).toHaveLength(1); | ||
| expect(errors).toStrictEqual([ | ||
| subgraphValidationError(subgraphA.name, [ | ||
| invalidProvidesOrRequiresDirectivesError(PROVIDES, [ | ||
| ` On field "Query.union":\n -` + | ||
| unknownInlineFragmentTypeConditionErrorMessage( | ||
| `... on Unknown { name }`, | ||
| ['Query.union'], | ||
| UNION, | ||
| 'Unknown', | ||
| ), | ||
| ]), | ||
| ]), | ||
| ]); | ||
| expect(warnings).toHaveLength(0); | ||
| }); | ||
|
|
||
| test('that an error is returned if a Union fieldset defines a non-member type fragment', () => { | ||
| const subgraphA = createSubgraph( | ||
| 'a', | ||
| ` | ||
| type Query { | ||
| union: Union @provides(fields: "... on EntityB { name }") | ||
| } | ||
|
|
||
| type EntityA @key(fields: "id") { | ||
| id: ID! | ||
| name: String! @shareable | ||
| } | ||
|
|
||
| type EntityB @key(fields: "id") { | ||
| id: ID! | ||
| } | ||
|
|
||
| union Union = EntityA | ||
| `, | ||
| ); | ||
| const subgraphB = createSubgraph( | ||
| 'b', | ||
| ` | ||
| type EntityA @key(fields: "id") { | ||
| id: ID! | ||
| name: String! @shareable | ||
| } | ||
| `, | ||
| ); | ||
| const { errors, warnings } = federateSubgraphsFailure([subgraphA, subgraphB], ROUTER_COMPATIBILITY_VERSION_ONE); | ||
| expect(errors).toHaveLength(1); | ||
| expect(errors).toStrictEqual([ | ||
| subgraphValidationError(subgraphA.name, [ | ||
| invalidProvidesOrRequiresDirectivesError(PROVIDES, [ | ||
| ` On field "Query.union":\n -` + | ||
| invalidInlineFragmentTypeConditionErrorMessage( | ||
| `... on EntityB { name }`, | ||
| ['Query.union'], | ||
| 'EntityB', | ||
| UNION, | ||
| UNION, | ||
| OBJECT, | ||
| ), | ||
| ]), | ||
| ]), | ||
| ]); | ||
| expect(warnings).toHaveLength(0); | ||
| }); | ||
|
|
||
| test('that an error is returned if a Union fieldset defines a non-member type fragment (in the current subgraph)', () => { | ||
| const subgraphA = createSubgraph( | ||
| 'a', | ||
| ` | ||
| type Query { | ||
| union: Union @provides(fields: "... on EntityB { name }") | ||
| } | ||
|
|
||
| type EntityA @key(fields: "id") { | ||
| id: ID! | ||
| name: String! @shareable | ||
| } | ||
|
|
||
| type EntityB @key(fields: "id") { | ||
| id: ID! | ||
| } | ||
|
|
||
| union Union = EntityA | ||
| `, | ||
| ); | ||
| const subgraphB = createSubgraph( | ||
| 'b', | ||
| ` | ||
| type EntityA @key(fields: "id") { | ||
| id: ID! | ||
| name: String! @shareable | ||
| } | ||
|
|
||
| type EntityB @key(fields: "id") { | ||
| id: ID! | ||
| } | ||
|
|
||
| union Union = EntityA | EntityB | ||
| `, | ||
| ); | ||
| const { errors, warnings } = federateSubgraphsFailure([subgraphA, subgraphB], ROUTER_COMPATIBILITY_VERSION_ONE); | ||
| expect(errors).toHaveLength(1); | ||
| expect(errors).toStrictEqual([ | ||
| subgraphValidationError(subgraphA.name, [ | ||
| invalidProvidesOrRequiresDirectivesError(PROVIDES, [ | ||
| ` On field "Query.union":\n -` + | ||
| invalidInlineFragmentTypeConditionErrorMessage( | ||
| `... on EntityB { name }`, | ||
| ['Query.union'], | ||
| 'EntityB', | ||
| UNION, | ||
| UNION, | ||
| OBJECT, | ||
| ), | ||
| ]), | ||
| ]), | ||
| ]); | ||
| expect(warnings).toHaveLength(0); | ||
| }); | ||
|
|
||
| test('that an error is returned if a field is directly provided on an Interface', () => { | ||
| const subgraphA = createSubgraph( | ||
| 'a', | ||
| ` | ||
| type Query { | ||
| interface: Interface! @provides(fields: "name") | ||
| } | ||
|
|
||
| interface Interface { | ||
| name: String! | ||
| } | ||
|
|
||
| type Object implements Interface { | ||
| name: String! | ||
| } | ||
| `, | ||
| ); | ||
| const { errors, warnings } = federateSubgraphsFailure([subgraphA], ROUTER_COMPATIBILITY_VERSION_ONE); | ||
| expect(errors).toHaveLength(1); | ||
| expect(errors).toStrictEqual([ | ||
| subgraphValidationError(subgraphA.name, [ | ||
| directlyProvidedInterfaceFieldError({ | ||
| directiveCoords: `Query.interface`, | ||
| directiveName: PROVIDES, | ||
| fieldSet: 'name', | ||
| selection: 'interface { name }', | ||
| subgraphName: subgraphA.name, | ||
| targetCoords: 'Interface.name', | ||
| }), | ||
| ]), | ||
| ]); | ||
| expect(warnings).toHaveLength(0); | ||
| }); | ||
|
|
||
| test('that an error is returned if a field is directly provided on a nested Interface (no external ancestor)', () => { | ||
| const subgraphA = createSubgraph( | ||
| 'a', | ||
| ` | ||
| type Query { | ||
| object: Object! @provides(fields: "interface { name }") | ||
| } | ||
|
|
||
| interface Interface { | ||
| name: String! | ||
| } | ||
|
|
||
| type Object implements Interface { | ||
| interface: Interface! | ||
| name: String! | ||
| } | ||
| `, | ||
| ); | ||
| const { errors, warnings } = federateSubgraphsFailure([subgraphA], ROUTER_COMPATIBILITY_VERSION_ONE); | ||
| expect(errors).toHaveLength(1); | ||
| expect(errors).toStrictEqual([ | ||
| subgraphValidationError(subgraphA.name, [ | ||
| directlyProvidedInterfaceFieldError({ | ||
| directiveCoords: `Query.object`, | ||
| directiveName: PROVIDES, | ||
| fieldSet: 'interface { name }', | ||
| selection: 'interface { name }', | ||
| subgraphName: subgraphA.name, | ||
| targetCoords: 'Interface.name', | ||
| }), | ||
| ]), | ||
| ]); | ||
| expect(warnings).toHaveLength(0); | ||
| }); | ||
|
|
||
| test('that an error is returned if a field is directly provided on a nested Interface (with external ancestor)', () => { | ||
| const subgraphA = createSubgraph( | ||
| 'a', | ||
| ` | ||
| type Query { | ||
| objectA: ObjectA! @provides(fields: "interface { name }") | ||
| } | ||
|
|
||
| interface Interface { | ||
| name: String! | ||
| } | ||
|
|
||
| type ObjectA @shareable { | ||
| interface: Interface! @external | ||
| name: String! | ||
| } | ||
|
|
||
| type ObjectB implements Interface @shareable { | ||
| name: String! | ||
| } | ||
| `, | ||
| ); | ||
| const subgraphB = createSubgraph( | ||
| 'b', | ||
| ` | ||
| interface Interface { | ||
| name: String! | ||
| } | ||
|
|
||
| type ObjectA @shareable { | ||
| interface: Interface! | ||
| } | ||
|
|
||
| type ObjectB implements Interface @shareable { | ||
| name: String! | ||
| } | ||
| `, | ||
| ); | ||
| const { warnings } = federateSubgraphsSuccess([subgraphA, subgraphB], ROUTER_COMPATIBILITY_VERSION_ONE); | ||
| expect(warnings).toHaveLength(0); | ||
| }); | ||
|
|
||
| test('that provides on an Interface field returning an Interface can be valid (all implementations provide an @external ancestor)', () => { | ||
| const subgraphA = createSubgraph( | ||
| 'a', | ||
| ` | ||
| extend schema | ||
| @link( | ||
| url: "https://specs.apollo.dev/federation/v2.3" | ||
| import: ["@key", "@shareable", "@external", "@provides"] | ||
| ) | ||
|
|
||
| type Query { | ||
| media: Media @shareable | ||
| book: Book @provides(fields: "animals { ... on Dog { name } }") | ||
| } | ||
|
|
||
| interface Media { | ||
| id: ID! | ||
| } | ||
|
|
||
| interface Animal { | ||
| id: ID! | ||
| } | ||
|
|
||
| type Book implements Media @key(fields: "id") { | ||
| id: ID! | ||
| animals: [Animal] @shareable | ||
| } | ||
|
|
||
| type Dog implements Animal @key(fields: "id") { | ||
| id: ID! @external | ||
| name: String @external | ||
| } | ||
|
|
||
| type Cat implements Animal @key(fields: "id") { | ||
| id: ID! @external | ||
| } | ||
| `, | ||
| ); | ||
| const subgraphB = createSubgraph( | ||
| 'b', | ||
| ` | ||
| extend schema | ||
| @link( | ||
| url: "https://specs.apollo.dev/federation/v2.3" | ||
| import: ["@key", "@shareable", "@provides", "@external"] | ||
| ) | ||
|
|
||
| type Query { | ||
| media: Media @shareable @provides(fields: "animals { id name }") | ||
| } | ||
|
|
||
| interface Media { | ||
| id: ID! | ||
| animals: [Animal] | ||
| } | ||
|
|
||
| interface Animal { | ||
| id: ID! | ||
| name: String | ||
| } | ||
|
|
||
| type Book implements Media { | ||
| id: ID! @shareable | ||
| animals: [Animal] @external | ||
| } | ||
|
|
||
| type Dog implements Animal { | ||
| id: ID! @external | ||
| name: String @external | ||
| } | ||
|
|
||
| type Cat implements Animal { | ||
| id: ID! @external | ||
| name: String @external | ||
| } | ||
| `, | ||
| ); | ||
| const subgraphC = createSubgraph( | ||
| 'c', | ||
| ` | ||
| extend schema | ||
| @link( | ||
| url: "https://specs.apollo.dev/federation/v2.3" | ||
| import: ["@key", "@shareable"] | ||
| ) | ||
|
|
||
| interface Media { | ||
| id: ID! | ||
| animals: [Animal] | ||
| } | ||
|
|
||
| interface Animal { | ||
| id: ID! | ||
| name: String | ||
| } | ||
|
|
||
| type Book implements Media @key(fields: "id") { | ||
| id: ID! | ||
| animals: [Animal] @shareable | ||
| } | ||
|
|
||
| type Dog implements Animal @key(fields: "id") { | ||
| id: ID! | ||
| name: String @shareable | ||
| age: Int | ||
| } | ||
|
|
||
| type Cat implements Animal @key(fields: "id") { | ||
| id: ID! | ||
| name: String @shareable | ||
| age: Int | ||
| } | ||
| `, | ||
| ); | ||
| const { subgraphConfigBySubgraphName, warnings } = federateSubgraphsSuccess( | ||
| [subgraphA, subgraphB, subgraphC], | ||
| ROUTER_COMPATIBILITY_VERSION_ONE, | ||
| ); | ||
| expect(warnings).toHaveLength(0); | ||
| }); | ||
|
|
||
| test('that provides on an Interface field returning an Interface can be valid (only one implementations provides an @external ancestor)', () => { | ||
| const subgraphA = createSubgraph( | ||
| 'a', | ||
| ` | ||
| extend schema | ||
| @link( | ||
| url: "https://specs.apollo.dev/federation/v2.3" | ||
| import: ["@key", "@shareable", "@external", "@provides"] | ||
| ) | ||
|
|
||
| type Query { | ||
| media: Media @shareable | ||
| book: Book @provides(fields: "animals { ... on Dog { name } }") | ||
| } | ||
|
|
||
| interface Media { | ||
| id: ID! | ||
| } | ||
|
|
||
| interface Animal { | ||
| id: ID! | ||
| } | ||
|
|
||
| type Book implements Media @key(fields: "id") { | ||
| id: ID! | ||
| animals: [Animal] @shareable | ||
| } | ||
|
|
||
| type Dog implements Animal @key(fields: "id") { | ||
| id: ID! @external | ||
| name: String @external | ||
| } | ||
|
|
||
| type Cat implements Animal @key(fields: "id") { | ||
| id: ID! @external | ||
| } | ||
| `, | ||
| ); | ||
| const subgraphB = createSubgraph( | ||
| 'b', | ||
| ` | ||
| extend schema | ||
| @link( | ||
| url: "https://specs.apollo.dev/federation/v2.3" | ||
| import: ["@key", "@shareable", "@provides", "@external"] | ||
| ) | ||
|
|
||
| type Query { | ||
| media: Media @shareable @provides(fields: "animals { id name }") | ||
| } | ||
|
|
||
| interface Media { | ||
| id: ID! | ||
| animals: [Animal] | ||
| } | ||
|
|
||
| interface Animal { | ||
| id: ID! | ||
| name: String | ||
| } | ||
|
|
||
| type Book implements Media @shareable { | ||
| id: ID! | ||
| animals: [Animal] | ||
| } | ||
|
|
||
| type Video implements Media { | ||
| id: ID! @shareable | ||
| animals: [Animal] @external | ||
| } | ||
|
|
||
| type Dog implements Animal { | ||
| id: ID! @external | ||
| name: String @external | ||
| } | ||
|
|
||
| type Cat implements Animal { | ||
| id: ID! @external | ||
| name: String @external | ||
| } | ||
| `, | ||
| ); | ||
| const subgraphC = createSubgraph( | ||
| 'c', | ||
| ` | ||
| extend schema | ||
| @link( | ||
| url: "https://specs.apollo.dev/federation/v2.3" | ||
| import: ["@key", "@shareable"] | ||
| ) | ||
|
|
||
| interface Media { | ||
| id: ID! | ||
| animals: [Animal] | ||
| } | ||
|
|
||
| interface Animal { | ||
| id: ID! | ||
| name: String | ||
| } | ||
|
|
||
| type Book implements Media @key(fields: "id") { | ||
| id: ID! | ||
| animals: [Animal] @shareable | ||
| } | ||
|
|
||
| type Video implements Media @key(fields: "id") { | ||
| id: ID! | ||
| animals: [Animal] @shareable | ||
| } | ||
|
|
||
| type Dog implements Animal @key(fields: "id") { | ||
| id: ID! | ||
| name: String @shareable | ||
| age: Int | ||
| } | ||
|
|
||
| type Cat implements Animal @key(fields: "id") { | ||
| id: ID! | ||
| name: String @shareable | ||
| age: Int | ||
| } | ||
| `, | ||
| ); | ||
| const { warnings } = federateSubgraphsSuccess( | ||
| [subgraphA, subgraphB, subgraphC], | ||
| ROUTER_COMPATIBILITY_VERSION_ONE, | ||
| ); | ||
| expect(warnings).toHaveLength(0); | ||
| }); | ||
|
|
||
| test('that an error is returned if no there no implementation type provides an external ancestor for an Interface field', () => { | ||
| const subgraphA = createSubgraph( | ||
| 'a', | ||
| ` | ||
| extend schema | ||
| @link( | ||
| url: "https://specs.apollo.dev/federation/v2.3" | ||
| import: ["@key", "@shareable", "@external", "@provides"] | ||
| ) | ||
|
|
||
| type Query { | ||
| media: Media @shareable | ||
| book: Book @provides(fields: "animals { ... on Dog { name } }") | ||
| } | ||
|
|
||
| interface Media { | ||
| id: ID! | ||
| } | ||
|
|
||
| interface Animal { | ||
| id: ID! | ||
| } | ||
|
|
||
| type Book implements Media @key(fields: "id") { | ||
| id: ID! | ||
| animals: [Animal] @shareable | ||
| } | ||
|
|
||
| type Dog implements Animal @key(fields: "id") { | ||
| id: ID! @external | ||
| name: String @external | ||
| } | ||
|
|
||
| type Cat implements Animal @key(fields: "id") { | ||
| id: ID! @external | ||
| } | ||
| `, | ||
| ); | ||
| const subgraphB = createSubgraph( | ||
| 'b', | ||
| ` | ||
| extend schema | ||
| @link( | ||
| url: "https://specs.apollo.dev/federation/v2.3" | ||
| import: ["@key", "@shareable", "@provides", "@external"] | ||
| ) | ||
|
|
||
| type Query { | ||
| media: Media @shareable @provides(fields: "animals { id name }") | ||
| } | ||
|
|
||
| interface Media { | ||
| id: ID! | ||
| animals: [Animal] | ||
| } | ||
|
|
||
| interface Animal { | ||
| id: ID! | ||
| name: String | ||
| } | ||
|
|
||
| type Book implements Media { | ||
| id: ID! @shareable | ||
| animals: [Animal] @shareable | ||
| } | ||
|
|
||
| type Dog implements Animal { | ||
| id: ID! @external | ||
| name: String @external | ||
| } | ||
|
|
||
| type Cat implements Animal { | ||
| id: ID! @external | ||
| name: String @external | ||
| } | ||
| `, | ||
| ); | ||
| const subgraphC = createSubgraph( | ||
| 'c', | ||
| ` | ||
| extend schema | ||
| @link( | ||
| url: "https://specs.apollo.dev/federation/v2.3" | ||
| import: ["@key", "@shareable"] | ||
| ) | ||
|
|
||
| interface Media { | ||
| id: ID! | ||
| animals: [Animal] | ||
| } | ||
|
|
||
| interface Animal { | ||
| id: ID! | ||
| name: String | ||
| } | ||
|
|
||
| type Book implements Media @key(fields: "id") { | ||
| id: ID! | ||
| animals: [Animal] @shareable | ||
| } | ||
|
|
||
| type Dog implements Animal @key(fields: "id") { | ||
| id: ID! | ||
| name: String @shareable | ||
| age: Int | ||
| } | ||
|
|
||
| type Cat implements Animal @key(fields: "id") { | ||
| id: ID! | ||
| name: String @shareable | ||
| age: Int | ||
| } | ||
| `, | ||
| ); | ||
| const { errors, warnings } = federateSubgraphsFailure( | ||
| [subgraphA, subgraphB, subgraphC], | ||
| ROUTER_COMPATIBILITY_VERSION_ONE, | ||
| ); | ||
| expect(errors).toHaveLength(1); | ||
| expect(errors).toStrictEqual([ | ||
| subgraphValidationError(subgraphB.name, [ | ||
| directlyProvidedInterfaceFieldError({ | ||
| directiveCoords: 'Query.media', | ||
| directiveName: PROVIDES, | ||
| fieldSet: 'animals { id name }', | ||
| selection: 'animals { id }', | ||
| subgraphName: subgraphB.name, | ||
| targetCoords: 'Animal.id', | ||
| }), | ||
| directlyProvidedInterfaceFieldError({ | ||
| directiveCoords: 'Query.media', | ||
| directiveName: PROVIDES, | ||
| fieldSet: 'animals { id name }', | ||
| selection: 'animals { name }', | ||
| subgraphName: subgraphB.name, | ||
| targetCoords: 'Animal.name', | ||
| }), | ||
| ]), | ||
| ]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Duplicate test block breaks the build — leftover pre-replacement tests were not removed.
This block duplicates tests already present at lines 1247-1789 (same test names: "that provides on a field that returns a Union is valid #1/#2", the Union fieldset error tests, etc.), but with incorrect warning-count assertions. The originals correctly expect 1 warning (providesOnUnionWarning is still emitted by unchanged code), while these new copies wrongly expect 0. This exactly matches the CI failures reported in the pipeline logs at lines 1817, 1894, 1935, 1978, 2027, and 2082 ("expected warnings to have length 0 but got 1").
Per the PR's own change summary, this range was meant to replace the federation tests starting with "provides on a field that returns a Union is valid #1" — the corresponding old block (lines 1247-1789) needs to be deleted so only one (correct) copy of each test remains. The genuinely new interface coverage (from "that an error is returned if a field is directly provided on an Interface" onward, ~line 2085) should be kept.
🐛 Suggested fix direction
- (delete lines 1247-1789, the superseded original versions of:
- "that provides on a field that returns a Union is valid `#1`"
- "that provides on a field that returns a Union is valid `#2`"
- "that an error is returned if a non-external field is provided through a Union type fragment"
- "that an error is returned if a Union fields et defines an unknown type fragment"
- "that an error is returned if a Union field set defines a non-member type fragment"
- "that an error is returned if a Union field set defines a non-member type fragment (in the current subgraph)")
+ (keep only the single, correctly-asserted copy of each test — currently the copies at
+ lines 1791-2083 have wrong `warnings` length expectations and should either be removed
+ in favor of the originals, or have their assertions corrected to `toHaveLength(1)`)🧰 Tools
🪛 GitHub Actions: Composition CI / 0_build_test.txt
[error] 1817-1817: AssertionError in @provides directive tests (Federation tests) 'that provides on a field that returns a Union is valid #1': expected warnings toHaveLength(0) but got 1.
[error] 1894-1894: AssertionError in @provides directive tests (Federation tests) 'that provides on a field that returns a Union is valid #2': expected warnings toHaveLength(0) but got 1.
[error] 1935-1935: AssertionError in @provides directive tests (Federation tests) 'that an error is returned if a non-external field is provided through a Union type fragment': expected warnings toHaveLength(0) but got 1.
[error] 1978-1978: AssertionError in @provides directive tests (Federation tests) 'that an error is returned if a Union fieldset defines an unknown type fragment': expected warnings toHaveLength(0) but got 1.
[error] 2027-2027: AssertionError in @provides directive tests (Federation tests) 'that an error is returned if a Union fieldset defines a non-member type fragment': expected warnings toHaveLength(0) but got 1.
[error] 2082-2082: AssertionError in @provides directive tests (Federation tests) 'that an error is returned if a Union fieldset defines a non-member type fragment (in the current subgraph)': expected warnings toHaveLength(0) but got 1.
🪛 GitHub Actions: Composition CI / build_test
[error] 1817-1817: Vitest assertion failed in test '@provides directive tests > Federation tests > that provides on a field that returns a Union is valid #1': expected warnings to have length 0 but got 1.
[error] 1894-1894: Vitest assertion failed in test '@provides directive tests > Federation tests > that provides on a field that returns a Union is valid #2': expected warnings to have length 0 but got 1.
[error] 1935-1935: Vitest assertion failed in test '@provides directive tests > Federation tests > that an error is returned if a non-external field is provided through a Union type fragment': expected warnings to have length 0 but got 1.
[error] 1978-1978: Vitest assertion failed in test '@provides directive tests > Federation tests > that an error is returned if a Union fieldset defines an unknown type fragment': expected warnings to have length 0 but got 1.
[error] 2027-2027: Vitest assertion failed in test '@provides directive tests > Federation tests > that an error is returned if a Union fieldset defines a non-member type fragment': expected warnings to have length 0 but got 1.
[error] 2082-2082: Vitest assertion failed in test '@provides directive tests > Federation tests > that an error is returned if a Union fieldset defines a non-member type fragment (in the current subgraph)': expected warnings to have length 0 but got 1.
🪛 GitHub Check: build_test
[failure] 2082-2082: tests/v1/directives/provides.test.ts > @provides directive tests > Federation tests > that an error is returned if a Union fieldset defines a non-member type fragment (in the current subgraph)
AssertionError: expected [ …(1) ] to have a length of +0 but got 1
- Expected
- Received
- 0
- 1
❯ tests/v1/directives/provides.test.ts:2082:24
[failure] 2027-2027: tests/v1/directives/provides.test.ts > @provides directive tests > Federation tests > that an error is returned if a Union fieldset defines a non-member type fragment
AssertionError: expected [ …(1) ] to have a length of +0 but got 1
- Expected
- Received
- 0
- 1
❯ tests/v1/directives/provides.test.ts:2027:24
[failure] 1978-1978: tests/v1/directives/provides.test.ts > @provides directive tests > Federation tests > that an error is returned if a Union fieldset defines an unknown type fragment
AssertionError: expected [ …(1) ] to have a length of +0 but got 1
- Expected
- Received
- 0
- 1
❯ tests/v1/directives/provides.test.ts:1978:24
[failure] 1935-1935: tests/v1/directives/provides.test.ts > @provides directive tests > Federation tests > that an error is returned if a non-external field is provided through a Union type fragment
AssertionError: expected [ …(1) ] to have a length of +0 but got 1
- Expected
- Received
- 0
- 1
❯ tests/v1/directives/provides.test.ts:1935:24
[failure] 1894-1894: tests/v1/directives/provides.test.ts > @provides directive tests > Federation tests > that provides on a field that returns a Union is valid #2
AssertionError: expected [ …(1) ] to have a length of +0 but got 1
- Expected
- Received
- 0
- 1
❯ tests/v1/directives/provides.test.ts:1894:24
[failure] 1817-1817: tests/v1/directives/provides.test.ts > @provides directive tests > Federation tests > that provides on a field that returns a Union is valid #1
AssertionError: expected [ …(1) ] to have a length of +0 but got 1
- Expected
- Received
- 0
- 1
❯ tests/v1/directives/provides.test.ts:1817:24
🤖 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 1791 - 2586,
Remove the duplicated federation test block that starts with the Union
“provides” cases in provides.test.ts and keep only one copy of each test. The
copied tests with names like “that provides on a field that returns a Union is
valid `#1/`#2” and the related Union error cases are stale leftovers and should be
deleted, since the original versions already exist and correctly assert the
warning behavior. Preserve the newer Interface-related tests that follow, and
verify the remaining tests still reference the same helpers such as
federateSubgraphsSuccess, federateSubgraphsFailure, and subgraphValidationError.
Sources: Linters/SAST tools, Pipeline failures
1 participant
… in a @provides fieldset
Summary by CodeRabbit
New Features
@provideson interface-related field sets.Bug Fixes
@provideschecks so interface implementation cases are handled more accurately.Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.