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

mapSchema carries over schema validation state #4087

Open
Tracked by #5201 ...
yaacovCR opened this issue Jan 7, 2022 · 2 comments
Open
Tracked by #5201 ...

mapSchema carries over schema validation state #4087

yaacovCR opened this issue Jan 7, 2022 · 2 comments

Comments

@yaacovCR
Copy link
Collaborator

yaacovCR commented Jan 7, 2022

Describe the bug

A schema that is explicitly assumed to be validated using the assumeValid flag or a schema that has actually been validated is marked as such using an internal flag.

mapSchema carries over this flag to the new schema. This is debatable in the former case (when the schema is explicitly marked as assumeValid, presumably to ensure that the validation step is skipped), but is definitely incorrect in the latter scenario.

To Reproduce

This was surfaced in #4066 where a schema will fail validation when incompletely pruned, but only fails when using graphql-executor. This is presumably because when using graphql-executor, we get a different order of test execution, and the propertySchema has not been validated elsewhere. If you step through the test code line by line, just running the failing test, the test failed even when using the graphql-js execute function!

Expected behavior

This flag should not be carried over automatically. This would be the expected behavior when the flag is not set explicitly. The flag is set explicitly in cases where the schema creator wants to skip validation altogether. Whether validation should also be skipped for any schemas mapped for the original schema is unclear, and could differ in each case. This should probably be an argument for the executor, and should be raised as such in graphql-js.

@yaacovCR
Copy link
Collaborator Author

yaacovCR commented Jan 7, 2022

I am not sure that this can be fixed in a non-breaking manner. I also opened graphql/graphql-js#3448 to discuss whether there are any upstream issues that need looking into.

@yaacovCR
Copy link
Collaborator Author

Fixed upstream for v17 by graphql/graphql-js#4244

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant