From b3ee3066f93459ea9a2d4fa945a712dee2e3cf95 Mon Sep 17 00:00:00 2001 From: Dylan Anthony Date: Thu, 30 Jan 2025 01:25:02 -0700 Subject: [PATCH] Use shapes to validate bodies (#6694) Co-authored-by: Matthew Hawkins --- .../sources/connect/validation/expression.rs | 218 ++++++++++++------ .../connect/validation/extended_type.rs | 6 +- .../connect/validation/http/headers.rs | 3 +- .../sources/connect/validation/http/url.rs | 3 +- .../src/sources/connect/validation/mod.rs | 6 +- .../sources/connect/validation/selection.rs | 114 ++++----- ...lidation_tests@body_selection.graphql.snap | 31 +-- ...idation_tests@empty_selection.graphql.snap | 6 +- ...d_namespace_in_body_selection.graphql.snap | 6 +- ...ested_paths_in_json_selection.graphql.snap | 18 +- ...ests@invalid_selection_syntax.graphql.snap | 6 +- ...@valid_selection_with_escapes.graphql.snap | 10 +- .../test_data/body_selection.graphql | 18 +- .../src/sources/connect/variable.rs | 21 +- 14 files changed, 264 insertions(+), 202 deletions(-) diff --git a/apollo-federation/src/sources/connect/validation/expression.rs b/apollo-federation/src/sources/connect/validation/expression.rs index 2f42c0ed94..f280b31091 100644 --- a/apollo-federation/src/sources/connect/validation/expression.rs +++ b/apollo-federation/src/sources/connect/validation/expression.rs @@ -95,72 +95,96 @@ impl<'schema> Context<'schema> { } } +pub(crate) fn scalars() -> Shape { + Shape::one( + vec![ + Shape::int([]), + Shape::float([]), + Shape::bool(None), + Shape::string(None), + Shape::null([]), + Shape::none(), + ], + [], + ) +} + /// Take a single expression and check that it's valid for the given context. This checks that /// the expression can be executed given the known args and that the output shape is as expected. -/// -/// TODO: this is only useful for URIs and headers right now, because it assumes objects/arrays are invalid. -pub(crate) fn validate(expression: &Expression, context: &Context) -> Result<(), Message> { - let Expression { - expression, - location, - } = expression; - let shape = expression.shape(); - - validate_shape(&shape, context, location.start) +pub(crate) fn validate( + expression: &Expression, + context: &Context, + expected_shape: &Shape, +) -> Result<(), Message> { + let shape = expression.expression.shape(); + + let actual_shape = resolve_shape(&shape, context, expression)?; + if let Some(mismatch) = expected_shape + .validate(&actual_shape) + .into_iter() + // Unknown satisfies nothing, but we have to allow it for things like `$config` + .find(|mismatch| !mismatch.received.is_unknown()) + { + Err(Message { + code: context.code, + message: format!( + "{} values aren't valid here", + shape_name(&mismatch.received) + ), + locations: transform_locations(&mismatch.received.locations, context, expression), + }) + } else { + Ok(()) + } } /// Validate that the shape is an acceptable output shape for an Expression. -/// -/// TODO: Some day, whether objects or arrays are allowed will be dependent on &self (i.e., is the * modifier used) -fn validate_shape( +fn resolve_shape( shape: &Shape, context: &Context, - expression_offset: usize, -) -> Result<(), Message> { + expression: &Expression, +) -> Result { match shape.case() { - ShapeCase::Array { .. } => Err(Message { - code: context.code, - message: "array values aren't valid here".to_string(), - locations: transform_locations(&shape.locations, context, expression_offset), - }), - ShapeCase::Object { .. } => Err(Message { - code: context.code, - message: "object values aren't valid here".to_string(), - locations: transform_locations(&shape.locations, context, expression_offset), - }), ShapeCase::One(shapes) => { + let mut inners = Vec::new(); for inner in shapes { - validate_shape( + inners.push(resolve_shape( &inner.with_locations(shape.locations.clone()), context, - expression_offset, - )?; + expression, + )?); } - Ok(()) + Ok(Shape::one(inners, [])) } ShapeCase::All(shapes) => { + let mut inners = Vec::new(); for inner in shapes { - validate_shape( + inners.push(resolve_shape( &inner.with_locations(shape.locations.clone()), context, - expression_offset, - )?; + expression, + )?); } - Ok(()) + Ok(Shape::all(inners, [])) } ShapeCase::Name(name, key) => { let mut resolved = if name.value == "$root" { + let mut key_str = key.iter().map(|key| key.to_string()).join("."); + if !key_str.is_empty() { + key_str = format!("`{key_str}` "); + } return Err(Message { code: context.code, message: format!( - "`{key}` must start with one of {namespaces}", - key = key.iter().map(|key| key.to_string()).join("."), + "{key_str}must start with one of {namespaces}", namespaces = context.var_lookup.keys().map(|ns| ns.as_str()).join(", "), ), locations: transform_locations( - key.first().iter().flat_map(|key| &key.locations), + key.first() + .map(|key| &key.locations) + .unwrap_or(&shape.locations), context, - expression_offset, + expression, ), }); } else if name.value.starts_with('$') { @@ -170,7 +194,7 @@ fn validate_shape( "unknown variable `{name}`, must be one of {namespaces}", namespaces = context.var_lookup.keys().map(|ns| ns.as_str()).join(", ") ), - locations: transform_locations(&shape.locations, context, expression_offset), + locations: transform_locations(&shape.locations, context, expression), })?; context .var_lookup @@ -181,11 +205,7 @@ fn validate_shape( "{namespace} is not valid here, must be one of {namespaces}", namespaces = context.var_lookup.keys().map(|ns| ns.as_str()).join(", "), ), - locations: transform_locations( - &shape.locations, - context, - expression_offset, - ), + locations: transform_locations(&shape.locations, context, expression), })? .clone() } else { @@ -197,7 +217,7 @@ fn validate_shape( .ok_or_else(|| Message { code: context.code, message: format!("unknown type `{name}`"), - locations: transform_locations(&name.locations, context, expression_offset), + locations: transform_locations(&name.locations, context, expression), })? }; resolved.locations.extend(shape.locations.iter().cloned()); @@ -217,35 +237,55 @@ fn validate_shape( return Err(Message { code: context.code, message, - locations: transform_locations(&key.locations, context, expression_offset), + locations: transform_locations(&key.locations, context, expression), }); } resolved = child; path = format!("{path}.{key}"); } - validate_shape(&resolved, context, expression_offset) + resolve_shape(&resolved, context, expression) } ShapeCase::Error(shape::Error { message, .. }) => Err(Message { code: context.code, message: message.clone(), - locations: transform_locations(&shape.locations, context, expression_offset), + locations: transform_locations(&shape.locations, context, expression), }), + ShapeCase::Array { prefix, tail } => { + let prefix = prefix + .iter() + .map(|shape| resolve_shape(shape, context, expression)) + .collect::, _>>()?; + let tail = resolve_shape(tail, context, expression)?; + Ok(Shape::array(prefix, tail, shape.locations.clone())) + } + ShapeCase::Object { fields, rest } => { + let mut resolved_fields = Shape::empty_map(); + for (key, value) in fields { + resolved_fields.insert(key.clone(), resolve_shape(value, context, expression)?); + } + let resolved_rest = resolve_shape(rest, context, expression)?; + Ok(Shape::object( + resolved_fields, + resolved_rest, + shape.locations.clone(), + )) + } ShapeCase::None | ShapeCase::Bool(_) | ShapeCase::String(_) | ShapeCase::Int(_) | ShapeCase::Float | ShapeCase::Null - | ShapeCase::Unknown => Ok(()), + | ShapeCase::Unknown => Ok(shape.clone()), } } fn transform_locations<'a>( locations: impl IntoIterator, context: &Context, - expression_offset: usize, + expression: &Expression, ) -> Vec> { - locations + let mut locations: Vec<_> = locations .into_iter() .filter_map(|location| match &location.source_id { SourceId::GraphQL(file_id) => context @@ -256,12 +296,40 @@ fn transform_locations<'a>( SourceId::Other(_) => { // Right now, this always refers to the JSONSelection location context.source.line_col_for_subslice( - location.span.start + expression_offset..location.span.end + expression_offset, + location.span.start + expression.location.start + ..location.span.end + expression.location.start, context.schema, ) } }) - .collect() + .collect(); + if locations.is_empty() { + // Highlight the whole expression + locations.extend(context.source.line_col_for_subslice( + expression.location.start..expression.location.end, + context.schema, + )) + } + locations +} + +/// A simplified shape name for error messages +fn shape_name(shape: &Shape) -> &'static str { + match shape.case() { + ShapeCase::Bool(_) => "boolean", + ShapeCase::String(_) => "string", + ShapeCase::Int(_) => "number", + ShapeCase::Float => "number", + ShapeCase::Null => "null", + ShapeCase::Array { .. } => "array", + ShapeCase::Object { .. } => "object", + ShapeCase::One(_) => "union", + ShapeCase::All(_) => "intersection", + ShapeCase::Name(_, _) => "unknown", + ShapeCase::Unknown => "unknown", + ShapeCase::None => "none", + ShapeCase::Error(_) => "error", + } } #[cfg(test)] @@ -289,7 +357,7 @@ mod tests { import: ["@connect", "@source"] ) @source(name: "v2", http: { baseURL: "http://127.0.0.1" }) - + type Query { aField( int: Int @@ -301,21 +369,21 @@ mod tests { ): AnObject @connect(source: "v2", http: {GET: """{EXPRESSION}"""}) something: String } - + scalar CustomScalar - + input InputObject { bool: Boolean } - + type AnObject { bool: Boolean } - + input MultiLevelInput { inner: MultiLevel } - + type MultiLevel { nested: String } @@ -325,7 +393,7 @@ mod tests { SCHEMA.replace("EXPRESSION", selection) } - fn validate_with_context(selection: &str) -> Result<(), Message> { + fn validate_with_context(selection: &str, expected: Shape) -> Result<(), Message> { let schema_str = schema_for(selection); let schema = Schema::parse(&schema_str, "schema").unwrap(); let object = schema.get_object("Query").unwrap(); @@ -351,7 +419,7 @@ mod tests { }; let context = Context::for_connect_request(&schema_info, coordinate, &expr_string, Code::InvalidUrl); - validate(&expression(selection), &context) + validate(&expression(selection), &context, &expected) } /// Given a full expression replaced in `{EXPRESSION}` above, find the line/col of a substring. @@ -407,7 +475,7 @@ mod tests { #[case::last("$args.array->last.bool")] #[case::multi_level_input("$args.multiLevel.inner.nested")] fn valid_expressions(#[case] selection: &str) { - validate_with_context(selection).unwrap(); + validate_with_context(selection, scalars()).unwrap(); } #[rstest] @@ -433,7 +501,7 @@ mod tests { #[case::this_on_query("$this.something")] #[case::bare_field_no_var("something")] fn invalid_expressions(#[case] selection: &str) { - let err = validate_with_context(selection); + let err = validate_with_context(selection, scalars()); assert!(err.is_err()); assert!( !err.err().unwrap().locations.is_empty(), @@ -444,7 +512,8 @@ mod tests { #[test] fn bare_field_with_path() { let selection = "something.blah"; - let err = validate_with_context(selection).expect_err("missing property is unknown"); + let err = + validate_with_context(selection, scalars()).expect_err("missing property is unknown"); let expected_location = location_of_expression("something", selection); assert!( err.message.contains("`something.blah`"), @@ -467,7 +536,7 @@ mod tests { #[test] fn object_in_url() { let selection = "$args.object"; - let err = validate_with_context(selection).expect_err("objects are not allowed"); + let err = validate_with_context(selection, scalars()).expect_err("objects are not allowed"); let expected_location = location_of_expression("object", selection); assert!( err.locations.contains(&expected_location), @@ -480,7 +549,8 @@ mod tests { #[test] fn nested_unknown_property() { let selection = "$args.multiLevel.inner.unknown"; - let err = validate_with_context(selection).expect_err("missing property is unknown"); + let err = + validate_with_context(selection, scalars()).expect_err("missing property is unknown"); assert!( err.message.contains("`MultiLevel`"), "{} didn't reference type", @@ -498,4 +568,22 @@ mod tests { err.locations ); } + + #[test] + fn unknown_var_in_scalar() { + let selection = r#"$({"something": $blahblahblah})"#; + let err = validate_with_context(selection, Shape::unknown([])) + .expect_err("unknown variable is unknown"); + assert!( + err.message.contains("`$blahblahblah`"), + "{} didn't reference variable", + err.message + ); + assert!( + err.locations + .contains(&location_of_expression("$blahblahblah", selection)), + "The relevant piece of the expression wasn't included in {:?}", + err.locations + ); + } } diff --git a/apollo-federation/src/sources/connect/validation/extended_type.rs b/apollo-federation/src/sources/connect/validation/extended_type.rs index ae8e6d9fb9..a14104d129 100644 --- a/apollo-federation/src/sources/connect/validation/extended_type.rs +++ b/apollo-federation/src/sources/connect/validation/extended_type.rs @@ -248,16 +248,14 @@ fn validate_field( .iter() .find(|(name, _)| name == &CONNECT_BODY_ARGUMENT_NAME) { - if let Err(err) = validate_body_selection( + errors.extend(validate_body_selection( connect_directive, connect_coordinate, object, field, schema, body, - ) { - errors.push(err); - } + )); } if let Some(source_name) = connect_directive diff --git a/apollo-federation/src/sources/connect/validation/http/headers.rs b/apollo-federation/src/sources/connect/validation/http/headers.rs index 8fc492df93..5406e891f4 100644 --- a/apollo-federation/src/sources/connect/validation/http/headers.rs +++ b/apollo-federation/src/sources/connect/validation/http/headers.rs @@ -10,6 +10,7 @@ use crate::sources::connect::spec::schema::HEADERS_ARGUMENT_NAME; use crate::sources::connect::string_template; use crate::sources::connect::validation::coordinates::HttpHeadersCoordinate; use crate::sources::connect::validation::expression; +use crate::sources::connect::validation::expression::scalars; use crate::sources::connect::validation::graphql::GraphQLString; use crate::sources::connect::validation::graphql::SchemaInfo; use crate::sources::connect::validation::Code; @@ -114,7 +115,7 @@ pub(crate) fn validate_arg<'a>( header_value .expressions() .filter_map(|expression| { - expression::validate(expression, &expression_context).err() + expression::validate(expression, &expression_context, &scalars()).err() }) .map(|mut err| { err.message = format!("In {coordinate}: {}", err.message); diff --git a/apollo-federation/src/sources/connect/validation/http/url.rs b/apollo-federation/src/sources/connect/validation/http/url.rs index b956100c96..1490ef89f4 100644 --- a/apollo-federation/src/sources/connect/validation/http/url.rs +++ b/apollo-federation/src/sources/connect/validation/http/url.rs @@ -8,6 +8,7 @@ use url::Url; use crate::sources::connect::string_template; use crate::sources::connect::validation::coordinates::HttpMethodCoordinate; use crate::sources::connect::validation::expression; +use crate::sources::connect::validation::expression::scalars; use crate::sources::connect::validation::graphql::GraphQLString; use crate::sources::connect::validation::graphql::SchemaInfo; use crate::sources::connect::validation::Code; @@ -36,7 +37,7 @@ pub(crate) fn validate_template( for expression in template.expressions() { messages.extend( - expression::validate(expression, &expression_context) + expression::validate(expression, &expression_context, &scalars()) .err() .into_iter() .map(|mut err| { diff --git a/apollo-federation/src/sources/connect/validation/mod.rs b/apollo-federation/src/sources/connect/validation/mod.rs index 449973cdeb..c932c3940e 100644 --- a/apollo-federation/src/sources/connect/validation/mod.rs +++ b/apollo-federation/src/sources/connect/validation/mod.rs @@ -577,8 +577,10 @@ pub enum Code { EntityTypeInvalid, /// A @key is defined without a cooresponding entity connector. MissingEntityConnector, - /// A syntax error in `selection` - InvalidJsonSelection, + /// An error in `selection` + InvalidSelection, + /// A problem with `http.body` + InvalidBody, /// A cycle was detected within a `selection` CircularReference, /// A field was selected but is not defined on the type diff --git a/apollo-federation/src/sources/connect/validation/selection.rs b/apollo-federation/src/sources/connect/validation/selection.rs index b8db1e2b73..32d83db652 100644 --- a/apollo-federation/src/sources/connect/validation/selection.rs +++ b/apollo-federation/src/sources/connect/validation/selection.rs @@ -11,9 +11,11 @@ use apollo_compiler::schema::ExtendedType; use apollo_compiler::schema::ObjectType; use apollo_compiler::Node; use itertools::Itertools; +use shape::Shape; use super::coordinates::ConnectDirectiveCoordinate; use super::coordinates::SelectionCoordinate; +use super::expression; use super::Code; use super::Message; use super::Name; @@ -24,7 +26,9 @@ use crate::sources::connect::json_selection::ExternalVarPaths; use crate::sources::connect::json_selection::NamedSelection; use crate::sources::connect::json_selection::Ranged; use crate::sources::connect::spec::schema::CONNECT_SELECTION_ARGUMENT_NAME; +use crate::sources::connect::string_template::Expression; use crate::sources::connect::validation::coordinates::connect_directive_http_body_coordinate; +use crate::sources::connect::validation::expression::Context; use crate::sources::connect::validation::graphql::GraphQLString; use crate::sources::connect::validation::graphql::SchemaInfo; use crate::sources::connect::validation::variable::VariableResolver; @@ -91,65 +95,67 @@ pub(super) fn validate_body_selection( field: &Component, schema: &SchemaInfo, selection_node: &Node, -) -> Result<(), Message> { +) -> Vec { let coordinate = connect_directive_http_body_coordinate(&connect_directive.name, parent_type, &field.name); - let selection_str = - GraphQLString::new(selection_node, &schema.sources).map_err(|_| Message { - code: Code::GraphQLError, - message: format!("{coordinate} must be a string."), - locations: selection_node - .line_column_range(&schema.sources) - .into_iter() - .collect(), - })?; - - let selection = JSONSelection::parse(selection_str.as_str()).map_err(|err| Message { - code: Code::InvalidJsonSelection, - message: format!("{coordinate} is not a valid JSONSelection: {err}"), - locations: selection_node - .line_column_range(&schema.sources) - .into_iter() - .collect(), - })?; - + // Ensure that the body selection is a valid JSON selection string + let selection_str = match GraphQLString::new(selection_node, &schema.sources) { + Ok(selection_str) => selection_str, + Err(_) => { + return vec![Message { + code: Code::GraphQLError, + message: format!("{coordinate} must be a string."), + locations: selection_node + .line_column_range(&schema.sources) + .into_iter() + .collect(), + }] + } + }; + let selection = match JSONSelection::parse(selection_str.as_str()) { + Ok(selection) => selection, + Err(err) => { + return vec![Message { + code: Code::InvalidBody, + message: format!("{coordinate} is not valid: {err}"), + locations: selection_node + .line_column_range(&schema.sources) + .into_iter() + .collect(), + }] + } + }; if selection.is_empty() { - return Err(Message { - code: Code::InvalidJsonSelection, + return vec![Message { + code: Code::InvalidBody, message: format!("{coordinate} is empty"), locations: selection_node .line_column_range(&schema.sources) .into_iter() .collect(), - }); + }]; } - let var_paths = selection.external_var_paths(); - if var_paths.is_empty() { - return Err(Message { - code: Code::InvalidJsonSelection, - message: format!("{coordinate} must contain at least one variable reference"), - locations: selection_node - .line_column_range(&schema.sources) - .into_iter() - .collect(), - }); + + // Validate the selection shape + if let Err(mut message) = expression::validate( + &Expression { + expression: selection, + location: 0..selection_str.as_str().len(), + }, + &Context::for_connect_request( + schema, + connect_coordinate, + &selection_str, + Code::InvalidBody, + ), + &Shape::unknown([]), + ) { + message.message = format!("In {coordinate}: {message}", message = message.message); + return vec![message]; } - let context = VariableContext::new( - connect_coordinate.field_coordinate.object, - connect_coordinate.field_coordinate.field, - Phase::Request, - Target::Body, - ); - validate_selection_variables( - &VariableResolver::new(context.clone(), schema), - coordinate, - selection_str, - schema, - context, - var_paths, - ) + Vec::new() } /// Validate variable references in a JSON Selection @@ -220,8 +226,8 @@ fn get_json_selection<'a>( })?; let selection = JSONSelection::parse(selection_str.as_str()).map_err(|err| Message { - code: Code::InvalidJsonSelection, - message: format!("{coordinate} is not a valid JSONSelection: {err}",), + code: Code::InvalidSelection, + message: format!("{coordinate} is not valid: {err}",), locations: selection_str .line_col_for_subslice(err.offset..err.offset + 1, schema) .into_iter() @@ -230,7 +236,7 @@ fn get_json_selection<'a>( if selection.is_empty() { return Err(Message { - code: Code::InvalidJsonSelection, + code: Code::InvalidSelection, message: format!("{coordinate} is empty",), locations: selection_arg .value @@ -286,7 +292,7 @@ impl SelectionValidator<'_, '_> { // TODO: make a helper function for easier range collection locations: self.get_range_location(field.inner_range()) // Skip over fields which duplicate the location of the selection - .chain(if depth > 1 {ancestor_field.and_then(|def| def.line_column_range(&self.schema.sources))} else {None}) + .chain(if depth > 1 { ancestor_field.and_then(|def| def.line_column_range(&self.schema.sources)) } else { None }) .chain(field.definition.line_column_range(&self.schema.sources)) .collect(), }); @@ -465,7 +471,7 @@ impl<'schema> FieldVisitor> for SelectionValidator<'schema, '_> { match (field_type, is_group) { (ExtendedType::Object(object), true) => { self.check_for_circular_reference(field, object) - }, + } (_, true) => { Err(Message { code: Code::GroupSelectionIsNotObject, @@ -475,7 +481,7 @@ impl<'schema> FieldVisitor> for SelectionValidator<'schema, '_> { ), locations: self.get_range_location(field.inner_range()).chain(field.definition.line_column_range(&self.schema.sources)).collect(), }) - }, + } (ExtendedType::Object(_), false) => { Err(Message { code: Code::GroupSelectionRequiredForObject, @@ -485,7 +491,7 @@ impl<'schema> FieldVisitor> for SelectionValidator<'schema, '_> { ), locations: self.get_range_location(field.inner_range()).chain(field.definition.line_column_range(&self.schema.sources)).collect(), }) - }, + } (_, false) => Ok(()), } } diff --git a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@body_selection.graphql.snap b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@body_selection.graphql.snap index e4a7787605..70a534e0fa 100644 --- a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@body_selection.graphql.snap +++ b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@body_selection.graphql.snap @@ -5,38 +5,31 @@ input_file: apollo-federation/src/sources/connect/validation/test_data/body_sele --- [ Message { - code: InvalidJsonSelection, - message: "`@connect(http: {body:})` on `Query.dollar` must contain at least one variable reference", + code: InvalidBody, + message: "In `@connect(http: {body:})` on `Query.dollar`: must start with one of $args, $config, $context", locations: [ - 12:19..12:22, + 12:20..12:21, ], }, Message { - code: InvalidJsonSelection, - message: "`@connect(http: {body:})` on `Query.dollarField` must contain at least one variable reference", + code: InvalidBody, + message: "In `@connect(http: {body:})` on `Query.dollarField`: `foo` must start with one of $args, $config, $context", locations: [ - 20:19..20:26, + 20:22..20:25, ], }, Message { - code: InvalidJsonSelection, - message: "`@connect(http: {body:})` on `Query.objectLiteral` must contain at least one variable reference", + code: InvalidBody, + message: "In `@connect(http: {body:})` on `Query.invalidArrowMethod`: Method ->no_such_method not found", locations: [ - 28:19..28:41, + 44:49..44:63, ], }, Message { - code: InvalidJsonSelection, - message: "`@connect(http: {body:})` on `Query.stringLiteral` must contain at least one variable reference", + code: InvalidBody, + message: "In `@connect(http: {body:})` on `Query.invalidVariable`: unknown variable `$nosuchvariable`, must be one of $args, $config, $context", locations: [ - 44:19..44:29, - ], - }, - Message { - code: InvalidJsonSelection, - message: "`@connect(http: {body:})` on `Query.numericLiteral` must contain at least one variable reference", - locations: [ - 60:19..60:25, + 52:32..52:47, ], }, ] diff --git a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@empty_selection.graphql.snap b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@empty_selection.graphql.snap index d46635f3e3..ead02e5bcf 100644 --- a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@empty_selection.graphql.snap +++ b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@empty_selection.graphql.snap @@ -1,18 +1,18 @@ --- source: apollo-federation/src/sources/connect/validation/mod.rs -expression: "format!(\"{:#?}\", errors)" +expression: "format!(\"{:#?}\", result.errors)" input_file: apollo-federation/src/sources/connect/validation/test_data/empty_selection.graphql --- [ Message { - code: InvalidJsonSelection, + code: InvalidSelection, message: "`@connect(selection:)` on `Query.resources` is empty", locations: [ 13:18..13:29, ], }, Message { - code: InvalidJsonSelection, + code: InvalidBody, message: "`@connect(http: {body:})` on `Query.resources` is empty", locations: [ 12:41..12:45, diff --git a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid_namespace_in_body_selection.graphql.snap b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid_namespace_in_body_selection.graphql.snap index cf30789ed7..33643a5024 100644 --- a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid_namespace_in_body_selection.graphql.snap +++ b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid_namespace_in_body_selection.graphql.snap @@ -1,12 +1,12 @@ --- source: apollo-federation/src/sources/connect/validation/mod.rs -expression: "format!(\"{:#?}\", errors)" +expression: "format!(\"{:#?}\", result.errors)" input_file: apollo-federation/src/sources/connect/validation/test_data/invalid_namespace_in_body_selection.graphql --- [ Message { - code: InvalidJsonSelection, - message: "In `@connect(http: {body:})` on `Mutation.createUser`: variable `$status` is not valid at this location, must be one of $args, $config, $context, $this", + code: InvalidBody, + message: "In `@connect(http: {body:})` on `Mutation.createUser`: $status is not valid here, must be one of $args, $config, $context", locations: [ 21:17..21:24, ], diff --git a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid_nested_paths_in_json_selection.graphql.snap b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid_nested_paths_in_json_selection.graphql.snap index f032451623..a61b8afc57 100644 --- a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid_nested_paths_in_json_selection.graphql.snap +++ b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid_nested_paths_in_json_selection.graphql.snap @@ -1,33 +1,33 @@ --- source: apollo-federation/src/sources/connect/validation/mod.rs -expression: "format!(\"{:#?}\", errors)" +expression: "format!(\"{:#?}\", result.errors)" input_file: apollo-federation/src/sources/connect/validation/test_data/invalid_nested_paths_in_json_selection.graphql --- [ Message { - code: UndefinedField, - message: "In `@connect(http: {body:})` on `Query.scalar`: `String` does not have a field named `blah`.", + code: InvalidBody, + message: "In `@connect(http: {body:})` on `Query.scalar`: `$args.scalar` doesn't have a field named `blah`", locations: [ 12:34..12:38, ], }, Message { - code: UndefinedField, - message: "In `@connect(http: {body:})` on `Query.object`: `InputObject` does not have a field named `fieldThatDoesntExist`.", + code: InvalidBody, + message: "In `@connect(http: {body:})` on `Query.object`: `InputObject` doesn't have a field named `fieldThatDoesntExist`", locations: [ 20:33..20:53, ], }, Message { - code: UndefinedField, - message: "In `@connect(http: {body:})` on `Query.enum`: `Enum` does not have a field named `cantHaveFields`.", + code: InvalidBody, + message: "In `@connect(http: {body:})` on `Query.enum`: `Enum` doesn't have a field named `cantHaveFields`", locations: [ 28:30..28:44, ], }, Message { - code: UndefinedField, - message: "In `@connect(http: {body:})` on `Object.newField`: `Object` does not have a field named `fieldThatDoesntExist`", + code: InvalidBody, + message: "In `@connect(http: {body:})` on `Object.newField`: `$this` doesn't have a field named `fieldThatDoesntExist`", locations: [ 40:27..40:47, ], diff --git a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid_selection_syntax.graphql.snap b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid_selection_syntax.graphql.snap index 7eff4959c2..dd588bf776 100644 --- a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid_selection_syntax.graphql.snap +++ b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@invalid_selection_syntax.graphql.snap @@ -1,12 +1,12 @@ --- source: apollo-federation/src/sources/connect/validation/mod.rs -expression: "format!(\"{:#?}\", errors)" +expression: "format!(\"{:#?}\", result.errors)" input_file: apollo-federation/src/sources/connect/validation/test_data/invalid_selection_syntax.graphql --- [ Message { - code: InvalidJsonSelection, - message: "`@connect(selection:)` on `Query.something` is not a valid JSONSelection: nom::error::ErrorKind::Eof: &how", + code: InvalidSelection, + message: "`@connect(selection:)` on `Query.something` is not valid: nom::error::ErrorKind::Eof: &how", locations: [ 8:87..8:88, ], diff --git a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_selection_with_escapes.graphql.snap b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_selection_with_escapes.graphql.snap index f303f11f84..c6ea589194 100644 --- a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_selection_with_escapes.graphql.snap +++ b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@valid_selection_with_escapes.graphql.snap @@ -1,19 +1,19 @@ --- source: apollo-federation/src/sources/connect/validation/mod.rs -expression: "format!(\"{:#?}\", errors)" +expression: "format!(\"{:#?}\", result.errors)" input_file: apollo-federation/src/sources/connect/validation/test_data/valid_selection_with_escapes.graphql --- [ Message { - code: InvalidJsonSelection, - message: "`@connect(selection:)` on `Query.block` is not a valid JSONSelection: Path selection . must be followed by key (identifier or quoted string literal): .", + code: InvalidSelection, + message: "`@connect(selection:)` on `Query.block` is not valid: Path selection . must be followed by key (identifier or quoted string literal): .", locations: [ 16:30..16:31, ], }, Message { - code: InvalidJsonSelection, - message: "`@connect(selection:)` on `Query.standard` is not a valid JSONSelection: Path selection . must be followed by key (identifier or quoted string literal): .", + code: InvalidSelection, + message: "`@connect(selection:)` on `Query.standard` is not valid: Path selection . must be followed by key (identifier or quoted string literal): .", locations: [ 22:95..22:96, ], diff --git a/apollo-federation/src/sources/connect/validation/test_data/body_selection.graphql b/apollo-federation/src/sources/connect/validation/test_data/body_selection.graphql index a79ba94eb0..617cbf3455 100644 --- a/apollo-federation/src/sources/connect/validation/test_data/body_selection.graphql +++ b/apollo-federation/src/sources/connect/validation/test_data/body_selection.graphql @@ -25,7 +25,7 @@ type Query { @connect( http: { POST: "http://127.0.0.1", - body: "$({ userid: 'foo' })" # Could be valid, but currently not allowed + body: "$({ userid: 'foo' })" # VALID } selection: "$" ) @@ -37,27 +37,19 @@ type Query { } selection: "$" ) - stringLiteral: String + invalidArrowMethod(userid: ID!): String @connect( http: { POST: "http://127.0.0.1", - body: "$('foo')" # INVALID - output is not valid JSON + body: "$({ userid: $args.userid })->no_such_method" # INVALID - no such method } selection: "$" ) - stringLiteralWithVariable(userid: ID!): String + invalidVariable(userid: ID!): String @connect( http: { POST: "http://127.0.0.1", - body: "$($args.userid)" # INVALID, but currently allowed - output is not valid JSON - } - selection: "$" - ) - numericLiteral(userid: ID!): String - @connect( - http: { - POST: "http://127.0.0.1", - body: "$(1)" # INVALID - output is not valid JSON + body: "$({ userid: $nosuchvariable })" # INVALID - no such variable } selection: "$" ) diff --git a/apollo-federation/src/sources/connect/variable.rs b/apollo-federation/src/sources/connect/variable.rs index 930c047ab8..c40e23caf5 100644 --- a/apollo-federation/src/sources/connect/variable.rs +++ b/apollo-federation/src/sources/connect/variable.rs @@ -53,14 +53,6 @@ impl<'schema> VariableContext<'schema> { Namespace::This, ] } - Phase::Request => { - vec![ - Namespace::Config, - Namespace::Context, - Namespace::This, - Namespace::Args, - ] - } } .into_iter() } @@ -76,9 +68,7 @@ impl<'schema> VariableContext<'schema> { /// Get the error code for this context pub(crate) fn error_code(&self) -> Code { match self.target { - Target::Url => Code::InvalidUrl, - Target::Header => Code::InvalidHeader, - Target::Body => Code::InvalidJsonSelection, + Target::Body => Code::InvalidSelection, } } } @@ -86,9 +76,6 @@ impl<'schema> VariableContext<'schema> { /// The phase an expression is associated with #[derive(Clone, Copy, PartialEq)] pub(crate) enum Phase { - /// The request phase - Request, - /// The response phase Response, } @@ -97,12 +84,6 @@ pub(crate) enum Phase { #[allow(unused)] #[derive(Clone, Copy, PartialEq)] pub(crate) enum Target { - /// The expression is used in an HTTP header - Header, - - /// The expression is used in a URL - Url, - /// The expression is used in the body of a request or response Body, }