-
Notifications
You must be signed in to change notification settings - Fork 268
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
Remove legacy validation #4551
Remove legacy validation #4551
Conversation
This comment has been minimized.
This comment has been minimized.
CI performance tests
|
@Geal we do not define it as a built-in in apollo-compiler. The supergraphs that support defer, will have a defer definition already defined. And it looked like a bunch of tests [1] [2] [3] in the router already had them defined as part of the schema. I assume this test also needs it defined. |
alright, that's an easy fix then |
add the defer directive manually for now
This reverts commit e30c32a.
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.
Looks good overall, but I still think we should use .to_json()
over .to_string()
for parse errors: #4551 (comment)
The latter formats errors for a CLI-like output with a monospace font
{ | ||
"request_id": "[REDACTED]", | ||
"stats": { | ||
"## GraphQLValidationFailure\n": { |
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 we want to replace this metric with something else?
Marked as approved to mean I don’t need to approve again after this change, but I do think the parse error thing should be changed. |
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.
Awesome 🤩 everything LGTM except a few places in tests where we should use Schema::parse_test
Instead of CLI string formatting
Co-authored-by: Renée <renee.kooi@apollographql.com>
We are using a plugin defining a |
@yanns What was the request validation that you were doing at the supergraph service? Something GraphQL-specific? |
There are 2 use-cases:
So I guess our main issue is that our plugin cannot adapt the response in case of invalid GraphQL queries. |
Helllo Team, To reproduce
and say
Any help is appreciated, thank you. |
@sushant3524 Accepting invalid queries in the router may cause all kind of unsupported glitches in the router and in the query planner. The router does not do validation in JS but I think the query planner may validate its own outputs at the end of query planning. Subgraphs also usually validate queries and if they return an error the router will propagate it. Everything the router does relies on the query being valid so trying to pass through something invalid is asking for trouble. |
Thanks @goto-bus-stop for the quick revert. Have reviewed much of your code in the router repository and it is a pleasure to finally engage with you. Is it because of #4964 ? |
after searching I have found out that this is coming in bridge_query_planner from |
Fix #4407
Fix #4409
This removes GraphQL validation from the query planner, to use the Rust version instead. Validation has now moved to the query analysis layer, which means we can remove a lot of code that was there to accommodate parsing and validating the query but not rejecting it outright, because we needed to compare the validation result with the planner's.
This will greatly reduce the load on the planner, which will now only be used for planning queries, not validating.
This new validation process has been running in production for months concurrently with the JavaScript version, allowing us to detect and fix any discrepancies in the new implementation. We now have enough confidence in the new Rust-based validation to entirely switch off the less performant, JavaScript validation.
Remaining issues:
@defer
directive: a test is failing because the directive is used in the query but not found by the schema. @lrlna what is apollo-compiler expecting here? Is@defer
considered builtin, or does it need to be declared in the schema?Checklist
Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.
Exceptions
Note any exceptions here
Notes
Footnotes
It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩