fix: support opt-out of strict variable validation#8884
Conversation
✅ Docs preview readyThe preview is ready to be viewed. View the preview File Changes 0 new, 1 changed, 0 removedBuild ID: 3c4be840d877a2330bec6ad8 URL: https://www.apollographql.com/docs/deploy-preview/3c4be840d877a2330bec6ad8 |
This comment has been minimized.
This comment has been minimized.
conwuegb
left a comment
conwuegb
left a comment
There was a problem hiding this comment.
first pass comments, lgtm!
| .expect("could not parse query"); | ||
| query.validate_variables(&request, &schema) | ||
| }}; | ||
| fn run_validation_enforce_mode( |
There was a problem hiding this comment.
I'll let you decide whether it's best to merge this and the warn_mode function into one function. I'm okay with either option!
conwuegb
changed the title
There was a problem hiding this comment.
@aaronArinder, in the team discussion on this opt-out feature you mentioned that the changeset should be super clear that an action is required by the customer. Is this what you had in mind or could it be clearer?
|
tagging @theJC for visibility |
| fn default_strict_variable_validation() -> Mode { | ||
| Mode::Enforce | ||
| } |
There was a problem hiding this comment.
guessing an impl Default wouldn't have worked here?
There was a problem hiding this comment.
Yep, since we want each use of mode to potentially have different defaults!
| #[allow(clippy::result_large_err)] | ||
| fn run_validation( | ||
| schema: String, |
There was a problem hiding this comment.
so much easier to read (and the other macros-to-functions below as well)
default is specified in the new fn
| @@ -1,4 +1,4 @@ | |||
| ### Fix Router's validation of ObjectValue variables ([PR #8821](https://github.com/apollographql/router/pull/8821)) | |||
| ### Fix Router's validation of `ObjectValue` variables ([PR #8821](https://github.com/apollographql/router/pull/8821) and [PR #8884](https://github.com/apollographql/router/pull/8884)) | |||
There was a problem hiding this comment.
Trying to get caught up on the changes -- did the changeset I created for this pr (8884) get deleted in favor of updating the changeset for the previous pr (8821) with the opt-out information from this new pr?
There was a problem hiding this comment.
Yep! Since the two will go out in the same release, I think it will be easier for users to read about the changes in one place - they're really part of the same fix.
|
Force merging without @apollographql/docs approval. |
4 participants
#8821 fixed a gap in variable validation whereby the presence of unknown fields on an input object variable did not cause a request error as they should have. As a result, the stricter validation may cause breakages for customers.
To alleviate that potential pain point while customers update their variables to be compliant, this change introduces a router config option to retain the previous level of validation and issue a warning instead of an error.
Please Note: the stricter validation will be enabled by default. If you need to opt out, you must set the config option to
measureinstead.Enabled:
Disabled:
Docs have also been updated to reflect this change.
Additional notes
Checklist
Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.
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. ↩
A lot of (if not most) features benefit from built-in observability and
debug-level logs. Please read this guidance on metrics best-practices. ↩Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩