diff --git a/src/__testUtils__/__tests__/spyOn-test.ts b/src/__testUtils__/__tests__/spyOn-test.ts index 2e6208c343..15ef532eb5 100644 --- a/src/__testUtils__/__tests__/spyOn-test.ts +++ b/src/__testUtils__/__tests__/spyOn-test.ts @@ -27,6 +27,23 @@ describe('spyOn', () => { expect(obj.addToBase(5)).to.equal(15); expect(obj.addToBase.callCount).to.equal(1); }); + + it('passes an empty stack to the matcher when stack traces are unavailable', () => { + const originalStackTraceLimit = Error.stackTraceLimit; + (Error as { stackTraceLimit: number | undefined }).stackTraceLimit = + undefined; + + try { + const spy = spyOn(() => 42, { + stackMatcher: (stack) => stack === '', + }); + + expect(spy()).to.equal(42); + expect(spy.callCount).to.equal(1); + } finally { + Error.stackTraceLimit = originalStackTraceLimit; + } + }); }); describe('spyOnMethod', () => { @@ -58,6 +75,30 @@ describe('spyOnMethod', () => { expect(spy.callCount).to.equal(1); }); + it('can count only method invocations matching the call stack', () => { + const calculator = { + add(a: number, b: number) { + return a + b; + }, + }; + + const spy = spyOnMethod(calculator, 'add', { + stackMatcher: (stack) => stack.includes('callTrackedMethod'), + }); + + expect(callTrackedMethod()).to.equal(5); + expect(callUntrackedMethod()).to.equal(9); + expect(spy.callCount).to.equal(1); + + function callTrackedMethod(): number { + return calculator.add(2, 3); + } + + function callUntrackedMethod(): number { + return calculator.add(4, 5); + } + }); + it('throws when target property is not a function', () => { const obj: { maybeMethod?: (value: string) => string } = {}; diff --git a/src/__testUtils__/spyOn.ts b/src/__testUtils__/spyOn.ts index 9ac38e3450..3b7f0975b9 100644 --- a/src/__testUtils__/spyOn.ts +++ b/src/__testUtils__/spyOn.ts @@ -5,13 +5,22 @@ export interface MethodSpy { restore: () => void; } +export interface SpyOptions { + readonly stackMatcher?: (stack: string) => boolean; +} + export type SpyFn = T & MethodSpy; -export function spyOn(fn: T): SpyFn { +export function spyOn(fn: T, options?: SpyOptions): SpyFn { let callCount = 0; const spy = function (this: unknown, ...args: Parameters): ReturnType { - callCount += 1; + if ( + options?.stackMatcher === undefined || + options.stackMatcher(new Error().stack ?? '') + ) { + callCount += 1; + } return fn.apply(this, args) as ReturnType; }; @@ -28,6 +37,7 @@ export function spyOn(fn: T): SpyFn { export function spyOnMethod( target: T, key: keyof T, + options?: SpyOptions, ): MethodSpy { const original = target[key]; const wasOwnProperty = Object.hasOwn(target, key); @@ -38,7 +48,7 @@ export function spyOnMethod( ); } - const spy = spyOn(original as AnyFn); + const spy = spyOn(original as AnyFn, options); target[key] = spy as T[keyof T]; const methodSpy: MethodSpy = { diff --git a/src/type/__tests__/validation-test.ts b/src/type/__tests__/validation-test.ts index 8973013fa1..f8c31d3184 100644 --- a/src/type/__tests__/validation-test.ts +++ b/src/type/__tests__/validation-test.ts @@ -4,6 +4,7 @@ import { assert, expect } from 'chai'; import { dedent } from '../../__testUtils__/dedent.ts'; import { expectJSON } from '../../__testUtils__/expectJSON.ts'; +import { spyOnMethod } from '../../__testUtils__/spyOn.ts'; import { inspect } from '../../jsutils/inspect.ts'; @@ -925,7 +926,7 @@ describe('Type System: Input Objects must have fields', () => { expectJSON(validateSchema(schema)).toDeepEqual([ { message: - 'Invalid circular reference. The Input Object SomeInputObject references itself in the non-null field SomeInputObject.nonNullSelf.', + 'Input Object SomeInputObject cannot be provided a finite value because it references itself through fields: SomeInputObject.nonNullSelf.', locations: [{ line: 7, column: 9 }], }, ]); @@ -953,7 +954,7 @@ describe('Type System: Input Objects must have fields', () => { expectJSON(validateSchema(schema)).toDeepEqual([ { message: - 'Invalid circular reference. The Input Object SomeInputObject references itself via the non-null fields: SomeInputObject.startLoop, AnotherInputObject.nextInLoop, YetAnotherInputObject.closeLoop.', + 'Input Object SomeInputObject cannot be provided a finite value because it references itself through fields: SomeInputObject.startLoop, AnotherInputObject.nextInLoop, YetAnotherInputObject.closeLoop.', locations: [ { line: 7, column: 9 }, { line: 11, column: 9 }, @@ -987,7 +988,7 @@ describe('Type System: Input Objects must have fields', () => { expectJSON(validateSchema(schema)).toDeepEqual([ { message: - 'Invalid circular reference. The Input Object SomeInputObject references itself via the non-null fields: SomeInputObject.startLoop, AnotherInputObject.closeLoop.', + 'Input Object SomeInputObject cannot be provided a finite value because it references itself through fields: SomeInputObject.startLoop, AnotherInputObject.closeLoop.', locations: [ { line: 7, column: 9 }, { line: 11, column: 9 }, @@ -995,7 +996,7 @@ describe('Type System: Input Objects must have fields', () => { }, { message: - 'Invalid circular reference. The Input Object AnotherInputObject references itself via the non-null fields: AnotherInputObject.startSecondLoop, YetAnotherInputObject.closeSecondLoop.', + 'Input Object AnotherInputObject cannot be provided a finite value because it references itself through fields: AnotherInputObject.startSecondLoop, YetAnotherInputObject.closeSecondLoop.', locations: [ { line: 12, column: 9 }, { line: 16, column: 9 }, @@ -1003,12 +1004,52 @@ describe('Type System: Input Objects must have fields', () => { }, { message: - 'Invalid circular reference. The Input Object YetAnotherInputObject references itself in the non-null field YetAnotherInputObject.nonNullSelf.', + 'Input Object YetAnotherInputObject cannot be provided a finite value because it references itself through fields: YetAnotherInputObject.nonNullSelf.', locations: [{ line: 17, column: 9 }], }, ]); }); + it('rejects an Input Object with multiple non-breakable circular references', () => { + const schema = buildSchema(` + type Query { + field(arg: A): String + } + + input A { + b: B! + c: C! + } + + input B { + a: A! + } + + input C { + a: A! + } + `); + + expectJSON(validateSchema(schema)).toDeepEqual([ + { + message: + 'Input Object A cannot be provided a finite value because it references itself through fields: A.b, B.a.', + locations: [ + { line: 7, column: 9 }, + { line: 12, column: 9 }, + ], + }, + { + message: + 'Input Object A cannot be provided a finite value because it references itself through fields: A.c, C.a.', + locations: [ + { line: 8, column: 9 }, + { line: 16, column: 9 }, + ], + }, + ]); + }); + it('accepts Input Objects with default values without circular references (SDL)', () => { const validSchema = buildSchema(` type Query { @@ -2370,6 +2411,175 @@ describe('Type System: Input Object field default values must be valid', () => { }); describe('Type System: OneOf Input Object fields must be nullable', () => { + it('accepts a OneOf Input Object with a scalar field', () => { + const schema = buildSchema(` + type Query { + test(arg: A): Int + } + + input A @oneOf { + a: Int + } + `); + expectJSON(validateSchema(schema)).toDeepEqual([]); + }); + + it('accepts a OneOf Input Object with a recursive list field', () => { + const schema = buildSchema(` + type Query { + test(arg: A): Int + } + + input A @oneOf { + a: [A!] + } + `); + expectJSON(validateSchema(schema)).toDeepEqual([]); + }); + + it('accepts a OneOf Input Object referencing a non-OneOf input object', () => { + const schema = buildSchema(` + type Query { + test(arg: A): Int + } + + input A @oneOf { + b: B + } + + input B { + x: Int + } + `); + expectJSON(validateSchema(schema)).toDeepEqual([]); + }); + + it('accepts a OneOf Input Object referencing an already checked input object', () => { + const schema = buildSchema(` + type Query { + a(arg: A): Int + } + + input B { + value: Int + } + + input A @oneOf { + b: B + } + `); + expectJSON(validateSchema(schema)).toDeepEqual([]); + }); + + it('accepts a OneOf Input Object with multiple acyclic input object fields', () => { + const schema = buildSchema(` + type Query { + test(arg: A): Int + } + + input A @oneOf { + b: B + c: C + } + + input B { + value: Int + } + + input C { + value: Int + } + `); + expectJSON(validateSchema(schema)).toDeepEqual([]); + }); + + it('accepts a OneOf/OneOf cycle with a scalar escape', () => { + const schema = buildSchema(` + type Query { + test(arg: A): Int + } + + input A @oneOf { + b: B + escape: Int + } + + input B @oneOf { + a: A + } + `); + expectJSON(validateSchema(schema)).toDeepEqual([]); + }); + + it('accepts a OneOf/non-OneOf cycle with a nullable escape', () => { + const schema = buildSchema(` + type Query { + test(arg: A): Int + } + + input A @oneOf { + b: B + } + + input B { + a: A + } + `); + expectJSON(validateSchema(schema)).toDeepEqual([]); + }); + + it('accepts a OneOf/non-OneOf with scalar escape', () => { + const schema = buildSchema(` + type Query { + test(arg: A): Int + } + + input A @oneOf { + b: B + escape: Int + } + + input B { + a: A! + } + `); + expectJSON(validateSchema(schema)).toDeepEqual([]); + }); + + it('accepts a non-OneOf/non-OneOf cycle with a nullable escape', () => { + const schema = buildSchema(` + type Query { + test(arg: A): Int + } + + input A { + b: B! + } + + input B { + a: A + } + `); + expectJSON(validateSchema(schema)).toDeepEqual([]); + }); + + it('accepts a non-OneOf/non-OneOf cycle with a non-null list of non-null items escape', () => { + const schema = buildSchema(` + type Query { + test(arg: A): Int + } + + input A { + b: [B!]! + } + + input B { + a: A! + } + `); + expectJSON(validateSchema(schema)).toDeepEqual([]); + }); + it('rejects non-nullable fields', () => { const schema = buildSchema(` type Query { @@ -2408,6 +2618,224 @@ describe('Type System: OneOf Input Object fields must be nullable', () => { }, ]); }); + + it('rejects a self-referencing OneOf type with no escapes', () => { + const schema = buildSchema(` + type Query { + test(arg: A): Int + } + + input A @oneOf { + self: A + } + `); + expectJSON(validateSchema(schema)).toDeepEqual([ + { + message: + 'Input Object A cannot be provided a finite value because it references itself through fields: A.self.', + locations: [{ line: 7, column: 9 }], + }, + ]); + }); + + it('rejects a non-OneOf Input Object requiring an unbreakable OneOf cycle', () => { + const schema = buildSchema(` + type Query { + a(arg: A): Int + } + + input T @oneOf { + self: T + } + + input A { + t: T! + } + `); + expectJSON(validateSchema(schema)).toDeepEqual([ + { + message: + 'Input Object T cannot be provided a finite value because it references itself through fields: T.self.', + locations: [{ line: 7, column: 9 }], + }, + ]); + }); + + it('checks each shared unbreakable OneOf subgraph once', () => { + const chainLength = 16; + const types: Array = []; + types[0] = new GraphQLInputObjectType({ + name: 'T0', + isOneOf: true, + fields: () => ({ self: { type: types[0] } }), + }); + + for (let i = 1; i <= chainLength; ++i) { + const previousType = types[i - 1]; + types[i] = new GraphQLInputObjectType({ + name: `T${i}`, + isOneOf: true, + fields: { + a: { type: previousType }, + b: { type: previousType }, + }, + }); + } + + const getFieldsSpies = types.map((type) => + spyOnMethod(type, 'getFields', { + stackMatcher: (stack) => + stack.includes('detectInputObjectNonFiniteValues'), + }), + ); + + const schema = new GraphQLSchema({ + query: new GraphQLObjectType({ + name: 'Query', + fields: { + test: { + type: GraphQLInt, + args: { input: { type: types[chainLength] } }, + }, + }, + }), + types, + }); + + expect(validateSchema(schema)).to.have.lengthOf(1); + expect( + getFieldsSpies.reduce((sum, spy) => sum + spy.callCount, 0), + ).to.equal(17); + }); + + it('rejects a mixed OneOf/non-OneOf cycle with no escapes', () => { + const schema = buildSchema(` + type Query { + test(arg: A): Int + } + + input A @oneOf { + b: B + } + + input B { + a: A! + } + `); + expectJSON(validateSchema(schema)).toDeepEqual([ + { + message: + 'Input Object A cannot be provided a finite value because it references itself through fields: A.b, B.a.', + locations: [ + { line: 7, column: 9 }, + { line: 11, column: 9 }, + ], + }, + ]); + }); + + it('rejects multiple OneOf branches without duplicate cycle reports', () => { + const schema = buildSchema(` + type Query { + test(arg: A): Int + } + + input A @oneOf { + b: B + c: C + } + + input B { + a: A! + } + + input C { + a: A! + } + `); + expectJSON(validateSchema(schema)).toDeepEqual([ + { + message: + 'Input Object A cannot be provided a finite value because it references itself through fields: A.b, B.a.', + locations: [ + { line: 7, column: 9 }, + { line: 12, column: 9 }, + ], + }, + { + message: + 'Input Object A cannot be provided a finite value because it references itself through fields: A.c, C.a.', + locations: [ + { line: 8, column: 9 }, + { line: 16, column: 9 }, + ], + }, + ]); + }); + + it('rejects a non-OneOf/non-OneOf cycle with required scalar, list, and finite input fields', () => { + const schema = buildSchema(` + type Query { + test(arg: A): Int + } + + input A { + list: [B]! + finite: Finite! + b: B! + } + + input B { + value: Int! + a: A! + } + + input Finite { + value: Int! + } + `); + expectJSON(validateSchema(schema)).toDeepEqual([ + { + message: + 'Input Object A cannot be provided a finite value because it references itself through fields: A.b, B.a.', + locations: [ + { line: 9, column: 9 }, + { line: 14, column: 9 }, + ], + }, + ]); + }); + + it('rejects a larger mixed OneOf/non-OneOf cycle with no escapes', () => { + const schema = buildSchema(` + type Query { + test(arg: A): Int + } + + input A @oneOf { + b: B + } + + input B { + c: C! + } + + input C @oneOf { + a: A + } + `); + expectJSON(validateSchema(schema)).toDeepEqual([ + { + message: + 'Input Object A cannot be provided a finite value because it references itself through fields: A.b, B.c, C.a.', + locations: [ + { line: 7, column: 9 }, + { line: 11, column: 9 }, + { line: 15, column: 9 }, + ], + }, + ]); + }); }); describe('Objects must adhere to Interface they implement', () => { diff --git a/src/type/validate.ts b/src/type/validate.ts index 4579901e2e..e09122869f 100644 --- a/src/type/validate.ts +++ b/src/type/validate.ts @@ -405,12 +405,14 @@ function validateName( } function validateTypes(context: SchemaValidationContext): void { - // Ensure Input Objects do not contain non-nullable circular references. - const validateInputObjectNonNullCircularRefs = - createInputObjectNonNullCircularRefsValidator(context); const validateInputObjectDefaultValueCircularRefs = createInputObjectDefaultValueCircularRefsValidator(context); const typeMap = context.schema.getTypeMap(); + const finiteValueStates = new Map< + GraphQLInputObjectType, + InputObjectFiniteValueState + >(); + for (const type of Object.values(typeMap)) { // Ensure all provided types are in fact GraphQL type. if (!isNamedType(type)) { @@ -448,14 +450,26 @@ function validateTypes(context: SchemaValidationContext): void { // Ensure Input Object fields are valid. validateInputFields(context, type); - // Ensure Input Objects do not contain invalid field circular references. - // Ensure Input Objects do not contain non-nullable circular references. - validateInputObjectNonNullCircularRefs(type); + initializeInputObjectFiniteValueState(type); // Ensure Input Objects do not contain invalid default value circular references. validateInputObjectDefaultValueCircularRefs(type); } } + + detectInputObjectNonFiniteValues(context, finiteValueStates); + + function initializeInputObjectFiniteValueState( + inputObj: GraphQLInputObjectType, + ): void { + finiteValueStates.set(inputObj, { + inputObj, + targets: [], + dependents: [], + unresolvedTargetCount: 0, + hasFiniteValue: false, + }); + } } function validateFields( @@ -771,65 +785,159 @@ function validateOneOfInputObjectField( } } -function createInputObjectNonNullCircularRefsValidator( +interface InputObjectFiniteValueTarget { + field: GraphQLInputField; + target: GraphQLInputObjectType; +} + +interface InputObjectFiniteValueState { + inputObj: GraphQLInputObjectType; + targets: Array; + dependents: Array; + unresolvedTargetCount: number; + hasFiniteValue: boolean; +} + +// Implements the spec's InputObjectHasUnbreakableCycle algorithm for all Input +// Objects in one pass by propagating known breakable types through reverse edges. +function detectInputObjectNonFiniteValues( context: SchemaValidationContext, -): (inputObj: GraphQLInputObjectType) => void { - // Modified copy of algorithm from 'src/validation/rules/NoFragmentCycles.js'. - // Tracks already visited types to maintain O(N) and to ensure that cycles - // are not redundantly reported. + finiteValueStates: ReadonlyMap< + GraphQLInputObjectType, + InputObjectFiniteValueState + >, +): void { + const inputObjectsWithFiniteValues: Array = []; + + for (const state of finiteValueStates.values()) { + const inputObj = state.inputObj; + const fields = Object.values(inputObj.getFields()); + + for (const field of fields) { + const target = getFiniteValueTarget(inputObj, field.type); + if (target === undefined) { + continue; + } + + state.targets.push({ field, target }); + const targetState = finiteValueStates.get(target); + if (targetState !== undefined) { + targetState.dependents.push(state); + } + } + + if (inputObj.isOneOf) { + // OneOf Input Objects have an unbreakable cycle if every field leads to an unbreakable cycle. + if (fields.length === 0 || state.targets.length < fields.length) { + markInputObjectHasFiniteValue(state); + } + } else { + // Non-OneOf Input Objects have an unbreakable cycle if any non-null field has one. + state.unresolvedTargetCount = state.targets.length; + if (state.targets.length === 0) { + markInputObjectHasFiniteValue(state); + } + } + } + + let nextFiniteValueState: InputObjectFiniteValueState | undefined; + while ( + (nextFiniteValueState = inputObjectsWithFiniteValues.pop()) !== undefined + ) { + for (const dependentState of nextFiniteValueState.dependents) { + if (dependentState.hasFiniteValue) { + continue; + } + + if (dependentState.inputObj.isOneOf) { + markInputObjectHasFiniteValue(dependentState); + continue; + } + + --dependentState.unresolvedTargetCount; + if (dependentState.unresolvedTargetCount === 0) { + markInputObjectHasFiniteValue(dependentState); + } + } + } + + // Tracks already visited types to ensure that cycles are not redundantly + // reported. const visitedTypes = new Set(); - // Array of types nodes used to produce meaningful errors + // Array of fields used to produce meaningful errors. const fieldPath: Array<{ fieldStr: string; astNode: Maybe }> = []; - // Position in the type path - const fieldPathIndexByTypeName: ObjMap = - Object.create(null); + // Position in the field path. + const fieldPathIndexByType = new Map(); - return detectCycleRecursive; + for (const state of finiteValueStates.values()) { + if (!state.hasFiniteValue) { + reportCycleRecursive(state); + } + } - // This does a straight-forward DFS to find cycles. - // It does not terminate when a cycle was found but continues to explore - // the graph to find all possible cycles. - function detectCycleRecursive(inputObj: GraphQLInputObjectType): void { + function markInputObjectHasFiniteValue( + finiteValueState: InputObjectFiniteValueState, + ): void { + if (!finiteValueState.hasFiniteValue) { + finiteValueState.hasFiniteValue = true; + inputObjectsWithFiniteValues.push(finiteValueState); + } + } + + function reportCycleRecursive(state: InputObjectFiniteValueState): void { + const inputObj = state.inputObj; if (visitedTypes.has(inputObj)) { return; } visitedTypes.add(inputObj); - fieldPathIndexByTypeName[inputObj.name] = fieldPath.length; + fieldPathIndexByType.set(inputObj, fieldPath.length); - const fields = Object.values(inputObj.getFields()); - for (const field of fields) { - if (isNonNullType(field.type) && isInputObjectType(field.type.ofType)) { - const fieldType = field.type.ofType; - const cycleIndex = fieldPathIndexByTypeName[fieldType.name]; - - fieldPath.push({ - fieldStr: `${inputObj}.${field.name}`, - astNode: field.astNode, - }); - if (cycleIndex === undefined) { - detectCycleRecursive(fieldType); - } else { - const cyclePath = fieldPath.slice(cycleIndex); - const pathStr = cyclePath - .map((fieldObj) => fieldObj.fieldStr) - .join(', '); - context.reportError( - `Invalid circular reference. The Input Object ${fieldType} references itself ${ - cyclePath.length > 1 - ? 'via the non-null fields:' - : 'in the non-null field' - } ${pathStr}.`, - cyclePath.map((fieldObj) => fieldObj.astNode), - ); - } - fieldPath.pop(); + for (const { field, target } of state.targets) { + const targetState = finiteValueStates.get(target); + if (targetState?.hasFiniteValue !== false) { + continue; } + + const cycleIndex = fieldPathIndexByType.get(target); + fieldPath.push({ + fieldStr: `${inputObj}.${field.name}`, + astNode: field.astNode, + }); + + if (cycleIndex === undefined) { + reportCycleRecursive(targetState); + } else { + const cyclePath = fieldPath.slice(cycleIndex); + const pathStr = cyclePath.map((p) => p.fieldStr).join(', '); + context.reportError( + `Input Object ${target} cannot be provided a finite value because it references itself through fields: ${pathStr}.`, + cyclePath.map((p) => p.astNode), + ); + } + + fieldPath.pop(); } - fieldPathIndexByTypeName[inputObj.name] = undefined; + fieldPathIndexByType.delete(inputObj); + } +} + +function getFiniteValueTarget( + inputObj: GraphQLInputObjectType, + fieldType: GraphQLInputType, +): GraphQLInputObjectType | undefined { + if (inputObj.isOneOf) { + if (isInputObjectType(fieldType)) { + return fieldType; + } + return; + } + + if (isNonNullType(fieldType) && isInputObjectType(fieldType.ofType)) { + return fieldType.ofType; } }