-
Notifications
You must be signed in to change notification settings - Fork 2k
Create more flexible context-specific validation model #719
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
Comments
Just throwing my $.02 - If I understood the schema validation step correctly (that is, I'm assuming there isn't a validation rule that checks something other than the integrity of the schema), then I think an invalid schema should never exist. #717 and related touch on a specific situation where we try invalidating what was a valid schema before by enforcing a restriction to the schema generation - I feel like this should be treated as an edge case and probably something we'd need to consider in addition when we'd like to add more restrictions to the specifications/implementations later on. GraphiQL is an interesting use case because it probably needs both variations of schema concepts - "source of truth" schema to execute a query and "reference" schema to power IDE-like features without the said computation cost. With that said, I feel like we'd be walking a dangerous zone if GraphiQL is allowed to interact with an invalid schema. Another thought I had was the possible discrepancy between two variations of the schema. Is it possible to have a "reference" schema that is different (e.g. has invalid new/modified field/types) from the "source-of-truth" schema? |
I think it would a very good addition to the library 👍 Just would like to share my experience with validation rules in sangria. When I first started working on schema validation, I introduced a rule-based mechanism similar to the query validations. After some time I removed it in favor of in-place validations mostly because it felt like an overkill for a very small set of rules that I had at the beginning. After some time the number of validations became much bigger and the validations themselves more complex, so I decided to re-introduce the rule-based mechanism I had at the beginning (though I haven't refactored all validations yet): This makes it really easy to organize and maintain these rules, so I definitely can recommend this approach. It also has another big advantage - user of the library can pick and choose validations they would like to execute against a schema. For example, with recent restriction on double underscore ( I personally find the solution introduced in graphql-js to deal with this issue quite inflexible (a console warning). Users that just start with the library probably don't want to use I think it becomes even more important when a schema is generated dynamically based on some external definition (possibly coming from DB). Meanwhile, there are quite a few services that rely on this (like graphcool or scaphold). I myself work on project that has parts of the schema generated based on user-defined data structures that are coming from DB. For these services, schema definition and names are validated beforehand elsewhere and one can just disable schema validation entirely to save on CPU cycles (this can be very useful because in some scenarios schema is loaded and re-created for every single request). Also when we look at migration process for these services, rule-based schema validation becomes quite valuable:
I also agree that tools like GraphiQL should not enforce schema validation rules unless it is necessary for their functionality. For example, I can imagine scenarios where people may relax name character set restrictions on their own risk. They still would be able to use GraphiQL with disabled name validation rule but apollo-ios code generator probably will include it since it requires proper names for swift codegen. That said, if GraphiQL does need specific name pattern for, let's say, autocompletion, then it probably better to keep the validation rule. |
Extracting @leebyron's comment from #717:
Adding the "discussion" label.
The text was updated successfully, but these errors were encountered: