From 619b794360d81489e75f8af6c37db99f922406ad Mon Sep 17 00:00:00 2001 From: Dylan Anthony Date: Fri, 11 Oct 2024 16:22:51 -0600 Subject: [PATCH] Set up a place for body validations & add the first new check --- .../src/sources/connect/json_selection/mod.rs | 1 + .../sources/connect/json_selection/parser.rs | 6 +- .../sources/connect/validation/coordinates.rs | 35 +++- .../connect/validation/extended_type.rs | 14 +- .../src/sources/connect/validation/http.rs | 3 + .../sources/connect/validation/http/body.rs | 156 ++++++++++++++++++ .../src/sources/connect/validation/mod.rs | 19 ++- .../sources/connect/validation/selection.rs | 128 +++++--------- ...ference_arg_that_doesnt_exist.graphql.snap | 14 ++ ...tion_tests@body__valid_bodies.graphql.snap | 6 + .../reference_arg_that_doesnt_exist.graphql | 13 ++ .../test_data/body/valid_bodies.graphql | 34 ++++ 12 files changed, 318 insertions(+), 111 deletions(-) create mode 100644 apollo-federation/src/sources/connect/validation/http/body.rs create mode 100644 apollo-federation/src/sources/connect/validation/snapshots/validation_tests@body__reference_arg_that_doesnt_exist.graphql.snap create mode 100644 apollo-federation/src/sources/connect/validation/snapshots/validation_tests@body__valid_bodies.graphql.snap create mode 100644 apollo-federation/src/sources/connect/validation/test_data/body/reference_arg_that_doesnt_exist.graphql create mode 100644 apollo-federation/src/sources/connect/validation/test_data/body/valid_bodies.graphql diff --git a/apollo-federation/src/sources/connect/json_selection/mod.rs b/apollo-federation/src/sources/connect/json_selection/mod.rs index 6f2abb8533..b05cf8442b 100644 --- a/apollo-federation/src/sources/connect/json_selection/mod.rs +++ b/apollo-federation/src/sources/connect/json_selection/mod.rs @@ -15,6 +15,7 @@ pub use apply_to::*; // remove the `#[cfg(test)]`. pub(crate) use known_var::*; pub(crate) use location::Ranged; +pub(crate) use location::WithRange; pub use parser::*; #[cfg(test)] pub(crate) use pretty::*; diff --git a/apollo-federation/src/sources/connect/json_selection/parser.rs b/apollo-federation/src/sources/connect/json_selection/parser.rs index b0264995c4..a73ddd7542 100644 --- a/apollo-federation/src/sources/connect/json_selection/parser.rs +++ b/apollo-federation/src/sources/connect/json_selection/parser.rs @@ -297,7 +297,7 @@ impl ExternalVarPaths for NamedSelection { #[derive(Debug, PartialEq, Eq, Clone)] pub struct PathSelection { - pub(super) path: WithRange, + pub(crate) path: WithRange, } // Like NamedSelection, PathSelection is an AST structure that takes its range @@ -378,7 +378,7 @@ impl From for PathSelection { } #[derive(Debug, PartialEq, Eq, Clone)] -pub(super) enum PathList { +pub(crate) enum PathList { // A VarPath must start with a variable (either $identifier, $, or @), // followed by any number of PathStep items (the WithRange). // Because we represent the @ quasi-variable using PathList::Var, this @@ -1079,7 +1079,7 @@ pub(crate) fn parse_string_literal(input: Span) -> IResult>, pub(super) range: OffsetRange, } diff --git a/apollo-federation/src/sources/connect/validation/coordinates.rs b/apollo-federation/src/sources/connect/validation/coordinates.rs index 598753ad16..b3d83e572d 100644 --- a/apollo-federation/src/sources/connect/validation/coordinates.rs +++ b/apollo-federation/src/sources/connect/validation/coordinates.rs @@ -157,12 +157,35 @@ impl Display for BaseUrlCoordinate<'_> { } } -pub(super) fn connect_directive_http_body_coordinate( - connect_directive_name: &Name, - object: &Node, - field: &Name, -) -> String { - format!("`@{connect_directive_name}({HTTP_ARGUMENT_NAME}: {{{CONNECT_BODY_ARGUMENT_NAME}:}})` on `{object_name}.{field}`", object_name = object.name) +/// The coordinate of a `@connect(http:body:)`. +#[derive(Clone, Copy)] +pub(super) struct BodyCoordinate<'a> { + pub(super) connect_directive_name: &'a Name, + pub(super) field_coordinate: FieldCoordinate<'a>, +} + +impl Display for BodyCoordinate<'_> { + fn fmt(&self, f: &mut Formatter) -> Result<(), fmt::Error> { + let BodyCoordinate { + connect_directive_name, + field_coordinate, + } = self; + write!(f, "`@{connect_directive_name}({HTTP_ARGUMENT_NAME}: {{{CONNECT_BODY_ARGUMENT_NAME}:}})` on {field_coordinate}") + } +} + +impl<'a> From> for BodyCoordinate<'a> { + fn from(connect_directive_coordinate: ConnectDirectiveCoordinate<'a>) -> Self { + let ConnectDirectiveCoordinate { + directive: _, + connect_directive_name, + field_coordinate, + } = connect_directive_coordinate; + Self { + connect_directive_name, + field_coordinate, + } + } } pub(super) fn source_http_argument_coordinate(source_directive_name: &DirectiveName) -> String { diff --git a/apollo-federation/src/sources/connect/validation/extended_type.rs b/apollo-federation/src/sources/connect/validation/extended_type.rs index c8ef5d1e73..b09acf3df7 100644 --- a/apollo-federation/src/sources/connect/validation/extended_type.rs +++ b/apollo-federation/src/sources/connect/validation/extended_type.rs @@ -17,14 +17,13 @@ use super::coordinates::HttpHeadersCoordinate; use super::entity::validate_entity_arg; use super::http::headers; use super::http::method; +use super::http::validate_body; use super::resolvable_key_fields; -use super::selection::validate_body_selection; use super::selection::validate_selection; use super::source_name::validate_source_name_arg; use super::source_name::SourceName; use super::Code; use super::Message; -use crate::sources::connect::spec::schema::CONNECT_BODY_ARGUMENT_NAME; use crate::sources::connect::spec::schema::CONNECT_SOURCE_ARGUMENT_NAME; use crate::sources::connect::spec::schema::HTTP_ARGUMENT_NAME; use crate::sources::connect::validation::graphql::SchemaInfo; @@ -250,15 +249,8 @@ fn validate_field( } }; - if let Some((_, body)) = http_arg - .iter() - .find(|(name, _)| name == &CONNECT_BODY_ARGUMENT_NAME) - { - if let Err(err) = - validate_body_selection(connect_directive, object, field, schema, body) - { - errors.push(err); - } + if let Err(err) = validate_body(http_arg, connect_coordinate, schema) { + errors.push(err); } if let Some(source_name) = connect_directive diff --git a/apollo-federation/src/sources/connect/validation/http.rs b/apollo-federation/src/sources/connect/validation/http.rs index 28d0ff8d6a..f1e021db21 100644 --- a/apollo-federation/src/sources/connect/validation/http.rs +++ b/apollo-federation/src/sources/connect/validation/http.rs @@ -1,3 +1,6 @@ +mod body; pub(super) mod headers; pub(super) mod method; pub(super) mod url; + +pub(super) use body::validate_body; diff --git a/apollo-federation/src/sources/connect/validation/http/body.rs b/apollo-federation/src/sources/connect/validation/http/body.rs new file mode 100644 index 0000000000..f8d0a0d8ca --- /dev/null +++ b/apollo-federation/src/sources/connect/validation/http/body.rs @@ -0,0 +1,156 @@ +use std::ops::Range; + +use apollo_compiler::ast::Value; +use apollo_compiler::parser::LineColumn; +use apollo_compiler::Name; +use apollo_compiler::Node; + +use crate::sources::connect::json_selection::KnownVariable; +use crate::sources::connect::json_selection::PathList; +use crate::sources::connect::json_selection::Ranged; +use crate::sources::connect::json_selection::WithRange; +use crate::sources::connect::spec::schema::CONNECT_BODY_ARGUMENT_NAME; +use crate::sources::connect::validation::coordinates::BodyCoordinate; +use crate::sources::connect::validation::coordinates::ConnectDirectiveCoordinate; +use crate::sources::connect::validation::graphql::GraphQLString; +use crate::sources::connect::validation::graphql::SchemaInfo; +use crate::sources::connect::validation::selection::get_json_selection; +use crate::sources::connect::validation::Code; +use crate::sources::connect::validation::Message; +use crate::sources::connect::JSONSelection; +use crate::sources::connect::PathSelection; + +pub(crate) fn validate_body( + http_arg: &[(Name, Node)], + connect_coordinate: ConnectDirectiveCoordinate, + schema: &SchemaInfo, +) -> Result<(), Message> { + let Some(body) = http_arg + .iter() + .find_map(|(name, value)| (name == &CONNECT_BODY_ARGUMENT_NAME).then_some(value)) + else { + return Ok(()); + }; + let coordinate = BodyCoordinate::from(connect_coordinate); + let (value, json_selection) = get_json_selection(coordinate, &schema.sources, body)?; + let arg = Arg { + value, + coordinate, + schema, + }; + + match json_selection { + JSONSelection::Named(_) => Ok(()), // TODO: validate_sub_selection(sub_selection, arg), + JSONSelection::Path(path_selection) => validate_path_selection(path_selection, arg), + } +} + +fn validate_path_selection(path_selection: PathSelection, arg: Arg) -> Result<(), Message> { + let PathSelection { path } = path_selection; + match path.as_ref() { + PathList::Var(variable, trailing) => { + match variable.as_ref() { + KnownVariable::This => { + // TODO: Verify that the name & shape matches arg.coordinate.field_coordinate.object + Ok(()) + } + KnownVariable::Args => validate_args_path(trailing, arg) + .map_err(|err| err.with_fallback_locations(arg.get_selection_location(&path))), + KnownVariable::Config => Ok(()), // We have no way of knowing is this is valid, yet + KnownVariable::Dollar => { + // TODO: Validate that this is followed only by a method + Ok(()) + } + KnownVariable::AtSign => { + // TODO: This is probably just not allowed? + Ok(()) + } + } + } + PathList::Key(_, _) => { + // TODO: Make sure this is aliased & built from data we have + Ok(()) + } + PathList::Expr(_, _) => { + Ok(()) // We don't know what shape to expect, so any is fine + } + PathList::Method(_, _, _) => { + // TODO: This is a parse error, but return an error here just in case + Ok(()) + } + PathList::Selection(_) => { + // TODO: This is a parse error, but return an error here just in case + Ok(()) + } + PathList::Empty => { + // TODO: This is a parse error, but return an error here just in case + Ok(()) + } + } +} + +// Validate a reference to `$args` +fn validate_args_path(path: &WithRange, arg: Arg) -> Result<(), Message> { + match path.as_ref() { + PathList::Var(var_type, _) => { + // This is probably caught by the parser, but we can't type-safely guarantee that yet + Err(Message { + code: Code::InvalidJsonSelection, + message: format!( + "Can't reference a path within another path. `$args.{var_type}` is invalid.", + var_type = var_type.as_str() + ), + locations: arg.get_selection_location(var_type).collect(), + }) + } + PathList::Key(_, _) => { + // TODO: Make sure that the path matches an argument, then validate the shape of that path + Ok(()) + } + PathList::Expr(_, _) => Err(Message { + code: Code::InvalidJsonSelection, + message: "Can't use a literal expression after `$args`.".to_string(), + locations: arg.get_selection_location(path).collect(), + }), + PathList::Method(_, _, _) => { + // TODO: Validate that the method can be called directly on `$args` + Ok(()) + } + PathList::Selection(_) => { + // TODO: Validate that the `SubSelection` is valid for `$args` + Ok(()) + } + PathList::Empty => { + // They're selecting the entirety of `$args`, this is okay as long as there are any args! + if arg.coordinate.field_coordinate.field.arguments.is_empty() { + Err(Message { + code: Code::InvalidJsonSelection, + message: "Can't use `$args` when there are no arguments.".to_string(), + locations: vec![], + }) + } else { + Ok(()) + } + } + } +} + +/// The `@connect(http.body:)` argument. +#[derive(Clone, Copy)] +struct Arg<'schema> { + value: GraphQLString<'schema>, + coordinate: BodyCoordinate<'schema>, + schema: &'schema SchemaInfo<'schema>, +} + +impl Arg<'_> { + fn get_selection_location( + &self, + selection: &impl Ranged, + ) -> impl Iterator> { + selection + .range() + .and_then(|range| self.value.line_col_for_subslice(range, self.schema)) + .into_iter() + } +} diff --git a/apollo-federation/src/sources/connect/validation/mod.rs b/apollo-federation/src/sources/connect/validation/mod.rs index 296f62a1fa..f53d3abc78 100644 --- a/apollo-federation/src/sources/connect/validation/mod.rs +++ b/apollo-federation/src/sources/connect/validation/mod.rs @@ -462,6 +462,23 @@ pub struct Message { pub locations: Vec>, } +impl Message { + /// If there was no location for this message yet, add some. Otherwise, do nothing. + pub(crate) fn with_fallback_locations( + self, + fallback_locations: impl Iterator>, + ) -> Self { + if self.locations.is_empty() { + Self { + locations: fallback_locations.collect(), + ..self + } + } else { + self + } + } +} + #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub enum Code { /// A problem with GraphQL syntax or semantics was found. These will usually be caught before @@ -574,7 +591,7 @@ mod test_validate_source { #[test] fn validation_tests() { insta::with_settings!({prepend_module_to_snapshot => false}, { - glob!("test_data", "*.graphql", |path| { + glob!("test_data", "**/*.graphql", |path| { let schema = read_to_string(path).unwrap(); let errors = validate(&schema, path.to_str().unwrap()); assert_snapshot!(format!("{:#?}", errors)); diff --git a/apollo-federation/src/sources/connect/validation/selection.rs b/apollo-federation/src/sources/connect/validation/selection.rs index 628945e04f..353ffd7b74 100644 --- a/apollo-federation/src/sources/connect/validation/selection.rs +++ b/apollo-federation/src/sources/connect/validation/selection.rs @@ -1,3 +1,4 @@ +use std::fmt::Display; use std::iter::once; use std::ops::Range; @@ -5,17 +6,13 @@ use apollo_compiler::ast::FieldDefinition; use apollo_compiler::collections::IndexSet; use apollo_compiler::parser::LineColumn; use apollo_compiler::parser::SourceMap; -use apollo_compiler::schema::Component; -use apollo_compiler::schema::Directive; use apollo_compiler::schema::ExtendedType; use apollo_compiler::schema::ObjectType; use apollo_compiler::Node; -use apollo_compiler::Schema; use itertools::Itertools; use super::coordinates::ConnectDirectiveCoordinate; use super::coordinates::SelectionCoordinate; -use super::require_value_is_str; use super::Code; use super::Message; use super::Name; @@ -25,19 +22,34 @@ use crate::sources::connect::expand::visitors::GroupVisitor; 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::validation::coordinates::connect_directive_http_body_coordinate; use crate::sources::connect::validation::graphql::GraphQLString; use crate::sources::connect::validation::graphql::SchemaInfo; use crate::sources::connect::JSONSelection; use crate::sources::connect::SubSelection; pub(super) fn validate_selection( - coordinate: ConnectDirectiveCoordinate, + connect_directive_coordinate: ConnectDirectiveCoordinate, schema: &SchemaInfo, seen_fields: &mut IndexSet<(Name, Name)>, ) -> Result<(), Message> { - let (selection_arg, json_selection) = get_json_selection(coordinate, &schema.sources)?; - let field = coordinate.field_coordinate.field; + let selection_coordinate = SelectionCoordinate::from(connect_directive_coordinate); + let argument = connect_directive_coordinate + .directive + .arguments + .iter() + .find(|arg| arg.name == CONNECT_SELECTION_ARGUMENT_NAME) + .ok_or_else(|| Message { + code: Code::GraphQLError, + message: format!("{selection_coordinate} is required."), + locations: connect_directive_coordinate + .directive + .line_column_range(&schema.sources) + .into_iter() + .collect(), + })?; + let (value, json_selection) = + get_json_selection(selection_coordinate, &schema.sources, &argument.value)?; + let field = connect_directive_coordinate.field_coordinate.field; let Some(return_type) = schema.get_object(field.ty.inner_named_type()) else { // TODO: Validate scalar return types @@ -55,110 +67,46 @@ pub(super) fn validate_selection( }; SelectionValidator { - root: PathPart::Root(coordinate.field_coordinate.object), + root: PathPart::Root(connect_directive_coordinate.field_coordinate.object), schema, path: Vec::new(), - selection_arg, + selection_arg: SelectionArg { + value, + coordinate: selection_coordinate, + }, seen_fields, } .walk(group) } -pub(super) fn validate_body_selection( - connect_directive: &Node, - parent_type: &Node, - field: &Component, - schema: &Schema, - selection_node: &Node, -) -> Result<(), Message> { - let coordinate = - connect_directive_http_body_coordinate(&connect_directive.name, parent_type, &field.name); - - let selection_str = require_value_is_str(selection_node, &coordinate, &schema.sources)?; - - let (_rest, selection) = JSONSelection::parse(selection_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(), - })?; - - if selection.is_empty() { - return Err(Message { - code: Code::InvalidJsonSelection, - message: format!("{coordinate} is empty"), - locations: selection_node - .line_column_range(&schema.sources) - .into_iter() - .collect(), - }); - } - - // TODO: validate JSONSelection - Ok(()) -} - -fn get_json_selection<'a>( - connect_directive: ConnectDirectiveCoordinate<'a>, +/// Parse JSONSelection from a value in the GraphQL AST. +pub(super) fn get_json_selection<'a>( + coordinate: impl Display, source_map: &'a SourceMap, -) -> Result<(SelectionArg<'a>, JSONSelection), Message> { - let coordinate = SelectionCoordinate::from(connect_directive); - let selection_arg = connect_directive - .directive - .arguments - .iter() - .find(|arg| arg.name == CONNECT_SELECTION_ARGUMENT_NAME) - .ok_or_else(|| Message { - code: Code::GraphQLError, - message: format!("{coordinate} is required."), - locations: connect_directive - .directive - .line_column_range(source_map) - .into_iter() - .collect(), - })?; - let selection_str = - GraphQLString::new(&selection_arg.value, source_map).map_err(|_| Message { - code: Code::GraphQLError, - message: format!("{coordinate} must be a string."), - locations: selection_arg - .line_column_range(source_map) - .into_iter() - .collect(), - })?; + value: &'a Node, +) -> Result<(GraphQLString<'a>, JSONSelection), Message> { + let selection_str = GraphQLString::new(value, source_map).map_err(|_| Message { + code: Code::GraphQLError, + message: format!("{coordinate} must be a string."), + locations: value.line_column_range(source_map).into_iter().collect(), + })?; let (_rest, 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_arg - .value - .line_column_range(source_map) - .into_iter() - .collect(), + locations: value.line_column_range(source_map).into_iter().collect(), })?; if selection.is_empty() { return Err(Message { code: Code::InvalidJsonSelection, message: format!("{coordinate} is empty",), - locations: selection_arg - .value - .line_column_range(source_map) - .into_iter() - .collect(), + locations: value.line_column_range(source_map).into_iter().collect(), }); } - Ok(( - SelectionArg { - value: selection_str, - coordinate, - }, - selection, - )) + Ok((selection_str, selection)) } struct SelectionArg<'schema> { diff --git a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@body__reference_arg_that_doesnt_exist.graphql.snap b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@body__reference_arg_that_doesnt_exist.graphql.snap new file mode 100644 index 0000000000..fce55833a8 --- /dev/null +++ b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@body__reference_arg_that_doesnt_exist.graphql.snap @@ -0,0 +1,14 @@ +--- +source: apollo-federation/src/sources/connect/validation/mod.rs +expression: "format!(\"{:#?}\", errors)" +input_file: apollo-federation/src/sources/connect/validation/test_data/body/reference_arg_that_doesnt_exist.graphql +--- +[ + Message { + code: InvalidJsonSelection, + message: "Can't use `$args` when there are no arguments.", + locations: [ + 10:35..10:40, + ], + }, +] diff --git a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@body__valid_bodies.graphql.snap b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@body__valid_bodies.graphql.snap new file mode 100644 index 0000000000..4d5a31da1c --- /dev/null +++ b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@body__valid_bodies.graphql.snap @@ -0,0 +1,6 @@ +--- +source: apollo-federation/src/sources/connect/validation/mod.rs +expression: "format!(\"{:#?}\", errors)" +input_file: apollo-federation/src/sources/connect/validation/test_data/body/valid_bodies.graphql +--- +[] diff --git a/apollo-federation/src/sources/connect/validation/test_data/body/reference_arg_that_doesnt_exist.graphql b/apollo-federation/src/sources/connect/validation/test_data/body/reference_arg_that_doesnt_exist.graphql new file mode 100644 index 0000000000..78045292f7 --- /dev/null +++ b/apollo-federation/src/sources/connect/validation/test_data/body/reference_arg_that_doesnt_exist.graphql @@ -0,0 +1,13 @@ +extend schema +@link(url: "https://specs.apollo.dev/federation/v2.10", import: ["@key"]) +@link(url: "https://specs.apollo.dev/connect/v0.1", import: ["@connect", "@source"]) +@source(name: "v2", http: { baseURL: "http://127.0.0.1" }) + +type Query { + args: String + @connect( + source: "v2" + http: { POST: "/", body: "$args" } + selection: "$" + ) +} diff --git a/apollo-federation/src/sources/connect/validation/test_data/body/valid_bodies.graphql b/apollo-federation/src/sources/connect/validation/test_data/body/valid_bodies.graphql new file mode 100644 index 0000000000..756fa73d06 --- /dev/null +++ b/apollo-federation/src/sources/connect/validation/test_data/body/valid_bodies.graphql @@ -0,0 +1,34 @@ +extend schema +@link(url: "https://specs.apollo.dev/federation/v2.10", import: ["@key"]) +@link(url: "https://specs.apollo.dev/connect/v0.1", import: ["@connect", "@source"]) +@source(name: "v2", http: { baseURL: "http://127.0.0.1" }) + +type Query { + justLiteral: String + @connect( + source: "v2" + http: { POST: "/", body: "$('literal')" } + selection: "$" + ) + args(anArg: AnInput): String + @connect( + source: "v2" + http: { POST: "/", body: "$args.anArg" } + selection: "$" + ) +} + +input AnInput { + something: String + somethingElse: Int +} + +type Entity @key(fields: "id") { + id: ID! + this: String + @connect( + source: "v2" + http: { POST: "/", body: "$this.id" } + selection: "$" + ) +}