From 2360d3c7b2d233d24328af9eb92abf4fdd2548d9 Mon Sep 17 00:00:00 2001 From: Sylvain Lebresne Date: Thu, 23 Mar 2023 11:30:44 +0100 Subject: [PATCH] Fix incorrectly rejected composition and subgraph issues with `@interfaceObject` There is 2 main issues with the handling of `@interfaceObject` that this patch fixes: 1. given some `@interfaceObject` type `I` in some subgraph `S1`, if another subgraph `S2` was declaring an _implementation_ type `T` of `I` but _without_ declaring `I`, then the code missed creating an "edge" between `T` in `S2` and `I` in `S1`, which resulted in composition rejecting such cases, even though it should work. 2. there were an additional issue in the handling of `@requires` so that the "input rewrites" generated were not generic enough, leading to sometimes sending incorrect queries to subgraph (more precisely, we were sometimes not rewriting the typename when querying an `@interfaceObject` subgraph, leading that subgraph to complain that it is asked for type it doesn't know). Note that fixing those 2 issues surfaced the fact that the handling of "rewrites" in the gateway was not working exactly as it should have. More precisely, hen a path had a fragment `... on I`, if `I` was an interface, then we would no select objects that implements `I` correctly. As it happens, the router implementation of those rewrites was both cleaner and didn't had that issue, so this patch also updates the handling of "rewrites" to mimick what the router implementation does (fixing the issue, and overall cleaning the code). Fixes #2485 --- .changeset/hungry-berries-destroy.md | 11 + .../src/__tests__/executeQueryPlan.test.ts | 263 ++++++++++++++++++ gateway-js/src/dataRewrites.ts | 130 +++++++++ gateway-js/src/executeQueryPlan.ts | 144 +--------- internals-js/src/operations.ts | 17 +- query-graphs-js/src/graphPath.ts | 2 +- query-graphs-js/src/querygraph.ts | 56 ++-- query-planner-js/src/QueryPlan.ts | 26 +- .../buildPlan.interfaceObject.test.ts | 1 + query-planner-js/src/buildPlan.ts | 72 +++-- 10 files changed, 524 insertions(+), 198 deletions(-) create mode 100644 .changeset/hungry-berries-destroy.md create mode 100644 gateway-js/src/dataRewrites.ts diff --git a/.changeset/hungry-berries-destroy.md b/.changeset/hungry-berries-destroy.md new file mode 100644 index 0000000000..5292b47f50 --- /dev/null +++ b/.changeset/hungry-berries-destroy.md @@ -0,0 +1,11 @@ +--- +"@apollo/query-planner": patch +"@apollo/query-graphs": patch +"@apollo/federation-internals": patch +"@apollo/gateway": patch +--- + +Fix issues (incorrectly rejected composition and/or subgraph errors) with `@interfaceObject`. Those issues may occur +either due to some use of `@requires` in an `@interfaceObject` type, or when some subgraph `S` defines a type that is an +implementation of an interface `I` in the supergraph, and there is an `@interfaceObject` for `I` in another subgraph, +but `S` does not itself defines `I`. diff --git a/gateway-js/src/__tests__/executeQueryPlan.test.ts b/gateway-js/src/__tests__/executeQueryPlan.test.ts index 451acfd0e5..b35c244bd0 100644 --- a/gateway-js/src/__tests__/executeQueryPlan.test.ts +++ b/gateway-js/src/__tests__/executeQueryPlan.test.ts @@ -4600,6 +4600,269 @@ describe('executeQueryPlan', () => { } `); }); + + test('handles querying @interfaceObject from a specific implementation', async () => { + const s1 = { + name: 's1', + typeDefs: gql` + extend schema + @link(url: "https://specs.apollo.dev/federation/v2.4", import: ["@key"]) + + type Query { + ts: [T!]! + } + + interface I { + id: ID! + } + + type T implements I @key(fields: "id") { + id: ID! + } + `, + resolvers: { + Query: { + ts: () => [ { id: '2' }, { id: '4' }, { id: '1' } ] + }, + } + } + + const s2 = { + name: 's2', + typeDefs: gql` + extend schema + @link(url: "https://specs.apollo.dev/federation/v2.4", import: ["@key", "@interfaceObject", "@external", "@requires"]) + + type I @key(fields: "id") @interfaceObject { + id: ID! + v: String + } + `, + resolvers: { + I: { + __resolveReference(ref: any) { + return { + ...ref, + v: `id=${ref.id}` + }; + }, + } + } + } + + const { serviceMap, schema, queryPlanner} = getFederatedTestingSchema([ s1, s2 ]); + + let operation = parseOp(` + { + ts { + v + } + } + `, schema); + + let queryPlan = buildPlan(operation, queryPlanner); + const expectedPlan = ` + QueryPlan { + Sequence { + Fetch(service: "s1") { + { + ts { + __typename + id + } + } + }, + Flatten(path: "ts.@") { + Fetch(service: "s2") { + { + ... on T { + __typename + id + } + } => + { + ... on I { + v + } + } + }, + }, + }, + } + `; + expect(queryPlan).toMatchInlineSnapshot(expectedPlan); + + let response = await executePlan(queryPlan, operation, undefined, schema, serviceMap); + expect(response.data).toMatchInlineSnapshot(` + Object { + "ts": Array [ + Object { + "v": "id=2", + }, + Object { + "v": "id=4", + }, + Object { + "v": "id=1", + }, + ], + } + `); + }); + + test('handles querying @interfaceObject from a specific implementation (even when the subgraph does not have the corresponding interface)', async () => { + const s1 = { + name: 's1', + typeDefs: gql` + extend schema + @link(url: "https://specs.apollo.dev/federation/v2.4", import: ["@key"]) + + type Query { + ts: [T!]! + } + + type T @key(fields: "id", resolvable: false) { + id: ID! + } + `, + resolvers: { + Query: { + ts: () => [ { id: '2' }, { id: '4' }, { id: '1' } ] + }, + } + } + + const s2 = { + name: 's2', + typeDefs: gql` + extend schema + @link(url: "https://specs.apollo.dev/federation/v2.4", import: ["@key", "@interfaceObject"]) + + interface I @key(fields: "id") { + id: ID! + required: String + } + + type T implements I @key(fields: "id") { + id: ID! + required: String + } + + `, + resolvers: { + I: { + __resolveReference(ref: any) { + return [ + { id: '1', __typename: "T", required: "r1" }, + { id: '2', __typename: "T", required: "r2" }, + { id: '3', __typename: "T", required: "r3" }, + { id: '4', __typename: "T", required: "r4" }, + ].find(({id}) => id === ref.id); + }, + }, + } + } + + const s3 = { + name: 's3', + typeDefs: gql` + extend schema + @link(url: "https://specs.apollo.dev/federation/v2.4", import: ["@key", "@interfaceObject", "@external", "@requires"]) + + type I @key(fields: "id") @interfaceObject { + id: ID! + required: String @external + v: String @requires(fields: "required") + } + `, + resolvers: { + I: { + __resolveReference(ref: any) { + return { + ...ref, + v: `req=${ref.required}` + }; + }, + } + } + } + + const { serviceMap, schema, queryPlanner} = getFederatedTestingSchema([ s1, s2, s3 ]); + + let operation = parseOp(` + { + ts { + v + } + } + `, schema); + + let queryPlan = buildPlan(operation, queryPlanner); + const expectedPlan = ` + QueryPlan { + Sequence { + Fetch(service: "s1") { + { + ts { + __typename + id + } + } + }, + Flatten(path: "ts.@") { + Fetch(service: "s2") { + { + ... on I { + __typename + id + } + } => + { + ... on I { + __typename + required + } + } + }, + }, + Flatten(path: "ts.@") { + Fetch(service: "s3") { + { + ... on I { + __typename + required + id + } + } => + { + ... on I { + v + } + } + }, + }, + }, + } + `; + expect(queryPlan).toMatchInlineSnapshot(expectedPlan); + + let response = await executePlan(queryPlan, operation, undefined, schema, serviceMap); + expect(response.data).toMatchInlineSnapshot(` + Object { + "ts": Array [ + Object { + "v": "req=r2", + }, + Object { + "v": "req=r4", + }, + Object { + "v": "req=r1", + }, + ], + } + `); + }); }); describe('fields with conflicting types needing aliasing', () => { diff --git a/gateway-js/src/dataRewrites.ts b/gateway-js/src/dataRewrites.ts new file mode 100644 index 0000000000..dd8eece2ba --- /dev/null +++ b/gateway-js/src/dataRewrites.ts @@ -0,0 +1,130 @@ +import { FetchDataRewrite } from "@apollo/query-planner"; +import { assert } from "console"; +import { GraphQLSchema, isAbstractType, isInterfaceType, isObjectType } from "graphql"; + +const FRAGMENT_PREFIX = '... on '; + +export function applyRewrites(schema: GraphQLSchema, rewrites: FetchDataRewrite[] | undefined, value: Record) { + if (!rewrites) { + return; + } + + for (const rewrite of rewrites) { + applyRewrite(schema, rewrite, value); + } +} + +function applyRewrite(schema: GraphQLSchema, rewrite: FetchDataRewrite, value: Record) { + const splitted = splitPathLastElement(rewrite.path); + if (!splitted) { + return; + } + + const [parent, last] = splitted; + const { kind, value: fieldName } = parsePathElement(last); + // So far, all rewrites finish by a field name. If this ever changes, this assertion will catch it early and we can update. + assert(kind === 'fieldName', () => `Unexpected fragment as last element of ${rewrite.path}`); + applyAtPath(schema, parent, value, rewriteAtPathFunction(rewrite, fieldName)); +} + +function rewriteAtPathFunction(rewrite: FetchDataRewrite, fieldAtPath: string): (obj: Record) => void { + switch (rewrite.kind) { + case 'ValueSetter': + return (obj) => { + obj[fieldAtPath] = rewrite.setValueTo; + }; + case 'KeyRenamer': + return (obj) => { + obj[rewrite.renameKeyTo] = obj[fieldAtPath]; + obj[fieldAtPath] = undefined; + }; + } +} + + +/** + * Given a path, separates the last element of path and the rest of it and return them as a pair. + * This will return `undefined` if the path is empty. + */ +function splitPathLastElement(path: string[]): [string[], string] | undefined { + if (path.length === 0) { + return undefined; + } + + const lastIdx = path.length - 1; + return [path.slice(0, lastIdx), path[lastIdx]]; +} + +function applyAtPath(schema: GraphQLSchema, path: string[], value: any, fct: (objAtPath: Record) => void) { + if (Array.isArray(value)) { + for (const arrayValue of value) { + applyAtPath(schema, path, arrayValue, fct); + } + return; + } + + if (typeof value !== 'object') { + return; + } + + if (path.length === 0) { + fct(value); + return; + } + + const [first, ...rest] = path; + const { kind, value: eltValue } = parsePathElement(first); + switch (kind) { + case 'fieldName': + applyAtPath(schema, rest, value[eltValue], fct); + break; + case 'typeName': + // When we apply rewrites, we don't always have the __typename of all object we would need to, but the code expects that + // this does not stop the rewrite to applying, hence the modified to `true` when the object typename is not found. + if (isObjectOfType(schema, value, eltValue, true)) { + applyAtPath(schema, rest, value, fct); + } + break; + } +} + +function parsePathElement(elt: string): { kind: 'fieldName' | 'typeName', value: string } { + if (elt.startsWith(FRAGMENT_PREFIX)) { + return { kind: 'typeName', value: elt.slice(FRAGMENT_PREFIX.length) }; + } else { + return { kind: 'fieldName', value: elt }; + } +} + + +export function isObjectOfType( + schema: GraphQLSchema, + obj: Record, + typeCondition: string, + defaultOnUnknownObjectType: boolean = false, +): boolean { + const objTypename = obj['__typename']; + if (!objTypename) { + return defaultOnUnknownObjectType; + } + + if (typeCondition === objTypename) { + return true; + } + + const type = schema.getType(objTypename); + if (!type) { + return false; + } + + const conditionalType = schema.getType(typeCondition); + if (!conditionalType) { + return false; + } + + if (isAbstractType(conditionalType)) { + return (isObjectType(type) || isInterfaceType(type)) && schema.isSubType(conditionalType, type); + } + + return false; +} diff --git a/gateway-js/src/executeQueryPlan.ts b/gateway-js/src/executeQueryPlan.ts index ad6dee283e..39ea49a337 100644 --- a/gateway-js/src/executeQueryPlan.ts +++ b/gateway-js/src/executeQueryPlan.ts @@ -5,10 +5,7 @@ import { TypeNameMetaFieldDef, GraphQLFieldResolver, GraphQLFormattedError, - isAbstractType, GraphQLSchema, - isObjectType, - isInterfaceType, GraphQLErrorOptions, DocumentNode, executeSync, @@ -26,17 +23,16 @@ import { QueryPlanSelectionNode, QueryPlanFieldNode, getResponseName, - FetchDataInputRewrite, - FetchDataOutputRewrite, evaluateCondition, } from '@apollo/query-planner'; import { deepMerge } from './utilities/deepMerge'; import { isNotNullOrUndefined } from './utilities/array'; import { SpanStatusCode } from "@opentelemetry/api"; import { OpenTelemetrySpanNames, tracer } from "./utilities/opentelemetry"; -import { assert, defaultRootName, errorCodeDef, ERRORS, isDefined, Operation, operationFromDocument, Schema } from '@apollo/federation-internals'; +import { assert, defaultRootName, errorCodeDef, ERRORS, Operation, operationFromDocument, Schema } from '@apollo/federation-internals'; import { GatewayGraphQLRequestContext, GatewayExecutionResult } from '@apollo/server-gateway-interface'; import { computeResponse } from './resultShaping'; +import { applyRewrites, isObjectOfType } from './dataRewrites'; export type ServiceMap = { [serviceName: string]: GraphQLDataSource; @@ -416,9 +412,12 @@ async function executeFetch( if (!fetch.requires) { const dataReceivedFromService = await sendOperation(variables); + if (dataReceivedFromService) { + applyRewrites(context.supergraphSchema, fetch.outputRewrites, dataReceivedFromService); + } for (const entity of entities) { - deepMerge(entity, withFetchRewrites(dataReceivedFromService, fetch.outputRewrites)); + deepMerge(entity, dataReceivedFromService); } } else { const requires = fetch.requires; @@ -434,9 +433,9 @@ async function executeFetch( context.supergraphSchema, entity, requires, - fetch.inputRewrites, ); if (representation && representation[TypeNameMetaFieldDef.name]) { + applyRewrites(context.supergraphSchema, fetch.inputRewrites, representation); representations.push(representation); representationToEntity.push(index); } @@ -473,8 +472,11 @@ async function executeFetch( ); } + for (let i = 0; i < entities.length; i++) { - deepMerge(entities[representationToEntity[i]], withFetchRewrites(receivedEntities[i], filterEntityRewrites(representations[i], fetch.outputRewrites))); + const receivedEntity = receivedEntities[i]; + applyRewrites(context.supergraphSchema, fetch.outputRewrites, receivedEntity); + deepMerge(entities[representationToEntity[i]], receivedEntity); } } } @@ -707,84 +709,6 @@ export function generateHydratedPaths( } } -function applyOrMapRecursive(value: any | any[], fct: (v: any) => any | undefined): any | any[] | undefined { - if (Array.isArray(value)) { - const res = value.map((elt) => applyOrMapRecursive(elt, fct)).filter(isDefined); - return res.length === 0 ? undefined : res; - } - return fct(value); -} - -function withFetchRewrites(fetchResult: ResultMap | null | void, rewrites: FetchDataOutputRewrite[] | undefined): ResultMap | null | void { - if (!rewrites || !fetchResult) { - return fetchResult; - } - - for (const rewrite of rewrites) { - let obj: any = fetchResult; - let i = 0; - while (obj && i < rewrite.path.length - 1) { - const p = rewrite.path[i++]; - if (p.startsWith('... on ')) { - const typename = p.slice('... on '.length); - // Filter only objects that match the condition. - obj = applyOrMapRecursive(obj, (elt) => elt[TypeNameMetaFieldDef.name] === typename ? elt : undefined); - } else { - obj = applyOrMapRecursive(obj, (elt) => elt[p]); - } - } - if (obj) { - applyOrMapRecursive(obj, (elt) => { - if (typeof elt === 'object') { - // We need to move the value at path[i] to `renameKeyTo`. - const removedKey = rewrite.path[i]; - elt[rewrite.renameKeyTo] = elt[removedKey]; - elt[removedKey] = undefined; - } - }); - } - } - return fetchResult; -} - -function filterEntityRewrites(entity: Record, rewrites: FetchDataOutputRewrite[] | undefined): FetchDataOutputRewrite[] | undefined { - if (!rewrites) { - return undefined; - } - - const typename = entity[TypeNameMetaFieldDef.name] as string; - const typenameAsFragment = `... on ${typename}`; - return rewrites.map((r) => r.path[0] === typenameAsFragment ? { ...r, path: r.path.slice(1) } : undefined).filter(isDefined) -} - -function updateRewrites(rewrites: FetchDataInputRewrite[] | undefined, pathElement: string): { - updated: FetchDataInputRewrite[], - completeRewrite?: any, -} | undefined { - if (!rewrites) { - return undefined; - } - - let completeRewrite: any = undefined; - const updated = rewrites - .map((r) => { - let u: FetchDataInputRewrite | undefined = undefined; - if (r.path[0] === pathElement) { - const updatedPath = r.path.slice(1); - if (updatedPath.length === 0) { - completeRewrite = r.setValueTo; - } else { - u = { ...r, path: updatedPath }; - } - } - return u; - }) - .filter(isDefined); - return updated.length === 0 && completeRewrite === undefined - ? undefined - : { updated, completeRewrite }; -} - /** * * @param source Result of GraphQL execution. @@ -794,7 +718,6 @@ function executeSelectionSet( schema: GraphQLSchema, source: Record | null, selections: QueryPlanSelectionNode[], - activeRewrites?: FetchDataInputRewrite[], ): Record | null { // If the underlying service has returned null for the parent (source) @@ -825,16 +748,10 @@ function executeSelectionSet( return null; } - const updatedRewrites = updateRewrites(activeRewrites, responseName); - if (updatedRewrites?.completeRewrite !== undefined) { - result[responseName] = updatedRewrites.completeRewrite; - continue; - } - if (Array.isArray(source[responseName])) { result[responseName] = source[responseName].map((value: any) => selections - ? executeSelectionSet(schema, value, selections, updatedRewrites?.updated) + ? executeSelectionSet(schema, value, selections) : value, ); } else if (selections) { @@ -842,23 +759,18 @@ function executeSelectionSet( schema, source[responseName], selections, - updatedRewrites?.updated, ); } else { result[responseName] = source[responseName]; } break; case Kind.INLINE_FRAGMENT: - if (!selection.typeCondition) continue; + if (!selection.typeCondition || !source) continue; - const typename = source && source['__typename']; - if (!typename) continue; - - if (doesTypeConditionMatch(schema, selection.typeCondition, typename)) { - const updatedRewrites = activeRewrites ? updateRewrites(activeRewrites, `... on ${selection.typeCondition}`) : undefined; + if (isObjectOfType(schema, source, selection.typeCondition)) { deepMerge( result, - executeSelectionSet(schema, source, selection.selections, updatedRewrites?.updated), + executeSelectionSet(schema, source, selection.selections), ); } break; @@ -868,32 +780,6 @@ function executeSelectionSet( return result; } -function doesTypeConditionMatch( - schema: GraphQLSchema, - typeCondition: string, - typename: string, -): boolean { - if (typeCondition === typename) { - return true; - } - - const type = schema.getType(typename); - if (!type) { - return false; - } - - const conditionalType = schema.getType(typeCondition); - if (!conditionalType) { - return false; - } - - if (isAbstractType(conditionalType)) { - return (isObjectType(type) || isInterfaceType(type)) && schema.isSubType(conditionalType, type); - } - - return false; -} - function moveIntoCursor(cursor: ResultCursor, pathInCursor: ResponsePath): ResultCursor | undefined { const data = flattenResultsAtPath(cursor.data, pathInCursor); return data ? { diff --git a/internals-js/src/operations.ts b/internals-js/src/operations.ts index db06602fa2..fddc623604 100644 --- a/internals-js/src/operations.ts +++ b/internals-js/src/operations.ts @@ -48,6 +48,7 @@ import { Variables, isObjectType, } from "./definitions"; +import { isInterfaceObjectType } from "./federation"; import { ERRORS } from "./error"; import { isDirectSubtype, sameType } from "./types"; import { assert, mapEntries, mapValues, MapWithCachedArrays, MultiMap, SetMultiMap } from "./utils"; @@ -304,16 +305,18 @@ export class Field ex } private canRebaseOn(parentType: CompositeType) { + const fieldParentType = this.definition.parent // There is 2 valid cases we want to allow: // 1. either `selectionParent` and `fieldParent` are the same underlying type (same name) but from different underlying schema. Typically, // happens when we're building subgraph queries but using selections from the original query which is against the supergraph API schema. - // 2. or they are not the same underlying type, and we only accept this if we're adding an interface field to a selection of one of its - // subtype, and this for convenience. Note that in that case too, `selectinParent` and `fieldParent` may or may be from the same exact - // underlying schema, and so we avoid relying on `isDirectSubtype` in the check. - // In both cases, we just get the field from `selectionParent`, ensuring the return field parent _is_ `selectionParent`. - const fieldParentType = this.definition.parent + // 2. or they are not the same underlying type, but the field parent type is from an interface (or an interface object, which is the same + // here), in which case we may be rebasing an interface field on one of the implementation type, which is ok. Note that we don't verify + // that `parentType` is indeed an implementation of `fieldParentType` because it's possible that this implementation relationship exists + // in the supergraph, but not in any of the subgraph schema involved here. So we just let it be. Not that `rebaseOn` will complain anyway + // if the field name simply does not exists in `parentType`. return parentType.name === fieldParentType.name - || (isInterfaceType(fieldParentType) && fieldParentType.allImplementations().some(i => i.name === parentType.name)); + || isInterfaceType(fieldParentType) + || isInterfaceObjectType(fieldParentType); } typeIfAddedTo(parentType: CompositeType): Type | undefined { @@ -458,7 +461,7 @@ export class FragmentElement extends AbstractOperationElement { return newFragment; } - rebaseOn(parentType: CompositeType): FragmentElement{ + rebaseOn(parentType: CompositeType): FragmentElement { const fragmentParent = this.parentType; const typeCondition = this.typeCondition; if (parentType === fragmentParent) { diff --git a/query-graphs-js/src/graphPath.ts b/query-graphs-js/src/graphPath.ts index 5fc58e4990..8bec284f58 100644 --- a/query-graphs-js/src/graphPath.ts +++ b/query-graphs-js/src/graphPath.ts @@ -974,7 +974,7 @@ export function advancePathWithTransition( debug.group(`Computing indirect paths:`); const pathsWithNonCollecting = subgraphPath.indirectOptions(); if (pathsWithNonCollecting.paths.length > 0) { - debug.groupEnd(() => `${pathsWithNonCollecting.paths.length} indirect paths`); + debug.groupEnd(() => `${pathsWithNonCollecting.paths.length} indirect paths: ${pathsWithNonCollecting.paths}`); debug.group('Validating indirect options:'); for (const nonCollectingPath of pathsWithNonCollecting.paths) { debug.group(() => `For indirect path ${nonCollectingPath}:`); diff --git a/query-graphs-js/src/querygraph.ts b/query-graphs-js/src/querygraph.ts index 7f99b4c091..3b648b4e4f 100644 --- a/query-graphs-js/src/querygraph.ts +++ b/query-graphs-js/src/querygraph.ts @@ -664,42 +664,54 @@ function federateSubgraphs(supergraph: Schema, subgraphs: QueryGraph[]): QueryGr assert(isInterfaceType(type) || isObjectType(type), () => `Invalid "@key" application on non Object || Interface type "${type}"`); const isInterfaceObject = subgraphMetadata.isInterfaceObjectType(type); const conditions = parseFieldSetArgument({ parentType: type, directive: keyApplication }); + + // We'll look at adding edges from "other subgraphs" to the current type. So the tail of all the edges + // we'll build in this branch is always going to be the same. + const tail = copyPointers[i].copiedVertex(v); // Note that each subgraph has a key edge to itself (when i === j below). We usually ignore // this edges, but they exists for the special case of @defer, where we technically may have // to take such "edge-to-self" as a mean to "re-enter" a subgraph for a deferred section. for (const [j, otherSubgraph] of subgraphs.entries()) { const otherVertices = otherSubgraph.verticesForType(type.name); - if (otherVertices.length == 0) { - continue; + if (otherVertices.length > 0) { + // Note that later, when we've handled @provides, this might not be true anymore as @provides may create copy of a + // certain type. But for now, it's true. + assert( + otherVertices.length == 1, + () => `Subgraph ${j} should have a single vertex for type ${type.name} but got ${otherVertices.length}: ${inspect(otherVertices)}`); + + const otherVertex = otherVertices[0]; + // The edge goes from the otherSubgraph to the current one. + const head = copyPointers[j].copiedVertex(otherVertex); + const tail = copyPointers[i].copiedVertex(v); + builder.addEdge(head, tail, new KeyResolution(), conditions); } - // Note that later, when we've handled @provides, this might not be true anymore as @provides may create copy of a - // certain type. But for now, it's true. - assert( - otherVertices.length == 1, - () => `Subgraph ${j} should have a single vertex for type ${type.name} but got ${otherVertices.length}: ${inspect(otherVertices)}`); - - const otherVertice = otherVertices[0]; - // The edge goes from the otherSubgraph to the current one. - const head = copyPointers[j].copiedVertex(otherVertice); - const tail = copyPointers[i].copiedVertex(v); - builder.addEdge(head, tail, new KeyResolution(), conditions); - - // Additionally, if the key is on an @interfaceObject and this "other" subgraph has the type as - // a proper interface, then we need an edge from each of those implementation (to the @interfaceObject). + + // Additionally, if the key is on an @interfaceObject and this "other" subgraph has some of the implementations + // of the corresponding interface, then we need an edge from each of those implementations (to the @interfaceObject). // This is used when an entity of specific implementation is queried first, but then some of the // requested fields are only provided by that @interfaceObject. - const otherType = otherVertice.type; - if (isInterfaceObject && isInterfaceType(otherType)) { - for (const implemType of otherType.possibleRuntimeTypes()) { - // Note that we're only taking the implementation types from "otherSubgraph", so we're guaranteed - // to have a corresponding vertice (and only one for the same reason than mentioned in the assert above). - const implemVertice = otherSubgraph.verticesForType(implemType.name)[0]; + if (isInterfaceObject) { + const typeInSupergraph = supergraph.type(type.name); + assert(typeInSupergraph && isInterfaceType(typeInSupergraph), () => `Type ${type} is an interfaceObject in subgraph ${i}; should be an interface in the supergraph`); + for (const implemTypeInSupergraph of typeInSupergraph.possibleRuntimeTypes()) { + // That implementation type may or may not exists in "otherSubgraph". If it doesn't, we just have nothing to + // do for that particular impelmentation. If it does, we'll add the proper edge, but note that we're guaranteed + // to have at most one vertex for the same reason than mentioned above (only the handling @provides will make it + // so that there can be more than one vertex per type). + const implemVertice = otherSubgraph.verticesForType(implemTypeInSupergraph.name)[0]; + if (!implemVertice) { + continue; + } + const implemHead = copyPointers[j].copiedVertex(implemVertice); // The key goes from the implementation type to the @interfaceObject one, so the conditions // will be "fetched" on the implementation type, but `conditions` has been parsed on the // interface type, so it will use fields from the interface, not the implementation type. // So we re-parse the condition using the implementation type: this could fail, but in // that case it just mean that key is not usable. + const implemType = implemVertice.type; + assert(isCompositeType(implemType), () => `${implemType} should be composite since it implements ${typeInSupergraph} in the supergraph`); try { const implConditions = parseFieldSetArgument({ parentType: implemType, directive: keyApplication, validate: false }); builder.addEdge(implemHead, tail, new KeyResolution(), implConditions); diff --git a/query-planner-js/src/QueryPlan.ts b/query-planner-js/src/QueryPlan.ts index efeb75a12e..dd57cc0555 100644 --- a/query-planner-js/src/QueryPlan.ts +++ b/query-planner-js/src/QueryPlan.ts @@ -46,29 +46,19 @@ export interface FetchNode { operationKind: OperationTypeNode; operationDocumentNode?: DocumentNode; // Optionally describe a number of "rewrites" that query plan executors should apply to the data that is sent as input of this fetch. - inputRewrites?: FetchDataInputRewrite[]; - // Similar, but for optional "rewrites" to apply to the data that received from a fetch (and before it is apply to the current in-memory results). - outputRewrites?: FetchDataOutputRewrite[]; + // Note that such rewrites should only impact the inputs of the fetch they are applied to (meaning that, as those inputs are collected + // from the current in-memory result, the rewrite should _not_ impact said in-memory results, only what is sent in the fetch). + inputRewrites?: FetchDataRewrite[]; + // Similar, but for optional "rewrites" to apply to the data that received from a fetch (and before it is applied to the current in-memory results). + outputRewrites?: FetchDataRewrite[]; } /** - * The type of rewrites currently supported on the input data of fetches. + * The type of rewrites currently supported on the input/output data of fetches. * - * A rewrite usually identifies some subpart of the input data and some action to perform on that subpart. - * Note that input rewrites should only impact the inputs of the fetch they are applied to (meaning that, as - * those inputs are collected from the current in-memory result, the rewrite should _not_ impact said in-memory - * results, only what is sent in the fetch). + * A rewrite usually identifies some subpart of thedata and some action to perform on that subpart. */ -export type FetchDataInputRewrite = FetchDataValueSetter; - -/** - * The type of rewrites currently supported on the output data of fetches. - * - * A rewrite usually identifies some subpart of the ouput data and some action to perform on that subpart. - * Note that ouput rewrites should only impact the outputs of the fetch they are applied to (meaning that - * the rewrites must be applied before the data from the fetch is merged to the current in-memory result). - */ -export type FetchDataOutputRewrite = FetchDataKeyRenamer; +export type FetchDataRewrite = FetchDataValueSetter | FetchDataKeyRenamer; /** * A rewrite that sets a value at the provided path of the data it is applied to. diff --git a/query-planner-js/src/__tests__/buildPlan.interfaceObject.test.ts b/query-planner-js/src/__tests__/buildPlan.interfaceObject.test.ts index 29180822c3..27335501ec 100644 --- a/query-planner-js/src/__tests__/buildPlan.interfaceObject.test.ts +++ b/query-planner-js/src/__tests__/buildPlan.interfaceObject.test.ts @@ -312,6 +312,7 @@ describe('basic @key on interface/@interfaceObject handling', () => { expect(rewrites).toBeDefined(); expect(rewrites?.length).toBe(1); const rewrite = rewrites![0]; + assert(rewrite.kind === 'ValueSetter', JSON.stringify(rewrite)); expect(rewrite.path).toEqual(['... on A', '__typename']); expect(rewrite.setValueTo).toBe('I'); }); diff --git a/query-planner-js/src/buildPlan.ts b/query-planner-js/src/buildPlan.ts index 0e01bbd866..28c31cf81c 100644 --- a/query-planner-js/src/buildPlan.ts +++ b/query-planner-js/src/buildPlan.ts @@ -96,7 +96,7 @@ import { FEDERATED_GRAPH_ROOT_SOURCE, } from "@apollo/query-graphs"; import { stripIgnoredCharacters, print, OperationTypeNode, SelectionSetNode, Kind } from "graphql"; -import { DeferredNode, FetchDataInputRewrite, FetchDataOutputRewrite } from "."; +import { DeferredNode, FetchDataRewrite } from "."; import { Conditions, conditionsOfSelectionSet, isConstantCondition, mergeConditions, removeConditionsFromSelectionSet, updatedConditions } from "./conditions"; import { enforceQueryPlannerConfigDefaults, QueryPlannerConfig } from "./config"; import { generateAllPlansAndFindBest } from "./generateAllPlans"; @@ -774,6 +774,18 @@ class GroupInputs { } return cloned; } + + toString(): string { + const inputs = mapValues(this.perType); + if (inputs.length === 0) { + return '{}'; + } + if (inputs.length === 1) { + return inputs[0].toString(); + } + + return '[' + inputs.join(',') + ']'; + } } /** @@ -789,7 +801,7 @@ class FetchGroup { // Set in some code-path to indicate that the selection of the group not be optimized away even if it "looks" useless. mustPreserveSelection: boolean = false; - private readonly inputRewrites: FetchDataInputRewrite[] = []; + private readonly inputRewrites: FetchDataRewrite[] = []; private constructor( readonly dependencyGraph: FetchDependencyGraph, @@ -1000,7 +1012,7 @@ class FetchGroup { return this._children; } - addInputs(selection: Selection | SelectionSet, rewrites?: FetchDataInputRewrite[]) { + addInputs(selection: Selection | SelectionSet, rewrites?: FetchDataRewrite[]) { assert(this._inputs, "Shouldn't try to add inputs to a root fetch group"); this._inputs.add(selection); @@ -1294,7 +1306,7 @@ class FetchGroup { private finalizeSelection( variableDefinitions: VariableDefinitions, handledConditions: Conditions, - ): { selection: SelectionSet, outputRewrites: FetchDataOutputRewrite[] } { + ): { selection: SelectionSet, outputRewrites: FetchDataRewrite[] } { // Finalizing the selection involves the following: // 1. removing any @include/@skip that are not necessary because they are already handled earlier in the query plan by // some `ConditionNode`. @@ -1476,11 +1488,11 @@ function computeAliasesForNonMergingFields(selections: SelectionSetAtPath[], ali } } -function addAliasesForNonMergingFields(selectionSet: SelectionSet): { updated: SelectionSet, outputRewrites: FetchDataOutputRewrite[] } { +function addAliasesForNonMergingFields(selectionSet: SelectionSet): { updated: SelectionSet, outputRewrites: FetchDataRewrite[] } { const aliases: FieldToAlias[] = []; computeAliasesForNonMergingFields([{ path: [], selections: selectionSet}], aliases); const updated = withFieldAliased(selectionSet, aliases); - const outputRewrites = aliases.map(({path, responseName, alias}) => ({ + const outputRewrites = aliases.map(({path, responseName, alias}) => ({ kind: 'KeyRenamer', path: path.concat(alias), renameKeyTo: responseName, @@ -1594,6 +1606,20 @@ function deferContextAfterSubgraphJump(baseContext: DeferContext): DeferContext }; } +/** + * Filter any fragment element in the provided path whose type condition does not exists in the provide schema. + * Not that if the fragment element should be filtered but it has applied directives, then we preserve those applications by + * replacing with a fragment with no condition (but if there is not directives, we simply remove the fragment from the path). + */ +function filterOperationPath(path: OperationPath, schema: Schema): OperationPath { + return path.map((elt) => { + if (elt.kind === 'FragmentElement' && elt.typeCondition && !schema.type(elt.typeCondition.name)) { + return elt.appliedDirectives.length > 0 ? elt.withUpdatedCondition(undefined) : undefined; + } + return elt; + }).filter(isDefined); +} + class GroupPath { private constructor( private readonly fullPath: OperationPath, @@ -1626,10 +1652,14 @@ class GroupPath { ); } - forParentOfGroup(pathOfGroupInParent: OperationPath): GroupPath { + forParentOfGroup(pathOfGroupInParent: OperationPath, parentSchema: Schema): GroupPath { return new GroupPath( this.fullPath, - concatOperationPaths(pathOfGroupInParent, this.pathInGroup), + // The group refered by `this` may have types that do not exists in the group "parent", so we filter + // out any type conditions on those. This typically happens jumping to a group that use an @interfaceObject + // from a (parent) group that does not know the corresponding interface but has some of the type that + // implements it (in the supergraph). + concatOperationPaths(pathOfGroupInParent, filterOperationPath(this.pathInGroup, parentSchema)), this.responsePath, ); } @@ -3902,8 +3932,12 @@ function computeGroupsForTree( return createdGroups; } -function computeInputRewritesOnKeyFetch(inputTypeName: string, destType: CompositeType): FetchDataInputRewrite[] | undefined { - if (isInterfaceObjectType(destType)) { +function computeInputRewritesOnKeyFetch(inputTypeName: string, destType: CompositeType): FetchDataRewrite[] | undefined { + // When we send a fetch to a subgraph, the inputs __typename must essentially match `destType` so the proper __resolveReference + // is called. If `destType` is a "normal" object type, that's going to be fine by default, but if `destType` is an interface + // in the supergraph (meaning that it is either an interface or an interface object), then the underlying object might have + // a __typename that is the concrete implementation type of the object, and we need to rewrite it. + if (isInterfaceObjectType(destType) || isInterfaceType(destType)) { return [{ kind: 'ValueSetter', path: [ `... on ${inputTypeName}`, typenameFieldName ], @@ -4218,7 +4252,7 @@ function handleRequires( assert(parent.path, `Missing path-in-parent for @require on ${edge} with group ${group} and parent ${parent}`); addPostRequireInputs( dependencyGraph, - path.forParentOfGroup(parent.path), + path.forParentOfGroup(parent.path, parent.group.parentType.schema()), entityType, edge, context, @@ -4277,16 +4311,12 @@ function addPostRequireInputs( postRequireGroup: FetchGroup, ) { const { inputs, keyInputs } = inputsForRequire(dependencyGraph, entityType, edge, context); - let rewrites: FetchDataInputRewrite[] | undefined = undefined; - // This method is used both for "normal" user @requires, but also to handle the internally injected requirement for interface objects - // when the "true" __typename needs to be retrieved. In those case, the post-require group is basically resuming fetch on the - // @interfaceObject subgraph after we found the real __typename of objects of that interface. But that also mean we need to make - // sure to rewrite that "real" __typename into the interface object name for that fetch (as we do for normal key edge toward an - // interface object). - if (edge.transition.kind === 'InterfaceObjectFakeDownCast') { - rewrites = computeInputRewritesOnKeyFetch(edge.transition.castedTypeName, entityType); - } - postRequireGroup.addInputs(inputs, rewrites); + // Note that `computeInputRewritesOnKeyFetch` will return `undefined` in general, but if `entityType` is an interface/interface object, + // then we need those rewrites to ensure the underlying fetch is valid. + postRequireGroup.addInputs( + inputs, + computeInputRewritesOnKeyFetch(entityType.name, entityType) + ); if (keyInputs) { // It could be the key used to resume fetching after the @require is already fetched in the original group, but we cannot // guarantee it, so we add it now (and if it was already selected, this is a no-op).