From d10def5dca11d0558a646377cafaec9edb82b6aa Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Mon, 4 Nov 2024 15:38:40 +0100 Subject: [PATCH] Make variables in const contexts validation errors Fixes https://github.com/apollographql/apollo-rs/issues/852 GraphQL values can be of different kinds, including variables with the `$x` syntax. Variables are not allowed in some contexts, such as schemas. The parser already rejects them as syntax errors, but the Rust API allows mutating a document and representing variable values in contexts where they are not allowed. It was already the case that would usually cause a validation error like ``variable `$x` is not defined``. This PR ensures and tests that this is the case for every relevant context. It also makes related drive-by fixes: * Validate the default value of input fields and arguments * Validate the default value of variables once at their definition, not at every usage of the variable. This fixes duplicate diagnostics for a default value of incorrect type for a variable correctly used multiple times. --- crates/apollo-compiler/CHANGELOG.md | 2 + .../src/validation/input_object.rs | 5 + .../apollo-compiler/src/validation/value.rs | 12 - .../src/validation/variable.rs | 7 +- .../0102_invalid_string_values.graphql | 1 + .../0102_invalid_string_values.txt | 16 +- .../diagnostics/0111_const_value.txt | 7 + .../0102_invalid_string_values.graphql | 1 + .../tests/validation/variable.rs | 413 ++++++++++++++++++ 9 files changed, 443 insertions(+), 21 deletions(-) diff --git a/crates/apollo-compiler/CHANGELOG.md b/crates/apollo-compiler/CHANGELOG.md index 4c6d6d8c9..33086f49a 100644 --- a/crates/apollo-compiler/CHANGELOG.md +++ b/crates/apollo-compiler/CHANGELOG.md @@ -21,9 +21,11 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ## Fixes - **Validate against reserved names starting with `__` in schemas - [SimonSapin], [pull/923].** +- **Validate the default value of input fields and arguments - [SimonSapin], [pull/925].** [SimonSapin]: https://github.com/SimonSapin [pull/923]: https://github.com/apollographql/apollo-rs/issues/923 +[pull/925]: https://github.com/apollographql/apollo-rs/issues/925 # [1.0.0-beta.24](https://crates.io/crates/apollo-compiler/1.0.0-beta.24) - 2024-09-24 diff --git a/crates/apollo-compiler/src/validation/input_object.rs b/crates/apollo-compiler/src/validation/input_object.rs index 77c796f52..08635d32b 100644 --- a/crates/apollo-compiler/src/validation/input_object.rs +++ b/crates/apollo-compiler/src/validation/input_object.rs @@ -3,6 +3,7 @@ use crate::collections::HashMap; use crate::schema::validation::BuiltInScalars; use crate::schema::InputObjectType; use crate::validation::diagnostics::DiagnosticData; +use crate::validation::value::value_of_correct_type; use crate::validation::CycleError; use crate::validation::DiagnosticList; use crate::validation::RecursionGuard; @@ -208,6 +209,10 @@ pub(crate) fn validate_input_value_definitions( }, ); } + if let Some(default) = &input_value.default_value { + let var_defs = &[]; + value_of_correct_type(diagnostics, schema, &input_value.ty, default, var_defs); + } } else if is_built_in { // `validate_schema()` will insert the missing definition } else { diff --git a/crates/apollo-compiler/src/validation/value.rs b/crates/apollo-compiler/src/validation/value.rs index f2bebe961..308701a4c 100644 --- a/crates/apollo-compiler/src/validation/value.rs +++ b/crates/apollo-compiler/src/validation/value.rs @@ -143,18 +143,6 @@ pub(crate) fn value_of_correct_type( // TODO(@goto-bus-stop) This should use the is_assignable_to check if var_def.ty.inner_named_type() != ty.inner_named_type() { unsupported_type(diagnostics, arg_value, ty); - } else if let Some(default_value) = &var_def.default_value { - if var_def.ty.is_non_null() && default_value.is_null() { - unsupported_type(diagnostics, default_value, &var_def.ty) - } else { - value_of_correct_type( - diagnostics, - schema, - &var_def.ty, - default_value, - var_defs, - ) - } } } _ => unsupported_type(diagnostics, arg_value, ty), diff --git a/crates/apollo-compiler/src/validation/variable.rs b/crates/apollo-compiler/src/validation/variable.rs index 4aa3ca786..54dae2e7a 100644 --- a/crates/apollo-compiler/src/validation/variable.rs +++ b/crates/apollo-compiler/src/validation/variable.rs @@ -2,6 +2,7 @@ use crate::ast; use crate::collections::HashMap; use crate::executable; use crate::validation::diagnostics::DiagnosticData; +use crate::validation::value::value_of_correct_type; use crate::validation::DiagnosticList; use crate::validation::RecursionGuard; use crate::validation::RecursionLimitError; @@ -35,7 +36,11 @@ pub(crate) fn validate_variable_definitions( match type_definition { Some(type_definition) if type_definition.is_input_type() => { - // OK! + if let Some(default) = &variable.default_value { + // Default values are "const", not allowed to refer to other variables: + let var_defs_in_scope = &[]; + value_of_correct_type(diagnostics, schema, ty, default, var_defs_in_scope); + } } Some(type_definition) => { diagnostics.push( diff --git a/crates/apollo-compiler/test_data/diagnostics/0102_invalid_string_values.graphql b/crates/apollo-compiler/test_data/diagnostics/0102_invalid_string_values.graphql index 61a52638c..9d7648bd2 100644 --- a/crates/apollo-compiler/test_data/diagnostics/0102_invalid_string_values.graphql +++ b/crates/apollo-compiler/test_data/diagnostics/0102_invalid_string_values.graphql @@ -311,6 +311,7 @@ query variablesWithInvalidDefaultValues( complexArgField(complexArg: $c) intArgField(intArg: $a) stringArgField(stringArg: $b) + again: stringArgField(stringArg: $b) } } diff --git a/crates/apollo-compiler/test_data/diagnostics/0102_invalid_string_values.txt b/crates/apollo-compiler/test_data/diagnostics/0102_invalid_string_values.txt index 22a61141e..a5bb3ed28 100644 --- a/crates/apollo-compiler/test_data/diagnostics/0102_invalid_string_values.txt +++ b/crates/apollo-compiler/test_data/diagnostics/0102_invalid_string_values.txt @@ -512,42 +512,42 @@ Error: expected value of type ComplexInput, found a string │ ╰───────── provided value is a string ─────╯ Error: expected value of type Boolean!, found an integer - ╭─[0102_invalid_string_values.graphql:318:39] + ╭─[0102_invalid_string_values.graphql:319:39] │ 32 │ requiredField: Boolean! │ ────┬─── │ ╰───── expected type declared here as Boolean! │ - 318 │ $a: ComplexInput = { requiredField: 123, intField: "abc" } + 319 │ $a: ComplexInput = { requiredField: 123, intField: "abc" } │ ─┬─ │ ╰─── provided value is an integer ─────╯ Error: expected value of type Int, found a string - ╭─[0102_invalid_string_values.graphql:318:54] + ╭─[0102_invalid_string_values.graphql:319:54] │ 34 │ intField: Int │ ─┬─ │ ╰─── expected type declared here as Int │ - 318 │ $a: ComplexInput = { requiredField: 123, intField: "abc" } + 319 │ $a: ComplexInput = { requiredField: 123, intField: "abc" } │ ──┬── │ ╰──── provided value is a string ─────╯ Error: the required field `ComplexInput.requiredField` is not provided - ╭─[0102_invalid_string_values.graphql:326:22] + ╭─[0102_invalid_string_values.graphql:327:22] │ 32 │ requiredField: Boolean! │ ───────────┬─────────── │ ╰───────────── field defined here │ - 326 │ $a: ComplexInput = {intField: 3} + 327 │ $a: ComplexInput = {intField: 3} │ ──────┬────── │ ╰──────── missing value for field `requiredField` ─────╯ Error: expected value of type String, found an integer - ╭─[0102_invalid_string_values.graphql:335:26] + ╭─[0102_invalid_string_values.graphql:336:26] │ - 335 │ $a: [String] = ["one", 2] + 336 │ $a: [String] = ["one", 2] │ ────┬─── ┬ │ ╰───────────────── expected type declared here as String │ │ diff --git a/crates/apollo-compiler/test_data/diagnostics/0111_const_value.txt b/crates/apollo-compiler/test_data/diagnostics/0111_const_value.txt index 59c5d1693..3a47a850f 100644 --- a/crates/apollo-compiler/test_data/diagnostics/0111_const_value.txt +++ b/crates/apollo-compiler/test_data/diagnostics/0111_const_value.txt @@ -5,6 +5,13 @@ Error: syntax error: unexpected variable value in a Const context │ ┬ │ ╰── unexpected variable value in a Const context ───╯ +Error: variable `$var1` is not defined + ╭─[0111_const_value.graphql:3:23] + │ + 3 │ $var2: Boolean! = $var1 + │ ──┬── + │ ╰──── not found in this scope +───╯ Error: syntax error: unexpected variable value in a Const context ╭─[0111_const_value.graphql:11:26] │ diff --git a/crates/apollo-compiler/test_data/serializer/diagnostics/0102_invalid_string_values.graphql b/crates/apollo-compiler/test_data/serializer/diagnostics/0102_invalid_string_values.graphql index b4ed19600..dee04b063 100644 --- a/crates/apollo-compiler/test_data/serializer/diagnostics/0102_invalid_string_values.graphql +++ b/crates/apollo-compiler/test_data/serializer/diagnostics/0102_invalid_string_values.graphql @@ -284,6 +284,7 @@ query variablesWithInvalidDefaultValues($a: Int = "one", $b: String = 4, $c: Com complexArgField(complexArg: $c) intArgField(intArg: $a) stringArgField(stringArg: $b) + again: stringArgField(stringArg: $b) } } diff --git a/crates/apollo-compiler/tests/validation/variable.rs b/crates/apollo-compiler/tests/validation/variable.rs index 7faae9d09..6aa4ac8ea 100644 --- a/crates/apollo-compiler/tests/validation/variable.rs +++ b/crates/apollo-compiler/tests/validation/variable.rs @@ -1,4 +1,9 @@ +use apollo_compiler::ast; +use apollo_compiler::ast::Value; +use apollo_compiler::name; use apollo_compiler::parse_mixed_validate; +use apollo_compiler::schema::ExtendedType; +use apollo_compiler::Node; #[test] fn it_raises_undefined_variable_in_query_error() { @@ -132,3 +137,411 @@ type Product { "{errors}" ); } + +/// apollo-parser already emits parse errors for variable syntax in const context, +/// but it is still possible to mutate Rust data structures to create `Value::Variable(x)` +/// that should be validation errors. +/// +/// Here we parse a document that uses a string value `"x"` in all places a const value can show up +/// then programatically replace them with `$x` variable usage. +/// We expect the original document to be valid, and the modified documents to have +/// as many validation errors as occurrences of `"x"` strings in the original. +#[test] +fn variables_in_const_contexts() { + let input = r#" + directive @dir( + arg: InputObj = {x: ["x"]} @dir2(arg: "x") + ) repeatable on + | QUERY + | MUTATION + | SUBSCRIPTION + | FIELD + | FRAGMENT_DEFINITION + | FRAGMENT_SPREAD + | INLINE_FRAGMENT + | VARIABLE_DEFINITION + | SCHEMA + | SCALAR + | OBJECT + | FIELD_DEFINITION + | ARGUMENT_DEFINITION + | INTERFACE + | UNION + | ENUM + | ENUM_VALUE + | INPUT_OBJECT + | INPUT_FIELD_DEFINITION + + directive @dir2( + arg: String + ) repeatable on + | ARGUMENT_DEFINITION + | INPUT_OBJECT + | INPUT_FIELD_DEFINITION + + schema @dir(arg: {x: ["x"]}) { + query: Query + } + extend schema @dir(arg: {x: ["x"]}) + + scalar S @dir(arg: {x: ["x"]}) + extend scalar S @dir(arg: {x: ["x"]}) + + type Query implements Inter @dir(arg: {x: ["x"]}) { + field( + arg1: String + arg2: InputObj = {x: ["x"]} @dir(arg: {x: ["x"]}) + ): String @dir(arg: {x: ["x"]}) + } + extend type Query @dir(arg: {x: ["x"]}) + + interface Inter @dir(arg: {x: ["x"]}) { + field( + arg1: String + arg2: InputObj = {x: ["x"]} @dir(arg: {x: ["x"]}) + ): String @dir(arg: {x: ["x"]}) + } + extend interface Inter @dir(arg: {x: ["x"]}) + + union U @dir(arg: {x: ["x"]}) = Query + extend union U @dir(arg: {x: ["x"]}) + + enum Maybe @dir(arg: {x: ["x"]}) { + YES @dir(arg: {x: ["x"]}) + NO @dir(arg: {x: ["x"]}) + } + extend enum Maybe @dir(arg: {x: ["x"]}) + + input InputObj @dir2(arg: "x") { + x: [String] = ["x"] @dir2(arg: "x") + } + extend input InputObj @dir2(arg: "x") + + query( + $x: String + $y: InputObj = {x: ["x"]} @dir(arg: {x: ["x"]}) + ) { + field(arg1: $x, arg2: $y) + } + "#; + fn mutate_dir_arg(directive: &mut Node) { + mutate_input_obj_value(&mut directive.make_mut().arguments[0].make_mut().value) + } + + fn mutate_input_obj_value(value: &mut Node) { + let Value::Object(fields) = value.make_mut() else { + panic!("expected object") + }; + let Value::List(items) = fields[0].1.make_mut() else { + panic!("expected list") + }; + mutate_string_value(&mut items[0]) + } + + fn mutate_string_value(value: &mut Node) { + *value.make_mut() = Value::Variable(name!(x)) + } + + let (schema, doc) = apollo_compiler::parse_mixed_validate(input, "input.graphql").unwrap(); + let mut doc = doc.into_inner(); + + let operation = doc.operations.anonymous.as_mut().unwrap().make_mut(); + let variable_def = operation.variables[1].make_mut(); + mutate_input_obj_value(variable_def.default_value.as_mut().unwrap()); + mutate_dir_arg(&mut variable_def.directives[0]); + + assert!( + !doc.to_string().contains("\"x\""), + "Did not replace all string values with variables:\n{}", + doc + ); + let errors = doc.validate(&schema).unwrap_err().errors; + let expected = expect_test::expect![[r#" + Error: variable `$x` is not defined + ╭─[input.graphql:72:33] + │ + 72 │ $y: InputObj = {x: ["x"]} @dir(arg: {x: ["x"]}) + │ ─┬─ + │ ╰─── not found in this scope + ────╯ + Error: variable `$x` is not defined + ╭─[input.graphql:72:54] + │ + 72 │ $y: InputObj = {x: ["x"]} @dir(arg: {x: ["x"]}) + │ ─┬─ + │ ╰─── not found in this scope + ────╯ + "#]]; + expected.assert_eq(&errors.to_string()); + let expected_executable_errors = 2; + assert_eq!(errors.len(), expected_executable_errors); + + let mut schema = schema.into_inner(); + + let dir_arg_def = schema.directive_definitions["dir"].make_mut().arguments[0].make_mut(); + let dir2 = dir_arg_def.directives[0].make_mut(); + mutate_input_obj_value(dir_arg_def.default_value.as_mut().unwrap()); + mutate_string_value(&mut dir2.arguments[0].make_mut().value); + + let def = schema.schema_definition.make_mut(); + mutate_dir_arg(&mut def.directives[0]); + mutate_dir_arg(&mut def.directives[1]); + + let ExtendedType::Scalar(def) = &mut schema.types["S"] else { + panic!("expected scalar") + }; + let def = def.make_mut(); + mutate_dir_arg(&mut def.directives[0]); + mutate_dir_arg(&mut def.directives[1]); + + let ExtendedType::Object(def) = &mut schema.types["Query"] else { + panic!("expected object") + }; + let def = def.make_mut(); + let field = def.fields[0].make_mut(); + let field_arg = field.arguments[1].make_mut(); + mutate_dir_arg(&mut def.directives[0]); + mutate_dir_arg(&mut def.directives[1]); + mutate_dir_arg(&mut field.directives[0]); + mutate_dir_arg(&mut field_arg.directives[0]); + mutate_input_obj_value(field_arg.default_value.as_mut().unwrap()); + + let ExtendedType::Interface(def) = &mut schema.types["Inter"] else { + panic!("expected interface") + }; + let def = def.make_mut(); + let field = def.fields[0].make_mut(); + let field_arg = field.arguments[1].make_mut(); + mutate_dir_arg(&mut def.directives[0]); + mutate_dir_arg(&mut def.directives[1]); + mutate_dir_arg(&mut field.directives[0]); + mutate_dir_arg(&mut field_arg.directives[0]); + mutate_input_obj_value(field_arg.default_value.as_mut().unwrap()); + + let ExtendedType::Union(def) = &mut schema.types["U"] else { + panic!("expected union") + }; + let def = def.make_mut(); + mutate_dir_arg(&mut def.directives[0]); + mutate_dir_arg(&mut def.directives[1]); + + let ExtendedType::Enum(def) = &mut schema.types["Maybe"] else { + panic!("expected enum") + }; + let def = def.make_mut(); + mutate_dir_arg(&mut def.directives[0]); + mutate_dir_arg(&mut def.directives[1]); + mutate_dir_arg(&mut def.values["YES"].make_mut().directives[0]); + mutate_dir_arg(&mut def.values["NO"].make_mut().directives[0]); + + let ExtendedType::InputObject(def) = &mut schema.types["InputObj"] else { + panic!("expected input object") + }; + let def = def.make_mut(); + let field = def.fields[0].make_mut(); + let Value::List(items) = field.default_value.as_mut().unwrap().make_mut() else { + panic!("expected list") + }; + mutate_string_value(&mut def.directives[0].make_mut().arguments[0].make_mut().value); + mutate_string_value(&mut def.directives[1].make_mut().arguments[0].make_mut().value); + mutate_string_value(&mut field.directives[0].make_mut().arguments[0].make_mut().value); + mutate_string_value(&mut items[0]); + + assert!( + !schema.to_string().contains("\"x\""), + "Did not replace all string values with variables:\n{}", + schema + ); + let errors = schema.validate().unwrap_err().errors; + let expected = expect_test::expect![[r#" + Error: variable `$x` is not defined + ╭─[input.graphql:3:34] + │ + 3 │ arg: InputObj = {x: ["x"]} @dir2(arg: "x") + │ ─┬─ + │ ╰─── not found in this scope + ───╯ + Error: variable `$x` is not defined + ╭─[input.graphql:3:51] + │ + 3 │ arg: InputObj = {x: ["x"]} @dir2(arg: "x") + │ ─┬─ + │ ╰─── not found in this scope + ───╯ + Error: variable `$x` is not defined + ╭─[input.graphql:32:31] + │ + 32 │ schema @dir(arg: {x: ["x"]}) { + │ ─┬─ + │ ╰─── not found in this scope + ────╯ + Error: variable `$x` is not defined + ╭─[input.graphql:35:38] + │ + 35 │ extend schema @dir(arg: {x: ["x"]}) + │ ─┬─ + │ ╰─── not found in this scope + ────╯ + Error: variable `$x` is not defined + ╭─[input.graphql:37:33] + │ + 37 │ scalar S @dir(arg: {x: ["x"]}) + │ ─┬─ + │ ╰─── not found in this scope + ────╯ + Error: variable `$x` is not defined + ╭─[input.graphql:38:40] + │ + 38 │ extend scalar S @dir(arg: {x: ["x"]}) + │ ─┬─ + │ ╰─── not found in this scope + ────╯ + Error: variable `$x` is not defined + ╭─[input.graphql:40:52] + │ + 40 │ type Query implements Inter @dir(arg: {x: ["x"]}) { + │ ─┬─ + │ ╰─── not found in this scope + ────╯ + Error: variable `$x` is not defined + ╭─[input.graphql:43:39] + │ + 43 │ arg2: InputObj = {x: ["x"]} @dir(arg: {x: ["x"]}) + │ ─┬─ + │ ╰─── not found in this scope + ────╯ + Error: variable `$x` is not defined + ╭─[input.graphql:43:60] + │ + 43 │ arg2: InputObj = {x: ["x"]} @dir(arg: {x: ["x"]}) + │ ─┬─ + │ ╰─── not found in this scope + ────╯ + Error: variable `$x` is not defined + ╭─[input.graphql:44:38] + │ + 44 │ ): String @dir(arg: {x: ["x"]}) + │ ─┬─ + │ ╰─── not found in this scope + ────╯ + Error: variable `$x` is not defined + ╭─[input.graphql:46:42] + │ + 46 │ extend type Query @dir(arg: {x: ["x"]}) + │ ─┬─ + │ ╰─── not found in this scope + ────╯ + Error: variable `$x` is not defined + ╭─[input.graphql:48:40] + │ + 48 │ interface Inter @dir(arg: {x: ["x"]}) { + │ ─┬─ + │ ╰─── not found in this scope + ────╯ + Error: variable `$x` is not defined + ╭─[input.graphql:51:39] + │ + 51 │ arg2: InputObj = {x: ["x"]} @dir(arg: {x: ["x"]}) + │ ─┬─ + │ ╰─── not found in this scope + ────╯ + Error: variable `$x` is not defined + ╭─[input.graphql:51:60] + │ + 51 │ arg2: InputObj = {x: ["x"]} @dir(arg: {x: ["x"]}) + │ ─┬─ + │ ╰─── not found in this scope + ────╯ + Error: variable `$x` is not defined + ╭─[input.graphql:52:38] + │ + 52 │ ): String @dir(arg: {x: ["x"]}) + │ ─┬─ + │ ╰─── not found in this scope + ────╯ + Error: variable `$x` is not defined + ╭─[input.graphql:54:47] + │ + 54 │ extend interface Inter @dir(arg: {x: ["x"]}) + │ ─┬─ + │ ╰─── not found in this scope + ────╯ + Error: variable `$x` is not defined + ╭─[input.graphql:56:32] + │ + 56 │ union U @dir(arg: {x: ["x"]}) = Query + │ ─┬─ + │ ╰─── not found in this scope + ────╯ + Error: variable `$x` is not defined + ╭─[input.graphql:57:39] + │ + 57 │ extend union U @dir(arg: {x: ["x"]}) + │ ─┬─ + │ ╰─── not found in this scope + ────╯ + Error: variable `$x` is not defined + ╭─[input.graphql:59:35] + │ + 59 │ enum Maybe @dir(arg: {x: ["x"]}) { + │ ─┬─ + │ ╰─── not found in this scope + ────╯ + Error: variable `$x` is not defined + ╭─[input.graphql:60:32] + │ + 60 │ YES @dir(arg: {x: ["x"]}) + │ ─┬─ + │ ╰─── not found in this scope + ────╯ + Error: variable `$x` is not defined + ╭─[input.graphql:61:31] + │ + 61 │ NO @dir(arg: {x: ["x"]}) + │ ─┬─ + │ ╰─── not found in this scope + ────╯ + Error: variable `$x` is not defined + ╭─[input.graphql:63:42] + │ + 63 │ extend enum Maybe @dir(arg: {x: ["x"]}) + │ ─┬─ + │ ╰─── not found in this scope + ────╯ + Error: variable `$x` is not defined + ╭─[input.graphql:65:35] + │ + 65 │ input InputObj @dir2(arg: "x") { + │ ─┬─ + │ ╰─── not found in this scope + ────╯ + Error: variable `$x` is not defined + ╭─[input.graphql:66:28] + │ + 66 │ x: [String] = ["x"] @dir2(arg: "x") + │ ─┬─ + │ ╰─── not found in this scope + ────╯ + Error: variable `$x` is not defined + ╭─[input.graphql:66:44] + │ + 66 │ x: [String] = ["x"] @dir2(arg: "x") + │ ─┬─ + │ ╰─── not found in this scope + ────╯ + Error: variable `$x` is not defined + ╭─[input.graphql:68:42] + │ + 68 │ extend input InputObj @dir2(arg: "x") + │ ─┬─ + │ ╰─── not found in this scope + ────╯ + "#]]; + expected.assert_eq(&errors.to_string()); + let expected_schema_errors = 26; + assert_eq!(errors.len(), expected_schema_errors); + assert_eq!( + input.matches("\"x\"").count(), + expected_schema_errors + expected_executable_errors + ) +}