From 6a3a36afd11ccb8489dd22e2a48d4b0ef3d90847 Mon Sep 17 00:00:00 2001 From: Boopathi Rajaa Date: Thu, 30 Mar 2023 14:29:06 +0200 Subject: [PATCH] fix: skip include logic handling multiple paths to same node (#197) * fix: skip include logic handling multiple paths to same node * fix lint errors * add compiler option to switch between old and new implementations; move tests * rename previousPath to parentResponsePath --- .eslintrc.json | 5 +- src/__tests__/directives.test.ts | 385 ++++++++++++++++++++++++++++++- src/ast.ts | 209 ++++++++++++----- src/execution.ts | 44 +++- 4 files changed, 569 insertions(+), 74 deletions(-) diff --git a/.eslintrc.json b/.eslintrc.json index 217c08a0..9da46acf 100644 --- a/.eslintrc.json +++ b/.eslintrc.json @@ -18,7 +18,8 @@ "@typescript-eslint/interface-name-prefix": "off", "comma-dangle": "off", "sort-keys": "off", - "max-classes-per-file": "off" + "max-classes-per-file": "off", + "no-use-before-define": "off" }, "env": { "es6": true, @@ -43,4 +44,4 @@ "dist", "node_modules" ] -} \ No newline at end of file +} diff --git a/src/__tests__/directives.test.ts b/src/__tests__/directives.test.ts index 81f0b588..e6f6ff46 100644 --- a/src/__tests__/directives.test.ts +++ b/src/__tests__/directives.test.ts @@ -4,7 +4,7 @@ import { makeExecutableSchema } from "@graphql-tools/schema"; import { parse } from "graphql"; -import { isCompiledQuery } from "../execution"; +import { CompilerOptions, isCompiledQuery } from "../execution"; import { compileQuery } from "../index"; const testSchema = makeExecutableSchema({ @@ -36,9 +36,17 @@ const testSchema = makeExecutableSchema({ const data = {}; -function executeTestQuery(query: string, variables = {}, schema = testSchema) { +function executeTestQuery( + query: string, + variables = {}, + schema = testSchema, + options: Partial = {} +) { const ast = parse(query); - const compiled: any = compileQuery(schema, ast, "", { debug: true } as any); + const compiled: any = compileQuery(schema, ast, "", { + debug: true, + ...options + } as any); if (!isCompiledQuery(compiled)) { return compiled; } @@ -1433,4 +1441,375 @@ describe("Execute: handles directives", () => { }); }); }); + + describe("multiple paths to reach same field with different skip/include directives", () => { + const schema = makeExecutableSchema({ + typeDefs: ` + type Query { + foo: Foo + } + type Foo { + a: String + foo: Foo + } + `, + resolvers: { + Query: { + foo: () => ({ a: "a", foo: { a: "a" } }) + }, + Foo: { + foo: () => ({ a: "a", foo: { a: "a" } }) + } + } + }); + + test("include directive on fragment spread", () => { + const result = executeTestQuery( + ` + query myQuery($inc: Boolean = true) { + foo { + ...fooFragment + foo { + ...fooFragment @include(if: $inc) + } + } + } + fragment fooFragment on Foo { + a + } + `, + { inc: false }, + schema + ); + + expect(result.data).toEqual({ + foo: { + // fooFragment is included + a: "a", + foo: { + // fooFragment is not included + } + } + }); + }); + + test("skip directive on fragment spread", () => { + const result = executeTestQuery( + ` + query myQuery($skip: Boolean = false) { + foo { + ...fooFragment + foo { + ...fooFragment @skip(if: $skip) + } + } + } + fragment fooFragment on Foo { + a + } + `, + { skip: true }, + schema + ); + + expect(result.data).toEqual({ + foo: { + // fooFragment is included + a: "a", + foo: { + // fooFragment is not included + } + } + }); + }); + }); + + describe("issue-166 - skip directive on fragment spread", () => { + const testSchema = makeExecutableSchema({ + typeDefs: `type Query { + detailContent: [Content!] + } + + type Post implements Content { + id: ID! + title: String! + type: String! + related: [Content] + } + + type Article implements Content { + id: ID! + title: String! + type: String! + } + + interface Content { + id: ID! + title: String! + type: String! + }`, + resolvers: { + Query: { + detailContent: () => [ + { + __typename: "Post", + id: "post:1", + title: "Introduction to GraphQL!", + related: [ + { + __typename: "Article", + id: "article:1", + title: "article Introduction to GraphQL!", + type: "article" + } + ] + }, + { + __typename: "Post", + id: "post:2", + title: "GraphQL-Jit a fast engine for GraphQL", + related: [ + { + __typename: "Article", + id: "article:2", + title: "article GraphQL-Jit a fast engine for GraphQL", + type: "article" + } + ] + }, + { + __typename: "Article", + id: "article:1", + title: "article Introduction to GraphQL!", + type: "article" + }, + { + __typename: "Article", + id: "article:2", + title: "article GraphQL-Jit a fast engine for GraphQL", + type: "article" + } + ] + } + } + }); + + test("skip directive on fragment spread reached via two different inline fragments", () => { + const result = executeTestQuery( + ` + query myQuery($skip: Boolean = false) { + detailContent { + id + ... on Post { + ...titleFragment + } + ... on Article { + ...titleFragment @skip(if: $skip) + } + } + } + fragment titleFragment on Content { + title + } + `, + { skip: true }, + testSchema + ); + + expect(result.data).toEqual({ + detailContent: [ + { + id: "post:1", + title: "Introduction to GraphQL!" + }, + { + id: "post:2", + title: "GraphQL-Jit a fast engine for GraphQL" + }, + { + id: "article:1" + }, + { + id: "article:2" + } + ] + }); + }); + + test("include directive on fragment spread at nested places", async () => { + const query = ` + query TEST($includeOnly: Boolean!) { + detailContent { + ...articleFragment + ... on Post { + id + related { + id + ...articleFragment @include(if: $includeOnly) + } + } + } + } + + fragment articleFragment on Article { + title + type + } + `; + const result = executeTestQuery( + query, + { includeOnly: false }, + testSchema, + { + useExperimentalPathBasedSkipInclude: true + } + ); + + expect(result.data).toEqual({ + detailContent: [ + { + id: "post:1", + related: [{ id: "article:1" }] + }, + { + id: "post:2", + related: [{ id: "article:2" }] + }, + { + title: "article Introduction to GraphQL!", + type: "article" + }, + { + title: "article GraphQL-Jit a fast engine for GraphQL", + type: "article" + } + ] + }); + }); + + describe("minimal reproduction", () => { + const schema = makeExecutableSchema({ + typeDefs: `type Query { + someQuery: [X!] + } + + interface X { + id: ID! + title: String! + } + + type A implements X { + id: ID! + title: String! + related: [X] + } + + type B implements X { + id: ID! + title: String! + } + `, + resolvers: { + Query: { + someQuery: () => [ + { + __typename: "A", + id: "a:1", + title: "a - one", + related: [ + { + __typename: "B", + id: "b:1", + title: "b - one" + } + ] + }, + { + __typename: "B", + id: "b:1", + title: "b - one" + } + ] + } + } + }); + + test("multiple paths to the same fieldnode in fragment - include directive", async () => { + const query = `query TEST($includeOnly: Boolean!) { + someQuery { + ...bFrag + ...on A { + id + related { + id + ...bFrag @include(if: $includeOnly) + } + } + } + } + + fragment bFrag on B { + title + }`; + const result = executeTestQuery(query, { includeOnly: false }, schema, { + useExperimentalPathBasedSkipInclude: true + }); + + expect(result.data).toEqual({ + someQuery: [ + { + id: "a:1", + related: [ + { + id: "b:1" + } + ] + }, + { + title: "b - one" + } + ] + }); + }); + + test("multiple paths to the same fieldnode in fragment - skip directive", async () => { + const query = `query TEST($skipOnly: Boolean!) { + someQuery { + ...bFrag + ...on A { + id + related { + id + ...bFrag @skip(if: $skipOnly) + } + } + } + } + + fragment bFrag on B { + title + }`; + const result = executeTestQuery(query, { skipOnly: true }, schema, { + useExperimentalPathBasedSkipInclude: true + }); + expect(result.errors).toBeUndefined(); + + expect(result.data).toEqual({ + someQuery: [ + { + id: "a:1", + related: [ + { + id: "b:1" + } + ] + }, + { + title: "b - one" + } + ] + }); + }); + }); + }); }); diff --git a/src/ast.ts b/src/ast.ts index a1bd4abf..86cd49d0 100644 --- a/src/ast.ts +++ b/src/ast.ts @@ -33,9 +33,23 @@ import { isAbstractType } from "graphql/type"; import { CompilationContext, GLOBAL_VARIABLES_NAME } from "./execution"; import createInspect from "./inspect"; import { getGraphQLErrorOptions, resolveFieldDef } from "./compat"; +import { memoize4 } from "./memoize"; export interface JitFieldNode extends FieldNode { + /** + * @deprecated Use __internalShouldIncludePath instead + * @see __internalShouldIncludePath + */ __internalShouldInclude?: string; + + // The shouldInclude logic is specific to the current path + // This is because the same fieldNode can be reached from different paths + // - for example, the same fragment used in two different places + __internalShouldIncludePath?: { + // Key is the stringified ObjectPath, + // Value is the shouldInclude logic for that path + [path: string]: string; + }; } export interface FieldsAndNodes { @@ -57,14 +71,17 @@ export function collectFields( runtimeType: GraphQLObjectType, selectionSet: SelectionSetNode, fields: FieldsAndNodes, - visitedFragmentNames: { [key: string]: boolean } + visitedFragmentNames: { [key: string]: boolean }, + parentResponsePath?: ObjectPath ): FieldsAndNodes { return collectFieldsImpl( compilationContext, runtimeType, selectionSet, fields, - visitedFragmentNames + visitedFragmentNames, + undefined, + serializeObjectPathForSkipInclude(parentResponsePath) ); } @@ -78,7 +95,8 @@ function collectFieldsImpl( selectionSet: SelectionSetNode, fields: FieldsAndNodes, visitedFragmentNames: { [key: string]: boolean }, - previousShouldInclude = "" + previousShouldInclude = "", + parentResponsePath = "" ): FieldsAndNodes { for (const selection of selectionSet.selections) { switch (selection.kind) { @@ -89,6 +107,25 @@ function collectFieldsImpl( } const fieldNode: JitFieldNode = selection; + // the current path of the field + // This is used to generate per path skip/include code + // because the same field can be reached from different paths (e.g. fragment reuse) + const currentPath = joinSkipIncludePath( + parentResponsePath, + + // use alias(instead of selection.name.value) if available as the responsePath used for lookup uses alias + name + ); + + // `should include`s generated for the current fieldNode + const compiledSkipInclude = compileSkipInclude( + compilationContext, + selection + ); + + if (!fieldNode.__internalShouldIncludePath) + fieldNode.__internalShouldIncludePath = {}; + /** * Carry over fragment's skip and include code * @@ -105,10 +142,18 @@ function collectFieldsImpl( * ----------------------------- * `should include`s generated for the current fieldNode */ + fieldNode.__internalShouldIncludePath[currentPath] = + joinShouldIncludeCompilations( + fieldNode.__internalShouldIncludePath?.[currentPath] ?? "", + previousShouldInclude, + compiledSkipInclude + ); + + // @deprecated fieldNode.__internalShouldInclude = joinShouldIncludeCompilations( fieldNode.__internalShouldInclude ?? "", previousShouldInclude, - compileSkipInclude(compilationContext, selection) + compiledSkipInclude ); /** @@ -118,13 +163,13 @@ function collectFieldsImpl( * * Refer the function definition for example. */ - augmentFieldNodeTree(compilationContext, fieldNode); + augmentFieldNodeTree(compilationContext, fieldNode, currentPath); fields[name].push(fieldNode); break; } - case Kind.INLINE_FRAGMENT: + case Kind.INLINE_FRAGMENT: { if ( !doesFragmentConditionMatch( compilationContext, @@ -134,6 +179,14 @@ function collectFieldsImpl( ) { continue; } + + // current fragment's shouldInclude + const compiledSkipInclude = compileSkipInclude( + compilationContext, + selection + ); + + // recurse collectFieldsImpl( compilationContext, runtimeType, @@ -144,10 +197,12 @@ function collectFieldsImpl( // `should include`s from previous fragments previousShouldInclude, // current fragment's shouldInclude - compileSkipInclude(compilationContext, selection) - ) + compiledSkipInclude + ), + parentResponsePath ); break; + } case Kind.FRAGMENT_SPREAD: { const fragName = selection.name.value; @@ -162,6 +217,14 @@ function collectFieldsImpl( ) { continue; } + + // current fragment's shouldInclude + const compiledSkipInclude = compileSkipInclude( + compilationContext, + selection + ); + + // recurse collectFieldsImpl( compilationContext, runtimeType, @@ -172,9 +235,11 @@ function collectFieldsImpl( // `should include`s from previous fragments previousShouldInclude, // current fragment's shouldInclude - compileSkipInclude(compilationContext, selection) - ) + compiledSkipInclude + ), + parentResponsePath ); + break; } } @@ -227,13 +292,16 @@ function collectFieldsImpl( * * @param rootFieldNode {JitFieldNode} The root field to traverse from for * adding __internalShouldInclude to all sub field nodes. + * + * @param parentResponsePath {string} The response path of the parent field. */ function augmentFieldNodeTree( compilationContext: CompilationContext, - rootFieldNode: JitFieldNode + rootFieldNode: JitFieldNode, + parentResponsePath: string ) { for (const selection of rootFieldNode.selectionSet?.selections ?? []) { - handle(rootFieldNode, selection); + handle(rootFieldNode, selection, false, parentResponsePath); } /** @@ -243,12 +311,32 @@ function augmentFieldNodeTree( function handle( parentFieldNode: JitFieldNode, selection: SelectionNode, - comesFromFragmentSpread = false + comesFromFragmentSpread = false, + parentResponsePath: string ) { switch (selection.kind) { case Kind.FIELD: { const jitFieldNode: JitFieldNode = selection; + const currentPath = joinSkipIncludePath( + parentResponsePath, + + // use alias(instead of selection.name.value) if available as the responsePath used for lookup uses alias + getFieldEntryKey(jitFieldNode) + ); + if (!comesFromFragmentSpread) { + if (!jitFieldNode.__internalShouldIncludePath) + jitFieldNode.__internalShouldIncludePath = {}; + + jitFieldNode.__internalShouldIncludePath[currentPath] = + joinShouldIncludeCompilations( + parentFieldNode.__internalShouldIncludePath?.[ + parentResponsePath + ] ?? "", + jitFieldNode.__internalShouldIncludePath?.[currentPath] ?? "" + ); + + // @deprecated jitFieldNode.__internalShouldInclude = joinShouldIncludeCompilations( parentFieldNode.__internalShouldInclude ?? "", jitFieldNode.__internalShouldInclude ?? "" @@ -256,20 +344,20 @@ function augmentFieldNodeTree( } // go further down the query tree for (const selection of jitFieldNode.selectionSet?.selections ?? []) { - handle(jitFieldNode, selection); + handle(jitFieldNode, selection, false, currentPath); } break; } case Kind.INLINE_FRAGMENT: { for (const subSelection of selection.selectionSet.selections) { - handle(parentFieldNode, subSelection, true); + handle(parentFieldNode, subSelection, true, parentResponsePath); } break; } case Kind.FRAGMENT_SPREAD: { const fragment = compilationContext.fragments[selection.name.value]; for (const subSelection of fragment.selectionSet.selections) { - handle(parentFieldNode, subSelection, true); + handle(parentFieldNode, subSelection, true, parentResponsePath); } } } @@ -540,12 +628,13 @@ export { resolveFieldDef }; * type. Memoizing ensures the subfields are not repeatedly calculated, which * saves overhead when resolving lists of values. */ -export const collectSubfields = memoize3(_collectSubfields); +export const collectSubfields = memoize4(_collectSubfields); function _collectSubfields( compilationContext: CompilationContext, returnType: GraphQLObjectType, - fieldNodes: FieldNode[] + fieldNodes: FieldNode[], + parentResponsePath?: ObjectPath ): { [key: string]: FieldNode[] } { let subFieldNodes = Object.create(null); const visitedFragmentNames = Object.create(null); @@ -557,57 +646,14 @@ function _collectSubfields( returnType, selectionSet, subFieldNodes, - visitedFragmentNames + visitedFragmentNames, + parentResponsePath ); } } return subFieldNodes; } -function memoize3( - fn: ( - compilationContext: CompilationContext, - returnType: GraphQLObjectType, - fieldNodes: FieldNode[] - ) => { [key: string]: FieldNode[] } -): ( - compilationContext: CompilationContext, - returnType: GraphQLObjectType, - fieldNodes: FieldNode[] -) => { [key: string]: FieldNode[] } { - let cache0: WeakMap; - - function memoized(a1: any, a2: any, a3: any) { - if (!cache0) { - cache0 = new WeakMap(); - } - let cache1 = cache0.get(a1); - let cache2; - if (cache1) { - cache2 = cache1.get(a2); - if (cache2) { - const cachedValue = cache2.get(a3); - if (cachedValue !== undefined) { - return cachedValue; - } - } - } else { - cache1 = new WeakMap(); - cache0.set(a1, cache1); - } - if (!cache2) { - cache2 = new WeakMap(); - cache1.set(a2, cache2); - } - // eslint-disable-next-line prefer-rest-params - const newValue = (fn as any)(...arguments); - cache2.set(a3, newValue); - return newValue; - } - - return memoized; -} - // response path is used for identifying // the info resolver function as well as the path in errros, // the meta type is used for elements that are only to be used for @@ -922,6 +968,43 @@ export function flattenPath( return flattened; } +/** + * Serialize a path for use in the skip/include directives. + * + * @param path The path to serialize + * @returns The path serialized as a string, with the root path first. + */ +export function serializeObjectPathForSkipInclude( + path: ObjectPath | undefined +) { + let serialized = ""; + let curr = path; + while (curr) { + if (curr.type === "literal") { + serialized = joinSkipIncludePath(curr.key, serialized); + } + curr = curr.prev; + } + return serialized; +} + +/** + * join two path segments to a dot notation, handling empty strings + * + * @param a path segment + * @param b path segment + * @returns combined path in dot notation + */ +export function joinSkipIncludePath(a: string, b: string) { + if (a) { + if (b) { + return `${a}.${b}`; + } + return a; + } + return b; +} + function isInvalid(value: any): boolean { // eslint-disable-next-line no-self-compare return value === undefined || value !== value; diff --git a/src/execution.ts b/src/execution.ts index 158115d5..dae6cf7f 100644 --- a/src/execution.ts +++ b/src/execution.ts @@ -44,8 +44,10 @@ import { flattenPath, getArgumentDefs, JitFieldNode, + joinSkipIncludePath, ObjectPath, - resolveFieldDef + resolveFieldDef, + serializeObjectPathForSkipInclude } from "./ast"; import { GraphQLError as GraphqlJitError } from "./error"; import createInspect from "./inspect"; @@ -81,6 +83,19 @@ export interface CompilerOptions { customSerializers: { [key: string]: (v: any) => any }; resolverInfoEnricher?: (inp: ResolveInfoEnricherInput) => object; + + /** + * This option is a temporary workaround to rollout and test the new skip/include behavior. + * It will be removed in the next version along with the old behavior. + * + * Set this to true if you face issues with skip/include in fragment spreads. + * + * default: false + * + * @see https://github.com/zalando-incubator/graphql-jit/pull/197 + * + */ + useExperimentalPathBasedSkipInclude: boolean; } interface ExecutionContext { @@ -224,6 +239,7 @@ export function compileQuery< customJSONSerializer: false, disableLeafSerialization: false, customSerializers: {}, + useExperimentalPathBasedSkipInclude: false, ...partialOptions }; @@ -686,7 +702,7 @@ function compileType( errorDestination ); } else if (isObjectType(type)) { - const fieldMap = collectSubfields(context, type, fieldNodes); + const fieldMap = collectSubfields(context, type, fieldNodes, previousPath); body += compileObjectType( context, type, @@ -852,13 +868,29 @@ function compileObjectType( * * `compilationFor($c1) || compilationFor($c2)` */ + const serializedResponsePath = joinSkipIncludePath( + serializeObjectPathForSkipInclude(responsePath), + name + ); + + const oldFieldCondition = + fieldNodes + .map((it) => it.__internalShouldInclude) + .filter((it) => it) + .join(" || ") || /* if(true) - default */ "true"; + + const fieldCondition = + fieldNodes + .map((it) => it.__internalShouldIncludePath?.[serializedResponsePath]) + .filter((it) => it) + .join(" || ") || /* if(true) - default */ "true"; + body(` ( ${ - fieldNodes - .map((it) => it.__internalShouldInclude) - .filter((it) => it) - .join(" || ") || /* if(true) - default */ "true" + context.options.useExperimentalPathBasedSkipInclude + ? fieldCondition + : oldFieldCondition } ) `);