validateSchema: validate Input Objects self-references

[IvanGoncharov]

Implements graphql/graphql-spec#445
Based on #1352

[IvanGoncharov]

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

[IvanGoncharov]

It's bad that we need to iterate over every type one more time.
Would be great to integrate this check into the loop inside validateTypes function without affecting its readability.

[IvanGoncharov]

Solved by integrating this check into the main cycle inside validateTypes.

[spawnia]

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.

[spawnia]

field -> fields

[IvanGoncharov]

@spawnia Fixed.
Should we have a separate error for the case where it's the only one field?

[spawnia]

It would not hurt, but i think this way it is also fine. The error should still be quite obvious.

[spawnia]

Shouldn't this be in a seperate rules file, similar to how NoFragmentCycles is implemented?

[IvanGoncharov]

@spawnia src/validate/rules/* is intended to validate AST only with a focus on the query syntax. You can create the schema using SDL or JS classes like here:

new GraphQLSchema({
  queryType: new GraphQLObjectType({
    name: 'Query',
    fields: {
      foo: {
        type: GraphQLString,
      },
    }
  }),
})

Also you construct schema from introspection using buildClientSchema.
So schema validation shouldn't be tied to AST and should only use GraphQL* classes.

On the other hand, your queries are always AST so they validation is also AST based.

That's why there is two completely different validation mechanisms: validate(AST based, mostly query) and validateSchema(GraphQL* classes, only for schema).

[spawnia]

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 }],
      },
    ]);
  });
 

[IvanGoncharov]

@spawnia Great test 👍 Added to PR.

[spawnia]

The implementation is solid and the test cases both cover validation errors but ensure no false-positives are thrown. Looks good to me!

[leebyron]

Do you mind factoring the changes to this file to a separate PR?

[IvanGoncharov]

@leebyron Done as #1381

[leebyron]

This is looking great! Let's make sure the spec text is clear as well before merging this

[spawnia]

@leebyron Anything you would change about the spec change? graphql/graphql-spec#445

[IvanGoncharov]

Merged since graphql/graphql-spec#445 is on Stage2 and spec is encourage implementing proposal on this stage:
https://github.com/graphql/graphql-spec/blob/master/CONTRIBUTING.md#stage-2-draft

GraphQL libraries should implement drafts to provide valuable feedback, though are encouraged not to enable the draft feature without explicit opt-in when possible.