From 30d30e5208511dda011c61ae668f7e35a95b1966 Mon Sep 17 00:00:00 2001 From: Boopathi Rajaa Date: Sun, 17 May 2020 15:17:14 +0200 Subject: [PATCH 1/8] fix infinite recursion in variables compilation --- src/variables.ts | 261 +++++++++++++++++++++++++++++++++++------------ 1 file changed, 196 insertions(+), 65 deletions(-) diff --git a/src/variables.ts b/src/variables.ts index a7ea581f..d081dd54 100644 --- a/src/variables.ts +++ b/src/variables.ts @@ -17,9 +17,10 @@ import { SourceLocation, typeFromAST, valueFromAST, - VariableDefinitionNode + VariableDefinitionNode, + GraphQLInputObjectType } from "graphql"; -import { addPath, computeLocations, ObjectPath } from "./ast"; +import { addPath, computeLocations, ObjectPath, flattenPath } from "./ast"; import { GraphQLError as GraphQLJITError } from "./error"; import createInspect from "./inspect"; @@ -46,6 +47,7 @@ interface CompilationContext { varDefNode: VariableDefinitionNode; dependencies: Map any>; errorMessage?: string; + hoistedFunctions: Map; } function createSubCompilationContext( @@ -53,6 +55,7 @@ function createSubCompilationContext( ): CompilationContext { return { ...context }; } + export function compileVariableParsing( schema: GraphQLSchema, varDefNodes: ReadonlyArray @@ -62,13 +65,15 @@ export function compileVariableParsing( let mainBody = ""; const dependencies = new Map(); + const hoistedFunctions = new Map(); for (const varDefNode of varDefNodes) { const context: CompilationContext = { varDefNode, depth: 0, inputPath: addPath(undefined, "input"), responsePath: addPath(undefined, "coerced"), - dependencies + dependencies, + hoistedFunctions }; const varName = varDefNode.variable.name.value; const varType = typeFromAST(schema, varDefNode.type as any); @@ -98,7 +103,15 @@ export function compileVariableParsing( )}, "${varName}");\n`; context.inputPath = addPath(context.inputPath, varName); context.responsePath = addPath(context.responsePath, varName); - mainBody += generateInput(context, varType, varName, hasValueName, false); + mainBody += generateInput( + context, + varType, + varName, + hasValueName, + false, + false, + false + ); } if (errors.length > 0) { @@ -107,6 +120,28 @@ export function compileVariableParsing( const gen = genFn(); gen(` + const currentStack = new Set(); + + function getPath(o, path) { + let current = o; + for (const part of path) { + current = current[part]; + } + return current; + } + + function setPath(o, path, value) { + let current = o; + for (let i = 0; i < path.length - 1; i++) { + current = current[path[i]]; + } + current[path[path.length - 1]] = value; + } + + ${Array.from(hoistedFunctions) + .map(([, value]) => value) + .join("\n")} + return function getVariables(input) { const errors = []; const coerced = ${JSON.stringify(coercedValues)} @@ -118,11 +153,13 @@ export function compileVariableParsing( } `); + const generatedFn = gen.toString(); + return Function.apply( null, ["GraphQLJITError", "inspect"] .concat(Array.from(dependencies.keys())) - .concat(gen.toString()) + .concat(generatedFn) ).apply( null, [GraphQLJITError, inspect].concat(Array.from(dependencies.values())) @@ -139,10 +176,17 @@ function generateInput( varType: GraphQLInputType, varName: string, hasValueName: string, - wrapInList: boolean + wrapInList: boolean, + useInputPath: boolean, + useResponsePath: boolean ) { const currentOutput = getObjectPath(context.responsePath); - const currentInput = getObjectPath(context.inputPath); + const responsePath = useResponsePath + ? "responsePath" + : pathToExpression(context.responsePath); + const currentInput = useInputPath + ? `getPath(input, inputPath)` + : getObjectPath(context.inputPath); const errorLocation = printErrorLocation( computeLocations([context.varDefNode]) ); @@ -173,7 +217,7 @@ function generateInput( `); } else { gen(` - if (${hasValueName}) { ${currentOutput} = null; } + if (${hasValueName}) { setPath(coerced, ${responsePath}, null); } `); } gen(`} else {`); @@ -182,9 +226,9 @@ function generateInput( case GraphQLID.name: gen(` if (typeof ${currentInput} === "string") { - ${currentOutput} = ${currentInput}; + setPath(coerced, ${responsePath}, ${currentInput}); } else if (Number.isInteger(${currentInput})) { - ${currentOutput} = ${currentInput}.toString(); + setPath(coerced, ${responsePath}, ${currentInput}.toString()); } else { errors.push(new GraphQLJITError('Variable "$${varName}" got invalid value ' + inspect(${currentInput}) + "; " + @@ -198,7 +242,7 @@ function generateInput( case GraphQLString.name: gen(` if (typeof ${currentInput} === "string") { - ${currentOutput} = ${currentInput}; + setPath(coerced, ${responsePath}, ${currentInput}); } else { errors.push(new GraphQLJITError('Variable "$${varName}" got invalid value ' + inspect(${currentInput}) + "; " + @@ -212,7 +256,7 @@ function generateInput( case GraphQLBoolean.name: gen(` if (typeof ${currentInput} === "boolean") { - ${currentOutput} = ${currentInput}; + setPath(coerced, ${responsePath}, ${currentInput}); } else { errors.push(new GraphQLJITError('Variable "$${varName}" got invalid value ' + inspect(${currentInput}) + "; " + @@ -234,7 +278,7 @@ function generateInput( } cannot represent non 32-bit signed integer value: ' + inspect(${currentInput}), ${errorLocation})); } else { - ${currentOutput} = ${currentInput}; + setPath(coerced, ${responsePath}, ${currentInput}); } } else { errors.push(new GraphQLJITError('Variable "$${varName}" got invalid value ' + @@ -249,7 +293,7 @@ function generateInput( case GraphQLFloat.name: gen(` if (Number.isFinite(${currentInput})) { - ${currentOutput} = ${currentInput}; + setPath(coerced, ${responsePath}, ${currentInput}); } else { errors.push(new GraphQLJITError('Variable "$${varName}" got invalid value ' + inspect(${currentInput}) + "; " + @@ -273,7 +317,7 @@ function generateInput( inspect(${currentInput}) + "; " + 'Expected type ${varType.name}.', ${errorLocation})); } - ${currentOutput} = parseResult; + setPath(coerced, ${responsePath}, parseResult); } catch (error) { errors.push(new GraphQLJITError('Variable "$${varName}" got invalid value ' + inspect(${currentInput}) + "; " + @@ -291,7 +335,7 @@ function generateInput( if (typeof ${currentInput} === "string") { const enumValue = ${varType.name}getValue(${currentInput}); if (enumValue) { - ${currentOutput} = enumValue.value; + setPath(coerced, ${responsePath}, enumValue.value); } else { errors.push( new GraphQLJITError('Variable "$${varName}" got invalid value ' + @@ -322,7 +366,7 @@ function generateInput( subContext.depth++; gen(` if (Array.isArray(${currentInput})) { - ${currentOutput} = []; + setPath(coerced, ${responsePath}, []); for (let ${index} = 0; ${index} < ${currentInput}.length; ++${index}) { const ${hasValueName} = ${getObjectPath(subContext.inputPath)} !== undefined; @@ -331,71 +375,158 @@ function generateInput( varType.ofType, varName, hasValueName, - false + false, + useInputPath, + useResponsePath )} } } else { - ${generateInput(context, varType.ofType, varName, hasValueName, true)} - } - `); - } else if (isInputType(varType)) { - gen(` - if (typeof ${currentInput} !== 'object') { - errors.push(new GraphQLJITError('Variable "$${varName}" got invalid value ' + - inspect(${currentInput}) + "; " + - 'Expected type ${varType.name} to be an object.', ${errorLocation})); - } else { - ${currentOutput} = {}; - `); - const fields = varType.getFields(); - const allowedFields = []; - for (const field of Object.values(fields)) { - const subContext = createSubCompilationContext(context); - allowedFields.push(field.name); - const hasValueName = hasValue(addPath(subContext.inputPath, field.name)); - gen(` - const ${hasValueName} = Object.prototype.hasOwnProperty.call( - ${getObjectPath(subContext.inputPath)}, "${field.name}" - ); - `); - subContext.inputPath = addPath(subContext.inputPath, field.name); - subContext.responsePath = addPath(subContext.responsePath, field.name); - subContext.errorMessage = `'Variable "$${varName}" got invalid value ' + inspect(${currentInput}) + '; '`; - gen(` ${generateInput( - subContext, - field.type, - field.name, + context, + varType.ofType, + varName, hasValueName, - false + true, + useInputPath, + useResponsePath )} - `); - } - - gen(` - const allowedFields = ${JSON.stringify(allowedFields)}; - for (const fieldName of Object.keys(${currentInput})) { - if (!allowedFields.includes(fieldName)) { - errors.push(new GraphQLJITError('Variable "$${varName}" got invalid value ' + - inspect(${currentInput}) + "; " + - 'Field "' + fieldName + '" is not defined by type ${ - varType.name - }.', ${errorLocation})); - break; - } } - }`); + `); + } else if (isInputType(varType)) { + gen( + compileInputObjectType( + context, + varType, + varName, + useInputPath, + useResponsePath + ) + ); } else { /* istanbul ignore next line */ throw new Error(`unknown type: ${varType}`); } if (wrapInList) { - gen(`${currentOutput} = [${currentOutput}];`); + gen( + `setPath(coerced, ${responsePath}, [getPath(coerced, ${responsePath})]);` + ); } gen(`}`); return gen.toString(); } +function compileInputObjectType( + context: CompilationContext, + varType: GraphQLInputObjectType, + varName: string, + useInputPath: boolean, + useResponsePath: boolean +) { + const responsePath = useResponsePath + ? "responsePath" + : pathToExpression(context.responsePath); + const currentInput = useInputPath + ? `getPath(input, inputPath)` + : getObjectPath(context.inputPath); + const errorLocation = printErrorLocation( + computeLocations([context.varDefNode]) + ); + + const gen = genFn(); + + gen(` + if (typeof ${currentInput} !== 'object') { + errors.push(new GraphQLJITError('Variable "$${varName}" got invalid value ' + + inspect(${currentInput}) + "; " + + 'Expected type ${varType.name} to be an object.', ${errorLocation})); + } else { + setPath(coerced, ${responsePath}, {}); + `); + + const fields = varType.getFields(); + const allowedFields = []; + for (const field of Object.values(fields)) { + const subContext = createSubCompilationContext(context); + allowedFields.push(field.name); + const hasValueName = hasValue(addPath(subContext.inputPath, field.name)); + + gen(` + const ${hasValueName} = Object.prototype.hasOwnProperty.call( + ${currentInput}, "${field.name}" + ); + `); + + subContext.inputPath = addPath(subContext.inputPath, field.name); + subContext.responsePath = addPath(subContext.responsePath, field.name); + subContext.errorMessage = `'Variable "$${varName}" got invalid value ' + inspect(${currentInput}) + '; '`; + + const varTypeParserName = "__fieldParser" + varType.name + field.name; + + const nextInput = flattenPath(subContext.inputPath) + .map(({ key }) => key) + .reverse() + .slice(1); + + gen(` + ${varTypeParserName}( + input, + ${JSON.stringify(nextInput)}, + coerced, + ${pathToExpression(subContext.responsePath)}, + errors, + ${hasValueName} + ); + `); + + if (!context.hoistedFunctions.has(varTypeParserName)) { + context.hoistedFunctions.set(varTypeParserName, ""); + context.hoistedFunctions.set( + varTypeParserName, + ` + function ${varTypeParserName} (input, inputPath, coerced, responsePath, errors, ${hasValueName}) { + ${generateInput( + subContext, + field.type, + field.name, + hasValueName, + false, + true, + true + )} + } + ` + ); + } + } + + gen(` + const allowedFields = ${JSON.stringify(allowedFields)}; + for (const fieldName of Object.keys(${currentInput})) { + if (!allowedFields.includes(fieldName)) { + errors.push(new GraphQLJITError('Variable "$${varName}" got invalid value ' + + inspect(${currentInput}) + "; " + + 'Field "' + fieldName + '" is not defined by type ${ + varType.name + }.', ${errorLocation})); + break; + } + } + `); + + gen(`}`); + + return gen.toString(); +} + +function pathToExpression(path: ObjectPath) { + return JSON.stringify( + flattenPath(path) + .map(({ key }) => key) + .reverse() + .slice(1) + ); +} + function hasValue(path: ObjectPath) { const flattened = []; let curr: ObjectPath | undefined = path; From 2f9022ff7da16a87beaa08ddafb56e77ef3503a0 Mon Sep 17 00:00:00 2001 From: Boopathi Rajaa Date: Mon, 18 May 2020 01:19:07 +0200 Subject: [PATCH 2/8] fix lists; handle runtime recursion and add tests for recursive variables --- src/__tests__/recursive-input.test.ts | 357 ++++++++++++++++++++++++++ src/variables.ts | 168 ++++++++---- 2 files changed, 471 insertions(+), 54 deletions(-) create mode 100644 src/__tests__/recursive-input.test.ts diff --git a/src/__tests__/recursive-input.test.ts b/src/__tests__/recursive-input.test.ts new file mode 100644 index 00000000..382af7ca --- /dev/null +++ b/src/__tests__/recursive-input.test.ts @@ -0,0 +1,357 @@ +import { DocumentNode, GraphQLSchema, parse, validate } from "graphql"; +import { makeExecutableSchema } from "graphql-tools"; +import { compileQuery, isCompiledQuery } from "../execution"; + +describe("recursive input types", () => { + describe("simple recursive input", () => { + const schema = makeExecutableSchema({ + typeDefs: ` + type Query { + foo(input: FooInput): String + } + input FooInput { + foo: FooInput + } + `, + resolvers: { + Query: { + foo(_, args) { + // used as the acutal value in test matchers + return JSON.stringify(args); + } + } + } + }); + + test("should not fail for recursive input without variables", () => { + const query = parse(` + { + foo(input: { + foo: { + foo: { + foo: { + foo: {} + } + } + } + }) + } + `); + + const result = executeQuery(schema, query); + + expect(result.errors).toBeUndefined(); + expect(result.data.foo).toBe( + JSON.stringify({ + input: { + foo: { foo: { foo: { foo: {} } } } + } + }) + ); + }); + + test("should not fail with variables using recursive input types", () => { + const document = parse(` + query ($f: FooInput) { + foo(input: $f) + } + `); + const variables = { + f: { + foo: { foo: { foo: {} } } + } + }; + + const result = executeQuery(schema, document, variables); + expect(result.errors).toBeUndefined(); + expect(result.data.foo).toBe( + JSON.stringify({ + input: { foo: { foo: { foo: {} } } } + }) + ); + }); + + // when the recursive variable appers at a nested level + test("should not fail with variables using recursive input types - 2", () => { + const document = parse(` + query ($f: FooInput) { + foo(input: { + foo: { foo: { foo: $f } } + }) + } + `); + const variables = { + f: { + foo: { foo: { foo: {} } } + } + }; + + const result = executeQuery(schema, document, variables); + expect(result.errors).toBeUndefined(); + expect(result.data.foo).toBe( + JSON.stringify({ + input: { foo: { foo: { foo: { foo: { foo: { foo: {} } } } } } } + }) + ); + }); + + test("should work with multiple variables using the same recursive input type", () => { + const document = parse(` + query ($f: FooInput, $g: FooInput) { + a: foo(input: $f) + b: foo(input: $g) + } + `); + const variables = { + f: { + foo: { foo: { foo: {} } } + }, + g: { + foo: {} + } + }; + + const result = executeQuery(schema, document, variables); + expect(result.errors).toBeUndefined(); + expect(result.data.a).toBe( + JSON.stringify({ + input: { foo: { foo: { foo: {} } } } + }) + ); + expect(result.data.b).toBe( + JSON.stringify({ + input: { foo: {} } + }) + ); + }); + + test("should work with multiple variables using the same recursive input type - 2 (reverse order)", () => { + const document = parse(` + query ($f: FooInput, $g: FooInput) { + a: foo(input: $g) + b: foo(input: $f) + } + `); + const variables = { + g: { + foo: {} + }, + f: { + foo: { foo: { foo: {} } } + } + }; + + const result = executeQuery(schema, document, variables); + expect(result.errors).toBeUndefined(); + expect(result.data.b).toBe( + JSON.stringify({ + input: { foo: { foo: { foo: {} } } } + }) + ); + expect(result.data.a).toBe( + JSON.stringify({ + input: { foo: {} } + }) + ); + }); + + test("should capture and throw error when recursive value is passed", () => { + const document = parse(` + query ($f: FooInput) { + foo(input: $f) + } + `); + const variables: any = { + f: { + foo: { foo: { foo: {} } } + } + }; + variables.f.foo = variables.f; + + const result = executeQuery(schema, document, variables); + expect(result.errors[0].message).toBe( + "Circular reference detected in input variable '$f' at foo.foo" + ); + }); + }); + + describe("mutually recursive input types", () => { + const schema = makeExecutableSchema({ + typeDefs: ` + type Query { + products(filter: Filter): String + } + input Filter { + and: AndFilter + or: OrFilter + like: String + } + input AndFilter { + left: Filter + right: Filter + } + input OrFilter { + left: Filter + right: Filter + } + `, + resolvers: { + Query: { + products(_, args) { + // used as the acutal value in test matchers + return JSON.stringify(args); + } + } + } + }); + + test("should not fail for mutually recursive variables", () => { + const document = parse(` + query ($filter1: Filter) { + products(filter: $filter1) + } + `); + + const variables = { + filter1: { + and: { + left: { + like: "windows" + }, + right: { + or: { + left: { + like: "xp" + }, + right: { + like: "vista" + } + } + } + } + } + }; + + const result = executeQuery(schema, document, variables); + expect(JSON.parse(result.data.products).filter).toEqual( + variables.filter1 + ); + }); + + test("should not fail for mutually recursive variables - multiple variables", () => { + const document = parse(` + query ($aFilter: Filter, $bFilter: Filter) { + a: products(filter: $aFilter) + b: products(filter: $bFilter) + } + `); + + const variables = { + aFilter: { + and: { + left: { + like: "windows" + }, + right: { + or: { + left: { + like: "xp" + }, + right: { + like: "vista" + } + } + } + } + }, + bFilter: { + like: "mac", + or: { + left: { + like: "10" + }, + right: { + like: "11" + } + } + } + }; + + const result = executeQuery(schema, document, variables); + expect(JSON.parse(result.data.a).filter).toEqual(variables.aFilter); + expect(JSON.parse(result.data.b).filter).toEqual(variables.bFilter); + }); + + // when the mutually recursive input type appears at nested level + // instead of the top-level variable + test("should not fail for mutually recursive variables - 2", () => { + const document = parse(` + query ($macFilter: OrFilter) { + products(filter: { + like: "mac" + and: { + left: { like: "User" } + right: { like: "foo" } + } + or: $macFilter + }) + } + `); + + const variables = { + macFilter: { + left: { like: "Applications/Safari" }, + right: { like: "Applications/Notes" } + } + }; + + const result = executeQuery(schema, document, variables); + expect(JSON.parse(result.data.products).filter.or).toEqual( + variables.macFilter + ); + }); + + test("should throw for mutually recursive runtime values", () => { + const document = parse(` + query ($filter1: Filter) { + products(filter: $filter1) + } + `); + + const variables: any = { + filter1: { + and: { + left: {} + }, + or: { + left: {} + } + } + }; + // create mututal recursion at + // and.left.or.left.and <-> or.left.and.left.or + variables.filter1.and.left.or = variables.filter1.or; + variables.filter1.or.left.and = variables.filter1.and; + + const result = executeQuery(schema, document, variables); + expect(result.errors[0].message).toBe( + "Circular reference detected in input variable '$filter1' at and.left.or.left.and" + ); + }); + }); +}); + +function executeQuery( + schema: GraphQLSchema, + document: DocumentNode, + variableValues?: any, + rootValue?: any, + contextValue?: any, + operationName?: string +) { + const prepared: any = compileQuery(schema, document as any, operationName); + if (!isCompiledQuery(prepared)) { + return prepared; + } + return prepared.query(rootValue, contextValue, variableValues || {}); +} diff --git a/src/variables.ts b/src/variables.ts index d081dd54..cf69a950 100644 --- a/src/variables.ts +++ b/src/variables.ts @@ -18,7 +18,9 @@ import { typeFromAST, valueFromAST, VariableDefinitionNode, - GraphQLInputObjectType + GraphQLInputObjectType, + GraphQLList, + isInputObjectType } from "graphql"; import { addPath, computeLocations, ObjectPath, flattenPath } from "./ast"; import { GraphQLError as GraphQLJITError } from "./error"; @@ -120,7 +122,7 @@ export function compileVariableParsing( const gen = genFn(); gen(` - const currentStack = new Set(); + const visitedInputValues = new Set(); function getPath(o, path) { let current = o; @@ -352,47 +354,16 @@ function generateInput( } `); } else if (isListType(varType)) { - context.errorMessage = `'Variable "$${varName}" got invalid value ' + inspect(${currentInput}) + '; '`; - const hasValueName = hasValue(context.inputPath); - const index = `idx${context.depth}`; - - const subContext = createSubCompilationContext(context); - subContext.responsePath = addPath( - subContext.responsePath, - index, - "variable" + gen( + compileInputListType( + context, + varType, + varName, + useInputPath, + useResponsePath + ) ); - subContext.inputPath = addPath(subContext.inputPath, index, "variable"); - subContext.depth++; - gen(` - if (Array.isArray(${currentInput})) { - setPath(coerced, ${responsePath}, []); - for (let ${index} = 0; ${index} < ${currentInput}.length; ++${index}) { - const ${hasValueName} = - ${getObjectPath(subContext.inputPath)} !== undefined; - ${generateInput( - subContext, - varType.ofType, - varName, - hasValueName, - false, - useInputPath, - useResponsePath - )} - } - } else { - ${generateInput( - context, - varType.ofType, - varName, - hasValueName, - true, - useInputPath, - useResponsePath - )} - } - `); - } else if (isInputType(varType)) { + } else if (isInputObjectType(varType)) { gen( compileInputObjectType( context, @@ -415,6 +386,71 @@ function generateInput( return gen.toString(); } +function compileInputListType( + context: CompilationContext, + varType: GraphQLList, + varName: string, + useInputPath: boolean, + useResponsePath: boolean +) { + const responsePath = useResponsePath + ? "responsePath" + : pathToExpression(context.responsePath); + const currentInput = useInputPath + ? `getPath(input, inputPath)` + : getObjectPath(context.inputPath); + + const gen = genFn(); + + context.errorMessage = `'Variable "$${varName}" got invalid value ' + inspect(${currentInput}) + '; '`; + const hasValueName = hasValue(context.inputPath); + const index = `idx${context.depth}`; + + const subContext = createSubCompilationContext(context); + subContext.responsePath = addPath(subContext.responsePath, index, "variable"); + subContext.inputPath = addPath(subContext.inputPath, index, "variable"); + subContext.depth++; + + const nextInputPath = useInputPath + ? `getPath(input, inputPath)[${index}]` + : getObjectPath(context.inputPath); + + gen(` + if (Array.isArray(${currentInput})) { + setPath(coerced, ${responsePath}, []); + for (let ${index} = 0; ${index} < ${currentInput}.length; ++${index}) { + const ${hasValueName} = + ${nextInputPath} !== undefined; + ${generateInput( + subContext, + varType.ofType, + varName, + hasValueName, + false, + // TODO(boopathi): Investigate a bit more + // why we cannot forward useInputPath, and useResponsePath + false, + false + )} + } + } else { + ${generateInput( + context, + varType.ofType, + varName, + hasValueName, + true, + // TODO(boopathi): Investigate a bit more + // why we cannot forward useInputPath, and useResponsePath + false, + false + )} + } + `); + + return gen.toString(); +} + function compileInputObjectType( context: CompilationContext, varType: GraphQLInputObjectType, @@ -462,17 +498,20 @@ function compileInputObjectType( const varTypeParserName = "__fieldParser" + varType.name + field.name; - const nextInput = flattenPath(subContext.inputPath) - .map(({ key }) => key) - .reverse() - .slice(1); + const nextInputPath = useInputPath + ? `inputPath.concat("${field.name}")` + : pathToExpression(subContext.inputPath); + + const nextResponsePath = useResponsePath + ? `responsePath.concat("${field.name}")` + : pathToExpression(subContext.responsePath); gen(` ${varTypeParserName}( input, - ${JSON.stringify(nextInput)}, + ${nextInputPath}, coerced, - ${pathToExpression(subContext.responsePath)}, + ${nextResponsePath}, errors, ${hasValueName} ); @@ -484,6 +523,23 @@ function compileInputObjectType( varTypeParserName, ` function ${varTypeParserName} (input, inputPath, coerced, responsePath, errors, ${hasValueName}) { + const __inputValue = getPath(input, inputPath); + if (visitedInputValues.has(__inputValue)) { + errors.push( + new GraphQLJITError( + "Circular reference detected in input variable '$" + + inputPath[0] + + "' at " + + inputPath.slice(1).join("."), + [${errorLocation}] + ) + ); + return; + } + if (__inputValue != null) { + visitedInputValues.add(__inputValue); + } + ${generateInput( subContext, field.type, @@ -519,12 +575,16 @@ function compileInputObjectType( } function pathToExpression(path: ObjectPath) { - return JSON.stringify( - flattenPath(path) - .map(({ key }) => key) - .reverse() - .slice(1) - ); + const expr = flattenPath(path) + // object access pattern - flatten returns in reverse order from leaf to root + .reverse() + // remove the variable name (input/coerced - are the cases in this file) + .slice(1) + // serialize + .map(({ key, type }) => (type === "literal" ? `"${key}"` : key)) + .join(","); + + return `[${expr}]`; } function hasValue(path: ObjectPath) { From 93553eac32444f17fd3a5070065ffb545bde3158 Mon Sep 17 00:00:00 2001 From: Boopathi Rajaa Date: Mon, 18 May 2020 01:20:02 +0200 Subject: [PATCH 3/8] fix lint errors --- src/variables.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/variables.ts b/src/variables.ts index cf69a950..1f42c618 100644 --- a/src/variables.ts +++ b/src/variables.ts @@ -4,11 +4,14 @@ import { GraphQLError, GraphQLFloat, GraphQLID, + GraphQLInputObjectType, GraphQLInputType, GraphQLInt, + GraphQLList, GraphQLSchema, GraphQLString, isEnumType, + isInputObjectType, isInputType, isListType, isNonNullType, @@ -17,12 +20,9 @@ import { SourceLocation, typeFromAST, valueFromAST, - VariableDefinitionNode, - GraphQLInputObjectType, - GraphQLList, - isInputObjectType + VariableDefinitionNode } from "graphql"; -import { addPath, computeLocations, ObjectPath, flattenPath } from "./ast"; +import { addPath, computeLocations, flattenPath, ObjectPath } from "./ast"; import { GraphQLError as GraphQLJITError } from "./error"; import createInspect from "./inspect"; From 18c752e88786e84f0abca0a2c4729782063726ce Mon Sep 17 00:00:00 2001 From: Boopathi Rajaa Date: Thu, 21 May 2020 11:45:28 +0200 Subject: [PATCH 4/8] fix recursion in lists; handle only objects in computing visited links --- src/__tests__/recursive-input.test.ts | 56 ++++++++++++++++++++++++- src/variables.ts | 59 +++++++++++++++++++-------- 2 files changed, 96 insertions(+), 19 deletions(-) diff --git a/src/__tests__/recursive-input.test.ts b/src/__tests__/recursive-input.test.ts index 382af7ca..ee615221 100644 --- a/src/__tests__/recursive-input.test.ts +++ b/src/__tests__/recursive-input.test.ts @@ -16,7 +16,7 @@ describe("recursive input types", () => { resolvers: { Query: { foo(_, args) { - // used as the acutal value in test matchers + // used as the actual value in test matchers return JSON.stringify(args); } } @@ -50,6 +50,11 @@ describe("recursive input types", () => { ); }); + // Reminder to self + // TODO(boopathi): write tests to handle same value inputs + // { foo: 3, bar: 3 } + // Solution: object types only + test("should not fail with variables using recursive input types", () => { const document = parse(` query ($f: FooInput) { @@ -198,7 +203,7 @@ describe("recursive input types", () => { resolvers: { Query: { products(_, args) { - // used as the acutal value in test matchers + // used as the actual value in test matchers return JSON.stringify(args); } } @@ -339,6 +344,53 @@ describe("recursive input types", () => { ); }); }); + + describe("lists and non-nulls", () => { + const schema = makeExecutableSchema({ + typeDefs: ` + type Query { + items(filters: [Filter]): String + } + input Filter { + or: [Filter] + and: [Filter] + like: String + } + `, + resolvers: { + Query: { + items(_, input) { + // used as the actual value in test matchers + return JSON.stringify(input); + } + } + } + }); + + test("should work with recursion in lists", () => { + const document = parse(` + query ($filters: [Filter]) { + items(filters: $filters) + } + `); + const variables = { + filters: [ + { + or: [ + { + like: "gallery", + or: [{ like: "photo" }, { like: "video" }] + } + ] + } + ] + }; + + const result = executeQuery(schema, document, variables); + expect(result.errors).toBeUndefined(); + expect(JSON.parse(result.data.items).filters).toEqual(variables.filters); + }); + }); }); function executeQuery( diff --git a/src/variables.ts b/src/variables.ts index 1f42c618..743a4fda 100644 --- a/src/variables.ts +++ b/src/variables.ts @@ -140,6 +140,10 @@ export function compileVariableParsing( current[path[path.length - 1]] = value; } + function isObject(o) { + return o != null && typeof o === "object" && !Array.isArray(o); + } + ${Array.from(hoistedFunctions) .map(([, value]) => value) .join("\n")} @@ -393,6 +397,9 @@ function compileInputListType( useInputPath: boolean, useResponsePath: boolean ) { + const inputPath = useInputPath + ? `inputPath` + : pathToExpression(context.inputPath); const responsePath = useResponsePath ? "responsePath" : pathToExpression(context.responsePath); @@ -411,26 +418,31 @@ function compileInputListType( subContext.inputPath = addPath(subContext.inputPath, index, "variable"); subContext.depth++; - const nextInputPath = useInputPath - ? `getPath(input, inputPath)[${index}]` - : getObjectPath(context.inputPath); - gen(` if (Array.isArray(${currentInput})) { setPath(coerced, ${responsePath}, []); + const previousInputPath = ${inputPath}; + const previousResponsePath = ${responsePath}; for (let ${index} = 0; ${index} < ${currentInput}.length; ++${index}) { - const ${hasValueName} = - ${nextInputPath} !== undefined; + const inputPath = previousInputPath.concat(${index}); + const responsePath = previousResponsePath.concat(${index}); + + const __inputListValue = getPath(input, inputPath); + + if (isObject(__inputListValue)) { + visitedInputValues.add(__inputListValue); + } + + const ${hasValueName} = __inputListValue !== undefined; + ${generateInput( subContext, varType.ofType, varName, hasValueName, false, - // TODO(boopathi): Investigate a bit more - // why we cannot forward useInputPath, and useResponsePath - false, - false + useInputPath, + useResponsePath )} } } else { @@ -440,10 +452,8 @@ function compileInputListType( varName, hasValueName, true, - // TODO(boopathi): Investigate a bit more - // why we cannot forward useInputPath, and useResponsePath - false, - false + useInputPath, + useResponsePath )} } `); @@ -506,6 +516,9 @@ function compileInputObjectType( ? `responsePath.concat("${field.name}")` : pathToExpression(subContext.responsePath); + const previousIndices = context.depth > 1 ? `indices` : `[]`; + const nextIndex = context.depth > 1 ? `idx${context.depth - 1}` : ""; + gen(` ${varTypeParserName}( input, @@ -513,7 +526,9 @@ function compileInputObjectType( coerced, ${nextResponsePath}, errors, - ${hasValueName} + ${hasValueName}, + ...${previousIndices}, + ${nextIndex} ); `); @@ -522,8 +537,17 @@ function compileInputObjectType( context.hoistedFunctions.set( varTypeParserName, ` - function ${varTypeParserName} (input, inputPath, coerced, responsePath, errors, ${hasValueName}) { + function ${varTypeParserName} ( + input, + inputPath, + coerced, + responsePath, + errors, + ${hasValueName}, + ...indices + ) { const __inputValue = getPath(input, inputPath); + if (visitedInputValues.has(__inputValue)) { errors.push( new GraphQLJITError( @@ -536,7 +560,8 @@ function compileInputObjectType( ); return; } - if (__inputValue != null) { + + if (isObject(__inputValue)) { visitedInputValues.add(__inputValue); } From 89918c73940db151e9cd94948f1aabefa06a864b Mon Sep 17 00:00:00 2001 From: Boopathi Rajaa Date: Thu, 21 May 2020 13:20:12 +0200 Subject: [PATCH 5/8] fix recursive list references; use object for generateInput params --- src/__tests__/recursive-input.test.ts | 146 ++++++++++++++++++++++- src/variables.ts | 160 ++++++++++++++++---------- 2 files changed, 244 insertions(+), 62 deletions(-) diff --git a/src/__tests__/recursive-input.test.ts b/src/__tests__/recursive-input.test.ts index ee615221..4e7aa75d 100644 --- a/src/__tests__/recursive-input.test.ts +++ b/src/__tests__/recursive-input.test.ts @@ -345,7 +345,7 @@ describe("recursive input types", () => { }); }); - describe("lists and non-nulls", () => { + describe("lists", () => { const schema = makeExecutableSchema({ typeDefs: ` type Query { @@ -367,7 +367,7 @@ describe("recursive input types", () => { } }); - test("should work with recursion in lists", () => { + test("should work with recursive types in lists", () => { const document = parse(` query ($filters: [Filter]) { items(filters: $filters) @@ -390,6 +390,148 @@ describe("recursive input types", () => { expect(result.errors).toBeUndefined(); expect(JSON.parse(result.data.items).filters).toEqual(variables.filters); }); + + test("should throw error for runtime circular dependencies in lists", () => { + const document = parse(` + query ($filters: [Filter]) { + items(filters: $filters) + } + `); + const variables: any = { + filters: [{}] + }; + variables.filters[0].or = variables.filters; + + const result = executeQuery(schema, document, variables); + expect(result.errors[0].message).toBe( + `Circular reference detected in input variable '$filters' at 0.or.0` + ); + }); + + test("should throw error for runtime circular dependencies in lists - 2", () => { + const document = parse(` + query ($filters: [Filter]) { + items(filters: $filters) + } + `); + const variables: any = { + filters: [ + { + or: [ + { + like: "gallery", + or: [{ like: "photo" }] + } + ] + } + ] + }; + variables.filters[0].or[0].or[0].or = variables.filters; + + const result = executeQuery(schema, document, variables); + expect(result.errors[0].message).toBe( + `Circular reference detected in input variable '$filters' at 0.or.0.or.0.or.0` + ); + }); + + test("should throw error for runtime circular dependencies in lists - 3", () => { + const document = parse(` + query ($filters: [Filter]) { + items(filters: $filters) + } + `); + const variables: any = { + filters: [ + { + or: [ + { + and: [{ like: "foo" }, { like: "bar" }] + } + ] + } + ] + }; + variables.filters[0].or[1] = variables.filters[0]; + + const result = executeQuery(schema, document, variables); + expect(result.errors[0].message).toBe( + `Circular reference detected in input variable '$filters' at 0.or.1` + ); + }); + }); + + describe("lists - 2", () => { + const schema = makeExecutableSchema({ + typeDefs: ` + type Query { + flatten(list: [[[[[Item]]]]]): String + } + input Item { + id: ID + } + `, + resolvers: { + Query: { + flatten(_, input) { + // used as the actual value in test matchers + return JSON.stringify(input); + } + } + } + }); + + test("should work with recursive types in lists", () => { + const document = parse(` + query ($list: [[[[[Item]]]]]) { + flatten(list: $list) + } + `); + const variables = { + list: [ + [[[[{ id: "1" }, { id: "2" }]]]], + [[[[{ id: "3" }, { id: "4" }]]]] + ] + }; + + const result = executeQuery(schema, document, variables); + expect(result.errors).toBeUndefined(); + expect(JSON.parse(result.data.flatten).list).toEqual(variables.list); + }); + + test("should throw when lists are circular referenced", () => { + const document = parse(` + query ($list: [[[[[Item]]]]]) { + flatten(list: $list) + } + `); + const variables: any = { + list: [] + }; + variables.list.push(variables.list); + + const result = executeQuery(schema, document, variables); + expect(result.errors[0].message).toBe( + "Circular reference detected in input variable '$list' at 0.0" + ); + }); + + test("should throw when lists are mutually circular referenced", () => { + const document = parse(` + query ($list: [[[[[Item]]]]]) { + flatten(list: $list) + } + `); + const variables: any = { + list: [[], []] + }; + variables.list[0].push(variables.list[1]); + variables.list[1].push(variables.list[0]); + + const result = executeQuery(schema, document, variables); + expect(result.errors[0].message).toBe( + "Circular reference detected in input variable '$list' at 0.0.0" + ); + }); }); }); diff --git a/src/variables.ts b/src/variables.ts index 743a4fda..6b5c5eb3 100644 --- a/src/variables.ts +++ b/src/variables.ts @@ -105,15 +105,15 @@ export function compileVariableParsing( )}, "${varName}");\n`; context.inputPath = addPath(context.inputPath, varName); context.responsePath = addPath(context.responsePath, varName); - mainBody += generateInput( + mainBody += generateInput({ context, varType, varName, hasValueName, - false, - false, - false - ); + wrapInList: false, + useInputPath: false, + useResponsePath: false + }); } if (errors.length > 0) { @@ -140,8 +140,8 @@ export function compileVariableParsing( current[path[path.length - 1]] = value; } - function isObject(o) { - return o != null && typeof o === "object" && !Array.isArray(o); + function isObjectLike(o) { + return o != null && typeof o === "object"; } ${Array.from(hoistedFunctions) @@ -161,6 +161,12 @@ export function compileVariableParsing( const generatedFn = gen.toString(); + // require("fs").writeFileSync( + // `${__dirname}/../test-variables.js`, + // require("prettier").format(generatedFn, { parser: "typescript" }), + // "utf8" + // ); + return Function.apply( null, ["GraphQLJITError", "inspect"] @@ -177,15 +183,25 @@ export function compileVariableParsing( const MAX_32BIT_INT = 2147483647; const MIN_32BIT_INT = -2147483648; -function generateInput( - context: CompilationContext, - varType: GraphQLInputType, - varName: string, - hasValueName: string, - wrapInList: boolean, - useInputPath: boolean, - useResponsePath: boolean -) { +interface GenerateInputParams { + context: CompilationContext; + varType: GraphQLInputType; + varName: string; + hasValueName: string; + wrapInList: boolean; + useInputPath: boolean; + useResponsePath: boolean; +} + +function generateInput({ + context, + varType, + varName, + hasValueName, + wrapInList, + useInputPath, + useResponsePath +}: GenerateInputParams) { const currentOutput = getObjectPath(context.responsePath); const responsePath = useResponsePath ? "responsePath" @@ -406,6 +422,9 @@ function compileInputListType( const currentInput = useInputPath ? `getPath(input, inputPath)` : getObjectPath(context.inputPath); + const errorLocation = printErrorLocation( + computeLocations([context.varDefNode]) + ); const gen = genFn(); @@ -429,32 +448,35 @@ function compileInputListType( const __inputListValue = getPath(input, inputPath); - if (isObject(__inputListValue)) { - visitedInputValues.add(__inputListValue); - } + ${generateCircularReferenceCheck( + "__inputListValue", + "inputPath", + errorLocation, + false + )} const ${hasValueName} = __inputListValue !== undefined; - ${generateInput( - subContext, - varType.ofType, + ${generateInput({ + context: subContext, + varType: varType.ofType, varName, hasValueName, - false, + wrapInList: false, useInputPath, useResponsePath - )} + })} } } else { - ${generateInput( + ${generateInput({ context, - varType.ofType, + varType: varType.ofType, varName, hasValueName, - true, + wrapInList: true, useInputPath, useResponsePath - )} + })} } `); @@ -516,9 +538,6 @@ function compileInputObjectType( ? `responsePath.concat("${field.name}")` : pathToExpression(subContext.responsePath); - const previousIndices = context.depth > 1 ? `indices` : `[]`; - const nextIndex = context.depth > 1 ? `idx${context.depth - 1}` : ""; - gen(` ${varTypeParserName}( input, @@ -526,9 +545,7 @@ function compileInputObjectType( coerced, ${nextResponsePath}, errors, - ${hasValueName}, - ...${previousIndices}, - ${nextIndex} + ${hasValueName} ); `); @@ -543,37 +560,26 @@ function compileInputObjectType( coerced, responsePath, errors, - ${hasValueName}, - ...indices + ${hasValueName} ) { const __inputValue = getPath(input, inputPath); - if (visitedInputValues.has(__inputValue)) { - errors.push( - new GraphQLJITError( - "Circular reference detected in input variable '$" - + inputPath[0] - + "' at " - + inputPath.slice(1).join("."), - [${errorLocation}] - ) - ); - return; - } - - if (isObject(__inputValue)) { - visitedInputValues.add(__inputValue); - } - - ${generateInput( - subContext, - field.type, - field.name, - hasValueName, - false, - true, + ${generateCircularReferenceCheck( + "__inputValue", + "inputPath", + errorLocation, true )} + + ${generateInput({ + context: subContext, + varType: field.type, + varName: field.name, + hasValueName, + wrapInList: false, + useInputPath: true, + useResponsePath: true + })} } ` ); @@ -599,6 +605,40 @@ function compileInputObjectType( return gen.toString(); } +function generateCircularReferenceCheck( + inputValueName: string, + inputPathName: string, + errorLocation: string, + shouldReturn: boolean +) { + const gen = genFn(); + gen(`if (visitedInputValues.has(${inputValueName})) {`); + gen(` + errors.push( + new GraphQLJITError( + "Circular reference detected in input variable '$" + + ${inputPathName}[0] + + "' at " + + ${inputPathName}.slice(1).join("."), + [${errorLocation}] + ) + ); + `); + + if (shouldReturn) { + gen(`return;`); + } + + gen(`}`); + gen(` + if (isObjectLike(${inputValueName})) { + visitedInputValues.add(${inputValueName}); + } + `); + + return gen.toString(); +} + function pathToExpression(path: ObjectPath) { const expr = flattenPath(path) // object access pattern - flatten returns in reverse order from leaf to root From 5623e3ec875e48bbc1412982f121a01a1bc106b2 Mon Sep 17 00:00:00 2001 From: Boopathi Rajaa Date: Thu, 21 May 2020 13:21:36 +0200 Subject: [PATCH 6/8] remove fn printing --- src/variables.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/variables.ts b/src/variables.ts index 6b5c5eb3..11c7a404 100644 --- a/src/variables.ts +++ b/src/variables.ts @@ -161,12 +161,6 @@ export function compileVariableParsing( const generatedFn = gen.toString(); - // require("fs").writeFileSync( - // `${__dirname}/../test-variables.js`, - // require("prettier").format(generatedFn, { parser: "typescript" }), - // "utf8" - // ); - return Function.apply( null, ["GraphQLJITError", "inspect"] From 94cad3ccbab93036864eea40d59b1fe9da098c94 Mon Sep 17 00:00:00 2001 From: Boopathi Rajaa Date: Fri, 22 May 2020 18:40:59 +0200 Subject: [PATCH 7/8] make circular reference checks optional --- src/__tests__/recursive-input.test.ts | 24 ++++++++++----------- src/execution.ts | 18 ++++++++++++++++ src/variables.ts | 30 +++++++++++++++++++++++++++ 3 files changed, 60 insertions(+), 12 deletions(-) diff --git a/src/__tests__/recursive-input.test.ts b/src/__tests__/recursive-input.test.ts index 4e7aa75d..e594e4e6 100644 --- a/src/__tests__/recursive-input.test.ts +++ b/src/__tests__/recursive-input.test.ts @@ -173,7 +173,7 @@ describe("recursive input types", () => { }; variables.f.foo = variables.f; - const result = executeQuery(schema, document, variables); + const result = executeQuery(schema, document, variables, true); expect(result.errors[0].message).toBe( "Circular reference detected in input variable '$f' at foo.foo" ); @@ -338,7 +338,7 @@ describe("recursive input types", () => { variables.filter1.and.left.or = variables.filter1.or; variables.filter1.or.left.and = variables.filter1.and; - const result = executeQuery(schema, document, variables); + const result = executeQuery(schema, document, variables, true); expect(result.errors[0].message).toBe( "Circular reference detected in input variable '$filter1' at and.left.or.left.and" ); @@ -402,7 +402,7 @@ describe("recursive input types", () => { }; variables.filters[0].or = variables.filters; - const result = executeQuery(schema, document, variables); + const result = executeQuery(schema, document, variables, true); expect(result.errors[0].message).toBe( `Circular reference detected in input variable '$filters' at 0.or.0` ); @@ -428,7 +428,7 @@ describe("recursive input types", () => { }; variables.filters[0].or[0].or[0].or = variables.filters; - const result = executeQuery(schema, document, variables); + const result = executeQuery(schema, document, variables, true); expect(result.errors[0].message).toBe( `Circular reference detected in input variable '$filters' at 0.or.0.or.0.or.0` ); @@ -453,7 +453,7 @@ describe("recursive input types", () => { }; variables.filters[0].or[1] = variables.filters[0]; - const result = executeQuery(schema, document, variables); + const result = executeQuery(schema, document, variables, true); expect(result.errors[0].message).toBe( `Circular reference detected in input variable '$filters' at 0.or.1` ); @@ -509,7 +509,7 @@ describe("recursive input types", () => { }; variables.list.push(variables.list); - const result = executeQuery(schema, document, variables); + const result = executeQuery(schema, document, variables, true); expect(result.errors[0].message).toBe( "Circular reference detected in input variable '$list' at 0.0" ); @@ -527,7 +527,7 @@ describe("recursive input types", () => { variables.list[0].push(variables.list[1]); variables.list[1].push(variables.list[0]); - const result = executeQuery(schema, document, variables); + const result = executeQuery(schema, document, variables, true); expect(result.errors[0].message).toBe( "Circular reference detected in input variable '$list' at 0.0.0" ); @@ -539,13 +539,13 @@ function executeQuery( schema: GraphQLSchema, document: DocumentNode, variableValues?: any, - rootValue?: any, - contextValue?: any, - operationName?: string + variablesCircularReferenceCheck?: boolean ) { - const prepared: any = compileQuery(schema, document as any, operationName); + const prepared: any = compileQuery(schema, document as any, undefined, { + variablesCircularReferenceCheck + }); if (!isCompiledQuery(prepared)) { return prepared; } - return prepared.query(rootValue, contextValue, variableValues || {}); + return prepared.query({}, {}, variableValues || {}); } diff --git a/src/execution.ts b/src/execution.ts index 592c9be3..4eea7a85 100644 --- a/src/execution.ts +++ b/src/execution.ts @@ -76,6 +76,20 @@ export interface CompilerOptions { // the key should be the name passed to the Scalar or Enum type customSerializers: { [key: string]: (v: any) => any }; + /** + * If true, the generated code for variables compilation validates + * that there are no circular references (at runtime). For most cases, + * the variables are the result of JSON.parse and in these cases, we + * do not need this. Enable this if the variables passed to the execute + * function may contain circular references. + * + * When enabled, the code checks for circular references in the + * variables input, and throws an error when found. + * + * Default: false + */ + variablesCircularReferenceCheck: boolean; + resolverInfoEnricher?: (inp: ResolveInfoEnricherInput) => object; } @@ -207,6 +221,7 @@ export function compileQuery( customJSONSerializer: false, disableLeafSerialization: false, customSerializers: {}, + variablesCircularReferenceCheck: false, ...partialOptions }; @@ -228,6 +243,9 @@ export function compileQuery( } const getVariables = compileVariableParsing( schema, + { + variablesCircularReferenceCheck: options.variablesCircularReferenceCheck + }, context.operation.variableDefinitions || [] ); diff --git a/src/variables.ts b/src/variables.ts index 11c7a404..ec492b9b 100644 --- a/src/variables.ts +++ b/src/variables.ts @@ -42,7 +42,24 @@ export function failToParseVariables(x: any): x is FailedVariableCoercion { return x.errors; } +export interface VariablesCompilerOptions { + /** + * If true, the generated code for variables compilation validates + * that there are no circular references (at runtime). For most cases, + * the variables are the result of JSON.parse and in these cases, we + * do not need this. Enable this if the variables passed to the execute + * function may contain circular references. + * + * When enabled, the code checks for circular references in the + * variables input, and throws an error when found. + * + * Default: false (set in execution.ts) + */ + variablesCircularReferenceCheck: boolean; +} + interface CompilationContext { + options: VariablesCompilerOptions; inputPath: ObjectPath; responsePath: ObjectPath; depth: number; @@ -60,6 +77,7 @@ function createSubCompilationContext( export function compileVariableParsing( schema: GraphQLSchema, + options: VariablesCompilerOptions, varDefNodes: ReadonlyArray ): (inputs: { [key: string]: any }) => CoercedVariableValues { const errors = []; @@ -70,6 +88,7 @@ export function compileVariableParsing( const hoistedFunctions = new Map(); for (const varDefNode of varDefNodes) { const context: CompilationContext = { + options, varDefNode, depth: 0, inputPath: addPath(undefined, "input"), @@ -443,6 +462,7 @@ function compileInputListType( const __inputListValue = getPath(input, inputPath); ${generateCircularReferenceCheck( + context, "__inputListValue", "inputPath", errorLocation, @@ -559,6 +579,7 @@ function compileInputObjectType( const __inputValue = getPath(input, inputPath); ${generateCircularReferenceCheck( + context, "__inputValue", "inputPath", errorLocation, @@ -600,11 +621,20 @@ function compileInputObjectType( } function generateCircularReferenceCheck( + context: CompilationContext, inputValueName: string, inputPathName: string, errorLocation: string, shouldReturn: boolean ) { + /** + * If the variablesCircularReferenceCheck is `false`, do not generate + * code to verify circular references. + */ + if (!context.options.variablesCircularReferenceCheck) { + return ""; + } + const gen = genFn(); gen(`if (visitedInputValues.has(${inputValueName})) {`); gen(` From ffa434daa8a9a8e1a44ba2c426028a3a246a2a84 Mon Sep 17 00:00:00 2001 From: Boopathi Rajaa Date: Sun, 14 Jun 2020 13:22:31 +0200 Subject: [PATCH 8/8] fix todo comment --- src/__tests__/recursive-input.test.ts | 47 ++++++++++++++++++++++++--- 1 file changed, 42 insertions(+), 5 deletions(-) diff --git a/src/__tests__/recursive-input.test.ts b/src/__tests__/recursive-input.test.ts index e594e4e6..28a5ca84 100644 --- a/src/__tests__/recursive-input.test.ts +++ b/src/__tests__/recursive-input.test.ts @@ -50,11 +50,6 @@ describe("recursive input types", () => { ); }); - // Reminder to self - // TODO(boopathi): write tests to handle same value inputs - // { foo: 3, bar: 3 } - // Solution: object types only - test("should not fail with variables using recursive input types", () => { const document = parse(` query ($f: FooInput) { @@ -180,6 +175,48 @@ describe("recursive input types", () => { }); }); + describe("simple recursive input - 2", () => { + const schema = makeExecutableSchema({ + typeDefs: ` + type Query { + foo(input: FooInput): String + } + input FooInput { + foo: FooInput + bar: String + } + `, + resolvers: { + Query: { + foo(_, args) { + // used as the actual value in test matchers + return JSON.stringify(args); + } + } + } + }); + + test("should nòt fail for same leaf values", () => { + const document = parse(` + query ($f: FooInput) { + foo(input: $f) + } + `); + const variables = { + f: { + foo: { + bar: "bar" + }, + bar: "bar" + } + }; + + const result = executeQuery(schema, document, variables); + expect(result.errors).toBeUndefined(); + expect(JSON.parse(result.data.foo).input).toEqual(variables.f); + }); + }); + describe("mutually recursive input types", () => { const schema = makeExecutableSchema({ typeDefs: `