From 6de2a875fd44c51077403b85f1557089aa1905ac Mon Sep 17 00:00:00 2001 From: dariuszkuc <9501705+dariuszkuc@users.noreply.github.com> Date: Mon, 4 Dec 2023 18:36:10 -0600 Subject: [PATCH 1/6] fix: correctly handle directive conditions on fragments when building query graphs 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 referecing 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 --- query-graphs-js/src/graphPath.ts | 9 +- .../src/__tests__/buildPlan.test.ts | 192 ++++++++++++++++++ 2 files changed, 199 insertions(+), 2 deletions(-) diff --git a/query-graphs-js/src/graphPath.ts b/query-graphs-js/src/graphPath.ts index 6fb38ba55..0ff74f8d1 100644 --- a/query-graphs-js/src/graphPath.ts +++ b/query-graphs-js/src/graphPath.ts @@ -2509,7 +2509,7 @@ function advanceWithOperation( const optionsByImplems: OpGraphPath[][][] = []; 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), @@ -2548,8 +2548,13 @@ function advanceWithOperation( 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 ]] }; } diff --git a/query-planner-js/src/__tests__/buildPlan.test.ts b/query-planner-js/src/__tests__/buildPlan.test.ts index f0717e03d..ab237d74e 100644 --- a/query-planner-js/src/__tests__/buildPlan.test.ts +++ b/query-planner-js/src/__tests__/buildPlan.test.ts @@ -7973,3 +7973,195 @@ describe('@requires references external field indirectly', () => { `); }); }); + +describe('handles fragments with directive conditions', () => { + test('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 @test { + id + } + } + } + `, + ); + + expect(operation.expandAllFragments().toString()).toMatchInlineSnapshot(` + "{ + 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 + } + } + } + } + }, + } + `); + }); +}); From b72c9581af95b61d34cd240d15573e234fa91c81 Mon Sep 17 00:00:00 2001 From: dariuszkuc <9501705+dariuszkuc@users.noreply.github.com> Date: Mon, 4 Dec 2023 18:49:33 -0600 Subject: [PATCH 2/6] add changeset --- .changeset/shiny-forks-happen.md | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 .changeset/shiny-forks-happen.md diff --git a/.changeset/shiny-forks-happen.md b/.changeset/shiny-forks-happen.md new file mode 100644 index 000000000..78ee8b033 --- /dev/null +++ b/.changeset/shiny-forks-happen.md @@ -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 referecing 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). From d8b80eda9c6bc53a96fad55c553700413dc115bd Mon Sep 17 00:00:00 2001 From: dariuszkuc <9501705+dariuszkuc@users.noreply.github.com> Date: Mon, 4 Dec 2023 18:50:15 -0600 Subject: [PATCH 3/6] lint --- query-planner-js/src/__tests__/buildPlan.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/query-planner-js/src/__tests__/buildPlan.test.ts b/query-planner-js/src/__tests__/buildPlan.test.ts index ab237d74e..3057bff48 100644 --- a/query-planner-js/src/__tests__/buildPlan.test.ts +++ b/query-planner-js/src/__tests__/buildPlan.test.ts @@ -8022,8 +8022,8 @@ describe('handles fragments with directive conditions', () => { const [api, queryPlanner] = composeAndCreatePlanner(subgraphA, subgraphB); const operation = operationFromDocument( - api, - gql` + api, + gql` query { i { _id @@ -8113,8 +8113,8 @@ describe('handles fragments with directive conditions', () => { const [api, queryPlanner] = composeAndCreatePlanner(subgraphA, subgraphB); const operation = operationFromDocument( - api, - gql` + api, + gql` query { i { _id From 0f7283d48e762e2a19ec9ca964015fc3fe8db6e3 Mon Sep 17 00:00:00 2001 From: dariuszkuc <9501705+dariuszkuc@users.noreply.github.com> Date: Mon, 4 Dec 2023 18:52:25 -0600 Subject: [PATCH 4/6] fix typo --- .changeset/shiny-forks-happen.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/shiny-forks-happen.md b/.changeset/shiny-forks-happen.md index 78ee8b033..9e305c180 100644 --- a/.changeset/shiny-forks-happen.md +++ b/.changeset/shiny-forks-happen.md @@ -6,4 +6,4 @@ 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 referecing 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). +* 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). From c74b38d80682d2f5796a80b4527a7248cb5cddb5 Mon Sep 17 00:00:00 2001 From: Dariusz Kuc <9501705+dariuszkuc@users.noreply.github.com> Date: Tue, 5 Dec 2023 13:57:06 -0600 Subject: [PATCH 5/6] Apply suggestions from code review Co-authored-by: Chris Lenfest --- query-planner-js/src/__tests__/buildPlan.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/query-planner-js/src/__tests__/buildPlan.test.ts b/query-planner-js/src/__tests__/buildPlan.test.ts index 3057bff48..9d9b94eda 100644 --- a/query-planner-js/src/__tests__/buildPlan.test.ts +++ b/query-planner-js/src/__tests__/buildPlan.test.ts @@ -7975,7 +7975,7 @@ describe('@requires references external field indirectly', () => { }); describe('handles fragments with directive conditions', () => { - test('fragment with interseting parent type and directive condition', () => { + test('fragment with intersecting parent type and directive condition', () => { const subgraphA = { typeDefs: gql` directive @test on FRAGMENT_SPREAD From 5077c5c9bac6219e895d766daa7eac099eb631a1 Mon Sep 17 00:00:00 2001 From: dariuszkuc <9501705+dariuszkuc@users.noreply.github.com> Date: Tue, 5 Dec 2023 13:58:31 -0600 Subject: [PATCH 6/6] remove redundant check from test --- query-planner-js/src/__tests__/buildPlan.test.ts | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/query-planner-js/src/__tests__/buildPlan.test.ts b/query-planner-js/src/__tests__/buildPlan.test.ts index 9d9b94eda..80ca229a7 100644 --- a/query-planner-js/src/__tests__/buildPlan.test.ts +++ b/query-planner-js/src/__tests__/buildPlan.test.ts @@ -8035,16 +8035,6 @@ describe('handles fragments with directive conditions', () => { `, ); - expect(operation.expandAllFragments().toString()).toMatchInlineSnapshot(` - "{ - i { - _id - ... on I2 @test { - id - } - } - }" - `); const queryPlan = queryPlanner.buildQueryPlan(operation); expect(queryPlan).toMatchInlineSnapshot(` QueryPlan {