diff --git a/gateway-js/CHANGELOG.md b/gateway-js/CHANGELOG.md index 59a6078bd..03b107191 100644 --- a/gateway-js/CHANGELOG.md +++ b/gateway-js/CHANGELOG.md @@ -17,6 +17,8 @@ This CHANGELOG pertains only to Apollo Federation packages in the 2.x range. The - Note that this require the use of the new 2.2 version of the federation spec introduced in this change. - Preserve default values of input object fields [PR #2218](https://github.com/apollographql/federation/pull/2218). - Drop support for node12 [PR #2202](https://github.com/apollographql/federation/pull/2202) +- Fix issue where QP was generating invalid plan missing some data [#361](https://github.com/apollographql/federation/issues/361). +- Avoid reusing named fragments that are invalid for the subgraph [PR #2255](https://github.com/apollographql/federation/pull/2255). - Fix QP not always type-exploding interface when necessary [PR #2246](https://github.com/apollographql/federation/pull/2246). - Fix potential QP issue with shareable root fields [PR #2239](https://github.com/apollographql/federation/pull/2239). - Correctly reject field names starting with `__` [PR #2237](https://github.com/apollographql/federation/pull/2237). diff --git a/internals-js/src/operations.ts b/internals-js/src/operations.ts index b17080739..b40d336f5 100644 --- a/internals-js/src/operations.ts +++ b/internals-js/src/operations.ts @@ -690,10 +690,35 @@ export class NamedFragmentDefinition extends DirectiveTargetElement 1, we add an edge. * - * Do not that in practice we only add those edges when we build a query graph for query planning + * Do note that in practice we only add those edges when we build a query graph for query planning * purposes, because not type-exploding is only an optimization but type-exploding will always "work" * and for composition validation, we don't care about being optimal, while limiting edges make * validation faster by limiting the choices to explore. Also, query planning is careful, as @@ -1162,18 +1162,46 @@ class GraphBuilderFromSchema extends GraphBuilder { * later type-exploding in impossible runtime types. */ addAdditionalAbstractTypeEdges() { + // As mentioned above, we only care about this on subgraphs query graphs and during query planning, and + // we'll have a supergraph when that happens. But if this ever get called in some other path, ignore this. + if (!this.supergraph) { + return; + } + // For each abstract type in the schema, it's runtime types. - const abstractTypesWithTheirRuntimeTypes: [AbstractType, readonly ObjectType[]][] = []; + type AbstractTypeWithRuntimes = { + type: AbstractType, + runtimeTypesInSubgraph: readonly ObjectType[], + runtimeTypesInSupergraph: readonly ObjectType[], + } + const abstractTypesWithTheirRuntimeTypes: AbstractTypeWithRuntimes[] = []; for (const type of this.schema.types()) { if (isAbstractType(type)) { - abstractTypesWithTheirRuntimeTypes.push([type, possibleRuntimeTypes(type)]); + const typeInSupergraph = this.supergraph.apiSchema.type(type.name); + // All "normal" types from subgraphs should be in the supergraph API, but there is a couple exceptions: + // - subgraphs have the `_Entity` type, which is not in the supergraph. + // - inaccessible types also won't be in the supergrah. + // In all those cases, we don't create any additional edges for those types. For inaccessible type, we + // could theoretically try to add them, but we would need the full supergraph while we currently only + // have access to the API schema, and besides, inacessible types can only be part of the query execution in + // indirect ways, through some @requires for instance, and you'd need pretty weird @requires for the + // optimization here to ever matter. + if (!typeInSupergraph) { + continue; + } + assert(isAbstractType(typeInSupergraph), () => `${type} should not be a ${type.kind} in a subgraph but a ${typeInSupergraph.kind} in the supergraph`); + abstractTypesWithTheirRuntimeTypes.push({ + type, + runtimeTypesInSubgraph: possibleRuntimeTypes(type), + runtimeTypesInSupergraph: possibleRuntimeTypes(typeInSupergraph), + }); } } // Check every pair of abstract type that intersect on at least 2 runtime types to see if have // edges to add. Note that in practice, we only care about 'Union -> Interface' and 'Interface -> Interface' for (let i = 0; i < abstractTypesWithTheirRuntimeTypes.length - 1; i++) { - const [t1, t1Runtimes] = abstractTypesWithTheirRuntimeTypes[i]; + const t1 = abstractTypesWithTheirRuntimeTypes[i]; // Note that in general, t1 is already part of the graph `addTypeRecursively` don't really add anything, it // just return the existing vertex. That said, if t1 is returned by no field (at least no field reachable from // a root type), the type will not be part of the graph. And in that case, we do add it. And it's actually @@ -1182,21 +1210,72 @@ class GraphBuilderFromSchema extends GraphBuilder { // in the first place (we could also try to purge such subset after this method, but it's probably not worth // it in general) and it's not a big deal: it will just use a bit more memory than necessary, and it's probably // pretty rare in the first place. - const t1Vertex = this.addTypeRecursively(t1); + const t1Vertex = this.addTypeRecursively(t1.type); for (let j = i; j < abstractTypesWithTheirRuntimeTypes.length; j++) { - const [t2, t2Runtimes] = abstractTypesWithTheirRuntimeTypes[j]; + const t2 = abstractTypesWithTheirRuntimeTypes[j]; + // We ignore the pair if both are interfaces and one implements the other. We'll already have appropriate // edges if that's the case. - if (isInterfaceType(t1) && isInterfaceType(t2) && (t1.implementsInterface(t2) || t2.implementsInterface(t1))) { + if (isInterfaceType(t1.type) && isInterfaceType(t2.type) && (t1.type.implementsInterface(t2.type) || t2.type.implementsInterface(t1.type))) { continue; } - // Note that as everything comes from the same subgraph schema, using reference equality is fine. - const intersecting = t1Runtimes.filter(o1 => t2Runtimes.includes(o1)); - if (intersecting.length >= 2) { + + let addT1ToT2 = false; + let addT2ToT1 = false; + if (t1.type === t2.type) { + // We always add an edge from a type to itself. This is just saying that if we're type-casting to the type we're already + // on, it's doing nothing, and in particular it shouldn't force us to type-explode anymore that if we didn't had the + // cast in the first place. Note that we only set `addT1ToT1` to true, otherwise we'd be adding the same edge twice. + addT1ToT2 = true; + } else { + // Otherwise, there is 2 aspects to take into account: + // - it's only worth adding an edge between types, meaining that we might save type-exploding into the runtime + // types of the "target" one, if the local intersection (of runtime types, in the current subgraph) for the + // abstract types is more than 2. If it's just 1 type, then going to that type directly is not less efficient + // and is more precise in a sense. And if the intersection is empty, then no point in polluting the query graphs + // with edges we'll never take. + // - _but_ we can only save type-exploding if that local intersection does not exclude any runtime types that + // are local to the "source" type, not local to the "target" type, *but* are global to the "taget" type, + // because such type should not be excluded and only type-explosion will achieve that (for some concrete + // example, see the "merged abstract types handling" tests in `buildPlan.test.ts`). + // In other words, we don't want to avoid the type explosion if there is a type in the intersection of + // the local "source" runtimes and global "target" runtimes that are not in the purely local runtimes + // intersection. + + // Everything comes from the same subgraph schema, using reference equality is fine here. + const intersectingLocal = t1.runtimeTypesInSubgraph.filter(o1 => t2.runtimeTypesInSubgraph.includes(o1)); + if (intersectingLocal.length >= 2) { + const isInLocalOtherTypeButNotLocalIntersection = (type: ObjectType, otherType: AbstractTypeWithRuntimes) => ( + otherType.runtimeTypesInSubgraph.some((t) => t.name === type.name) + && !intersectingLocal.some((t) => t.name === type.name) + ); + // TODO: we're currently _never_ adding the edge if the "target" is a union. We shouldn't be doing that, this + // will genuinely make some cases less efficient than they could be (though those cases are admittedly a bit convoluted), + // but this make sense *until* https://github.com/apollographql/federation/issues/2256 gets fixed. Because until + // then, we do not properly track unions through composition, and that means there is never a difference (in the query + // planner) between a local union definition and the supergraph one, even if that different actually exists. + // And so, never type-exploding in that case is somewhat safer, as not-type-exploding is ultimately an optimisation. + // Please note that this is *not* a fix for #2256, and most of the issues created by #2256 still needs fixing, but + // it avoids making it even worth for a few corner cases. We should remove the `isUnionType` below once the + // fix for #2256 is implemented. + if (!(isUnionType(t2.type) || t2.runtimeTypesInSupergraph.some((rt) => isInLocalOtherTypeButNotLocalIntersection(rt, t1)))) { + addT1ToT2 = true; + } + if (!(isUnionType(t1.type) ||t1.runtimeTypesInSupergraph.some((rt) => isInLocalOtherTypeButNotLocalIntersection(rt, t2)))) { + addT2ToT1 = true; + } + } + } + + if (addT1ToT2 || addT2ToT1) { // Same remark as for t1 above. - const t2Vertex = this.addTypeRecursively(t2); - this.addEdge(t1Vertex, t2Vertex, new DownCast(t1, t2)); - this.addEdge(t2Vertex, t1Vertex, new DownCast(t2, t1)); + const t2Vertex = this.addTypeRecursively(t2.type); + if (addT1ToT2) { + this.addEdge(t1Vertex, t2Vertex, new DownCast(t1.type, t2.type)); + } + if (addT2ToT1) { + this.addEdge(t2Vertex, t1Vertex, new DownCast(t2.type, t1.type)); + } } } } diff --git a/query-planner-js/CHANGELOG.md b/query-planner-js/CHANGELOG.md index f4a632ea7..076f5321a 100644 --- a/query-planner-js/CHANGELOG.md +++ b/query-planner-js/CHANGELOG.md @@ -9,6 +9,7 @@ This CHANGELOG pertains only to Apollo Federation packages in the 2.x range. The default), but users that have custom code making use of `GraphQLDataSourceProcessOptions.document` will now need to explicitly set `GatewayConfig.queryPlannerConfig.exposeDocumentNodeInFetchNode`. - Drop support for node12 [PR #2202](https://github.com/apollographql/federation/pull/2202) +- Avoid reusing named fragments that are invalid for the subgraph [PR #2255](https://github.com/apollographql/federation/pull/2255). - Fix QP not always type-exploding interface when necessary [PR #2246](https://github.com/apollographql/federation/pull/2246). - Fix potential QP issue with shareable root fields [PR #2239](https://github.com/apollographql/federation/pull/2239). diff --git a/query-planner-js/src/__tests__/buildPlan.test.ts b/query-planner-js/src/__tests__/buildPlan.test.ts index b1bda42f6..2929679c7 100644 --- a/query-planner-js/src/__tests__/buildPlan.test.ts +++ b/query-planner-js/src/__tests__/buildPlan.test.ts @@ -4052,6 +4052,88 @@ describe('Named fragments preservation', () => { } `); }); + + it('do not try to apply fragments that are not valid for the subgaph', () => { + // Slightly artificial example for simplicity, but this highlight the problem. + // In that example, the only queried subgraph is the first one (there is in fact + // no way to ever reach the 2nd one), so the plan should mostly simply forward + // the query to the 1st subgraph, but a subtlety is that the named fragment used + // in the query is *not* valid for Subgraph1, because it queries `b` on `I`, but + // there is no `I.b` in Subgraph1. + // So including the named fragment in the fetch would be erroneous: the subgraph + // server would reject it when validating the query, and we must make sure it + // is not reused. + const subgraph1 = { + name: 'Subgraph1', + typeDefs: gql` + type Query { + i1: I + i2: I + } + + interface I { + a: Int + } + + type T implements I { + a: Int + b: Int + } + ` + } + + const subgraph2 = { + name: 'Subgraph2', + typeDefs: gql` + interface I { + a: Int + b: Int + } + ` + } + + const [api, queryPlanner] = composeAndCreatePlanner(subgraph1, subgraph2); + const operation = operationFromDocument(api, gql` + query { + i1 { + ... on T { + ...Frag + } + } + i2 { + ... on T { + ...Frag + } + } + } + + fragment Frag on I { + b + } + `); + + const plan = queryPlanner.buildQueryPlan(operation); + expect(plan).toMatchInlineSnapshot(` + QueryPlan { + Fetch(service: "Subgraph1") { + { + i1 { + __typename + ... on T { + b + } + } + i2 { + __typename + ... on T { + b + } + } + } + }, + } + `); + }); }); test('works with key chains', () => { @@ -4586,3 +4668,593 @@ describe('interface type-explosion', () => { expect(queryPlanner.lastGeneratedPlanStatistics()?.evaluatedPlanCount).toBe(1); }); }); + +/* + * Those tests the cases where 2 abstract types (interface or union) interact (having some common runtime + * types intersection), but one of them include an runtime type that the other also include _in the supergraph_ + * but *not* in one of the subgraph. The tl;dr is that in some of those interaction, we must force a type-explosion + * to handle it properly, but no in other interactions, and this ensures this is handled properly. + */ +describe('merged abstract types handling', () => { + test('union/interface interaction', () => { + const subgraph1 = { + name: 'Subgraph1', + typeDefs: gql` + type Query { + u: U + } + + union U = A | B | C + + interface I { + v: Int + } + + type A { + v: Int @shareable + } + + type B implements I { + v: Int + } + + type C implements I { + v: Int + } + ` + } + + const subgraph2 = { + name: 'Subgraph2', + typeDefs: gql` + interface I { + v: Int + } + + type A implements I { + v: Int @shareable + } + ` + } + + const [api, queryPlanner] = composeAndCreatePlanner(subgraph1, subgraph2); + const operation = operationFromDocument(api, gql` + { + u { + ... on I { + v + } + } + } + `); + + const plan = queryPlanner.buildQueryPlan(operation); + // Type `A` can be returned by `u` and is a `I` *in the supergraph* but not in `Subgraph1`, so need to + // type-explode `I` in the query to `Subgraph1` so it doesn't exclude `A`. + expect(plan).toMatchInlineSnapshot(` + QueryPlan { + Fetch(service: "Subgraph1") { + { + u { + __typename + ... on A { + v + } + ... on B { + v + } + ... on C { + v + } + } + } + }, + } + `); + }); + + test('union/interface interaction, but no need to type-explode', () => { + const subgraph1 = { + name: 'Subgraph1', + typeDefs: gql` + type Query { + u: U + } + + union U = B | C + + interface I { + v: Int + } + + type A implements I { + v: Int @shareable + } + + type B implements I { + v: Int + } + + type C implements I { + v: Int + } + ` + } + + const subgraph2 = { + name: 'Subgraph2', + typeDefs: gql` + union U = A + + type A { + v: Int @shareable + } + ` + } + + const [api, queryPlanner] = composeAndCreatePlanner(subgraph1, subgraph2); + const operation = operationFromDocument(api, gql` + { + u { + ... on I { + v + } + } + } + `); + + const plan = queryPlanner.buildQueryPlan(operation); + // While `A` is a `U` in the supergraph while not in `Subgraph1`, since the `u` + // operation is resolved by `Subgraph1`, it cannot ever return a A, and so + // there is need to type-explode `I` in this query. + expect(plan).toMatchInlineSnapshot(` + QueryPlan { + Fetch(service: "Subgraph1") { + { + u { + __typename + ... on I { + v + } + } + } + }, + } + `); + }); + + test('interface/union interaction', () => { + const subgraph1 = { + name: 'Subgraph1', + typeDefs: gql` + type Query { + i: I + } + + union U = B | C + + interface I { + v: Int + } + + type A implements I { + v: Int @shareable + } + + type B implements I { + v: Int + } + + type C implements I { + v: Int + } + ` + } + + const subgraph2 = { + name: 'Subgraph2', + typeDefs: gql` + union U = A + + type A { + v: Int @shareable + } + ` + } + + const [api, queryPlanner] = composeAndCreatePlanner(subgraph1, subgraph2); + const operation = operationFromDocument(api, gql` + { + i { + ... on U { + ... on A { + v + } + } + } + } + `); + + const plan = queryPlanner.buildQueryPlan(operation); + // Type `A` can be returned by `i` and is a `U` *in the supergraph* but not in `Subgraph1`, so need to + // type-explode `U` in the query to `Subgraph1` so it doesn't exclude `A`. + expect(plan).toMatchInlineSnapshot(` + QueryPlan { + Fetch(service: "Subgraph1") { + { + i { + __typename + ... on A { + v + } + } + } + }, + } + `); + }); + + test('interface/union interaction, but no need to type-explode', () => { + const subgraph1 = { + name: 'Subgraph1', + typeDefs: gql` + type Query { + i: I + } + + union U = A | B | C + + interface I { + v: Int + } + + type A { + v: Int @shareable + } + + type B implements I { + v: Int + } + + type C implements I { + v: Int + } + ` + } + + const subgraph2 = { + name: 'Subgraph2', + typeDefs: gql` + interface I { + v: Int + } + + type A implements I { + v: Int @shareable + } + ` + } + + const [api, queryPlanner] = composeAndCreatePlanner(subgraph1, subgraph2); + const operation = operationFromDocument(api, gql` + { + i { + ... on U { + ... on A { + v + } + } + } + } + `); + + const plan = queryPlanner.buildQueryPlan(operation); + // While `A` is a `I` in the supergraph while not in `Subgraph1`, since the `i` operation is resolved by + // `Subgraph1`, it cannot ever return a A, and so we should skip the whole `v` selection; or at the very + // least, we should not send a query with `... on U { ... on A { }}` to `Subgraph1` since it + // would reject it as invalid. + expect(plan).toMatchInlineSnapshot(` + QueryPlan { + Fetch(service: "Subgraph1") { + { + i { + __typename + } + } + }, + } + `); + }); + + test('interface/interface interaction', () => { + const subgraph1 = { + name: 'Subgraph1', + typeDefs: gql` + type Query { + i1: I1 + } + + interface I1 { + v: Int + } + + interface I2 { + v: Int + } + + type A implements I1 { + v: Int @shareable + } + + type B implements I1 & I2 { + v: Int + } + + type C implements I1 & I2 { + v: Int + } + ` + } + + const subgraph2 = { + name: 'Subgraph2', + typeDefs: gql` + interface I2 { + v: Int + } + + type A implements I2 { + v: Int @shareable + } + ` + } + + const [api, queryPlanner] = composeAndCreatePlanner(subgraph1, subgraph2); + const operation = operationFromDocument(api, gql` + { + i1 { + ... on I2 { + v + } + } + } + `); + + const plan = queryPlanner.buildQueryPlan(operation); + // Type `A` can be returned by `i1` and is a `I2` *in the supergraph* but not in `Subgraph1`, so need to + // type-explode `I2` in the query to `Subgraph1` so it doesn't exclude `A`. + expect(plan).toMatchInlineSnapshot(` + QueryPlan { + Fetch(service: "Subgraph1") { + { + i1 { + __typename + ... on A { + v + } + ... on B { + v + } + ... on C { + v + } + } + } + }, + } + `); + }); + + test('interface/interface interaction, but no need to type-explode', () => { + const subgraph1 = { + name: 'Subgraph1', + typeDefs: gql` + type Query { + i1: I1 + } + + interface I1 { + v: Int + } + + interface I2 { + v: Int + } + + type A implements I2 { + v: Int @shareable + } + + type B implements I1 & I2 { + v: Int + } + + type C implements I1 & I2 { + v: Int + } + ` + } + + const subgraph2 = { + name: 'Subgraph2', + typeDefs: gql` + interface I1 { + v: Int + } + + type A implements I1 { + v: Int @shareable + } + ` + } + + const [api, queryPlanner] = composeAndCreatePlanner(subgraph1, subgraph2); + const operation = operationFromDocument(api, gql` + { + i1 { + ... on I2 { + v + } + } + } + `); + + const plan = queryPlanner.buildQueryPlan(operation); + // While `A` is a `I1` in the supergraph while not in `Subgraph1`, since the `i1` + // operation is resolved by `Subgraph1`, it cannot ever return a A, and so + // there is need to type-explode `I2` in this query (even if `Subgraph1` would + // otherwise not include `A` from a `... on I2`). + expect(plan).toMatchInlineSnapshot(` + QueryPlan { + Fetch(service: "Subgraph1") { + { + i1 { + __typename + ... on I2 { + v + } + } + } + }, + } + `); + }); + + test('union/union interaction', () => { + const subgraph1 = { + name: 'Subgraph1', + typeDefs: gql` + type Query { + u1: U1 + } + + union U1 = A | B | C + union U2 = B | C + + type A { + v: Int @shareable + } + + type B { + v: Int + } + + type C { + v: Int + } + ` + } + + const subgraph2 = { + name: 'Subgraph2', + typeDefs: gql` + union U2 = A + + type A { + v: Int @shareable + } + ` + } + + const [api, queryPlanner] = composeAndCreatePlanner(subgraph1, subgraph2); + const operation = operationFromDocument(api, gql` + { + u1 { + ... on U2 { + ... on A { + v + } + } + } + } + `); + + const plan = queryPlanner.buildQueryPlan(operation); + // Type `A` can be returned by `u1` and is a `U2` *in the supergraph* but not in `Subgraph1`, so need to + // type-explode `U2` in the query to `Subgraph1` so it doesn't exclude `A`. + expect(plan).toMatchInlineSnapshot(` + QueryPlan { + Fetch(service: "Subgraph1") { + { + u1 { + __typename + ... on A { + v + } + } + } + }, + } + `); + }); + + // TODO: this test currently doesn't work due to https://github.com/apollographql/federation/issues/2256 + // (it is not a direct test of that issue, but one of its consequence nonetheles). We should enable it + // with the fix of that issue. + test.skip('union/union interaction, but no need to type-explode', () => { + const subgraph1 = { + name: 'Subgraph1', + typeDefs: gql` + type Query { + u1: U1 + } + + union U1 = B | C + union U2 = A | B | C + + type A { + v: Int @shareable + } + + type B { + v: Int + } + + type C { + v: Int + } + ` + } + + const subgraph2 = { + name: 'Subgraph2', + typeDefs: gql` + union U1 = A + + type A { + v: Int @shareable + } + ` + } + + const [api, queryPlanner] = composeAndCreatePlanner(subgraph1, subgraph2); + const operation = operationFromDocument(api, gql` + { + u1 { + ... on U2 { + ... on A { + v + } + } + } + } + `); + + const plan = queryPlanner.buildQueryPlan(operation); + // While `A` is a `U1` in the supergraph while not in `Subgraph1`, since the `u1` operation is resolved by + // `Subgraph1`, it cannot ever return a A, and so we should skip the whole `v` selection; or at the very + // least, we should not send a query with `u1 { ... on A { }}` to `Subgraph1` since it + // would reject it as invalid. + expect(plan).toMatchInlineSnapshot(` + QueryPlan { + Fetch(service: "Subgraph1") { + { + u1 { + __typename + } + } + }, + } + `); + }); +});