diff --git a/apollo-federation/src/sources/connect/expand/visitor.rs b/apollo-federation/src/sources/connect/expand/visitor.rs deleted file mode 100644 index d8d8d30ca3..0000000000 --- a/apollo-federation/src/sources/connect/expand/visitor.rs +++ /dev/null @@ -1,240 +0,0 @@ -use apollo_compiler::ast::FieldDefinition; -use apollo_compiler::schema::Component; -use apollo_compiler::schema::DirectiveList; -use apollo_compiler::schema::ExtendedType; -use apollo_compiler::schema::ObjectType; -use apollo_compiler::Name; -use apollo_compiler::Node; -use indexmap::IndexMap; -use indexmap::IndexSet; - -use super::filter_directives; -use crate::error::FederationError; -use crate::schema::position::TypeDefinitionPosition; -use crate::schema::FederationSchema; -use crate::schema::ValidFederationSchema; -use crate::sources::connect::json_selection::JSONSelectionVisitor; - -/// A JSONSelection visitor for schema building. -/// -/// This implementation of the JSONSelection visitor walks a JSONSelection, -/// copying over all output types (and respective fields / sub types) as it goes -/// from a reference schema. -pub(super) struct ToSchemaVisitor<'a> { - /// List of directives to not copy over into the target schema. - directive_deny_list: &'a IndexSet, - - /// The original schema used for sourcing all types / fields / directives / etc. - original_schema: &'a ValidFederationSchema, - - /// The target schema for adding all types. - to_schema: &'a mut FederationSchema, - - /// A stack of parent types used for fetching subtypes - /// - /// Each entry corresponds to a nested subselect in the JSONSelection. - type_stack: Vec<(TypeDefinitionPosition, ExtendedType)>, -} - -impl<'a> ToSchemaVisitor<'a> { - pub fn new( - original_schema: &'a ValidFederationSchema, - to_schema: &'a mut FederationSchema, - initial_position: TypeDefinitionPosition, - initial_type: &ExtendedType, - directive_deny_list: &'a IndexSet, - ) -> ToSchemaVisitor<'a> { - // Get the type information for the initial position, making sure to strip - // off any unwanted directives. - let initial_type = match initial_type { - ExtendedType::Object(object) => ExtendedType::Object(Node::new(ObjectType { - description: object.description.clone(), - name: object.name.clone(), - implements_interfaces: object.implements_interfaces.clone(), - directives: filter_directives(directive_deny_list, &object.directives), - fields: IndexMap::with_hasher(Default::default()), // Will be filled in by subsequent visits - })), - ExtendedType::Scalar(_) => todo!(), - ExtendedType::Interface(_) => todo!(), - ExtendedType::Union(_) => todo!(), - ExtendedType::Enum(_) => todo!(), - ExtendedType::InputObject(_) => todo!(), - }; - - ToSchemaVisitor { - directive_deny_list, - original_schema, - to_schema, - type_stack: vec![(initial_position, initial_type)], - } - } -} - -impl JSONSelectionVisitor for ToSchemaVisitor<'_> { - type Error = FederationError; - - fn visit(&mut self, name: &str) -> Result<(), FederationError> { - let (definition, type_) = self.type_stack.last_mut().unwrap(); - - // Extract the node info - match (definition, type_) { - // Objects have fields - (TypeDefinitionPosition::Object(object), ExtendedType::Object(object_type)) => { - let field_name = Name::new(name)?; - let field = object - .field(field_name.clone()) - .get(self.original_schema.schema())?; - - // Add it to the currently processing object - object_type.make_mut().fields.insert( - field_name, - Component::new(FieldDefinition { - description: field.description.clone(), - name: field.name.clone(), - arguments: field.arguments.clone(), - ty: field.ty.clone(), - directives: filter_directives(self.directive_deny_list, &field.directives), - }), - ); - } - - (TypeDefinitionPosition::Scalar(_), ExtendedType::Scalar(_)) => todo!(), - (TypeDefinitionPosition::Interface(_), ExtendedType::Interface(_)) => todo!(), - (TypeDefinitionPosition::Union(_), ExtendedType::Union(_)) => todo!(), - (TypeDefinitionPosition::Enum(_), ExtendedType::Enum(_)) => todo!(), - (TypeDefinitionPosition::InputObject(_), ExtendedType::InputObject(_)) => todo!(), - - (_, _) => unreachable!("type definition did not match type"), - }; - - Ok(()) - } - - fn enter_group(&mut self, group: &str) -> Result<(), FederationError> { - let (definition, _) = self.type_stack.last().unwrap(); - - // Helper for making sure that the selected group is of a type that allows sub selections - let mut extract_sub_type = |type_: &TypeDefinitionPosition| { - match type_ { - TypeDefinitionPosition::Object(object) => { - // this is really an upsert — pre_insert/insert will error if the object already exists - if !self - .to_schema - .schema() - .types - .contains_key(&object.type_name) - { - object.pre_insert(self.to_schema)?; - } - let def = object.get(self.original_schema.schema())?; - - Ok(ExtendedType::Object(Node::new(ObjectType { - description: def.description.clone(), - name: def.name.clone(), - implements_interfaces: def.implements_interfaces.clone(), - directives: DirectiveList::new(), // TODO: Whitelist - fields: IndexMap::with_hasher(Default::default()), // Will be filled in by the `visit` method for each field - }))) - } - TypeDefinitionPosition::Interface(_) => todo!(), - TypeDefinitionPosition::Union(_) => todo!(), - TypeDefinitionPosition::InputObject(_) => todo!(), - - TypeDefinitionPosition::Enum(_) => { - Err(FederationError::internal("enums cannot have subselections")) - } - TypeDefinitionPosition::Scalar(_) => Err(FederationError::internal( - "scalars cannot have subselections", - )), - } - }; - - // Attempt to get the sub type by the field name specified - let (next, sub_type) = match definition { - // Objects have fields - TypeDefinitionPosition::Object(object) => { - let field = object - .field(Name::new(group)?) - .get(self.original_schema.schema())?; - let next = self - .original_schema - .get_type(field.ty.inner_named_type().clone())?; - - // Extract the extended type info for the output type - let output_type = extract_sub_type(&next)?; - (next, output_type) - } - - TypeDefinitionPosition::Interface(_) => todo!(), - TypeDefinitionPosition::Union(_) => todo!(), - TypeDefinitionPosition::InputObject(_) => todo!(), - - TypeDefinitionPosition::Enum(_) => { - return Err(FederationError::internal("cannot enter an enum")) - } - TypeDefinitionPosition::Scalar(_) => { - return Err(FederationError::internal("cannot enter a scalar")) - } - }; - - self.type_stack.push((next, sub_type)); - - Ok(()) - } - - fn exit_group(&mut self) -> Result<(), FederationError> { - let (definition, type_) = self.type_stack.pop().unwrap(); - - // Now actually consolidate the object into our schema - match (definition, type_) { - (TypeDefinitionPosition::Object(object), ExtendedType::Object(object_type)) => { - // this is really an upsert — pre_insert/insert will error if the object already exists - if !self - .to_schema - .schema() - .types - .contains_key(&object.type_name) - { - object.insert(self.to_schema, object_type) - } else { - Ok(()) - } - } - (TypeDefinitionPosition::Interface(_), ExtendedType::Interface(_)) => todo!(), - (TypeDefinitionPosition::Union(_), ExtendedType::Union(_)) => todo!(), - (TypeDefinitionPosition::InputObject(_), ExtendedType::InputObject(_)) => todo!(), - - (TypeDefinitionPosition::Enum(_), ExtendedType::Enum(_)) => { - unreachable!("enums should not have been entered") - } - (TypeDefinitionPosition::Scalar(_), ExtendedType::Scalar(_)) => { - unreachable!("scalars should not have been entered") - } - (_, _) => unreachable!("type definition did not match type"), - } - } - - fn finish(mut self) -> Result<(), FederationError> { - // Make sure to create the final object that we started with - let (definition, type_) = self.type_stack.pop().unwrap(); - - // Now actually consolidate the object into our schema - match (definition, type_) { - (TypeDefinitionPosition::Object(object), ExtendedType::Object(object_type)) => { - object.pre_insert(self.to_schema)?; - object.insert(self.to_schema, object_type) - } - (TypeDefinitionPosition::Interface(_), ExtendedType::Interface(_)) => todo!(), - (TypeDefinitionPosition::Union(_), ExtendedType::Union(_)) => todo!(), - (TypeDefinitionPosition::InputObject(_), ExtendedType::InputObject(_)) => todo!(), - - (TypeDefinitionPosition::Enum(_), ExtendedType::Enum(_)) => { - unreachable!("enums should not have been entered") - } - (TypeDefinitionPosition::Scalar(_), ExtendedType::Scalar(_)) => { - unreachable!("scalars should not have been entered") - } - (_, _) => unreachable!("type definition did not match type"), - } - } -} diff --git a/apollo-federation/src/sources/connect/expand/visitors/input.rs b/apollo-federation/src/sources/connect/expand/visitors/input.rs index 53f9a60d09..9dc7aecae4 100644 --- a/apollo-federation/src/sources/connect/expand/visitors/input.rs +++ b/apollo-federation/src/sources/connect/expand/visitors/input.rs @@ -58,17 +58,6 @@ impl FieldVisitor impl GroupVisitor for SchemaVisitor<'_, InputObjectTypeDefinitionPosition, InputObjectType> { - fn get_group_fields( - &self, - group: InputObjectTypeDefinitionPosition, - ) -> Result< - Vec, - >::Error, - > { - let def = group.get(self.original_schema.schema())?; - Ok(def.fields.keys().cloned().map(|f| group.field(f)).collect()) - } - fn try_get_group_for_field( &self, field: &InputObjectFieldDefinitionPosition, @@ -91,7 +80,7 @@ impl GroupVisitor( &mut self, - group: InputObjectTypeDefinitionPosition, + group: &InputObjectTypeDefinitionPosition, ) -> Result, FederationError> { try_pre_insert!(self.to_schema, group)?; @@ -104,7 +93,8 @@ impl GroupVisitor Result<(), FederationError> { diff --git a/apollo-federation/src/sources/connect/expand/visitors/mod.rs b/apollo-federation/src/sources/connect/expand/visitors/mod.rs index a91e23b0f1..69d7f8ccc2 100644 --- a/apollo-federation/src/sources/connect/expand/visitors/mod.rs +++ b/apollo-federation/src/sources/connect/expand/visitors/mod.rs @@ -87,18 +87,12 @@ pub(crate) trait FieldVisitor: Sized { /// Visitor for arbitrary tree-like structures where nodes can also have children /// /// This trait treats all nodes in the graph as Fields, checking if a Field is also -/// a group for handling children. +/// a group for handling children. Visiting order is depth-first. pub(crate) trait GroupVisitor where Self: FieldVisitor, Field: Clone, { - /// Get all of the fields for a specified group - fn get_group_fields( - &self, - group: Group, - ) -> Result, >::Error>; - /// Try to get a group from a field, returning None if the field is not a group fn try_get_group_for_field( &self, @@ -106,11 +100,11 @@ where ) -> Result, >::Error>; /// Enter a subselection group - /// Note: You can assume that the named selection corresponding to this + /// Note: You can assume that the field corresponding to this /// group will be visited first. fn enter_group( &mut self, - group: Group, + group: &Group, ) -> Result, >::Error>; /// Exit a subselection group @@ -118,13 +112,13 @@ where /// group will be visited and entered first. fn exit_group(&mut self) -> Result<(), >::Error>; - /// Walk through a [`JSONSelection`], visiting each output key. If at any point, one of the + /// Walk through the `Group`, visiting each output key. If at any point, one of the /// visitor methods returns an error, then the walk will be stopped and the error will be /// returned. fn walk(mut self, entry: Group) -> Result<(), >::Error> { // Start visiting each of the fields let mut to_visit = - VecDeque::from_iter(self.enter_group(entry)?.into_iter().map(|n| (0i32, n))); + VecDeque::from_iter(self.enter_group(&entry)?.into_iter().map(|n| (0i32, n))); let mut current_depth = 0; while let Some((depth, next)) = to_visit.pop_front() { for _ in depth..current_depth { @@ -141,7 +135,7 @@ where if let Some(group) = self.try_get_group_for_field(&next)? { current_depth += 1; - let fields = self.enter_group(group)?; + let fields = self.enter_group(&group)?; fields .into_iter() .rev() @@ -258,10 +252,12 @@ mod tests { Ok(field.next_subselection().cloned()) } - fn get_group_fields( - &self, - group: SubSelection, + fn enter_group( + &mut self, + group: &SubSelection, ) -> Result, FederationError> { + let next_depth = self.last_depth().map(|d| d + 1).unwrap_or(0); + self.depth_stack.push(next_depth); Ok(group .selections_iter() .sorted_by_key(|s| s.name()) @@ -269,15 +265,6 @@ mod tests { .collect()) } - fn enter_group( - &mut self, - group: SubSelection, - ) -> Result, FederationError> { - let next_depth = self.last_depth().map(|d| d + 1).unwrap_or(0); - self.depth_stack.push(next_depth); - self.get_group_fields(group) - } - fn exit_group(&mut self) -> Result<(), FederationError> { self.depth_stack.pop().unwrap(); Ok(()) diff --git a/apollo-federation/src/sources/connect/expand/visitors/selection.rs b/apollo-federation/src/sources/connect/expand/visitors/selection.rs index 18b325b81b..4d1f198c07 100644 --- a/apollo-federation/src/sources/connect/expand/visitors/selection.rs +++ b/apollo-federation/src/sources/connect/expand/visitors/selection.rs @@ -126,17 +126,6 @@ impl FieldVisitor for SchemaVisitor<'_, ObjectTypeDefinitionPosi impl GroupVisitor for SchemaVisitor<'_, ObjectTypeDefinitionPosition, ObjectType> { - fn get_group_fields( - &self, - (_, group): JSONSelectionGroup, - ) -> Result, >::Error> { - Ok(group - .selections_iter() - .sorted_by_key(|s| s.name()) - .cloned() - .collect()) - } - fn try_get_group_for_field( &self, field: &NamedSelection, @@ -163,7 +152,7 @@ impl GroupVisitor fn enter_group( &mut self, - (group_type, group): JSONSelectionGroup, + (group_type, group): &JSONSelectionGroup, ) -> Result, FederationError> { try_pre_insert!(self.to_schema, group_type)?; let def = group_type.get(self.original_schema.schema())?; @@ -177,7 +166,11 @@ impl GroupVisitor }; self.type_stack.push((group_type.clone(), sub_type)); - self.get_group_fields((group_type, group)) + Ok(group + .selections_iter() + .sorted_by_key(|s| s.name()) + .cloned() + .collect()) } fn exit_group(&mut self) -> Result<(), FederationError> { diff --git a/apollo-federation/src/sources/connect/json_selection/mod.rs b/apollo-federation/src/sources/connect/json_selection/mod.rs index 73f7e74a5e..fcdbe923d2 100644 --- a/apollo-federation/src/sources/connect/json_selection/mod.rs +++ b/apollo-federation/src/sources/connect/json_selection/mod.rs @@ -4,7 +4,6 @@ mod helpers; mod parser; mod pretty; mod selection_set; -mod visitor; pub use apply_to::*; pub use parser::*; @@ -13,4 +12,3 @@ pub use parser::*; // remove the `#[cfg(test)]`. #[cfg(test)] pub use pretty::*; -pub use visitor::*; diff --git a/apollo-federation/src/sources/connect/json_selection/visitor.rs b/apollo-federation/src/sources/connect/json_selection/visitor.rs deleted file mode 100644 index 8a57c1b970..0000000000 --- a/apollo-federation/src/sources/connect/json_selection/visitor.rs +++ /dev/null @@ -1,289 +0,0 @@ -//! JSONSelection Visitor -//! -//! In many cases it can be useful to visit all of the output keys in a JSONSelection. -//! This module defines a trait which allows for defining custom handling logic over -//! all output keys and their (optional) subkeys. - -use std::collections::VecDeque; - -use itertools::Itertools; - -use super::JSONSelection; - -/// A visitor for JSONSelection output keys. -/// -/// This trait allows for walking a JSONSelection over just its output -/// keys. Each key is `visit`ed in alphabetical order, with each subselection's -/// keys visited immediately after its named group. -/// -/// When given the following JSON Selection: -/// ```json_selection -/// a: something -/// b { -/// c -/// d: else -/// } -/// ``` -/// -/// The order of traversal would look like the following: -/// ```text -/// visit("a") -/// visit("b") -/// enter_group("b") -/// visit("c") -/// visit("d") -/// exit_group("b") -/// finish() -/// ``` -pub trait JSONSelectionVisitor: Sized { - type Error; - - /// Visit an output key - fn visit(&mut self, name: &str) -> Result<(), Self::Error>; - - /// Enter a subselection group - /// Note: You can assume that the named selection corresponding to this - /// group will be visited first. - fn enter_group(&mut self, group: &str) -> Result<(), Self::Error>; - - /// Exit a subselection group - /// Note: You can assume that the named selection corresponding to this - /// group will be visited and entered first. - fn exit_group(&mut self) -> Result<(), Self::Error>; - - /// Finish visiting the output keys. - fn finish(self) -> Result<(), Self::Error>; - - /// Walk through a [`JSONSelection`], visiting each output key. If at any point, one of the - /// visitor methods returns an error, then the walk will be stopped and the error will be - /// returned. - fn walk(mut self, selection: &JSONSelection) -> Result<(), Self::Error> { - let primed = match &selection { - JSONSelection::Named(named) => named.selections.iter(), - JSONSelection::Path(path) => path - .next_subselection() - .map(|sub| sub.selections.iter()) - .unwrap_or([].iter()), - }; - let mut to_visit = VecDeque::from_iter( - primed - .sorted_by(|a, b| Ord::cmp(a.name(), b.name())) - .map(|n| (0, n)), - ); - - // Start visiting each of the selections - let mut current_depth = 0; - while let Some((depth, next)) = to_visit.pop_front() { - if depth < current_depth { - for _ in depth..current_depth { - self.exit_group()?; - } - current_depth = depth; - } - - self.visit(next.name())?; - - // If we have a named selection that has a subselection, then we want to - // make sure that we visit the children before all other siblings. - // - // Note: We sort by the reverse order here since we always push to the front. - if let Some(sub) = next.next_subselection() { - current_depth += 1; - self.enter_group(next.name())?; - sub.selections - .iter() - .sorted_by(|a, b| Ord::cmp(b.name(), a.name())) - .for_each(|s| to_visit.push_front((current_depth, s))); - } - } - - // Make sure that we exit until we are no longer nested - for _ in 0..current_depth { - self.exit_group()?; - } - - // Finish out the self - self.finish() - } -} - -#[cfg(test)] -mod tests { - use insta::assert_snapshot; - - use super::JSONSelectionVisitor; - use crate::error::FederationError; - use crate::sources::connect::JSONSelection; - - /// Visitor for tests. - /// - /// Each node visited is added, along with its depth. This is later printed - /// such that groups are indented based on depth. - struct TestVisitor<'a> { - depth_stack: Vec, - visited: &'a mut Vec<(usize, String)>, - } - - impl<'a> TestVisitor<'a> { - fn new(visited: &'a mut Vec<(usize, String)>) -> Self { - Self { - depth_stack: Vec::new(), - visited, - } - } - - fn last_depth(&self) -> usize { - *self.depth_stack.last().unwrap_or(&0) - } - } - - fn print_visited(visited: Vec<(usize, String)>) -> String { - let mut result = String::new(); - for (depth, visited) in visited { - result.push_str(&format!("{}{visited}\n", "| ".repeat(depth))); - } - - result - } - - impl JSONSelectionVisitor for TestVisitor<'_> { - type Error = FederationError; - - fn visit(&mut self, name: &str) -> Result<(), Self::Error> { - self.visited.push((self.last_depth(), name.to_string())); - - Ok(()) - } - - fn enter_group(&mut self, _: &str) -> Result<(), Self::Error> { - self.depth_stack.push(self.last_depth() + 1); - Ok(()) - } - - fn exit_group(&mut self) -> Result<(), Self::Error> { - self.depth_stack.pop().unwrap(); - Ok(()) - } - - fn finish(self) -> Result<(), Self::Error> { - Ok(()) - } - } - - #[test] - fn it_iterates_over_empty_path() { - let mut visited = Vec::new(); - let visitor = TestVisitor::new(&mut visited); - let (unmatched, selection) = JSONSelection::parse("").unwrap(); - assert!(unmatched.is_empty()); - - visitor.walk(&selection).unwrap(); - assert_snapshot!(print_visited(visited), @""); - } - - #[test] - fn it_iterates_over_simple_selection() { - let mut visited = Vec::new(); - let visitor = TestVisitor::new(&mut visited); - let (unmatched, selection) = JSONSelection::parse("a b c d").unwrap(); - assert!(unmatched.is_empty()); - - visitor.walk(&selection).unwrap(); - assert_snapshot!(print_visited(visited), @r###" - a - b - c - d - "###); - } - - #[test] - fn it_iterates_over_aliased_selection() { - let mut visited = Vec::new(); - let visitor = TestVisitor::new(&mut visited); - let (unmatched, selection) = - JSONSelection::parse("a: one b: two c: three d: four").unwrap(); - assert!(unmatched.is_empty()); - - visitor.walk(&selection).unwrap(); - assert_snapshot!(print_visited(visited), @r###" - a - b - c - d - "###); - } - - #[test] - fn it_iterates_over_nested_selection() { - let mut visited = Vec::new(); - let visitor = TestVisitor::new(&mut visited); - let (unmatched, selection) = JSONSelection::parse("a { b { c { d { e } } } f }").unwrap(); - assert!(unmatched.is_empty()); - - visitor.walk(&selection).unwrap(); - assert_snapshot!(print_visited(visited), @r###" - a - | b - | | c - | | | d - | | | | e - | f - "###); - } - - #[test] - fn it_iterates_over_complex_selection() { - let mut visited = Vec::new(); - let visitor = TestVisitor::new(&mut visited); - let (unmatched, selection) = JSONSelection::parse( - "id - name - username - email - address { - street - suite - city - zipcode - geo { - lat - lng - } - } - phone - website - company { - name - catchPhrase - bs - }", - ) - .unwrap(); - assert!(unmatched.is_empty()); - - visitor.walk(&selection).unwrap(); - assert_snapshot!(print_visited(visited), @r###" - address - | city - | geo - | | lat - | | lng - | street - | suite - | zipcode - company - | bs - | catchPhrase - | name - email - id - name - phone - username - website - "###); - // let iter = selection.iter(); - // assert_debug_snapshot!(iter.collect_vec()); - } -} diff --git a/apollo-federation/src/sources/connect/validation/mod.rs b/apollo-federation/src/sources/connect/validation/mod.rs index abb6f8316e..89e677c120 100644 --- a/apollo-federation/src/sources/connect/validation/mod.rs +++ b/apollo-federation/src/sources/connect/validation/mod.rs @@ -376,6 +376,8 @@ pub enum Code { UnsupportedAbstractType, /// Header does not define `from` or `value` MissingHeaderSource, + /// Fields that return an object type must use a group JSONSelection `{}` + GroupSelectionRequiredForObject, } impl Code { diff --git a/apollo-federation/src/sources/connect/validation/selection.rs b/apollo-federation/src/sources/connect/validation/selection.rs index 7cae05b2cf..db09a4aa8b 100644 --- a/apollo-federation/src/sources/connect/validation/selection.rs +++ b/apollo-federation/src/sources/connect/validation/selection.rs @@ -6,6 +6,7 @@ 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; @@ -17,9 +18,12 @@ use super::Code; use super::Message; use super::Name; use super::Value; -use crate::sources::connect::json_selection::JSONSelectionVisitor; +use crate::sources::connect::expand::visitors::FieldVisitor; +use crate::sources::connect::expand::visitors::GroupVisitor; +use crate::sources::connect::json_selection::NamedSelection; use crate::sources::connect::spec::schema::CONNECT_SELECTION_ARGUMENT_NAME; use crate::sources::connect::JSONSelection; +use crate::sources::connect::SubSelection; pub(super) fn validate_selection( field: &Component, @@ -34,17 +38,24 @@ pub(super) fn validate_selection( }; let Some(return_type) = schema.get_object(field.ty.inner_named_type()) else { - // TODO: Validate scalars + // TODO: Validate scalar return types + return None; + }; + let Some(selection) = json_selection.next_subselection() else { + // TODO: Validate scalar selections return None; }; + let group = Group { + selection, + ty: return_type, + definition: field, + }; + SelectionValidator { root: PathPart::Root(parent_type), schema, - path: vec![PathPart::Field { - definition: field, - ty: return_type, - }], + path: Vec::new(), selection_coordinate: connect_directive_selection_coordinate( &connect_directive.name, parent_type, @@ -52,7 +63,7 @@ pub(super) fn validate_selection( ), selection_location: selection_value.line_column_range(&schema.sources), } - .walk(&json_selection) + .walk(group) .err() } @@ -111,89 +122,175 @@ struct SelectionValidator<'schema> { selection_coordinate: String, } -#[derive(Clone, Copy, Debug)] -enum PathPart<'a> { - // Query, Mutation, Subscription OR an Entity type - Root(&'a Node), - Field { - definition: &'a Node, - ty: &'a Node, - }, -} -// TODO: Once there is location data for JSONSelection, return multiple errors instead of stopping -// at the first -impl<'schema> JSONSelectionVisitor for SelectionValidator<'schema> { - type Error = Message; - - fn visit(&mut self, _name: &str) -> Result<(), Self::Error> { - // TODO: Validate that the field exists - Ok(()) - } - - fn enter_group(&mut self, field_name: &str) -> Result<(), Self::Error> { - let parent = self.path.last().copied().unwrap_or(self.root); - let parent_type = match parent { - PathPart::Root(root) => root, - PathPart::Field { ty, .. } => ty, - }; - - let definition = parent_type.fields.get(field_name).ok_or_else(|| { - Message { - code: Code::SelectedFieldNotFound, - message: format!( - "{coordinate} contains field `{field_name}`, which does not exist on `{parent_type}`.", - coordinate = &self.selection_coordinate, - parent_type = parent_type.name - ), - locations: self.selection_location.iter().cloned().collect(), - }})?; - - let ty = self.schema.get_object(definition.ty.inner_named_type()).ok_or_else(|| { - Message { - code: Code::GroupSelectionIsNotObject, - message: format!( - "{coordinate} selects a group `{field_name}`, but `{parent_type}.{field_name}` is not an object.", - coordinate = &self.selection_coordinate, - parent_type = parent_type.name, - ), - locations: self.selection_location.iter().cloned().chain(definition.line_column_range(&self.schema.sources)).collect(), - }})?; - +impl SelectionValidator<'_> { + fn check_for_circular_reference( + &self, + field: Field, + object: &Node, + ) -> Result<(), Message> { for seen_part in self.path_with_root() { - let (seen_type, field_def) = match seen_part { + let (seen_type, ancestor_field) = match seen_part { PathPart::Root(root) => (root, None), PathPart::Field { ty, definition } => (ty, Some(definition)), }; - if seen_type == ty { + if seen_type == object { return Err(Message { code: Code::CircularReference, message: format!( "Circular reference detected in {coordinate}: type `{new_object_name}` appears more than once in `{selection_path}`. For more information, see https://go.apollo.dev/connectors/limitations#circular-references", coordinate = &self.selection_coordinate, - selection_path = self.path_string(&definition.name), - new_object_name = ty.name, + selection_path = self.path_string(field.definition), + new_object_name = object.name, ), locations: - self.selection_location.iter().cloned() + self.selection_location.iter().cloned() // Root field includes the selection location, which duplicates the diagnostic - .chain(field_def.and_then(|def| def.line_column_range(&self.schema.sources))) - .chain(definition.line_column_range(&self.schema.sources)) - .collect(), + .chain(ancestor_field.and_then(|def| def.line_column_range(&self.schema.sources))) + .chain(field.definition.line_column_range(&self.schema.sources)) + .collect(), }); } } - self.path.push(PathPart::Field { definition, ty }); Ok(()) } +} + +#[derive(Clone, Copy, Debug)] +struct Field<'schema> { + selection: &'schema NamedSelection, + definition: &'schema Node, +} + +#[derive(Clone, Copy, Debug)] +enum PathPart<'a> { + // Query, Mutation, Subscription OR an Entity type + Root(&'a Node), + Field { + definition: &'a Node, + ty: &'a Node, + }, +} + +impl<'a> PathPart<'a> { + fn ty(&self) -> &Node { + match self { + PathPart::Root(ty) => ty, + PathPart::Field { ty, .. } => ty, + } + } +} + +#[derive(Clone, Debug)] +struct Group<'schema> { + selection: &'schema SubSelection, + ty: &'schema Node, + definition: &'schema Node, +} + +// TODO: Once there is location data for JSONSelection, return multiple errors instead of stopping +// at the first +impl<'schema> GroupVisitor, Field<'schema>> for SelectionValidator<'schema> { + /// If the both the selection and the schema agree that this field is an object, then we + /// provide it back to the visitor to be walked. + /// + /// This does no validation, as we have to do that on the field level anyway. + fn try_get_group_for_field( + &self, + field: &Field<'schema>, + ) -> Result>, Self::Error> { + let Some(selection) = field.selection.next_subselection() else { + return Ok(None); + }; + let Some(ty) = self + .schema + .get_object(field.definition.ty.inner_named_type()) + else { + return Ok(None); + }; + Ok(Some(Group { + selection, + ty, + definition: field.definition, + })) + } + + /// Get all the fields for an object type / selection. + /// Returns an error if a selection points at a field which does not exist on the schema. + fn enter_group(&mut self, group: &Group<'schema>) -> Result>, Self::Error> { + self.path.push(PathPart::Field { + definition: group.definition, + ty: group.ty, + }); + group.selection.selections_iter().map(|selection| { + let field_name = selection.name(); + let definition = group.ty.fields.get(field_name).ok_or_else(|| { + Message { + code: Code::SelectedFieldNotFound, + message: format!( + "{coordinate} contains field `{field_name}`, which does not exist on `{parent_type}`.", + coordinate = &self.selection_coordinate, + parent_type = group.ty.name, + ), + locations: self.selection_location.iter().cloned().collect(), + } + })?; + Ok(Field { + selection, + definition, + }) + }).collect() + } fn exit_group(&mut self) -> Result<(), Self::Error> { self.path.pop(); Ok(()) } +} - fn finish(self) -> Result<(), Self::Error> { - Ok(()) +impl<'schema> FieldVisitor> for SelectionValidator<'schema> { + type Error = Message; + + fn visit(&mut self, field: Field<'schema>) -> Result<(), Self::Error> { + let field_name = field.definition.name.as_str(); + let type_name = field.definition.ty.inner_named_type(); + let field_type = self.schema.types.get(type_name).ok_or_else(|| Message { + code: Code::GraphQLError, + message: format!( + "{coordinate} contains field `{field_name}`, which has undefined type `{type_name}.", + coordinate = &self.selection_coordinate, + ), + locations: self.selection_location.iter().cloned().collect(), + })?; + let is_group = field.selection.next_subselection().is_some(); + + match (field_type, is_group) { + (ExtendedType::Object(object), true) => { + self.check_for_circular_reference(field, object) + }, + (_, true) => { + Err(Message { + code: Code::GroupSelectionIsNotObject, + message: format!( + "{coordinate} selects a group `{field_name}{{}}`, but `{parent_type}.{field_name}` is of type `{type_name}` which is not an object.", + coordinate = &self.selection_coordinate, + parent_type = self.last_field().ty().name, + ), + locations: self.selection_location.iter().cloned().chain(field.definition.line_column_range(&self.schema.sources)).collect(), + }) + }, + (ExtendedType::Object(_), false) => { + Err(Message { + code: Code::GroupSelectionRequiredForObject, + message: format!( + "`{type_name}.{field_name}` is an object, so `{coordinate}` must select a group `{field_name}{{}}`.", + coordinate = &self.selection_coordinate, + ), + locations: self.selection_location.iter().cloned().chain(field.definition.line_column_range(&self.schema.sources)).collect(), + }) + }, + (_, false) => Ok(()), + } } } @@ -202,13 +299,17 @@ impl SelectionValidator<'_> { once(self.root).chain(self.path.iter().copied()) } - fn path_string(&self, tail: &str) -> String { + fn path_string(&self, tail: &FieldDefinition) -> String { self.path_with_root() .map(|part| match part { PathPart::Root(ty) => ty.name.as_str(), PathPart::Field { definition, .. } => definition.name.as_str(), }) - .chain(once(tail)) + .chain(once(tail.name.as_str())) .join(".") } + + fn last_field(&self) -> &PathPart { + self.path.last().unwrap_or(&self.root) + } } diff --git a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@group_selection_on_scalar.graphql.snap b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@group_selection_on_scalar.graphql.snap index 7461a70add..0021d6511b 100644 --- a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@group_selection_on_scalar.graphql.snap +++ b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@group_selection_on_scalar.graphql.snap @@ -3,4 +3,4 @@ source: apollo-federation/src/sources/connect/validation/mod.rs expression: "format!(\"{:?}\", errors)" input_file: apollo-federation/src/sources/connect/validation/test_data/validation/group_selection_on_scalar.graphql --- -[Message { code: GroupSelectionIsNotObject, message: "`@connect(selection:)` on `Query.me` selects a group `id`, but `User.id` is not an object.", locations: [LineColumn { line: 8, column: 77 }..LineColumn { line: 8, column: 88 }, LineColumn { line: 12, column: 5 }..LineColumn { line: 12, column: 12 }] }] +[Message { code: GroupSelectionIsNotObject, message: "`@connect(selection:)` on `Query.me` selects a group `id{}`, but `User.id` is of type `ID` which is not an object.", locations: [LineColumn { line: 8, column: 77 }..LineColumn { line: 8, column: 88 }, LineColumn { line: 12, column: 5 }..LineColumn { line: 12, column: 12 }] }] diff --git a/apollo-federation/src/sources/connect/validation/test_data/validation/circular_reference_2.graphql b/apollo-federation/src/sources/connect/validation/test_data/validation/circular_reference_2.graphql index 4076ea6f1b..7abee48bac 100644 --- a/apollo-federation/src/sources/connect/validation/test_data/validation/circular_reference_2.graphql +++ b/apollo-federation/src/sources/connect/validation/test_data/validation/circular_reference_2.graphql @@ -5,7 +5,7 @@ type Query { track(id: ID!): Track @connect( http: { GET: "http://track/{$args.id}" } - selection: "id title" + selection: "id" entity: true ) }