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

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

spawnia
Copy link
Member

@spawnia spawnia commented May 16, 2018

…lar references

The first two to tests are there to avoid false positives when checking for circular references.

Tests 3 and 4 are failing right now but show what should happen.

graphql/graphql-spec#445 contains the proposed additions to the spec.

…lar references

The first two to tests are there to avoid false positives when checking for circular references.

Tests 3 and 4 are failing right now but show what should happen.

graphql/graphql-spec#445 contains the proposed additions to the spec.
@IvanGoncharov
Copy link
Member

@spawnia Thanks for PR, your SDL examples look great 👍

However parse function doesn't do any validation beyound checking that correctness of GraphQL grammar.
This check should be performed by validateSchema function:

/**
* Implements the "Type Validation" sub-sections of the specification's
* "Type System" section.
*
* Validation runs synchronously, returning an array of encountered errors, or
* an empty array if no errors were encountered and the Schema is valid.
*/
export function validateSchema(
schema: GraphQLSchema,
): $ReadOnlyArray<GraphQLError> {

Can you please move your tests here:
https://github.com/graphql/graphql-js/blob/master/src/type/__tests__/validation-test.js#L1397

@IvanGoncharov
Copy link
Member

@spawnia Preferably test cases should match examples from graphql/graphql-spec#445

@IvanGoncharov IvanGoncharov added the spec RFC Implementation of a proposed change to the GraphQL specification label May 17, 2018
…te file

Todo: Actually implement the validation rule, possibly add some more tests
@spawnia
Copy link
Member Author

spawnia commented May 17, 2018

Unfortunately, implementation of this is not trivial and i think i will not be able to do it soon, i might implement it in PHP since i know that better.

It would be cool if maybe someone who has more experience with the project can take a shot of doing the actual validation?

@spawnia spawnia changed the title Add tests to validate Input Objects do not contain non-nullable circu… Validate Input Objects do not contain non-nullable circu… May 17, 2018
@IvanGoncharov
Copy link
Member

@spawnia Don't want this proposal to stuck so I created a draft implementation in #1359. I don't think it's ready to be merged but hopefully, it allows us to continue the conversation in graphql/graphql-spec#445

@IvanGoncharov
Copy link
Member

@spawnia Since #1359 superseded this PR can I close it?

@spawnia
Copy link
Member Author

spawnia commented May 27, 2018

Yep, thank you a lot for implementing this! Closed in favour of #1359

@spawnia spawnia closed this May 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed spec RFC Implementation of a proposed change to the GraphQL specification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants