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).