Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .changeset/shiny-forks-happen.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
"@apollo/query-graphs": patch
---

fix: handle directive conditions on fragments when building query graphs

This fix addresses issues with handling fragments when they specify directive conditions:
* when exploding the types we were not propagating directive conditions
* when processing fragment that specifies super type of an existing type and also specifies directive condition, we were incorrectly preserving the unnecessary type condition. This type condition was problematic as it could be referencing types from supergraph that were not available in the local schema. Instead, we now drop the redundant type condition and only preserve the directives (if specified).
9 changes: 7 additions & 2 deletions query-graphs-js/src/graphPath.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2509,7 +2509,7 @@ function advanceWithOperation<V extends Vertex>(
const optionsByImplems: OpGraphPath<V>[][][] = [];
for (const tName of intersection) {
debug.group(() => `Trying ${tName}`);
const castOp = new FragmentElement(currentType, tName);
const castOp = new FragmentElement(currentType, tName, operation.appliedDirectives);
const implemOptions = advanceSimultaneousPathsWithOperation(
supergraphSchema,
new SimultaneousPathsWithLazyIndirectPaths([path], context, conditionResolver),
Expand Down Expand Up @@ -2548,8 +2548,13 @@ function advanceWithOperation<V extends Vertex>(
const conditionType = supergraphSchema.type(typeName)!;
if (isAbstractType(conditionType) && possibleRuntimeTypes(conditionType).some(t => t.name == currentType.name)) {
debug.groupEnd(() => `${typeName} is a super-type of current type ${currentType}: no edge to take`);
// Operation type condition is applicable on the current type, so the types are already exploded but the
// condition can reference types from the supergraph that are not present in the local subgraph.
//
// If operation has applied directives we need to convert to inline fragment without type condition, otherwise
// we ignore the fragment altogether.
const updatedPath = operation.appliedDirectives.length > 0
? path.add(operation, null, noConditionsResolution, operation.deferDirectiveArgs())
? path.add(operation.withUpdatedTypes(currentType, undefined), null, noConditionsResolution, operation.deferDirectiveArgs())
: path;
return { options: [[ updatedPath ]] };
}
Expand Down
182 changes: 182 additions & 0 deletions query-planner-js/src/__tests__/buildPlan.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7973,3 +7973,185 @@ describe('@requires references external field indirectly', () => {
`);
});
});

describe('handles fragments with directive conditions', () => {
test('fragment with intersecting parent type and directive condition', () => {
const subgraphA = {
typeDefs: gql`
directive @test on FRAGMENT_SPREAD
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a dumb question, but how do we restrict the types that the directive can be assigned to in the query? I assume the answer is "you can't"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK you can only specify the locations so you cannot restrict it by types. Guessing you would need some external linter/validation logic to enforce it by type.

type Query {
i: I
}
interface I {
_id: ID
}
type T1 implements I @key(fields: "id") {
_id: ID
id: ID
}
type T2 implements I @key(fields: "id") {
_id: ID
id: ID
}
`,
name: 'A',
};

const subgraphB = {
typeDefs: gql`
directive @test on FRAGMENT_SPREAD
type Query {
i2s: [I2]
}
interface I2 {
id: ID
title: String
}
type T1 implements I2 @key(fields: "id") {
id: ID
title: String
}
type T2 implements I2 @key(fields: "id") {
id: ID
title: String
}
`,
name: 'B',
};

const [api, queryPlanner] = composeAndCreatePlanner(subgraphA, subgraphB);

const operation = operationFromDocument(
api,
gql`
query {
i {
_id
... on I2 @test {
id
}
}
}
`,
);

const queryPlan = queryPlanner.buildQueryPlan(operation);
expect(queryPlan).toMatchInlineSnapshot(`
QueryPlan {
Fetch(service: "A") {
{
i {
__typename
_id
... on T1 @test {
id
}
... on T2 @test {
id
}
}
}
},
}
`);
});

test('nested fragment with interseting parent type and directive condition', () => {
const subgraphA = {
typeDefs: gql`
directive @test on FRAGMENT_SPREAD
type Query {
i: I
}
interface I {
_id: ID
}
type T1 implements I @key(fields: "id") {
_id: ID
id: ID
}
type T2 implements I @key(fields: "id") {
_id: ID
id: ID
}
`,
name: 'A',
};

const subgraphB = {
typeDefs: gql`
directive @test on FRAGMENT_SPREAD
type Query {
i2s: [I2]
}
interface I2 {
id: ID
title: String
}
type T1 implements I2 @key(fields: "id") {
id: ID
title: String
}
type T2 implements I2 @key(fields: "id") {
id: ID
title: String
}
`,
name: 'B',
};

const [api, queryPlanner] = composeAndCreatePlanner(subgraphA, subgraphB);

const operation = operationFromDocument(
api,
gql`
query {
i {
_id
... on I2 {
... on I2 @test {
id
}
}
}
}
`,
);

expect(operation.expandAllFragments().toString()).toMatchInlineSnapshot(`
"{
i {
_id
... on I2 {
... on I2 @test {
id
}
}
}
}"
`);
const queryPlan = queryPlanner.buildQueryPlan(operation);
expect(queryPlan).toMatchInlineSnapshot(`
QueryPlan {
Fetch(service: "A") {
{
i {
__typename
_id
... on T1 {
... @test {
id
}
}
... on T2 {
... @test {
id
}
}
}
}
},
}
`);
});
});