Validate variables of the executed operation only

juniper/CHANGELOG.md

@@ -1,5 +1,6 @@
 # master
 
+- 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.
 
 # [[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,
-        ToInputValue, Type,
+        Definition, Document, Fragment, FromInputValue, InputValue, Operation, OperationType,
+        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
 ///
@@ -609,9 +610,9 @@ impl<S> ExecutionError<S> {
     }
 }
 
-pub fn execute_validated_query<'a, QueryT, MutationT, CtxT, S>(
-    document: Document<S>,
-    operation_name: Option<&str>,
+pub fn execute_validated_query<'a, 'b, QueryT, MutationT, CtxT, S>(
+    document: &'b Document<S>,
+    operation: &'b Spanning<Operation<S>>,
     root_node: &RootNode<QueryT, MutationT, S>,
     variables: &Variables<S>,
     context: &CtxT,
@@ -620,39 +621,17 @@ where
     S: ScalarValue,
     QueryT: GraphQLType<S, Context = CtxT>,
     MutationT: GraphQLType<S, Context = CtxT>,
-    for<'b> &'b S: ScalarRefValue<'b>,
+    for<'c> &'c S: ScalarRefValue<'c>,
 {
     let mut fragments = vec![];
-    let mut operation = None;
-
-    for def in document {
+    for def in document.iter() {
         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 {
-        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| {
+    let default_variable_values = operation.item.variable_definitions.as_ref().map(|defs| {
         defs.item
             .items
             .iter()
@@ -681,7 +660,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
@@ -696,16 +675,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)
@@ -720,6 +699,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::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

@@ -806,50 +806,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

@@ -150,7 +150,7 @@ mod executor_tests;
 pub use crate::util::to_camel_case;
 
 use crate::{
-    executor::execute_validated_query,
+    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},
@@ -203,25 +203,28 @@ 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);
+        let mut ctx = ValidatorContext::new(&root_node.schema, &document);
+        visit_all_rules(&mut ctx, &document);
 
+        let errors = ctx.into_errors();
         if !errors.is_empty() {
             return Err(GraphQLError::ValidationError(errors));
         }
     }
 
+    let operation = get_operation(&document, operation_name)?;
+
     {
-        let mut ctx = ValidatorContext::new(&root_node.schema, &document);
-        visit_all_rules(&mut ctx, &document);
+        let errors = validate_input_values(variables, operation, &root_node.schema);
 
-        let errors = ctx.into_errors();
         if !errors.is_empty() {
             return Err(GraphQLError::ValidationError(errors));
         }
     }
 
-    execute_validated_query(document, operation_name, root_node, variables, context)
+    execute_validated_query(&document, operation, root_node, variables, context)
 }
 
 /// Execute the reference introspection query in the provided schema

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
@@ -30,12 +30,8 @@ where
 {
     let mut errs = vec![];
 
-    for def in document {
-        if let Definition::Operation(ref op) = *def {
-            if let Some(ref vars) = op.item.variable_definitions {
-                validate_var_defs(values, &vars.item, schema, &mut errs);
-            }
-        }
+    if let Some(ref vars) = operation.item.variable_definitions {
+        validate_var_defs(values, &vars.item, schema, &mut errs);
     }
 
     errs.sort();
@@ -70,13 +66,10 @@ fn validate_var_defs<S>(
                     errors.append(&mut unify_value(name.item, &name.start, v, &ct, schema, Path::Root));
                 }
             }
-            _ => errors.push(RuleError::new(
-                &format!(
-                    r#"Variable "${}" expected value of type "{}" which cannot be used as an input type."#,
-                    name.item, def.var_type.item,
-                ),
-                &[ name.start ],
-            )),
+            _ => panic!(
+                r#"Variable "${}" expected value of type "{}" which cannot be used as an input type."#,
+                name.item, def.var_type.item,
+            ),
         }
     }
 }