validateSchema: validate Input Objects self-references by IvanGoncharov · Pull Request #1359 · graphql/graphql-js

IvanGoncharov · 2018-05-25T19:20:05Z

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

IvanGoncharov · 2018-05-25T19:21:42Z

+function validateInputObjectCircularReferences(
+  context: SchemaValidationContext,
+): void {
+  // Modified copy of algorithm from 'src/validation/rules/NoFragmentCycles.js'.


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

IvanGoncharov · 2018-05-25T19:25:59Z

+  const fieldPathIndexByTypeName = Object.create(null);
+
+  const typeMap = context.schema.getTypeMap();
+  for (const type of objectValues(typeMap)) {


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.

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

spawnia · 2018-05-26T10:46:14Z

    ]);
  });

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


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 · 2018-05-26T10:47:34Z

+          const fieldNames = cyclePath.map(fieldObj => fieldObj.name);
+          context.reportError(
+            `Cannot reference Input Object "${fieldType.name}" within itself ` +
+              `through a series of non-null field: "${fieldNames.join('.')}".`,


field -> fields

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

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

spawnia · 2018-05-26T10:51:31Z

  });
 }

+function validateInputObjectCircularReferences(


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

@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 · 2018-05-26T11:10:44Z

+      },
+    ]);
+  });
+


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

@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 · 2018-06-11T04:07:27Z

-      if (!visitedFrags[node.name.value]) {
-        detectCycleRecursive(node);
-      }
+      detectCycleRecursive(node);


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

@leebyron Done as #1381

leebyron · 2018-06-11T04:08:32Z

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

spawnia · 2018-06-11T15:15:20Z

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

IvanGoncharov · 2019-06-03T09:03:30Z

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.

facebook-github-bot added the CLA Signed label May 25, 2018

IvanGoncharov commented May 25, 2018

View reviewed changes

IvanGoncharov mentioned this pull request May 25, 2018

Validate Input Objects do not contain non-nullable circu… #1352

Closed

IvanGoncharov added the spec RFC Implementation of a proposed change to the GraphQL specification label May 25, 2018

spawnia reviewed May 26, 2018

View reviewed changes

IvanGoncharov force-pushed the validateInputObjSelfRef branch 2 times, most recently from b90c798 to cf0c922 Compare May 26, 2018 23:25

spawnia approved these changes May 27, 2018

View reviewed changes

IvanGoncharov changed the title ~~[Draft]validateSchema: validate Input Objects self-references~~ validateSchema: validate Input Objects self-references May 27, 2018

IvanGoncharov mentioned this pull request May 28, 2018

Disallow non-breakable chains of circular references in Input Objects graphql/graphql-spec#445

Closed

leebyron reviewed Jun 11, 2018

View reviewed changes

IvanGoncharov mentioned this pull request Jun 11, 2018

Refactor NoFragmentCycles rule #1381

Merged

IvanGoncharov force-pushed the validateInputObjSelfRef branch from fda0774 to 72f63b7 Compare June 11, 2018 06:46

leebyron force-pushed the master branch from 37196f4 to f2070c8 Compare June 11, 2018 07:38

IvanGoncharov force-pushed the validateInputObjSelfRef branch from 72f63b7 to 7b1b918 Compare June 11, 2018 08:10

IvanGoncharov force-pushed the validateInputObjSelfRef branch from 7b1b918 to 615c05a Compare July 19, 2018 11:48

bbakerman mentioned this pull request Aug 31, 2018

allow reference input type graphql-java/graphql-java#1185

Closed

validateSchema: validate Input Objects self-references

408bcda

IvanGoncharov force-pushed the validateInputObjSelfRef branch from 615c05a to 408bcda Compare June 3, 2019 08:57

IvanGoncharov added the PR: feature 🚀 requires increase of "minor" version number label Jun 3, 2019

IvanGoncharov merged commit ed74228 into graphql:master Jun 3, 2019

IvanGoncharov deleted the validateInputObjSelfRef branch June 3, 2019 09:03

spawnia mentioned this pull request Jun 10, 2019

Add schema validation: Input Objects must not contain non-nullable circular references webonyx/graphql-php#492

Merged

michaelstaib mentioned this pull request Aug 23, 2019

Add rules for how circular references in Input Objects are handled ChilliCream/graphql-platform#285

Closed

snyk-bot mentioned this pull request Aug 31, 2019

[Snyk] Upgrade graphql from 14.1.1 to 14.5.0 sandorTuranszky/production-ready-expressjs-server#13

Merged

Conversation

IvanGoncharov commented May 25, 2018

Uh oh!

IvanGoncharov May 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IvanGoncharov May 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spawnia May 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spawnia left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leebyron commented Jun 11, 2018

Uh oh!

spawnia commented Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IvanGoncharov commented Jun 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

IvanGoncharov May 25, 2018 •

edited

Loading

IvanGoncharov May 27, 2018 •

edited

Loading

spawnia May 26, 2018 •

edited

Loading

spawnia left a comment •

edited

Loading

spawnia commented Jun 11, 2018 •

edited

Loading