-
Notifications
You must be signed in to change notification settings - Fork 2k
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
validateSchema: validate Input Objects self-references #1359
Conversation
function validateInputObjectCircularReferences( | ||
context: SchemaValidationContext, | ||
): void { | ||
// Modified copy of algorithm from 'src/validation/rules/NoFragmentCycles.js'. |
There was a problem hiding this comment.
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
src/type/validate.js
Outdated
const fieldPathIndexByTypeName = Object.create(null); | ||
|
||
const typeMap = context.schema.getTypeMap(); | ||
for (const type of objectValues(typeMap)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solved by integrating this check into the main cycle inside validateTypes
.
@@ -687,6 +687,80 @@ describe('Type System: Input Objects must have fields', () => { | |||
]); | |||
}); | |||
|
|||
it('accepts an Input Object with breakable circular reference', () => { |
There was a problem hiding this comment.
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.
src/type/validate.js
Outdated
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('.')}".`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
field -> fields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spawnia Fixed.
Should we have a separate error for the case where it's the only one field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would not hurt, but i think this way it is also fine. The error should still be quite obvious.
src/type/validate.js
Outdated
@@ -578,6 +581,64 @@ function validateInputFields( | |||
}); | |||
} | |||
|
|||
function validateInputObjectCircularReferences( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be in a seperate rules file, similar to how NoFragmentCycles is implemented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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).
}, | ||
]); | ||
}); | ||
|
There was a problem hiding this comment.
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 }],
},
]);
});
There was a problem hiding this comment.
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.
b90c798
to
cf0c922
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation is solid and the test cases both cover validation errors but ensure no false-positives are thrown. Looks good to me!
if (!visitedFrags[node.name.value]) { | ||
detectCycleRecursive(node); | ||
} | ||
detectCycleRecursive(node); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind factoring the changes to this file to a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great! Let's make sure the spec text is clear as well before merging this |
fda0774
to
72f63b7
Compare
72f63b7
to
7b1b918
Compare
@leebyron Anything you would change about the spec change? graphql/graphql-spec#445 |
7b1b918
to
615c05a
Compare
615c05a
to
408bcda
Compare
Merged since graphql/graphql-spec#445 is on Stage2 and spec is encourage implementing proposal on this stage:
|
Implements graphql/graphql-spec#445
Based on #1352