Validate variables of the executed operation only (#462)

* 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`

juniper/CHANGELOG.md

@@ -1,24 +1,7 @@
 # master
 
-## Features
+- Fix incorrect validation with non-executed operations [#455](https://github.com/graphql-rust/juniper/issues/455)
-
+- Correctly handle raw identifiers in field and argument names.
-- Support raw identifiers in field and argument names. (#[object] macro)
-
-## Breaking Changes
-
-- remove old `graphql_object!` macro, rename `object` proc macro to `graphql_object`
-
-- Remove deprecated `ScalarValue` custom derive (renamed to GraphQLScalarValue)
-
-- `graphql_union!` macro removed, replaced by `#[graphql_union]` proc macro
-
-- ScalarRefValue trait  removed
-  Trait was not required.
-
-- Changed return type of GraphQLType::resolve to `ExecutionResult`
-  This was done to unify the return type of all resolver methods
-  The previous `Value` return type was just an internal artifact of 
-  error handling.
 
 # [[0.14.1] 2019-10-24](https://github.com/graphql-rust/juniper/releases/tag/juniper-0.14.1)
 

juniper/src/executor/mod.rs

@@ -4,8 +4,8 @@ use fnv::FnvHashMap;
 
 use crate::{
     ast::{
-        Definition, Document, Fragment, FromInputValue, InputValue, OperationType, Selection,
+        Definition, Document, Fragment, FromInputValue, InputValue, Operation, OperationType,
-        ToInputValue, Type,
+        Selection, ToInputValue, Type,
     },
     parser::SourcePosition,
     value::Value,
@@ -31,6 +31,7 @@ pub use self::look_ahead::{
     Applies, ChildSelection, ConcreteLookAheadSelection, LookAheadArgument, LookAheadMethods,
     LookAheadSelection, LookAheadValue,
 };
+use crate::parser::Spanning;
 
 /// A type registry used to build schemas
 ///
@@ -652,9 +653,9 @@ impl<S> ExecutionError<S> {
     }
 }
 
-pub fn execute_validated_query<'a, QueryT, MutationT, CtxT, S>(
+pub fn execute_validated_query<'a, 'b, QueryT, MutationT, CtxT, S>(
-    document: Document<S>,
+    document: &'b Document<S>,
-    operation_name: Option<&str>,
+    operation: &'b Spanning<Operation<S>>,
     root_node: &RootNode<QueryT, MutationT, S>,
     variables: &Variables<S>,
     context: &CtxT,
@@ -663,38 +664,17 @@ where
     S: ScalarValue,
     QueryT: GraphQLType<S, Context = CtxT>,
     MutationT: GraphQLType<S, Context = CtxT>,
+    for<'c> &'c S: ScalarRefValue<'c>,
 {
     let mut fragments = vec![];
-    let mut operation = None;
+    for def in document.iter() {
-
-    for def in document {
         match def {
-            Definition::Operation(op) => {
-                if operation_name.is_none() && operation.is_some() {
-                    return Err(GraphQLError::MultipleOperationsProvided);
-                }
-
-                let move_op = operation_name.is_none()
-                    || op.item.name.as_ref().map(|s| s.item) == operation_name;
-
-                if move_op {
-                    operation = Some(op);
-                }
-            }
             Definition::Fragment(f) => fragments.push(f),
+            _ => (),
         };
     }
 
-    let op = match operation {
+    let default_variable_values = operation.item.variable_definitions.as_ref().map(|defs| {
-        Some(op) => op,
-        None => return Err(GraphQLError::UnknownOperationName),
-    };
-
-    if op.item.operation_type == OperationType::Subscription {
-        return Err(GraphQLError::IsSubscription);
-    }
-
-    let default_variable_values = op.item.variable_definitions.map(|defs| {
         defs.item
             .items
             .iter()
@@ -723,7 +703,7 @@ where
             final_vars = &all_vars;
         }
 
-        let root_type = match op.item.operation_type {
+        let root_type = match operation.item.operation_type {
             OperationType::Query => root_node.schema.query_type(),
             OperationType::Mutation => root_node
                 .schema
@@ -738,16 +718,16 @@ where
                 .map(|f| (f.item.name.item, &f.item))
                 .collect(),
             variables: final_vars,
-            current_selection_set: Some(&op.item.selection_set[..]),
+            current_selection_set: Some(&operation.item.selection_set[..]),
             parent_selection_set: None,
             current_type: root_type,
             schema: &root_node.schema,
             context,
             errors: &errors,
-            field_path: FieldPath::Root(op.start),
+            field_path: FieldPath::Root(operation.start),
         };
 
-        value = match op.item.operation_type {
+        value = match operation.item.operation_type {
             OperationType::Query => executor.resolve_into_value(&root_node.query_info, &root_node),
             OperationType::Mutation => {
                 executor.resolve_into_value(&root_node.mutation_info, &root_node.mutation_type)
@@ -882,6 +862,41 @@ where
     Ok((value, errors))
 }
 
+pub fn get_operation<'a, 'b, S>(
+    document: &'b Document<'b, S>,
+    operation_name: Option<&str>,
+) -> Result<&'b Spanning<Operation<'b, S>>, GraphQLError<'a>>
+where
+    S: ScalarValue,
+{
+    let mut operation = None;
+    for def in document {
+        match def {
+            Definition::Operation(op) => {
+                if operation_name.is_none() && operation.is_some() {
+                    return Err(GraphQLError::MultipleOperationsProvided);
+                }
+
+                let move_op = operation_name.is_none()
+                    || op.item.name.as_ref().map(|s| s.item) == operation_name;
+
+                if move_op {
+                    operation = Some(op);
+                }
+            }
+            _ => (),
+        };
+    }
+    let op = match operation {
+        Some(op) => op,
+        None => return Err(GraphQLError::UnknownOperationName),
+    };
+    if op.item.operation_type == OperationType::Subscription {
+        return Err(GraphQLError::IsSubscription);
+    }
+    Ok(op)
+}
+
 impl<'r, S> Registry<'r, S>
 where
     S: ScalarValue + 'r,

juniper/src/executor_tests/executor.rs

@@ -1067,7 +1067,7 @@ mod named_operations {
 
     #[crate::graphql_object_internal]
     impl Schema {
-        fn a() -> &str {
+        fn a(p: Option<String>) -> &str {
             "b"
         }
     }
@@ -1112,7 +1112,8 @@ mod named_operations {
     #[test]
     fn uses_named_operation_if_name_provided() {
         let schema = RootNode::new(Schema, EmptyMutation::<()>::new());
-        let doc = r"query Example { first: a } query OtherExample { second: a }";
+        let doc =
+            r"query Example($p: String!) { first: a(p: $p) } query OtherExample { second: a }";
 
         let vars = vec![].into_iter().collect();
 

juniper/src/executor_tests/variables.rs

@@ -830,52 +830,6 @@ fn allow_non_null_lists_of_non_null_to_contain_values() {
     );
 }
 
-#[test]
-fn does_not_allow_invalid_types_to_be_used_as_values() {
-    let schema = RootNode::new(TestType, EmptyMutation::<()>::new());
-
-    let query = r#"query q($input: TestType!) { fieldWithObjectInput(input: $input) }"#;
-    let vars = vec![(
-        "value".to_owned(),
-        InputValue::list(vec![InputValue::scalar("A"), InputValue::scalar("B")]),
-    )]
-    .into_iter()
-    .collect();
-
-    let error = crate::execute(query, None, &schema, &vars, &()).unwrap_err();
-
-    assert_eq!(
-        error,
-        ValidationError(vec![RuleError::new(
-            r#"Variable "$input" expected value of type "TestType!" which cannot be used as an input type."#,
-            &[SourcePosition::new(8, 0, 8)],
-        ),])
-    );
-}
-
-#[test]
-fn does_not_allow_unknown_types_to_be_used_as_values() {
-    let schema = RootNode::new(TestType, EmptyMutation::<()>::new());
-
-    let query = r#"query q($input: UnknownType!) { fieldWithObjectInput(input: $input) }"#;
-    let vars = vec![(
-        "value".to_owned(),
-        InputValue::list(vec![InputValue::scalar("A"), InputValue::scalar("B")]),
-    )]
-    .into_iter()
-    .collect();
-
-    let error = crate::execute(query, None, &schema, &vars, &()).unwrap_err();
-
-    assert_eq!(
-        error,
-        ValidationError(vec![RuleError::new(
-            r#"Variable "$input" expected value of type "UnknownType!" which cannot be used as an input type."#,
-            &[SourcePosition::new(8, 0, 8)],
-        ),])
-    );
-}
-
 #[test]
 fn default_argument_when_not_provided() {
     run_query(r#"{ fieldWithDefaultArgumentValue }"#, |result| {

juniper/src/lib.rs

@@ -152,6 +152,7 @@ mod executor_tests;
 pub use crate::util::to_camel_case;
 
 use crate::{
+    executor::{execute_validated_query, get_operation},
     introspection::{INTROSPECTION_QUERY, INTROSPECTION_QUERY_WITHOUT_DESCRIPTIONS},
     parser::{parse_document_source, ParseError, Spanning},
     validation::{validate_input_values, visit_all_rules, ValidatorContext},
@@ -206,13 +207,6 @@ where
     MutationT: GraphQLType<S, Context = CtxT>,
 {
     let document = parse_document_source(document_source, &root_node.schema)?;
-    {
-        let errors = validate_input_values(variables, &document, &root_node.schema);
-
-        if !errors.is_empty() {
-            return Err(GraphQLError::ValidationError(errors));
-        }
-    }
 
     {
         let mut ctx = ValidatorContext::new(&root_node.schema, &document);
@@ -224,7 +218,17 @@ where
         }
     }
 
-    executor::execute_validated_query(document, operation_name, root_node, variables, context)
+    let operation = get_operation(&document, operation_name)?;
+
+    {
+        let errors = validate_input_values(variables, operation, &root_node.schema);
+
+        if !errors.is_empty() {
+            return Err(GraphQLError::ValidationError(errors));
+        }
+    }
+
+    execute_validated_query(document, operation_name, root_node, variables, context)
 }
 
 /// Execute a query in a provided schema
@@ -245,13 +249,6 @@ where
     CtxT: Send + Sync,
 {
     let document = parse_document_source(document_source, &root_node.schema)?;
-    {
-        let errors = validate_input_values(variables, &document, &root_node.schema);
-
-        if !errors.is_empty() {
-            return Err(GraphQLError::ValidationError(errors));
-        }
-    }
     
     {
         let mut ctx = ValidatorContext::new(&root_node.schema, &document);
@@ -263,6 +260,16 @@ where
         }
     }
 
+    let operation = get_operation(&document, operation_name)?;
+
+    {
+        let errors = validate_input_values(variables, operation, &root_node.schema);
+
+        if !errors.is_empty() {
+            return Err(GraphQLError::ValidationError(errors));
+        }
+    }
+
     executor::execute_validated_query_async(document, operation_name, root_node, variables, context)
         .await
 }

juniper/src/validation/input_value.rs

@@ -1,9 +1,9 @@
 use std::{collections::HashSet, fmt};
 
 use crate::{
-    ast::{Definition, Document, InputValue, VariableDefinitions},
+    ast::{InputValue, Operation, VariableDefinitions},
     executor::Variables,
-    parser::SourcePosition,
+    parser::{SourcePosition, Spanning},
     schema::{
         meta::{EnumMeta, InputObjectMeta, MetaType, ScalarMeta},
         model::{SchemaType, TypeType},
@@ -21,7 +21,7 @@ enum Path<'a> {
 
 pub fn validate_input_values<S>(
     values: &Variables<S>,
-    document: &Document<S>,
+    operation: &Spanning<Operation<S>>,
     schema: &SchemaType<S>,
 ) -> Vec<RuleError>
 where
@@ -29,12 +29,8 @@ where
 {
     let mut errs = vec![];
 
-    for def in document {
+    if let Some(ref vars) = operation.item.variable_definitions {
-        if let Definition::Operation(ref op) = *def {
+        validate_var_defs(values, &vars.item, schema, &mut errs);
-            if let Some(ref vars) = op.item.variable_definitions {
-                validate_var_defs(values, &vars.item, schema, &mut errs);
-            }
-        }
     }
 
     errs.sort();
@@ -59,22 +55,25 @@ fn validate_var_defs<S>(
                     errors.push(RuleError::new(
                         &format!(
                             r#"Variable "${}" of required type "{}" was not provided."#,
-                            name.item,
+                            name.item, def.var_type.item,
-                            def.var_type.item,
                         ),
                         &[name.start],
                     ));
                 } else if let Some(v) = values.get(name.item) {
-                    errors.append(&mut unify_value(name.item, &name.start, v, &ct, schema, Path::Root));
+                    errors.append(&mut unify_value(
+                        name.item,
+                        &name.start,
+                        v,
+                        &ct,
+                        schema,
+                        Path::Root,
+                    ));
                 }
             }
-            _ => errors.push(RuleError::new(
+            _ => unreachable!(
-                &format!(
+                r#"Variable "${}" has invalid input type "{}" after document validation."#,
-                    r#"Variable "${}" expected value of type "{}" which cannot be used as an input type."#,
+                name.item, def.var_type.item,
-                    name.item, def.var_type.item,
+            ),
-                ),
-                &[ name.start ],
-            )),
         }
     }
 }