From 47e8e4c2cb64d93d87fae3b30f1627095b7cb605 Mon Sep 17 00:00:00 2001 From: Duckki Oe Date: Fri, 22 Mar 2024 19:32:48 -0700 Subject: [PATCH 1/3] fix: fixed subgraph operations to copy directives https://github.com/apollographql/federation/issues/2961 --- internals-js/src/definitions.ts | 50 ++++++++++++++++++++----------- internals-js/src/operations.ts | 48 ++++++++++++++--------------- query-planner-js/src/QueryPlan.ts | 2 +- query-planner-js/src/buildPlan.ts | 26 +++++++++++----- 4 files changed, 77 insertions(+), 49 deletions(-) diff --git a/internals-js/src/definitions.ts b/internals-js/src/definitions.ts index 4a8df1c12..2d329d251 100644 --- a/internals-js/src/definitions.ts +++ b/internals-js/src/definitions.ts @@ -392,26 +392,11 @@ export class DirectiveTargetElement> { } appliedDirectivesToDirectiveNodes() : ConstDirectiveNode[] | undefined { - if (this.appliedDirectives.length == 0) { - return undefined; - } - - return this.appliedDirectives.map(directive => { - return { - kind: Kind.DIRECTIVE, - name: { - kind: Kind.NAME, - value: directive.name, - }, - arguments: directive.argumentsToAST() - }; - }); + return directivesToDirectiveNodes(this.appliedDirectives); } appliedDirectivesToString(): string { - return this.appliedDirectives.length == 0 - ? '' - : ' ' + this.appliedDirectives.join(' '); + return directivesToString(this.appliedDirectives); } collectVariablesInAppliedDirectives(collector: VariableCollector) { @@ -3257,6 +3242,37 @@ export class Directive< } } +/** + * Formats a Directive array as a string (with a leading space, if present). + */ +export function directivesToString(directives?: readonly Directive[]) + : string +{ + return (!directives || directives.length == 0) + ? '' + : ' ' + directives.join(' '); +} + +/** + * Converts a Directive array into DirectiveNode array. + */ +export function directivesToDirectiveNodes(directives?: readonly Directive[]) + : ConstDirectiveNode[] | undefined +{ + return (!directives || directives.length === 0) + ? undefined + : directives.map(directive => { + return { + kind: Kind.DIRECTIVE, + name: { + kind: Kind.NAME, + value: directive.name, + }, + arguments: directive.argumentsToAST() + }; + }); +} + /** * Checks if 2 directive applications should be considered equal. * diff --git a/internals-js/src/operations.ts b/internals-js/src/operations.ts index 71532577c..12d54d13a 100644 --- a/internals-js/src/operations.ts +++ b/internals-js/src/operations.ts @@ -48,6 +48,8 @@ import { isObjectType, NamedType, isUnionType, + directivesToString, + directivesToDirectiveNodes, } from "./definitions"; import { isInterfaceObjectType } from "./federation"; import { ERRORS } from "./error"; @@ -877,7 +879,6 @@ function computeFragmentsToKeep( return toExpand.size === 0 ? fragments : fragments.filter((f) => !toExpand.has(f.name)); } -// TODO Operations can also have directives export class Operation { constructor( readonly schema: Schema, @@ -885,7 +886,8 @@ export class Operation { readonly selectionSet: SelectionSet, readonly variableDefinitions: VariableDefinitions, readonly fragments?: NamedFragments, - readonly name?: string) { + readonly name?: string, + readonly directives?: readonly Directive[]) { } // Returns a copy of this operation with the provided updated selection set. @@ -901,7 +903,8 @@ export class Operation { newSelectionSet, this.variableDefinitions, this.fragments, - this.name + this.name, + this.directives ); } @@ -917,7 +920,8 @@ export class Operation { newSelectionSet, this.variableDefinitions, newFragments, - this.name + this.name, + this.directives ); } @@ -982,6 +986,7 @@ export class Operation { this.variableDefinitions, fragments, this.name, + this.directives ); } @@ -1053,7 +1058,7 @@ export class Operation { } toString(expandFragments: boolean = false, prettyPrint: boolean = true): string { - return this.selectionSet.toOperationString(this.rootKind, this.variableDefinitions, this.fragments, this.name, expandFragments, prettyPrint); + return this.selectionSet.toOperationString(this.rootKind, this.variableDefinitions, this.fragments, this.name, this.directives, expandFragments, prettyPrint); } } @@ -2077,6 +2082,7 @@ export class SelectionSet { variableDefinitions: VariableDefinitions, fragments: NamedFragments | undefined, operationName?: string, + directives?: readonly Directive[], expandFragments: boolean = false, prettyPrint: boolean = true ): string { @@ -2090,7 +2096,8 @@ export class SelectionSet { const nameAndVariables = operationName ? " " + (operationName + (variableDefinitions.isEmpty() ? "" : variableDefinitions.toString())) : (variableDefinitions.isEmpty() ? "" : " " + variableDefinitions.toString()); - return fragmentsDefinitions + rootKind + nameAndVariables + " " + this.toString(expandFragments, true, indent); + const directives_str = directivesToString(directives); + return fragmentsDefinitions + rootKind + nameAndVariables + directives_str + " " + this.toString(expandFragments, true, indent); } /** @@ -3562,7 +3569,7 @@ class FragmentSpreadSelection extends FragmentSelection { key(): string { if (!this.computedKey) { - this.computedKey = '...' + this.namedFragment.name + (this.spreadDirectives.length === 0 ? '' : ' ' + this.spreadDirectives.join(' ')); + this.computedKey = '...' + this.namedFragment.name + directivesToString(this.spreadDirectives); } return this.computedKey; } @@ -3588,18 +3595,7 @@ class FragmentSpreadSelection extends FragmentSelection { } toSelectionNode(): FragmentSpreadNode { - const directiveNodes = this.spreadDirectives.length === 0 - ? undefined - : this.spreadDirectives.map(directive => { - return { - kind: Kind.DIRECTIVE, - name: { - kind: Kind.NAME, - value: directive.name, - }, - arguments: directive.argumentsToAST() - } as DirectiveNode; - }); + const directiveNodes = directivesToDirectiveNodes(this.spreadDirectives); return { kind: Kind.FRAGMENT_SPREAD, name: { kind: Kind.NAME, value: this.namedFragment.name }, @@ -3744,9 +3740,7 @@ class FragmentSpreadSelection extends FragmentSelection { if (expandFragments) { return (indent ?? '') + this.element + ' ' + this.selectionSet.toString(true, true, indent); } else { - const directives = this.spreadDirectives; - const directiveString = directives.length == 0 ? '' : ' ' + directives.join(' '); - return (indent ?? '') + '...' + this.namedFragment.name + directiveString; + return (indent ?? '') + '...' + this.namedFragment.name + directivesToString(this.spreadDirectives); } } } @@ -3832,6 +3826,7 @@ export function operationFromDocument( } ) : Operation { let operation: OperationDefinitionNode | undefined; + let operation_directives: Directive[] | undefined; // the directives on `operation` const operationName = options?.operationName; const fragments = new NamedFragments(); // We do a first pass to collect the operation, and create all named fragment, but without their selection set yet. @@ -3842,6 +3837,7 @@ export function operationFromDocument( validate(!operation || operationName, () => 'Must provide operation name if query contains multiple operations.'); if (!operationName || (definition.name && definition.name.value === operationName)) { operation = definition; + operation_directives = directivesOfNodes(schema, definition.directives); } break; case Kind.FRAGMENT_DEFINITION: @@ -3875,18 +3871,20 @@ export function operationFromDocument( } }); fragments.validate(variableDefinitions); - return operationFromAST({schema, operation, variableDefinitions, fragments, validateInput: options?.validate}); + return operationFromAST({schema, operation, operation_directives, variableDefinitions, fragments, validateInput: options?.validate}); } function operationFromAST({ schema, operation, + operation_directives, variableDefinitions, fragments, validateInput, }:{ schema: Schema, operation: OperationDefinitionNode, + operation_directives?: Directive[], variableDefinitions: VariableDefinitions, fragments: NamedFragments, validateInput?: boolean, @@ -3906,7 +3904,8 @@ function operationFromAST({ }), variableDefinitions, fragmentsIfAny, - operation.name?.value + operation.name?.value, + operation_directives ); } @@ -3961,6 +3960,7 @@ export function operationToDocument(operation: Operation): DocumentNode { name: operation.name ? { kind: Kind.NAME, value: operation.name } : undefined, selectionSet: operation.selectionSet.toSelectionSetNode(), variableDefinitions: operation.variableDefinitions.toVariableDefinitionNodes(), + directives: directivesToDirectiveNodes(operation.directives), }; const fragmentASTs: DefinitionNode[] = operation.fragments ? operation.fragments?.toFragmentDefinitionNodes() diff --git a/query-planner-js/src/QueryPlan.ts b/query-planner-js/src/QueryPlan.ts index dd57cc055..5fd7870d1 100644 --- a/query-planner-js/src/QueryPlan.ts +++ b/query-planner-js/src/QueryPlan.ts @@ -46,7 +46,7 @@ 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. - // Note that such rewrites should only impact the inputs of the fetch they are applied to (meaning that, as those inputs are collected + // 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). diff --git a/query-planner-js/src/buildPlan.ts b/query-planner-js/src/buildPlan.ts index c20ebf3d0..823e7be52 100644 --- a/query-planner-js/src/buildPlan.ts +++ b/query-planner-js/src/buildPlan.ts @@ -1497,6 +1497,7 @@ class FetchGroup { variableDefinitions: VariableDefinitions, fragments?: RebasedFragments, operationName?: string, + directives?: readonly Directive[], ) : PlanNode | undefined { if (this.selection.isEmpty()) { return undefined; @@ -1513,6 +1514,7 @@ class FetchGroup { selection, variableDefinitions, operationName, + directives, ) : operationForQueryFetch( subgraphSchema, @@ -1520,6 +1522,7 @@ class FetchGroup { selection, variableDefinitions, operationName, + directives, ); if (this.generateQueryFragments) { @@ -3094,6 +3097,7 @@ export class QueryPlanner { variableDefinitions: operation.variableDefinitions, fragments: fragments ? new RebasedFragments(fragments) : undefined, operationName: operation.name, + directives: operation.directives, assignedDeferLabels, }); @@ -3304,7 +3308,8 @@ export class QueryPlanner { updatedSelectionSet, operation.variableDefinitions, operation.fragments, - operation.name + operation.name, + operation.directives, ); } @@ -3458,7 +3463,8 @@ function withoutIntrospection(operation: Operation): Operation { operation.selectionSet.lazyMap((s) => isIntrospectionSelection(s) ? undefined : s), operation.variableDefinitions, operation.fragments, - operation.name + operation.name, + operation.directives, ); } @@ -3594,19 +3600,21 @@ function fetchGroupToPlanProcessor({ variableDefinitions, fragments, operationName, + directives, assignedDeferLabels, }: { config: Concrete, variableDefinitions: VariableDefinitions, fragments?: RebasedFragments, operationName?: string, + directives?: readonly Directive[], assignedDeferLabels?: Set, }): FetchGroupProcessor { let counter = 0; return { onFetchGroup: (group: FetchGroup, handledConditions: Conditions) => { const opName = operationName ? `${operationName}__${toValidGraphQLName(group.subgraphName)}__${counter++}` : undefined; - return group.toPlanNode(config, handledConditions, variableDefinitions, fragments, opName); + return group.toPlanNode(config, handledConditions, variableDefinitions, fragments, opName, directives); }, onConditions: (conditions: Conditions, value: PlanNode | undefined) => { if (!value) { @@ -4664,7 +4672,8 @@ function operationForEntitiesFetch( subgraphSchema: Schema, selectionSet: SelectionSet, allVariableDefinitions: VariableDefinitions, - operationName?: string + operationName?: string, + directives?: readonly Directive[], ): Operation { const variableDefinitions = new VariableDefinitions(); variableDefinitions.add(representationsVariableDefinition(subgraphSchema)); @@ -4691,7 +4700,7 @@ function operationForEntitiesFetch( // Note that this is called _before_ named fragments reuse is attempted, so there is not spread in // the selection, hence the `undefined` for fragments. - return new Operation(subgraphSchema, 'query', entitiesCall, variableDefinitions, undefined, operationName); + return new Operation(subgraphSchema, 'query', entitiesCall, variableDefinitions, undefined, operationName, directives); } function operationForQueryFetch( @@ -4699,9 +4708,12 @@ function operationForQueryFetch( rootKind: SchemaRootKind, selectionSet: SelectionSet, allVariableDefinitions: VariableDefinitions, - operationName?: string + operationName?: string, + directives?: readonly Directive[], ): Operation { // Note that this is called _before_ named fragments reuse is attempted, so there is not spread in // the selection, hence the `undefined` for fragments. - return new Operation(subgraphSchema, rootKind, selectionSet, allVariableDefinitions.filter(selectionSet.usedVariables()), undefined, operationName); + return new Operation(subgraphSchema, rootKind, selectionSet, + allVariableDefinitions.filter(selectionSet.usedVariables()), + /*fragments*/undefined, operationName, directives); } From 1f212c08d3e27ce7d2e0a6cdbe80992a88a6d8d7 Mon Sep 17 00:00:00 2001 From: Duckki Oe Date: Fri, 22 Mar 2024 20:13:56 -0700 Subject: [PATCH 2/3] added a test --- .../src/__tests__/buildPlan.test.ts | 136 ++++++++++++++++++ 1 file changed, 136 insertions(+) diff --git a/query-planner-js/src/__tests__/buildPlan.test.ts b/query-planner-js/src/__tests__/buildPlan.test.ts index 35fad5afd..b72669244 100644 --- a/query-planner-js/src/__tests__/buildPlan.test.ts +++ b/query-planner-js/src/__tests__/buildPlan.test.ts @@ -16,6 +16,7 @@ import { FieldNode, OperationDefinitionNode, parse } from 'graphql'; import { composeAndCreatePlanner, composeAndCreatePlannerWithOptions, + findFetchNodes, } from './testHelper'; import { enforceQueryPlannerConfigDefaults } from '../config'; @@ -8386,3 +8387,138 @@ describe('handles fragments with directive conditions', () => { `); }); }); + +describe('handles operations with directives', () => { + const subgraphA = { + name: 'subgraphA', + typeDefs: gql` + directive @operation on MUTATION | QUERY | SUBSCRIPTION + directive @field on FIELD + + type Foo @key(fields: "id") { + id: ID! + bar: String + t: T! + } + + type T @key(fields: "id") { + id: ID! + } + + type Query { + foo: Foo + } + + type Mutation { + updateFoo(bar: String): Foo + } + `, + }; + + const subgraphB = { + name: 'subgraphB', + typeDefs: gql` + directive @operation on MUTATION | QUERY | SUBSCRIPTION + directive @field on FIELD + + type Foo @key(fields: "id") { + id: ID! + baz: Int + } + + type T @key(fields: "id") { + id: ID! + f1: String + } + `, + }; + + const [api, queryPlanner] = composeAndCreatePlanner(subgraphA, subgraphB); + + test('if directives at the operation level are passed down to subgraph queries', () => { + const operation = operationFromDocument( + api, + gql` + query Operation @operation { + foo @field { + bar @field + baz @field + t @field { + f1 @field + } + } + } + `, + ); + + const queryPlan = queryPlanner.buildQueryPlan(operation); + + const A_fetch_nodes = findFetchNodes(subgraphA.name, queryPlan.node); + expect(A_fetch_nodes).toHaveLength(1); + // Note: The query is expected to carry the `@operation` directive. + expect(parse(A_fetch_nodes[0].operation)).toMatchInlineSnapshot(` + query Operation__subgraphA__0 @operation { + foo @field { + __typename + id + bar @field + t @field { + __typename + id + } + } + } + `); + + const B_fetch_nodes = findFetchNodes(subgraphB.name, queryPlan.node); + expect(B_fetch_nodes).toHaveLength(2); + // Note: The query is expected to carry the `@operation` directive. + expect(parse(B_fetch_nodes[0].operation)).toMatchInlineSnapshot(` + query Operation__subgraphB__1($representations: [_Any!]!) @operation { + _entities(representations: $representations) { + ... on Foo { + baz @field + } + } + } + `); + // Note: The query is expected to carry the `@operation` directive. + expect(parse(B_fetch_nodes[1].operation)).toMatchInlineSnapshot(` + query Operation__subgraphB__2($representations: [_Any!]!) @operation { + _entities(representations: $representations) { + ... on T { + f1 @field + } + } + } + `); + }); // end of `test` + + test('if directives on mutations are passed down to subgraph queries', () => { + const operation = operationFromDocument( + api, + gql` + mutation TestMutation @operation { + updateFoo(bar: "something") @field { + id @field + bar @field + } + } + `, + ); + + const queryPlan = queryPlanner.buildQueryPlan(operation); + + const A_fetch_nodes = findFetchNodes(subgraphA.name, queryPlan.node); + expect(A_fetch_nodes).toHaveLength(1); + // Note: The query is expected to carry the `@operation` directive. + expect(parse(A_fetch_nodes[0].operation)).toMatchInlineSnapshot(` + mutation TestMutation__subgraphA__0 @operation { + updateFoo(bar: "something") @field { + id @field + bar @field + } + } + `); + }); // end of `test` +}); // end of `describe` From d3042ad10adc0aad4881c139ffb67d20f35e2786 Mon Sep 17 00:00:00 2001 From: Duckki Oe Date: Fri, 22 Mar 2024 20:34:26 -0700 Subject: [PATCH 3/3] added a changeset --- .changeset/six-shoes-fly.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/six-shoes-fly.md diff --git a/.changeset/six-shoes-fly.md b/.changeset/six-shoes-fly.md new file mode 100644 index 000000000..d3fd47310 --- /dev/null +++ b/.changeset/six-shoes-fly.md @@ -0,0 +1,6 @@ +--- +"@apollo/query-planner": patch +"@apollo/federation-internals": patch +--- + +Fixed query planner to pass the directives from original query to subgraph operations (#2961)