Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

validateSchema: validate Input Objects self-references #1359

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
114 changes: 114 additions & 0 deletions src/type/__tests__/validation-test.js
Expand Up @@ -736,6 +736,120 @@ describe('Type System: Input Objects must have fields', () => {
]);
});

it('accepts an Input Object with breakable circular reference', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how you considered all the possibilities of breaking a circular reference in the first test case, to ensure this does not cause false-positives.

const schema = buildSchema(`
type Query {
field(arg: SomeInputObject): String
}

input SomeInputObject {
self: SomeInputObject
arrayOfSelf: [SomeInputObject]
nonNullArrayOfSelf: [SomeInputObject]!
nonNullArrayOfNonNullSelf: [SomeInputObject!]!
intermediateSelf: AnotherInputObject
}

input AnotherInputObject {
parent: SomeInputObject
}
`);

expect(validateSchema(schema)).to.deep.equal([]);
});

it('rejects an Input Object with non-breakable circular reference', () => {
const schema = buildSchema(`
type Query {
field(arg: SomeInputObject): String
}

input SomeInputObject {
nonNullSelf: SomeInputObject!
}
`);

expect(validateSchema(schema)).to.deep.equal([
{
message:
'Cannot reference Input Object "SomeInputObject" within itself through a series of non-null fields: "nonNullSelf".',
locations: [{ line: 7, column: 9 }],
},
]);
});

it('rejects Input Objects with non-breakable circular reference spread across them', () => {
const schema = buildSchema(`
type Query {
field(arg: SomeInputObject): String
}

input SomeInputObject {
startLoop: AnotherInputObject!
}

input AnotherInputObject {
nextInLoop: YetAnotherInputObject!
}

input YetAnotherInputObject {
closeLoop: SomeInputObject!
}
`);

expect(validateSchema(schema)).to.deep.equal([
{
message:
'Cannot reference Input Object "SomeInputObject" within itself through a series of non-null fields: "startLoop.nextInLoop.closeLoop".',
locations: [
{ line: 7, column: 9 },
{ line: 11, column: 9 },
{ line: 15, column: 9 },
],
},
]);
});

Copy link
Member

@spawnia spawnia May 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to add another test-case:

  it('rejects Input Objects with multiple non-breakable circular reference', () => {
    const schema = buildSchema(`
      type Query {
        field(arg: SomeInputObject): String
      }
 
      input SomeInputObject {
        startLoop: AnotherInputObject!
      }
 
      input AnotherInputObject {
        closeLoop: SomeInputObject!
        startSecondLoop: YetAnotherInputObject!
      }
 
      input YetAnotherInputObject {
        closeSecondLoop: AnotherInputObject!
        nonNullSelf: YetAnotherInputObject!
      }
    `);
 
    expect(validateSchema(schema)).to.deep.equal([
      {
        message:
          'Cannot reference Input Object "SomeInputObject" within itself through a series of non-null field: "startLoop.closeLoop".',
        locations: [
          { line: 7, column: 9 },
          { line: 11, column: 9 },
        ],
      },
    ],[
      {
        message:
          'Cannot reference Input Object "AnotherInputObject" within itself through a series of non-null field: "startSecondLoop.closeSecondLoop".',
        locations: [
          { line: 12, column: 9 },
          { line: 16, column: 9 },
        ],
      },
    ],[
      {
        message:
          'Cannot reference Input Object "YetAnotherInputObject" within itself through a series of non-null field: "nonNullSelf".',
        locations: [{ line: 17, column: 9 }],
      },
    ]);
  });
 

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spawnia Great test 👍 Added to PR.

it('rejects Input Objects with multiple non-breakable circular reference', () => {
const schema = buildSchema(`
type Query {
field(arg: SomeInputObject): String
}

input SomeInputObject {
startLoop: AnotherInputObject!
}

input AnotherInputObject {
closeLoop: SomeInputObject!
startSecondLoop: YetAnotherInputObject!
}

input YetAnotherInputObject {
closeSecondLoop: AnotherInputObject!
nonNullSelf: YetAnotherInputObject!
}
`);

expect(validateSchema(schema)).to.deep.equal([
{
message:
'Cannot reference Input Object "SomeInputObject" within itself through a series of non-null fields: "startLoop.closeLoop".',
locations: [{ line: 7, column: 9 }, { line: 11, column: 9 }],
},
{
message:
'Cannot reference Input Object "AnotherInputObject" within itself through a series of non-null fields: "startSecondLoop.closeSecondLoop".',
locations: [{ line: 12, column: 9 }, { line: 16, column: 9 }],
},
{
message:
'Cannot reference Input Object "YetAnotherInputObject" within itself through a series of non-null fields: "nonNullSelf".',
locations: [{ line: 17, column: 9 }],
},
]);
});

it('rejects an Input Object type with incorrectly typed fields', () => {
const schema = buildSchema(`
type Query {
Expand Down
60 changes: 60 additions & 0 deletions src/type/validate.js
Expand Up @@ -23,6 +23,7 @@ import {
isEnumType,
isInputObjectType,
isNamedType,
isNonNullType,
isInputType,
isOutputType,
isRequiredArgument,
Expand Down Expand Up @@ -221,6 +222,9 @@ function validateName(
}

function validateTypes(context: SchemaValidationContext): void {
const validateInputObjectCircularRefs = createInputObjectCircularRefsValidator(
context,
);
const typeMap = context.schema.getTypeMap();
for (const type of objectValues(typeMap)) {
// Ensure all provided types are in fact GraphQL type.
Expand Down Expand Up @@ -255,6 +259,9 @@ function validateTypes(context: SchemaValidationContext): void {
} else if (isInputObjectType(type)) {
// Ensure Input Object fields are valid.
validateInputFields(context, type);

// Ensure Input Objects do not contain non-nullable circular references
validateInputObjectCircularRefs(type);
}
}
}
Expand Down Expand Up @@ -525,6 +532,59 @@ function validateInputFields(
}
}

function createInputObjectCircularRefsValidator(
context: SchemaValidationContext,
) {
// Modified copy of algorithm from 'src/validation/rules/NoFragmentCycles.js'.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this algorithm is not trivial would be great to extract duplicating code into jsutils

// Tracks already visited types to maintain O(N) and to ensure that cycles
// are not redundantly reported.
const visitedTypes = Object.create(null);

// Array of types nodes used to produce meaningful errors
const fieldPath = [];

// Position in the type path
const fieldPathIndexByTypeName = Object.create(null);

return detectCycleRecursive;

// 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) {
if (visitedTypes[inputObj.name]) {
return;
}

visitedTypes[inputObj.name] = true;
fieldPathIndexByTypeName[inputObj.name] = fieldPath.length;

const fields = objectValues(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(field);
if (cycleIndex === undefined) {
detectCycleRecursive(fieldType);
} else {
const cyclePath = fieldPath.slice(cycleIndex);
const fieldNames = cyclePath.map(fieldObj => fieldObj.name);
context.reportError(
`Cannot reference Input Object "${fieldType.name}" within itself ` +
`through a series of non-null fields: "${fieldNames.join('.')}".`,
cyclePath.map(fieldObj => fieldObj.astNode),
);
}
fieldPath.pop();
}
}

fieldPathIndexByTypeName[inputObj.name] = undefined;
}
}

type SDLDefinedObject<T, K> = {
+astNode: ?T,
+extensionASTNodes?: ?$ReadOnlyArray<K>,
Expand Down