Skip to content

Validate variables of the executed operation only #462

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

Merged
merged 3 commits into from
Dec 16, 2019

Conversation

cairomassimo
Copy link
Contributor

Fix for #455

This PR changes a larger number of files than expected. Here is the rationale.

  • We want to validate input variables only for the operation selected for execution. The code that extracts the selected operation was already present in execute_validated_query. However, we need the result in validate_input_values which is called before execute_validated_query.
  • So, I extracted the code to a get_operation method (the naming mimics the GraphQL spec, see https://graphql.github.io/graphql-spec/June2018/#GetOperation() ), meant to be called directly from execute. The resulting Operation can be then passed directly to validate_input_values instead of the whole document.
  • However, the get_operation code originally runs after the validation of the document, but validate_input_values is run before this validation. Calling get_operation before the validation of the document is dangerous, as it could impact the error reporting for invalid documents (say, multiple operations defined with the same name). I thought that it made much more sense to validate the document first, and then the input variables, so I made this change.
  • After this change, I got two failing unit tests. However, I realized that type of the input variables was validated twice (once as part of the document validation, and again during input values validation), and these failing tests were actually depending on which of the redundant validations is applied first.
  • After checking that the validation of the type of input variables as part of the document validation is already unit-tested, I decided to remove the tests does_not_allow_invalid_types_to_be_used_as_values and does_not_allow_unknown_types_to_be_used_as_values.

@codecov-io
Copy link

codecov-io commented Nov 15, 2019

Codecov Report

Merging #462 into master will decrease coverage by 0.03%.
The diff coverage is 90.9%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #462      +/-   ##
==========================================
- Coverage   85.73%   85.69%   -0.04%     
==========================================
  Files         111      111              
  Lines       15960    15938      -22     
==========================================
- Hits        13683    13658      -25     
- Misses       2277     2280       +3
Impacted Files Coverage Δ
juniper/src/executor_tests/variables.rs 96.19% <ø> (+0.17%) ⬆️
juniper/src/executor_tests/executor.rs 99.76% <100%> (ø) ⬆️
juniper/src/lib.rs 84.37% <100%> (+0.5%) ⬆️
juniper/src/validation/input_value.rs 83.5% <83.33%> (-1.08%) ⬇️
juniper/src/executor/mod.rs 91.64% <92%> (+0.1%) ⬆️
juniper/src/http/playground.rs 0% <0%> (-25%) ⬇️
juniper/src/integrations/serde.rs 57.35% <0%> (-2.21%) ⬇️
juniper_warp/src/lib.rs 93.62% <0%> (-0.5%) ⬇️
juniper_iron/src/lib.rs 72.98% <0%> (-0.16%) ⬇️
juniper_codegen/src/lib.rs 2.17% <0%> (+0.04%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1148c75...32806bf. Read the comment docs.

Copy link
Member

@LegNeato LegNeato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the change and detailed explanation! 🍻 . Two small things that I see.

Use `unreachable!` instead of `panic!` on invalid variable types,
since thay have already been checked during document validation.
@LegNeato LegNeato merged commit 675ae06 into graphql-rust:master Dec 16, 2019
@LegNeato
Copy link
Member

Thanks for the PR and the explanation!

@cairomassimo cairomassimo deleted the fix-multiop-validation branch December 16, 2019 10:26
LegNeato pushed a commit that referenced this pull request Jan 19, 2020
* Validate variables of the executed operation only

* Use `unreachable!` in `validate_var_defs`.

Use `unreachable!` instead of `panic!` on invalid variable types,
since thay have already been checked during document validation.

* Fix formatting in `validation/input_value.rs`
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

Successfully merging this pull request may close these issues.

3 participants