Skip to content

Commit 3f7392b

Browse files
dariuszkucclenfest
dariuszkuc
and
clenfest
authored
fix: handle directive conditions on fragments when building query graphs (#2875)
This PR 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). Related: * fixes #2862 * supersedes #2864 --------- Co-authored-by: Chris Lenfest <clenfest@apollographql.com>
1 parent 28d1709 commit 3f7392b

3 files changed

Lines changed: 198 additions & 2 deletions

File tree

‎.changeset/shiny-forks-happen.md‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
---
2+
"@apollo/query-graphs": patch
3+
---
4+
5+
fix: handle directive conditions on fragments when building query graphs
6+
7+
This fix addresses issues with handling fragments when they specify directive conditions:
8+
* when exploding the types we were not propagating directive conditions
9+
* 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).

‎query-graphs-js/src/graphPath.ts‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2509,7 +2509,7 @@ function advanceWithOperation<V extends Vertex>(
25092509
const optionsByImplems: OpGraphPath<V>[][][] = [];
25102510
for (const tName of intersection) {
25112511
debug.group(() => `Trying ${tName}`);
2512-
const castOp = new FragmentElement(currentType, tName);
2512+
const castOp = new FragmentElement(currentType, tName, operation.appliedDirectives);
25132513
const implemOptions = advanceSimultaneousPathsWithOperation(
25142514
supergraphSchema,
25152515
new SimultaneousPathsWithLazyIndirectPaths([path], context, conditionResolver),
@@ -2548,8 +2548,13 @@ function advanceWithOperation<V extends Vertex>(
25482548
const conditionType = supergraphSchema.type(typeName)!;
25492549
if (isAbstractType(conditionType) && possibleRuntimeTypes(conditionType).some(t => t.name == currentType.name)) {
25502550
debug.groupEnd(() => `${typeName} is a super-type of current type ${currentType}: no edge to take`);
2551+
// Operation type condition is applicable on the current type, so the types are already exploded but the
2552+
// condition can reference types from the supergraph that are not present in the local subgraph.
2553+
//
2554+
// If operation has applied directives we need to convert to inline fragment without type condition, otherwise
2555+
// we ignore the fragment altogether.
25512556
const updatedPath = operation.appliedDirectives.length > 0
2552-
? path.add(operation, null, noConditionsResolution, operation.deferDirectiveArgs())
2557+
? path.add(operation.withUpdatedTypes(currentType, undefined), null, noConditionsResolution, operation.deferDirectiveArgs())
25532558
: path;
25542559
return { options: [[ updatedPath ]] };
25552560
}

‎query-planner-js/src/__tests__/buildPlan.test.ts‎

Lines changed: 182 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7973,3 +7973,185 @@ describe('@requires references external field indirectly', () => {
79737973
`);
79747974
});
79757975
});
7976+
7977+
describe('handles fragments with directive conditions', () => {
7978+
test('fragment with intersecting parent type and directive condition', () => {
7979+
const subgraphA = {
7980+
typeDefs: gql`
7981+
directive @test on FRAGMENT_SPREAD
7982+
type Query {
7983+
i: I
7984+
}
7985+
interface I {
7986+
_id: ID
7987+
}
7988+
type T1 implements I @key(fields: "id") {
7989+
_id: ID
7990+
id: ID
7991+
}
7992+
type T2 implements I @key(fields: "id") {
7993+
_id: ID
7994+
id: ID
7995+
}
7996+
`,
7997+
name: 'A',
7998+
};
7999+
8000+
const subgraphB = {
8001+
typeDefs: gql`
8002+
directive @test on FRAGMENT_SPREAD
8003+
type Query {
8004+
i2s: [I2]
8005+
}
8006+
interface I2 {
8007+
id: ID
8008+
title: String
8009+
}
8010+
type T1 implements I2 @key(fields: "id") {
8011+
id: ID
8012+
title: String
8013+
}
8014+
type T2 implements I2 @key(fields: "id") {
8015+
id: ID
8016+
title: String
8017+
}
8018+
`,
8019+
name: 'B',
8020+
};
8021+
8022+
const [api, queryPlanner] = composeAndCreatePlanner(subgraphA, subgraphB);
8023+
8024+
const operation = operationFromDocument(
8025+
api,
8026+
gql`
8027+
query {
8028+
i {
8029+
_id
8030+
... on I2 @test {
8031+
id
8032+
}
8033+
}
8034+
}
8035+
`,
8036+
);
8037+
8038+
const queryPlan = queryPlanner.buildQueryPlan(operation);
8039+
expect(queryPlan).toMatchInlineSnapshot(`
8040+
QueryPlan {
8041+
Fetch(service: "A") {
8042+
{
8043+
i {
8044+
__typename
8045+
_id
8046+
... on T1 @test {
8047+
id
8048+
}
8049+
... on T2 @test {
8050+
id
8051+
}
8052+
}
8053+
}
8054+
},
8055+
}
8056+
`);
8057+
});
8058+
8059+
test('nested fragment with interseting parent type and directive condition', () => {
8060+
const subgraphA = {
8061+
typeDefs: gql`
8062+
directive @test on FRAGMENT_SPREAD
8063+
type Query {
8064+
i: I
8065+
}
8066+
interface I {
8067+
_id: ID
8068+
}
8069+
type T1 implements I @key(fields: "id") {
8070+
_id: ID
8071+
id: ID
8072+
}
8073+
type T2 implements I @key(fields: "id") {
8074+
_id: ID
8075+
id: ID
8076+
}
8077+
`,
8078+
name: 'A',
8079+
};
8080+
8081+
const subgraphB = {
8082+
typeDefs: gql`
8083+
directive @test on FRAGMENT_SPREAD
8084+
type Query {
8085+
i2s: [I2]
8086+
}
8087+
interface I2 {
8088+
id: ID
8089+
title: String
8090+
}
8091+
type T1 implements I2 @key(fields: "id") {
8092+
id: ID
8093+
title: String
8094+
}
8095+
type T2 implements I2 @key(fields: "id") {
8096+
id: ID
8097+
title: String
8098+
}
8099+
`,
8100+
name: 'B',
8101+
};
8102+
8103+
const [api, queryPlanner] = composeAndCreatePlanner(subgraphA, subgraphB);
8104+
8105+
const operation = operationFromDocument(
8106+
api,
8107+
gql`
8108+
query {
8109+
i {
8110+
_id
8111+
... on I2 {
8112+
... on I2 @test {
8113+
id
8114+
}
8115+
}
8116+
}
8117+
}
8118+
`,
8119+
);
8120+
8121+
expect(operation.expandAllFragments().toString()).toMatchInlineSnapshot(`
8122+
"{
8123+
i {
8124+
_id
8125+
... on I2 {
8126+
... on I2 @test {
8127+
id
8128+
}
8129+
}
8130+
}
8131+
}"
8132+
`);
8133+
const queryPlan = queryPlanner.buildQueryPlan(operation);
8134+
expect(queryPlan).toMatchInlineSnapshot(`
8135+
QueryPlan {
8136+
Fetch(service: "A") {
8137+
{
8138+
i {
8139+
__typename
8140+
_id
8141+
... on T1 {
8142+
... @test {
8143+
id
8144+
}
8145+
}
8146+
... on T2 {
8147+
... @test {
8148+
id
8149+
}
8150+
}
8151+
}
8152+
}
8153+
},
8154+
}
8155+
`);
8156+
});
8157+
});

0 commit comments

Comments
 (0)