diff --git a/crates/apollo-compiler/CHANGELOG.md b/crates/apollo-compiler/CHANGELOG.md index 744a3add..51210aaa 100644 --- a/crates/apollo-compiler/CHANGELOG.md +++ b/crates/apollo-compiler/CHANGELOG.md @@ -17,6 +17,21 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ## Maintenance ## Documentation--> +# [x.x.x] (unreleased) - 2024-mm-dd + +## Features +- **New field merging validation implementation - [goto-bus-stop], [pull/816]** + - Uses the much more scalable [Xing algorithm]. + - This also fixes some invalid selections that were previously accepted by apollo-compiler. + - Selections in fragment definitions are now only validated in the context of the operations they + are used in, reducing duplicate error reports for invalid fragments. This means invalid *unused* + fragments do not produce field merging errors, but they will still raise a different error because + fragments being unused is also an error according to the spec. + +[goto-bus-stop]: https://github.com/goto-bus-stop] +[pull/816]: https://github.com/apollographql/apollo-rs/pull/816 +[Xing algorithm]: https://tech.new-work.se/graphql-overlapping-fields-can-be-merged-fast-ea6e92e0a01 + # [1.0.0-beta.12](https://crates.io/crates/apollo-compiler/1.0.0-beta.12) - 2024-01-15 ## BREAKING diff --git a/crates/apollo-compiler/Cargo.toml b/crates/apollo-compiler/Cargo.toml index 65e60c48..5346b1a2 100644 --- a/crates/apollo-compiler/Cargo.toml +++ b/crates/apollo-compiler/Cargo.toml @@ -49,6 +49,11 @@ name = "multi-source" path = "benches/multi_source.rs" harness = false +[[bench]] +name = "fields-validation" +path = "benches/fields_validation.rs" +harness = false + [[test]] name = "main" diff --git a/crates/apollo-compiler/benches/fields_validation.rs b/crates/apollo-compiler/benches/fields_validation.rs new file mode 100644 index 00000000..e2b70a7e --- /dev/null +++ b/crates/apollo-compiler/benches/fields_validation.rs @@ -0,0 +1,124 @@ +use apollo_compiler::ExecutableDocument; +use apollo_compiler::Schema; +use criterion::*; + +fn bench_many_same_field(c: &mut Criterion) { + let schema = + Schema::parse_and_validate("type Query { hello: String! }", "schema.graphql").unwrap(); + let query = format!("{{ {} }}", "hello ".repeat(1_000)); + + c.bench_function("many_same_field", move |b| { + b.iter(|| { + let doc = + ExecutableDocument::parse_and_validate(&schema, &query, "query.graphql").unwrap(); + black_box(doc); + }); + }); +} + +fn bench_many_same_nested_field(c: &mut Criterion) { + let schema = Schema::parse_and_validate( + " + type Nested { hello: String! } + type Query { nested: Nested! } + ", + "schema.graphql", + ) + .unwrap(); + let query = format!("{{ {} }}", "nested { hello } ".repeat(1_000)); + + c.bench_function("many_same_nested_field", move |b| { + b.iter(|| { + let doc = + ExecutableDocument::parse_and_validate(&schema, &query, "query.graphql").unwrap(); + black_box(doc); + }); + }); +} + +fn bench_many_arguments(c: &mut Criterion) { + let schema = + Schema::parse_and_validate("type Query { hello: String! }", "schema.graphql").unwrap(); + let args = (0..2_000).fold(String::new(), |mut acc, i| { + use std::fmt::Write; + _ = writeln!(&mut acc, "arg{i}: {i}"); + acc + }); + let field = format!("hello({args})"); + let query = format!("{{ {field} {field} }}"); + + c.bench_function("many_arguments", move |b| { + b.iter(|| { + // Will return errors but that's cool + let doc = ExecutableDocument::parse_and_validate(&schema, &query, "query.graphql"); + _ = black_box(doc); + }); + }); +} + +fn bench_many_types(c: &mut Criterion) { + let schema = Schema::parse_and_validate( + " + interface Abstract { + field: Abstract + leaf: Int + } + interface Abstract1 { + field: Abstract + leaf: Int + } + interface Abstract2 { + field: Abstract + leaf: Int + } + type Concrete1 implements Abstract & Abstract1 { + field: Abstract + leaf: Int + } + type Concrete2 implements Abstract & Abstract2 { + field: Abstract + leaf: Int + } + type Query { + field: Abstract + } + ", + "schema.graphql", + ) + .unwrap(); + let query = format!( + " + fragment multiply on Abstract {{ + field {{ + ... on Abstract1 {{ field {{ leaf }} }} + ... on Abstract2 {{ field {{ leaf }} }} + ... on Concrete1 {{ field {{ leaf }} }} + ... on Concrete2 {{ field {{ leaf }} }} + }} + }} + + query DeepAbstractConcrete {{ + {open}{close} + }} + ", + open = "field { ...multiply ".repeat(100), + close = "}".repeat(100) + ); + + c.bench_function("many_types", move |b| { + b.iter(|| { + let doc = + ExecutableDocument::parse_and_validate(&schema, &query, "query.graphql").unwrap(); + black_box(doc); + }); + }); +} + +criterion_group!( + fields, + bench_many_same_field, + bench_many_same_nested_field, + bench_many_arguments, + bench_many_types, +); +criterion_main!(fields); diff --git a/crates/apollo-compiler/benches/testdata/supergraph_query.graphql b/crates/apollo-compiler/benches/testdata/supergraph_query.graphql index 40b8bc98..c70a1193 100644 --- a/crates/apollo-compiler/benches/testdata/supergraph_query.graphql +++ b/crates/apollo-compiler/benches/testdata/supergraph_query.graphql @@ -1,10 +1,5 @@ query Query { - allProducts { - id + topProducts { sku - createdBy { - email - totalProductsCreated - } } -} \ No newline at end of file +} diff --git a/crates/apollo-compiler/src/executable/mod.rs b/crates/apollo-compiler/src/executable/mod.rs index 38108862..cfd1cd87 100644 --- a/crates/apollo-compiler/src/executable/mod.rs +++ b/crates/apollo-compiler/src/executable/mod.rs @@ -2,6 +2,8 @@ //! which can contain operations and fragments. use crate::ast; +use crate::coordinate::FieldArgumentCoordinate; +use crate::coordinate::TypeAttributeCoordinate; use crate::schema; use crate::Node; use crate::Parser; @@ -69,20 +71,20 @@ pub struct Fragment { pub selection_set: SelectionSet, } -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct SelectionSet { pub ty: NamedType, pub selections: Vec, } -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq, Hash)] pub enum Selection { Field(Node), FragmentSpread(Node), InlineFragment(Node), } -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct Field { /// The definition of this field in an object type or interface type definition in the schema pub definition: Node, @@ -93,20 +95,21 @@ pub struct Field { pub selection_set: SelectionSet, } -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct FragmentSpread { pub fragment_name: Name, pub directives: DirectiveList, } -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct InlineFragment { pub type_condition: Option, pub directives: DirectiveList, pub selection_set: SelectionSet, } -/// AST node that has been skipped during conversion to `ExecutableDocument` +/// Errors that can occur during conversion from AST to executable document or +/// validation of an executable document. #[derive(thiserror::Error, Debug, Clone)] pub(crate) enum BuildError { #[error("an executable document must not contain {describe}")] @@ -161,6 +164,67 @@ pub(crate) enum BuildError { field_name: Name, path: SelectionPath, }, + + // Validation errors + #[error( + "{} can only have one root field", + subscription_name_or_anonymous(name) + )] + SubscriptionUsesMultipleFields { + name: Option, + fields: Vec, + }, + + #[error( + "{} can not have an introspection field as a root field", + subscription_name_or_anonymous(name) + )] + SubscriptionUsesIntrospection { + /// Name of the operation + name: Option, + /// Name of the introspection field + field: Name, + }, + + #[error("operation must not select different types using the same name `{alias}`")] + ConflictingFieldType { + /// Name or alias of the non-unique field. + alias: Name, + original_location: Option, + original_coordinate: TypeAttributeCoordinate, + original_type: Type, + conflicting_location: Option, + conflicting_coordinate: TypeAttributeCoordinate, + conflicting_type: Type, + }, + #[error("operation must not provide conflicting field arguments for the same name `{alias}`")] + ConflictingFieldArgument { + /// Name or alias of the non-unique field. + alias: Name, + original_location: Option, + original_coordinate: FieldArgumentCoordinate, + original_value: Option, + conflicting_location: Option, + conflicting_coordinate: FieldArgumentCoordinate, + conflicting_value: Option, + }, + #[error("cannot select different fields into the same alias `{alias}`")] + ConflictingFieldName { + /// Name of the non-unique field. + alias: Name, + original_location: Option, + original_selection: TypeAttributeCoordinate, + conflicting_location: Option, + conflicting_selection: TypeAttributeCoordinate, + }, +} + +fn subscription_name_or_anonymous(name: &Option) -> impl std::fmt::Display + '_ { + crate::validation::diagnostics::NameOrAnon { + name: name.as_ref(), + if_some_prefix: "subscription", + if_none: "anonymous subscription", + } } #[derive(Debug, Clone, PartialEq, Eq)] @@ -327,10 +391,26 @@ impl PartialEq for ExecutableDocument { } impl Operation { + /// Returns the name of the schema type this operation selects against. pub fn object_type(&self) -> &NamedType { &self.selection_set.ty } + /// Returns true if this is a query operation. + pub fn is_query(&self) -> bool { + self.operation_type == OperationType::Query + } + + /// Returns true if this is a mutation operation. + pub fn is_mutation(&self) -> bool { + self.operation_type == OperationType::Mutation + } + + /// Returns true if this is a subscription operation. + pub fn is_subscription(&self) -> bool { + self.operation_type == OperationType::Subscription + } + /// Return whether this operation is a query that only selects introspection meta-fields: /// `__type`, `__schema`, and `__typename` pub fn is_introspection(&self, document: &ExecutableDocument) -> bool { @@ -591,6 +671,11 @@ impl Field { schema.types.get(self.ty().inner_named_type()) } + /// Returns the argument by a given name. + pub fn argument_by_name(&self, name: &str) -> Option<&'_ Node> { + self.arguments.iter().find(|argument| argument.name == name) + } + serialize_method!(); } diff --git a/crates/apollo-compiler/src/executable/validation.rs b/crates/apollo-compiler/src/executable/validation.rs index caaad8b0..50d19203 100644 --- a/crates/apollo-compiler/src/executable/validation.rs +++ b/crates/apollo-compiler/src/executable/validation.rs @@ -1,5 +1,6 @@ use super::FieldSet; use crate::ast; +use crate::validation::selection::FieldsInSetCanMerge; use crate::validation::Details; use crate::validation::DiagnosticList; use crate::validation::FileId; @@ -30,11 +31,15 @@ pub(crate) fn validate_standalone_executable( } fn validate_with_schema( - _errors: &mut DiagnosticList, - _schema: &Schema, - _document: &ExecutableDocument, + errors: &mut DiagnosticList, + schema: &Schema, + document: &ExecutableDocument, ) { - // TODO + let mut fields_in_set_can_merge = FieldsInSetCanMerge::new(schema, document); + for operation in document.all_operations() { + crate::validation::operation::validate_subscription(document, operation, errors); + fields_in_set_can_merge.validate_operation(operation, errors); + } } pub(crate) fn validate_with_or_without_schema( diff --git a/crates/apollo-compiler/src/validation/diagnostics.rs b/crates/apollo-compiler/src/validation/diagnostics.rs index 528e4d97..30fb3246 100644 --- a/crates/apollo-compiler/src/validation/diagnostics.rs +++ b/crates/apollo-compiler/src/validation/diagnostics.rs @@ -2,7 +2,6 @@ use crate::ast; use crate::ast::DirectiveLocation; use crate::ast::Name; use crate::ast::Type; -use crate::ast::Value; use crate::coordinate::SchemaCoordinate; use crate::coordinate::TypeAttributeCoordinate; use crate::diagnostic::CliReport; @@ -51,14 +50,6 @@ pub(crate) enum DiagnosticData { original_definition: Option, redefined_definition: Option, }, - #[error( - "{} can only have one root field", - subscription_name_or_anonymous(name) - )] - SingleRootField { - name: Option, - fields: Vec, - }, #[error("the argument `{name}` is not supported by `{coordinate}`")] UndefinedArgument { name: Name, @@ -213,51 +204,11 @@ pub(crate) enum DiagnosticData { name: Name, original_application: Option, }, - #[error( - "{} can not have an introspection field as a root field", - subscription_name_or_anonymous(name) - )] - IntrospectionField { - /// Name of the operation - name: Option, - /// Name of the field - field: Name, - }, #[error("interface, union and object types must have a subselection set")] MissingSubselection { coordinate: TypeAttributeCoordinate, describe_type: &'static str, }, - #[error("operation must not select different types using the same field name `{field}`")] - ConflictingFieldType { - /// Name of the non-unique field. - field: Name, - original_selection: Option, - original_type: Type, - redefined_selection: Option, - redefined_type: Type, - }, - #[error( - "operation must not provide conflicting field arguments for the same field name `{field}`" - )] - ConflictingFieldArgument { - /// Name of the non-unique field. - field: Name, - argument: Name, - original_selection: Option, - original_value: Option, - redefined_selection: Option, - redefined_value: Option, - }, - #[error("operation must not select different fields to the same alias `{field}`")] - ConflictingFieldName { - /// Name of the non-unique field. - field: Name, - original_selection: Option, - original_name: Name, - redefined_selection: Option, - redefined_name: Name, - }, #[error( "{} must have a composite type in its type condition", fragment_name_or_inline(name) @@ -380,17 +331,6 @@ impl ValidationError { "`{name}` must only be defined once in this argument list or input object definition." )); } - DiagnosticData::SingleRootField { fields, .. } => { - report.with_label_opt( - self.location, - format_args!("subscription with {} root fields", fields.len()), - ); - report.with_help(format_args!( - "There are {} root fields: {}. This is not allowed.", - fields.len(), - CommaSeparated(fields) - )); - } DiagnosticData::UndefinedArgument { coordinate, definition_location, @@ -645,12 +585,6 @@ impl ValidationError { format_args!("directive `@{name}` called again here"), ); } - DiagnosticData::IntrospectionField { field, .. } => { - report.with_label_opt( - self.location, - format_args!("{field} is an introspection field"), - ); - } DiagnosticData::MissingSubselection { coordinate, describe_type, @@ -660,78 +594,6 @@ impl ValidationError { format_args!("{coordinate} is {describe_type} and must select fields"), ); } - DiagnosticData::ConflictingFieldType { - field, - original_selection, - original_type, - redefined_selection, - redefined_type, - } => { - report.with_label_opt( - *original_selection, - format_args!("`{field}` has type `{original_type}` here"), - ); - report.with_label_opt( - *redefined_selection, - format_args!("but the same field name has type `{redefined_type}` here"), - ); - } - DiagnosticData::ConflictingFieldArgument { - field, - argument, - original_selection, - original_value, - redefined_selection, - redefined_value, - } => { - match (original_value, redefined_value) { - (Some(_), Some(_)) => { - report.with_label_opt( - *original_selection, - format_args!("field `{field}` provides one argument value here"), - ); - report.with_label_opt(*redefined_selection, "but a different value here"); - } - (Some(_), None) => { - report.with_label_opt( - *original_selection, - format!("field `{field}` is selected with argument `{argument}` here",), - ); - report.with_label_opt( - *redefined_selection, - format!("but argument `{argument}` is not provided here"), - ); - } - (None, Some(_)) => { - report.with_label_opt( - *redefined_selection, - format!("field `{field}` is selected with argument `{argument}` here",), - ); - report.with_label_opt( - *original_selection, - format!("but argument `{argument}` is not provided here"), - ); - } - (None, None) => unreachable!(), - } - report.with_help("Fields with the same response name must provide the same set of arguments. Consider adding an alias if you need to select fields with different arguments."); - } - DiagnosticData::ConflictingFieldName { - field, - original_selection, - original_name, - redefined_selection, - redefined_name, - } => { - report.with_label_opt( - *original_selection, - format_args!("field `{field}` is selected from field `{original_name}` here"), - ); - report.with_label_opt( - *redefined_selection, - format_args!("but the same field `{field}` is also selected from field `{redefined_name}` here"), - ); - } DiagnosticData::InvalidFragmentTarget { name: _, ty } => { report.with_label_opt( self.location, @@ -832,10 +694,11 @@ where } } -struct NameOrAnon<'a, T> { - name: Option<&'a T>, - if_some_prefix: &'a str, - if_none: &'a str, +/// Formatter that describes a name, or describes an anonymous element if there is no name. +pub(crate) struct NameOrAnon<'a, T> { + pub name: Option<&'a T>, + pub if_some_prefix: &'a str, + pub if_none: &'a str, } impl fmt::Display for NameOrAnon<'_, T> where @@ -856,10 +719,3 @@ fn fragment_name_or_inline(name: &'_ Option) -> NameOrAnon<'_, T> { if_none: "inline fragment", } } -fn subscription_name_or_anonymous(name: &'_ Option) -> NameOrAnon<'_, T> { - NameOrAnon { - name: name.as_ref(), - if_some_prefix: "subscription", - if_none: "anonymous subscription", - } -} diff --git a/crates/apollo-compiler/src/validation/mod.rs b/crates/apollo-compiler/src/validation/mod.rs index 56ac5000..e9076854 100644 --- a/crates/apollo-compiler/src/validation/mod.rs +++ b/crates/apollo-compiler/src/validation/mod.rs @@ -206,6 +206,8 @@ pub(crate) enum Details { // TODO: Merge ValidationError into this enum #[error(transparent)] CompilerDiagnostic(diagnostics::ValidationError), + #[error("too much recursion")] + RecursionLimitError, } impl ToCliReport for DiagnosticData { @@ -409,7 +411,110 @@ impl ToCliReport for DiagnosticData { ); report.with_note(format_args!("path to the field: `{path}`")) } + ExecutableBuildError::SubscriptionUsesMultipleFields { fields, .. } => { + report.with_label_opt( + self.location, + format_args!("subscription with {} root fields", fields.len()), + ); + report.with_help(format_args!( + "There are {} root fields: {}. This is not allowed.", + fields.len(), + CommaSeparated(fields) + )); + } + ExecutableBuildError::SubscriptionUsesIntrospection { field, .. } => { + report.with_label_opt( + self.location, + format_args!("{field} is an introspection field"), + ); + } + ExecutableBuildError::ConflictingFieldType { + alias, + original_location, + original_coordinate, + original_type, + conflicting_location, + conflicting_coordinate, + conflicting_type, + } => { + report.with_label_opt( + *original_location, + format_args!( + "`{alias}` is selected from `{original_coordinate}: {original_type}` here" + ), + ); + report.with_label_opt( + *conflicting_location, + format_args!("`{alias}` is selected from `{conflicting_coordinate}: {conflicting_type}` here"), + ); + } + ExecutableBuildError::ConflictingFieldArgument { + alias, + original_location, + original_coordinate, + original_value, + conflicting_location, + conflicting_coordinate: _, + conflicting_value, + } => { + let argument = &original_coordinate.argument; + match (original_value, conflicting_value) { + (Some(_), Some(_)) => { + report.with_label_opt( + *original_location, + format_args!( + "`{original_coordinate}` is used with one argument value here" + ), + ); + report.with_label_opt( + *conflicting_location, + "but a different value here", + ); + } + (Some(_), None) => { + report.with_label_opt( + *original_location, + format!("`{alias}` is selected with argument `{argument}` here",), + ); + report.with_label_opt( + *conflicting_location, + format!("but argument `{argument}` is not provided here"), + ); + } + (None, Some(_)) => { + report.with_label_opt( + *conflicting_location, + format!("`{alias}` is selected with argument `{argument}` here",), + ); + report.with_label_opt( + *original_location, + format!("but argument `{argument}` is not provided here"), + ); + } + (None, None) => unreachable!(), + } + report.with_help("The same name cannot be selected multiple times with different arguments, because it's not clear which set of arguments should be used to fill the response. If you intend to use diverging arguments, consider adding an alias to differentiate"); + } + ExecutableBuildError::ConflictingFieldName { + alias: field, + original_selection, + original_location, + conflicting_selection, + conflicting_location, + } => { + report.with_label_opt( + *original_location, + format_args!("`{field}` is selected from `{original_selection}` here"), + ); + report.with_label_opt( + *conflicting_location, + format_args!("`{field}` is selected from `{conflicting_selection}` here"), + ); + + report.with_help("Both fields may be present on the schema type, so it's not clear which one should be used to fill the response"); + } }, + Details::RecursionLimitError => {} } } } @@ -627,3 +732,22 @@ impl CycleError { self } } + +struct CommaSeparated<'a, It>(&'a It); +impl<'a, T, It> fmt::Display for CommaSeparated<'a, It> +where + T: fmt::Display, + &'a It: IntoIterator, +{ + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let mut it = self.0.into_iter(); + if let Some(element) = it.next() { + element.fmt(f)?; + } + for element in it { + f.write_str(", ")?; + element.fmt(f)?; + } + Ok(()) + } +} diff --git a/crates/apollo-compiler/src/validation/operation.rs b/crates/apollo-compiler/src/validation/operation.rs index f0c4a3f0..55279420 100644 --- a/crates/apollo-compiler/src/validation/operation.rs +++ b/crates/apollo-compiler/src/validation/operation.rs @@ -1,6 +1,7 @@ -use crate::validation::diagnostics::{DiagnosticData, ValidationError}; +use crate::validation::diagnostics::ValidationError; +use crate::validation::DiagnosticList; use crate::validation::FileId; -use crate::{ast, name, Node, ValidationDatabase}; +use crate::{ast, executable, Node, ValidationDatabase}; #[derive(Debug, Clone, Hash, PartialEq, Eq)] pub(crate) struct OperationValidationConfig<'vars> { @@ -10,43 +11,28 @@ pub(crate) struct OperationValidationConfig<'vars> { pub variables: &'vars [Node], } -pub(crate) fn validate_operation( - db: &dyn ValidationDatabase, - file_id: FileId, - operation: Node, - has_schema: bool, -) -> Vec { - let mut diagnostics = vec![]; - - let config = OperationValidationConfig { - has_schema, - variables: &operation.variables, - }; - - let schema = db.schema(); - let against_type = schema.root_operation(operation.operation_type); - - let named_fragments = db.ast_named_fragments(file_id); - let q = name!("Query"); - - if operation.operation_type == ast::OperationType::Subscription { - let fields = super::selection::operation_fields( - &named_fragments, - against_type.unwrap_or(&q), - &operation.selection_set, +pub(crate) fn validate_subscription( + document: &executable::ExecutableDocument, + operation: &Node, + diagnostics: &mut DiagnosticList, +) { + if operation.is_subscription() { + let fields = super::selection::expand_selections( + &document.fragments, + std::iter::once(&operation.selection_set), ); if fields.len() > 1 { - diagnostics.push(ValidationError::new( + diagnostics.push( operation.location(), - DiagnosticData::SingleRootField { + executable::BuildError::SubscriptionUsesMultipleFields { name: operation.name.clone(), fields: fields .iter() .map(|field| field.field.name.clone()) .collect(), }, - )); + ); } let has_introspection_fields = fields @@ -57,17 +43,34 @@ pub(crate) fn validate_operation( "__type" | "__schema" | "__typename" ) }) - .map(|field| field.field); + .map(|field| &field.field); if let Some(field) = has_introspection_fields { - diagnostics.push(ValidationError::new( + diagnostics.push( field.location(), - DiagnosticData::IntrospectionField { + executable::BuildError::SubscriptionUsesIntrospection { name: operation.name.clone(), field: field.name.clone(), }, - )); + ); } } +} + +pub(crate) fn validate_operation( + db: &dyn ValidationDatabase, + file_id: FileId, + operation: Node, + has_schema: bool, +) -> Vec { + let mut diagnostics = vec![]; + + let config = OperationValidationConfig { + has_schema, + variables: &operation.variables, + }; + + let schema = db.schema(); + let against_type = schema.root_operation(operation.operation_type); diagnostics.extend(super::directive::validate_directives( db, diff --git a/crates/apollo-compiler/src/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs index d1a07023..672199f6 100644 --- a/crates/apollo-compiler/src/validation/selection.rs +++ b/crates/apollo-compiler/src/validation/selection.rs @@ -1,112 +1,209 @@ -use std::collections::{hash_map::Entry, HashMap}; - -use crate::validation::diagnostics::{DiagnosticData, ValidationError}; +use crate::coordinate::TypeAttributeCoordinate; +use crate::executable::BuildError; +use crate::validation::diagnostics::ValidationError; +use crate::validation::operation::OperationValidationConfig; +use crate::validation::DiagnosticList; use crate::validation::{FileId, ValidationDatabase}; -use crate::{ast, schema, Node}; - -use super::operation::OperationValidationConfig; -/// TODO(@goto-bus-stop) test pathological query with many of the same field +use crate::{ast, executable, schema, Node}; +use apollo_parser::LimitTracker; +use indexmap::IndexMap; +use std::cell::OnceCell; +use std::collections::{HashMap, HashSet, VecDeque}; +use std::rc::Rc; + +/// Represents a field selected against a parent type. +#[derive(Debug, Clone, Copy, Hash)] +pub(crate) struct FieldSelection<'a> { + /// The type of the selection set this field selection is part of. + pub parent_type: &'a ast::NamedType, + pub field: &'a Node, +} -/// A field and the type it selects from. -#[derive(Debug, Clone, Copy)] -pub(crate) struct FieldAgainstType<'a> { - pub against_type: &'a ast::NamedType, - pub field: &'a Node, +impl FieldSelection<'_> { + pub fn coordinate(&self) -> TypeAttributeCoordinate { + TypeAttributeCoordinate { + ty: self.parent_type.clone(), + attribute: self.field.name.clone(), + } + } } -// TODO(@goto-bus-stop) remove intermediate allocations -pub(crate) fn operation_fields<'a>( - named_fragments: &'a HashMap>, - against_type: &'a ast::NamedType, - selections: &'a [ast::Selection], -) -> Vec> { - fn inner<'a>( - named_fragments: &'a HashMap>, - seen: &mut std::collections::HashSet, - against_type: &'a ast::NamedType, - selections: &'a [ast::Selection], - ) -> Vec> { - selections - .iter() - .flat_map(|selection| match selection { - ast::Selection::Field(field) => vec![FieldAgainstType { - against_type, +/// Expand one or more selection sets to a list of all fields selected. +pub(crate) fn expand_selections<'doc>( + fragments: &'doc IndexMap>, + selection_sets: impl Iterator, +) -> Vec> { + let mut selections = vec![]; + let mut queue: VecDeque<&executable::SelectionSet> = selection_sets.collect(); + let mut seen_fragments = HashSet::new(); + + while let Some(next_set) = queue.pop_front() { + for selection in &next_set.selections { + match selection { + executable::Selection::Field(field) => selections.push(FieldSelection { + parent_type: &next_set.ty, field, - }], - ast::Selection::InlineFragment(inline) => inner( - named_fragments, - seen, - inline.type_condition.as_ref().unwrap_or(against_type), - &inline.selection_set, - ), - ast::Selection::FragmentSpread(spread) => { - if seen.contains(&spread.fragment_name) { - return vec![]; + }), + executable::Selection::InlineFragment(spread) => { + queue.push_back(&spread.selection_set) + } + executable::Selection::FragmentSpread(spread) + if !seen_fragments.contains(&spread.fragment_name) => + { + seen_fragments.insert(&spread.fragment_name); + if let Some(fragment) = fragments.get(&spread.fragment_name) { + queue.push_back(&fragment.selection_set); } - seen.insert(spread.fragment_name.clone()); - - named_fragments - .get(&spread.fragment_name) - .map(|fragment| { - inner( - named_fragments, - seen, - &fragment.type_condition, - &fragment.selection_set, - ) - }) - .unwrap_or_default() } + executable::Selection::FragmentSpread(_) => { + // Already seen + } + } + } + } + + selections +} + +fn is_composite(ty: &schema::ExtendedType) -> bool { + use schema::ExtendedType::*; + matches!(ty, Object(_) | Interface(_) | Union(_)) +} + +/// A temporary index for frequent argument lookups by name, using a hash map if there are many +/// arguments. +enum ArgumentLookup<'a> { + Map(HashMap<&'a ast::Name, &'a Node>), + List(&'a [Node]), +} +impl<'a> ArgumentLookup<'a> { + fn new(list: &'a [Node]) -> Self { + if list.len() > 20 { + Self::Map(list.iter().map(|arg| (&arg.name, arg)).collect()) + } else { + Self::List(list) + } + } + + fn by_name(&self, name: &ast::Name) -> Option<&'a Node> { + match self { + Self::Map(map) => map.get(name).copied(), + Self::List(list) => list.iter().find(|arg| arg.name == *name), + } + } +} + +/// Check if two field selections from the overlapping types are the same, so the fields can be merged. +fn same_name_and_arguments( + field_a: FieldSelection<'_>, + field_b: FieldSelection<'_>, +) -> Result<(), BuildError> { + // 2bi. fieldA and fieldB must have identical field names. + if field_a.field.name != field_b.field.name { + return Err(BuildError::ConflictingFieldName { + alias: field_a.field.response_key().clone(), + original_location: field_a.field.location(), + original_selection: field_a.coordinate(), + conflicting_location: field_b.field.location(), + conflicting_selection: field_b.coordinate(), + }); + } + + // 2bii. fieldA and fieldB must have identical sets of arguments. + let conflicting_field_argument = + |original_arg: Option<&Node>, + redefined_arg: Option<&Node>| { + debug_assert!( + original_arg.is_some() || redefined_arg.is_some(), + "a conflicting field argument error can only exist when at least one field has the argument", + ); + + // We can take the name from either one of the arguments as they are necessarily the same. + let arg = original_arg.or(redefined_arg).unwrap(); + + BuildError::ConflictingFieldArgument { + // field_a and field_b have the same name so we can use either one. + alias: field_b.field.name.clone(), + original_location: field_a.field.location(), + original_coordinate: field_a.coordinate().with_argument(arg.name.clone()), + original_value: original_arg.map(|arg| (*arg.value).clone()), + conflicting_location: field_b.field.location(), + conflicting_coordinate: field_b.coordinate().with_argument(arg.name.clone()), + conflicting_value: redefined_arg.map(|arg| (*arg.value).clone()), + } + }; + + // Check if fieldB provides the same argument names and values as fieldA (order-independent). + let self_args = ArgumentLookup::new(&field_a.field.arguments); + let other_args = ArgumentLookup::new(&field_b.field.arguments); + for arg in &field_a.field.arguments { + let Some(other_arg) = other_args.by_name(&arg.name) else { + return Err(conflicting_field_argument(Some(arg), None)); + }; + + if !same_value(&other_arg.value, &arg.value) { + return Err(conflicting_field_argument(Some(arg), Some(other_arg))); + } + } + // Check if fieldB provides any arguments that fieldA does not provide. + for arg in &field_b.field.arguments { + if self_args.by_name(&arg.name).is_none() { + return Err(conflicting_field_argument(None, Some(arg))); + }; + } + + Ok(()) +} + +/// Compare two input values, with two special cases for objects: assuming no duplicate keys, +/// and order-independence. +fn same_value(left: &ast::Value, right: &ast::Value) -> bool { + match (left, right) { + (ast::Value::Null, ast::Value::Null) => true, + (ast::Value::Enum(left), ast::Value::Enum(right)) => left == right, + (ast::Value::Variable(left), ast::Value::Variable(right)) => left == right, + (ast::Value::String(left), ast::Value::String(right)) => left == right, + (ast::Value::Float(left), ast::Value::Float(right)) => left == right, + (ast::Value::Int(left), ast::Value::Int(right)) => left == right, + (ast::Value::Boolean(left), ast::Value::Boolean(right)) => left == right, + (ast::Value::List(left), ast::Value::List(right)) => left + .iter() + .zip(right.iter()) + .all(|(left, right)| same_value(left, right)), + (ast::Value::Object(left), ast::Value::Object(right)) if left.len() == right.len() => { + // This check could miss out on keys that exist in `right`, but not in `left`, if `left` contains duplicate keys. + // We assume that that doesn't happen. GraphQL does not support duplicate keys and + // that is checked elsewhere in validation. + left.iter().all(|(key, value)| { + right + .iter() + .find(|(other_key, _)| key == other_key) + .is_some_and(|(_, other_value)| same_value(value, other_value)) }) - .collect() + } + _ => false, } - inner( - named_fragments, - &mut Default::default(), - against_type, - selections, - ) } -/// Check if two fields will output the same type. -/// -/// If the two fields output different types, returns an `Err` containing diagnostic information. -/// To simply check if outputs are the same, you can use `.is_ok()`: -/// ```rust,ignore -/// let is_same = same_response_shape(db, field_a, field_b).is_ok(); -/// // `is_same` is `bool` -/// ``` -/// -/// Spec: https://spec.graphql.org/October2021/#SameResponseShape() -pub(crate) fn same_response_shape( - db: &dyn ValidationDatabase, - file_id: FileId, - field_a: FieldAgainstType<'_>, - field_b: FieldAgainstType<'_>, -) -> Result<(), ValidationError> { - let schema = db.schema(); - // 1. Let typeA be the return type of fieldA. - let Ok(full_type_a) = schema.type_field(field_a.against_type, &field_a.field.name) else { - return Ok(()); // Can't do much if we don't know the type - }; - let mut type_a = &full_type_a.ty; - // 2. Let typeB be the return type of fieldB. - let Ok(full_type_b) = schema.type_field(field_b.against_type, &field_b.field.name) else { - return Ok(()); // Can't do much if we don't know the type - }; - let mut type_b = &full_type_b.ty; - - let mismatching_type_diagnostic = || { - ValidationError::new( - field_b.field.location(), - DiagnosticData::ConflictingFieldType { - field: field_a.field.response_name().clone(), - original_selection: field_a.field.location(), - original_type: full_type_a.ty.clone(), - redefined_selection: field_b.field.location(), - redefined_type: full_type_b.ty.clone(), - }, - ) +fn same_output_type_shape( + schema: &schema::Schema, + selection_a: FieldSelection<'_>, + selection_b: FieldSelection<'_>, +) -> Result<(), BuildError> { + let field_a = &selection_a.field.definition; + let field_b = &selection_b.field.definition; + + let mut type_a = &field_a.ty; + let mut type_b = &field_b.ty; + + let mismatching_type_diagnostic = || BuildError::ConflictingFieldType { + alias: selection_a.field.response_key().clone(), + original_location: selection_a.field.location(), + original_coordinate: selection_a.coordinate(), + original_type: field_a.ty.clone(), + conflicting_location: selection_b.field.location(), + conflicting_coordinate: selection_b.coordinate(), + conflicting_type: field_b.ty.clone(), }; // Steps 3 and 4 of the spec text unwrap both types simultaneously down to the named type. @@ -145,11 +242,6 @@ pub(crate) fn same_response_shape( return Ok(()); // Cannot do much if we don't know the type }; - fn is_composite(ty: &schema::ExtendedType) -> bool { - type T = schema::ExtendedType; - matches!(ty, T::Object(_) | T::Interface(_) | T::Union(_)) - } - match (def_a, def_b) { // 5. If typeA or typeB is Scalar or Enum. ( @@ -163,205 +255,286 @@ pub(crate) fn same_response_shape( Err(mismatching_type_diagnostic()) } } - // 6. Assert: typeA and typeB are both composite types. - (def_a, def_b) if is_composite(def_a) && is_composite(def_b) => { - let Ok(subfield_a_type) = schema.type_field(field_a.against_type, &field_a.field.name) - else { - // Missing field: error raised elsewhere, we can't check if the type is correct - return Ok(()); - }; - let Ok(subfield_b_type) = schema.type_field(field_b.against_type, &field_b.field.name) - else { - // Missing field: error raised elsewhere, we can't check if the type is correct - return Ok(()); + // 6. If typeA or typeB is not a composite type, return false. + (def_a, def_b) if is_composite(def_a) && is_composite(def_b) => Ok(()), + _ => Err(mismatching_type_diagnostic()), + } +} + +/// A boolean that turns on after the first check. +struct OnceBool(std::cell::Cell); +impl OnceBool { + fn new() -> Self { + Self(false.into()) + } + + /// Returns `false` the first time it is called, then returns `true` forever. + fn already_done(&self) -> bool { + self.0.replace(true) + } +} + +/// Represents a merged field set that may or may not be valid. +struct MergedFieldSet<'doc> { + selections: Vec>, + grouped_by_output_names: OnceCell>>>, + grouped_by_common_parents: OnceCell>>>, + same_response_shape_guard: OnceBool, + same_for_common_parents_guard: OnceBool, +} + +impl<'doc> MergedFieldSet<'doc> { + fn new(selections: Vec>) -> Self { + Self { + selections, + grouped_by_output_names: Default::default(), + grouped_by_common_parents: Default::default(), + same_response_shape_guard: OnceBool::new(), + same_for_common_parents_guard: OnceBool::new(), + } + } + + /// Given a set of fields, do all the fields that contribute to 1 output name have the same + /// shape? + /// + /// This prevents leaf output fields from having an inconsistent type. + fn same_response_shape_by_name( + &self, + validator: &mut FieldsInSetCanMerge<'_, 'doc>, + diagnostics: &mut DiagnosticList, + ) { + // No need to do this if this field set has been checked before. + if self.same_response_shape_guard.already_done() { + return; + } + + for fields_for_name in self.group_by_output_name().values() { + let Some((field_a, rest)) = fields_for_name.split_first() else { + continue; }; - let named_fragments = db.ast_named_fragments(file_id); - let mut merged_set = operation_fields( - &named_fragments, - &subfield_a_type.name, - &field_a.field.selection_set, - ); - merged_set.extend(operation_fields( - &named_fragments, - &subfield_b_type.name, - &field_b.field.selection_set, - )); - let grouped_by_name = group_fields_by_name(merged_set); - - for (_, fields_for_name) in grouped_by_name { - // 9. Given each pair of members subfieldA and subfieldB in fieldsForName: - let Some((subfield_a, rest)) = fields_for_name.split_first() else { + for field_b in rest { + // Covers steps 3-5 of the spec algorithm. + if let Err(err) = same_output_type_shape(validator.schema, *field_a, *field_b) { + diagnostics.push(field_b.field.location(), err); continue; - }; - for subfield_b in rest { - // 9a. If SameResponseShape(subfieldA, subfieldB) is false, return false. - same_response_shape(db, file_id, *subfield_a, *subfield_b)?; } } - Ok(()) + let mut nested_selection_sets = fields_for_name + .iter() + .map(|selection| &selection.field.selection_set) + .filter(|set| !set.selections.is_empty()) + .peekable(); + if nested_selection_sets.peek().is_some() { + let merged_set = + expand_selections(&validator.document.fragments, nested_selection_sets); + validator.same_response_shape_by_name(merged_set, diagnostics); + } } - (_, _) => Ok(()), } -} -/// Given a list of fields, group them by response name. -fn group_fields_by_name( - fields: Vec>, -) -> HashMap>> { - let mut map = HashMap::>>::new(); - for field in fields { - match map.entry(field.field.response_name().clone()) { - Entry::Occupied(mut entry) => { - entry.get_mut().push(field); - } - Entry::Vacant(entry) => { - entry.insert(vec![field]); + /// Given a set of fields, do all the fields selecting from potentially overlapping types + /// select from the same thing? + /// + /// This prevents selecting two different fields from the same type into the same name. That + /// would be a contradiction because there would be no way to know which field takes precedence. + fn same_for_common_parents_by_name( + &self, + validator: &mut FieldsInSetCanMerge<'_, 'doc>, + diagnostics: &mut DiagnosticList, + ) { + // No need to do this if this field set has been checked before. + if self.same_for_common_parents_guard.already_done() { + return; + } + + for fields_for_name in self.group_by_output_name().values() { + let selection_for_name = validator.lookup(fields_for_name.clone()); + for fields_for_parents in selection_for_name.group_by_common_parents(validator.schema) { + // 2bi. fieldA and fieldB must have identical field names. + // 2bii. fieldA and fieldB must have identical sets of arguments. + // The same arguments check is reflexive so we don't need to check all + // combinations. + let Some((field_a, rest)) = fields_for_parents.split_first() else { + continue; + }; + for field_b in rest { + if let Err(diagnostic) = same_name_and_arguments(*field_a, *field_b) { + diagnostics.push(field_b.field.location(), diagnostic); + continue; + } + } + + let mut nested_selection_sets = fields_for_parents + .iter() + .map(|selection| &selection.field.selection_set) + .filter(|set| !set.selections.is_empty()) + .peekable(); + if nested_selection_sets.peek().is_some() { + let merged_set = + expand_selections(&validator.document.fragments, nested_selection_sets); + validator.same_for_common_parents_by_name(merged_set, diagnostics); + } } } } - map -} - -/// Check if the arguments provided to two fields are the same, so the fields can be merged. -fn identical_arguments( - field_a: &Node, - field_b: &Node, -) -> Result<(), ValidationError> { - let args_a = &field_a.arguments; - let args_b = &field_b.arguments; - let loc_a = field_a.location(); - let loc_b = field_b.location(); + fn group_by_output_name(&self) -> &IndexMap>> { + self.grouped_by_output_names.get_or_init(|| { + let mut map = IndexMap::<_, Vec<_>>::new(); + for selection in &self.selections { + map.entry(selection.field.response_key().clone()) + .or_default() + .push(*selection); + } + map + }) + } - // Check if fieldB provides the same argument names and values as fieldA (order-independent). - for arg in args_a { - let Some(other_arg) = args_b.iter().find(|other_arg| other_arg.name == arg.name) else { - return Err(ValidationError::new( - loc_b, - DiagnosticData::ConflictingFieldArgument { - field: field_a.name.clone(), - argument: arg.name.clone(), - original_selection: loc_a, - original_value: Some((*arg.value).clone()), - redefined_selection: loc_b, - redefined_value: None, - }, - )); - }; + /// Returns potentially overlapping groups of fields. Fields overlap if they are selected from + /// the same concrete type or if they are selected from an abstract type (future schema changes + /// can make any abstract type overlap with any other type). + fn group_by_common_parents(&self, schema: &schema::Schema) -> &Vec>> { + self.grouped_by_common_parents.get_or_init(|| { + let mut abstract_parents = vec![]; + let mut concrete_parents = IndexMap::<_, Vec<_>>::new(); + for selection in &self.selections { + match schema.types.get(selection.parent_type) { + Some(schema::ExtendedType::Object(object)) => { + concrete_parents + .entry(object.name.clone()) + .or_default() + .push(*selection); + } + Some(schema::ExtendedType::Interface(_) | schema::ExtendedType::Union(_)) => { + abstract_parents.push(*selection); + } + _ => {} + } + } - if other_arg.value != arg.value { - return Err(ValidationError::new( - loc_b, - DiagnosticData::ConflictingFieldArgument { - field: field_a.name.clone(), - argument: arg.name.clone(), - original_selection: loc_a, - original_value: Some((*arg.value).clone()), - redefined_selection: loc_b, - redefined_value: Some((*other_arg.value).clone()), - }, - )); - } - } - // Check if fieldB provides any arguments that fieldA does not provide. - for arg in args_b { - if !args_a.iter().any(|other_arg| other_arg.name == arg.name) { - return Err(ValidationError::new( - loc_b, - DiagnosticData::ConflictingFieldArgument { - field: field_a.name.clone(), - argument: arg.name.clone(), - original_selection: loc_a, - original_value: None, - redefined_selection: loc_b, - redefined_value: Some((*arg.value).clone()), - }, - )); - }; + if concrete_parents.is_empty() { + vec![abstract_parents] + } else { + concrete_parents + .into_values() + .map(|mut group| { + group.extend(abstract_parents.iter().copied()); + group + }) + .collect() + } + }) } +} - Ok(()) +/// For use as a hash map key, avoiding a clone of a potentially large array into the key. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +struct FieldSelectionsId(u64); + +impl FieldSelectionsId { + fn new(selections: &[FieldSelection<'_>]) -> Self { + use std::collections::hash_map::DefaultHasher; + use std::hash::Hash; + use std::hash::Hasher; + + // We can use the unseeded default hasher because the output will be + // hashed again with a randomly seeded hasher and still lead to unpredictable output. + let mut hasher = DefaultHasher::new(); + selections.hash(&mut hasher); + Self(hasher.finish()) + } } -/// Check if the fields in a given selection set can be merged. +// The field depth in the field merging validation matches the nesting level in the resulting +// data. It makes sense to use the same limit as serde_json. +const FIELD_DEPTH_LIMIT: usize = 128; + +/// Implements the `FieldsInSetCanMerge()` validation. +/// https://spec.graphql.org/draft/#sec-Field-Selection-Merging /// -/// If the fields cannot be merged, returns an `Err` containing diagnostic information. +/// This uses the [validation algorithm described by XING][0] ([archived][1]), which +/// scales much better with larger selection sets that may have many overlapping fields, +/// and with widespread use of fragments. /// -/// Spec: https://spec.graphql.org/October2021/#FieldsInSetCanMerge() -pub(crate) fn fields_in_set_can_merge( - db: &dyn ValidationDatabase, - file_id: FileId, - named_fragments: &HashMap>, - against_type: &ast::NamedType, - selection_set: &[ast::Selection], -) -> Result<(), Vec> { - let schema = db.schema(); +/// [0]: https://tech.new-work.se/graphql-overlapping-fields-can-be-merged-fast-ea6e92e0a01 +/// [1]: https://web.archive.org/web/20240208084612/https://tech.new-work.se/graphql-overlapping-fields-can-be-merged-fast-ea6e92e0a01 +pub(crate) struct FieldsInSetCanMerge<'s, 'doc> { + schema: &'s schema::Schema, + document: &'doc executable::ExecutableDocument, + /// Stores merged field sets. + /// + /// The value is an Rc because it needs to have an independent lifetime from `self`, + /// so the cache can be updated while a field set is borrowed. + cache: HashMap>>, + // The recursion limit is used for two separate recursions, but they are not interleaved, + // so the effective limit does apply to field nesting levels in both cases. + recursion_limit: LimitTracker, +} - // 1. Let `fieldsForName` be the set of selections with a given response name in set including visiting fragments and inline fragments. - let fields = operation_fields(named_fragments, against_type, selection_set); - let grouped_by_name = group_fields_by_name(fields); +impl<'s, 'doc> FieldsInSetCanMerge<'s, 'doc> { + pub(crate) fn new( + schema: &'s schema::Schema, + document: &'doc executable::ExecutableDocument, + ) -> Self { + Self { + schema, + document, + cache: Default::default(), + recursion_limit: LimitTracker::new(FIELD_DEPTH_LIMIT), + } + } - let mut diagnostics = vec![]; + pub(crate) fn validate_operation( + &mut self, + operation: &'doc Node, + diagnostics: &mut DiagnosticList, + ) { + let fields = expand_selections( + &self.document.fragments, + std::iter::once(&operation.selection_set), + ); + let set = self.lookup(fields); + set.same_response_shape_by_name(self, diagnostics); + set.same_for_common_parents_by_name(self, diagnostics); + + if self.recursion_limit.high > self.recursion_limit.limit { + diagnostics.push(operation.location(), super::Details::RecursionLimitError); + } + } - for (_, fields_for_name) in grouped_by_name { - let Some((field_a, rest)) = fields_for_name.split_first() else { - continue; // Nothing to merge - }; - let Ok(parent_a) = schema.type_field(field_a.against_type, &field_a.field.name) else { - continue; // Can't do much if we don't know the type - }; + fn lookup(&mut self, selections: Vec>) -> Rc> { + let id = FieldSelectionsId::new(&selections); + self.cache + .entry(id) + .or_insert_with(|| Rc::new(MergedFieldSet::new(selections))) + .clone() + } - // 2. Given each pair of members fieldA and fieldB in fieldsForName: - for field_b in rest { - // 2a. SameResponseShape(fieldA, fieldB) must be true. - if let Err(diagnostic) = same_response_shape(db, file_id, *field_a, *field_b) { - diagnostics.push(diagnostic); - continue; - } - // 2b. If the parent types of fieldA and fieldB are equal or if either is not an Object Type: - if field_a.against_type == field_b.against_type { - // 2bi. fieldA and fieldB must have identical field names. - if field_a.field.name != field_b.field.name { - diagnostics.push(ValidationError::new( - field_b.field.location(), - DiagnosticData::ConflictingFieldName { - field: field_a.field.response_name().clone(), - original_selection: field_a.field.location(), - original_name: field_a.field.name.clone(), - redefined_selection: field_b.field.location(), - redefined_name: field_b.field.name.clone(), - }, - )); - continue; - } - // 2bii. fieldA and fieldB must have identical sets of arguments. - if let Err(diagnostic) = identical_arguments(field_a.field, field_b.field) { - diagnostics.push(diagnostic); - continue; - } - // 2biii. Let mergedSet be the result of adding the selection set of fieldA and the selection set of fieldB. - let mut merged_set = field_a.field.selection_set.clone(); - merged_set.extend_from_slice(&field_b.field.selection_set); - // 2biv. FieldsInSetCanMerge(mergedSet) must be true. - if let Err(sub_diagnostics) = fields_in_set_can_merge( - db, - file_id, - named_fragments, - parent_a.ty.inner_named_type(), - &merged_set, - ) { - diagnostics.extend(sub_diagnostics); - continue; - } - } + fn same_for_common_parents_by_name( + &mut self, + selections: Vec>, + diagnostics: &mut DiagnosticList, + ) { + if self.recursion_limit.check_and_increment() { + return; } + let field_set = self.lookup(selections); + field_set.same_for_common_parents_by_name(self, diagnostics); + self.recursion_limit.decrement(); } - if diagnostics.is_empty() { - Ok(()) - } else { - Err(diagnostics) + fn same_response_shape_by_name( + &mut self, + selections: Vec>, + diagnostics: &mut DiagnosticList, + ) { + if self.recursion_limit.check_and_increment() { + return; + } + let field_set = self.lookup(selections); + field_set.same_response_shape_by_name(self, diagnostics); + self.recursion_limit.decrement(); } } @@ -374,19 +547,6 @@ pub(crate) fn validate_selection_set( ) -> Vec { let mut diagnostics = vec![]; - let named_fragments = Some(db.ast_named_fragments(file_id)); - // `named_fragments` will be None if we have 0 fields, where this validation is irrelevant - // anyways. - // If `against_type` is None, we don't know the actual type of anything, so we cannot run this - // validation. - if let (Some(named_fragments), Some(against_type)) = (named_fragments, against_type) { - if let Err(diagnostic) = - fields_in_set_can_merge(db, file_id, &named_fragments, against_type, selection_set) - { - diagnostics.extend(diagnostic); - } - } - diagnostics.extend(validate_selections( db, file_id, diff --git a/crates/apollo-compiler/test_data/diagnostics/0074_merge_identical_fields.txt b/crates/apollo-compiler/test_data/diagnostics/0074_merge_identical_fields.txt index 587e4746..15d16eca 100644 --- a/crates/apollo-compiler/test_data/diagnostics/0074_merge_identical_fields.txt +++ b/crates/apollo-compiler/test_data/diagnostics/0074_merge_identical_fields.txt @@ -1,41 +1,45 @@ -Error: operation must not select different types using the same field name `name` +Error: operation must not select different types using the same name `name` ╭─[0074_merge_identical_fields.graphql:19:3] β”‚ 18 β”‚ name: nickname β”‚ ───────┬────── - β”‚ ╰──────── `name` has type `String` here + β”‚ ╰──────── `name` is selected from `Dog.nickname: String` here 19 β”‚ name β”‚ ──┬─ - β”‚ ╰─── but the same field name has type `String!` here + β”‚ ╰─── `name` is selected from `Dog.name: String!` here ────╯ -Error: operation must not select different types using the same field name `name` +Error: cannot select different fields into the same alias `name` ╭─[0074_merge_identical_fields.graphql:19:3] β”‚ 18 β”‚ name: nickname β”‚ ───────┬────── - β”‚ ╰──────── `name` has type `String` here + β”‚ ╰──────── `name` is selected from `Dog.nickname` here 19 β”‚ name β”‚ ──┬─ - β”‚ ╰─── but the same field name has type `String!` here + β”‚ ╰─── `name` is selected from `Dog.name` here + β”‚ + β”‚ Help: Both fields may be present on the schema type, so it's not clear which one should be used to fill the response ────╯ -Error: operation must not select different types using the same field name `fido` +Error: operation must not select different types using the same name `fido` ╭─[0074_merge_identical_fields.graphql:24:3] β”‚ 23 β”‚ fido: name β”‚ ─────┬──── - β”‚ ╰────── `fido` has type `String!` here + β”‚ ╰────── `fido` is selected from `Dog.name: String!` here 24 β”‚ fido: nickname β”‚ ───────┬────── - β”‚ ╰──────── but the same field name has type `String` here + β”‚ ╰──────── `fido` is selected from `Dog.nickname: String` here ────╯ -Error: operation must not select different types using the same field name `fido` +Error: cannot select different fields into the same alias `fido` ╭─[0074_merge_identical_fields.graphql:24:3] β”‚ 23 β”‚ fido: name β”‚ ─────┬──── - β”‚ ╰────── `fido` has type `String!` here + β”‚ ╰────── `fido` is selected from `Dog.name` here 24 β”‚ fido: nickname β”‚ ───────┬────── - β”‚ ╰──────── but the same field name has type `String` here + β”‚ ╰──────── `fido` is selected from `Dog.nickname` here + β”‚ + β”‚ Help: Both fields may be present on the schema type, so it's not clear which one should be used to fill the response ────╯ diff --git a/crates/apollo-compiler/test_data/diagnostics/0075_merge_conflicting_args.txt b/crates/apollo-compiler/test_data/diagnostics/0075_merge_conflicting_args.txt index 41588a57..e454ec53 100644 --- a/crates/apollo-compiler/test_data/diagnostics/0075_merge_conflicting_args.txt +++ b/crates/apollo-compiler/test_data/diagnostics/0075_merge_conflicting_args.txt @@ -1,121 +1,61 @@ -Error: operation must not provide conflicting field arguments for the same field name `doesKnowCommand` +Error: operation must not provide conflicting field arguments for the same name `doesKnowCommand` ╭─[0075_merge_conflicting_args.graphql:47:3] β”‚ 46 β”‚ doesKnowCommand(dogCommand: SIT) β”‚ ────────────────┬─────────────── - β”‚ ╰───────────────── field `doesKnowCommand` provides one argument value here + β”‚ ╰───────────────── `Dog.doesKnowCommand(dogCommand:)` is used with one argument value here 47 β”‚ doesKnowCommand(dogCommand: HEEL) β”‚ ────────────────┬──────────────── β”‚ ╰────────────────── but a different value here β”‚ - β”‚ Help: Fields with the same response name must provide the same set of arguments. Consider adding an alias if you need to select fields with different arguments. + β”‚ Help: The same name cannot be selected multiple times with different arguments, because it's not clear which set of arguments should be used to fill the response. If you intend to use diverging arguments, consider adding an alias to differentiate ────╯ -Error: operation must not provide conflicting field arguments for the same field name `doesKnowCommand` - ╭─[0075_merge_conflicting_args.graphql:47:3] - β”‚ - 46 β”‚ doesKnowCommand(dogCommand: SIT) - β”‚ ────────────────┬─────────────── - β”‚ ╰───────────────── field `doesKnowCommand` provides one argument value here - 47 β”‚ doesKnowCommand(dogCommand: HEEL) - β”‚ ────────────────┬──────────────── - β”‚ ╰────────────────── but a different value here - β”‚ - β”‚ Help: Fields with the same response name must provide the same set of arguments. Consider adding an alias if you need to select fields with different arguments. -────╯ -Error: operation must not provide conflicting field arguments for the same field name `doesKnowCommand` +Error: operation must not provide conflicting field arguments for the same name `doesKnowCommand` ╭─[0075_merge_conflicting_args.graphql:52:3] β”‚ 51 β”‚ doesKnowCommand(dogCommand: SIT) β”‚ ────────────────┬─────────────── - β”‚ ╰───────────────── field `doesKnowCommand` provides one argument value here + β”‚ ╰───────────────── `Dog.doesKnowCommand(dogCommand:)` is used with one argument value here 52 β”‚ doesKnowCommand(dogCommand: $dogCommand) β”‚ ────────────────────┬─────────────────── β”‚ ╰───────────────────── but a different value here β”‚ - β”‚ Help: Fields with the same response name must provide the same set of arguments. Consider adding an alias if you need to select fields with different arguments. + β”‚ Help: The same name cannot be selected multiple times with different arguments, because it's not clear which set of arguments should be used to fill the response. If you intend to use diverging arguments, consider adding an alias to differentiate ────╯ -Error: operation must not provide conflicting field arguments for the same field name `doesKnowCommand` - ╭─[0075_merge_conflicting_args.graphql:52:3] - β”‚ - 51 β”‚ doesKnowCommand(dogCommand: SIT) - β”‚ ────────────────┬─────────────── - β”‚ ╰───────────────── field `doesKnowCommand` provides one argument value here - 52 β”‚ doesKnowCommand(dogCommand: $dogCommand) - β”‚ ────────────────────┬─────────────────── - β”‚ ╰───────────────────── but a different value here - β”‚ - β”‚ Help: Fields with the same response name must provide the same set of arguments. Consider adding an alias if you need to select fields with different arguments. -────╯ -Error: operation must not provide conflicting field arguments for the same field name `doesKnowCommand` +Error: operation must not provide conflicting field arguments for the same name `doesKnowCommand` ╭─[0075_merge_conflicting_args.graphql:57:3] β”‚ 56 β”‚ doesKnowCommand(dogCommand: $varOne) β”‚ ──────────────────┬───────────────── - β”‚ ╰─────────────────── field `doesKnowCommand` provides one argument value here + β”‚ ╰─────────────────── `Dog.doesKnowCommand(dogCommand:)` is used with one argument value here 57 β”‚ doesKnowCommand(dogCommand: $varTwo) β”‚ ──────────────────┬───────────────── β”‚ ╰─────────────────── but a different value here β”‚ - β”‚ Help: Fields with the same response name must provide the same set of arguments. Consider adding an alias if you need to select fields with different arguments. + β”‚ Help: The same name cannot be selected multiple times with different arguments, because it's not clear which set of arguments should be used to fill the response. If you intend to use diverging arguments, consider adding an alias to differentiate ────╯ -Error: operation must not provide conflicting field arguments for the same field name `doesKnowCommand` - ╭─[0075_merge_conflicting_args.graphql:57:3] - β”‚ - 56 β”‚ doesKnowCommand(dogCommand: $varOne) - β”‚ ──────────────────┬───────────────── - β”‚ ╰─────────────────── field `doesKnowCommand` provides one argument value here - 57 β”‚ doesKnowCommand(dogCommand: $varTwo) - β”‚ ──────────────────┬───────────────── - β”‚ ╰─────────────────── but a different value here - β”‚ - β”‚ Help: Fields with the same response name must provide the same set of arguments. Consider adding an alias if you need to select fields with different arguments. -────╯ -Error: operation must not provide conflicting field arguments for the same field name `doesKnowCommand` - ╭─[0075_merge_conflicting_args.graphql:62:3] - β”‚ - 61 β”‚ doesKnowCommand(dogCommand: SIT) - β”‚ ────────────────┬─────────────── - β”‚ ╰───────────────── field `doesKnowCommand` is selected with argument `dogCommand` here - 62 β”‚ doesKnowCommand - β”‚ ───────┬─────── - β”‚ ╰───────── but argument `dogCommand` is not provided here - β”‚ - β”‚ Help: Fields with the same response name must provide the same set of arguments. Consider adding an alias if you need to select fields with different arguments. -────╯ -Error: operation must not provide conflicting field arguments for the same field name `doesKnowCommand` +Error: operation must not provide conflicting field arguments for the same name `doesKnowCommand` ╭─[0075_merge_conflicting_args.graphql:62:3] β”‚ 61 β”‚ doesKnowCommand(dogCommand: SIT) β”‚ ────────────────┬─────────────── - β”‚ ╰───────────────── field `doesKnowCommand` is selected with argument `dogCommand` here + β”‚ ╰───────────────── `doesKnowCommand` is selected with argument `dogCommand` here 62 β”‚ doesKnowCommand β”‚ ───────┬─────── β”‚ ╰───────── but argument `dogCommand` is not provided here β”‚ - β”‚ Help: Fields with the same response name must provide the same set of arguments. Consider adding an alias if you need to select fields with different arguments. -────╯ -Error: operation must not provide conflicting field arguments for the same field name `isAtLocation` - ╭─[0075_merge_conflicting_args.graphql:67:3] - β”‚ - 66 β”‚ isAtLocation(x: 0) - β”‚ ─────────┬──────── - β”‚ ╰────────── field `isAtLocation` is selected with argument `x` here - 67 β”‚ isAtLocation(y: 0) - β”‚ ─────────┬──────── - β”‚ ╰────────── but argument `x` is not provided here - β”‚ - β”‚ Help: Fields with the same response name must provide the same set of arguments. Consider adding an alias if you need to select fields with different arguments. + β”‚ Help: The same name cannot be selected multiple times with different arguments, because it's not clear which set of arguments should be used to fill the response. If you intend to use diverging arguments, consider adding an alias to differentiate ────╯ -Error: operation must not provide conflicting field arguments for the same field name `isAtLocation` +Error: operation must not provide conflicting field arguments for the same name `isAtLocation` ╭─[0075_merge_conflicting_args.graphql:67:3] β”‚ 66 β”‚ isAtLocation(x: 0) β”‚ ─────────┬──────── - β”‚ ╰────────── field `isAtLocation` is selected with argument `x` here + β”‚ ╰────────── `isAtLocation` is selected with argument `x` here 67 β”‚ isAtLocation(y: 0) β”‚ ─────────┬──────── β”‚ ╰────────── but argument `x` is not provided here β”‚ - β”‚ Help: Fields with the same response name must provide the same set of arguments. Consider adding an alias if you need to select fields with different arguments. + β”‚ Help: The same name cannot be selected multiple times with different arguments, because it's not clear which set of arguments should be used to fill the response. If you intend to use diverging arguments, consider adding an alias to differentiate ────╯ diff --git a/crates/apollo-compiler/test_data/diagnostics/0076_merge_differing_responses.txt b/crates/apollo-compiler/test_data/diagnostics/0076_merge_differing_responses.txt index 6e34797a..a33b5964 100644 --- a/crates/apollo-compiler/test_data/diagnostics/0076_merge_differing_responses.txt +++ b/crates/apollo-compiler/test_data/diagnostics/0076_merge_differing_responses.txt @@ -1,23 +1,12 @@ -Error: operation must not select different types using the same field name `someValue` +Error: operation must not select different types using the same name `someValue` ╭─[0076_merge_differing_responses.graphql:42:5] β”‚ 39 β”‚ someValue: nickname β”‚ ─────────┬───────── - β”‚ ╰─────────── `someValue` has type `String!` here + β”‚ ╰─────────── `someValue` is selected from `Dog.nickname: String!` here β”‚ 42 β”‚ someValue: meowVolume β”‚ ──────────┬────────── - β”‚ ╰──────────── but the same field name has type `Int!` here -────╯ -Error: operation must not select different types using the same field name `someValue` - ╭─[0076_merge_differing_responses.graphql:42:5] - β”‚ - 39 β”‚ someValue: nickname - β”‚ ─────────┬───────── - β”‚ ╰─────────── `someValue` has type `String!` here - β”‚ - 42 β”‚ someValue: meowVolume - β”‚ ──────────┬────────── - β”‚ ╰──────────── but the same field name has type `Int!` here + β”‚ ╰──────────── `someValue` is selected from `Cat.meowVolume: Int!` here ────╯ diff --git a/crates/apollo-compiler/test_data/diagnostics/0077_merge_conflict_deep.txt b/crates/apollo-compiler/test_data/diagnostics/0077_merge_conflict_deep.txt index b5fd3782..f1f7f163 100644 --- a/crates/apollo-compiler/test_data/diagnostics/0077_merge_conflict_deep.txt +++ b/crates/apollo-compiler/test_data/diagnostics/0077_merge_conflict_deep.txt @@ -1,12 +1,14 @@ -Error: operation must not select different fields to the same alias `x` +Error: cannot select different fields into the same alias `x` ╭─[0077_merge_conflict_deep.graphql:14:5] β”‚ 11 β”‚ x: a β”‚ ──┬─ - β”‚ ╰─── field `x` is selected from field `a` here + β”‚ ╰─── `x` is selected from `O.a` here β”‚ 14 β”‚ x: b β”‚ ──┬─ - β”‚ ╰─── but the same field `x` is also selected from field `b` here + β”‚ ╰─── `x` is selected from `O.b` here + β”‚ + β”‚ Help: Both fields may be present on the schema type, so it's not clear which one should be used to fill the response ────╯ diff --git a/crates/apollo-compiler/test_data/diagnostics/0078_merge_conflict_nested_fragments.txt b/crates/apollo-compiler/test_data/diagnostics/0078_merge_conflict_nested_fragments.txt index 2e8eb5f0..3ac73017 100644 --- a/crates/apollo-compiler/test_data/diagnostics/0078_merge_conflict_nested_fragments.txt +++ b/crates/apollo-compiler/test_data/diagnostics/0078_merge_conflict_nested_fragments.txt @@ -1,23 +1,27 @@ -Error: operation must not select different fields to the same alias `y` - ╭─[0078_merge_conflict_nested_fragments.graphql:28:3] +Error: cannot select different fields into the same alias `y` + ╭─[0078_merge_conflict_nested_fragments.graphql:25:3] β”‚ 25 β”‚ y: c β”‚ ──┬─ - β”‚ ╰─── field `y` is selected from field `c` here + β”‚ ╰─── `y` is selected from `T.c` here β”‚ 28 β”‚ y: d β”‚ ──┬─ - β”‚ ╰─── but the same field `y` is also selected from field `d` here + β”‚ ╰─── `y` is selected from `T.d` here + β”‚ + β”‚ Help: Both fields may be present on the schema type, so it's not clear which one should be used to fill the response ────╯ -Error: operation must not select different fields to the same alias `x` +Error: cannot select different fields into the same alias `x` ╭─[0078_merge_conflict_nested_fragments.graphql:32:3] β”‚ 21 β”‚ x: a β”‚ ──┬─ - β”‚ ╰─── field `x` is selected from field `a` here + β”‚ ╰─── `x` is selected from `T.a` here β”‚ 32 β”‚ x: b β”‚ ──┬─ - β”‚ ╰─── but the same field `x` is also selected from field `b` here + β”‚ ╰─── `x` is selected from `T.b` here + β”‚ + β”‚ Help: Both fields may be present on the schema type, so it's not clear which one should be used to fill the response ────╯ diff --git a/crates/apollo-compiler/test_data/diagnostics/0113_partially_overlapping_fragments.graphql b/crates/apollo-compiler/test_data/diagnostics/0113_partially_overlapping_fragments.graphql new file mode 100644 index 00000000..0def0ea5 --- /dev/null +++ b/crates/apollo-compiler/test_data/diagnostics/0113_partially_overlapping_fragments.graphql @@ -0,0 +1,29 @@ +type A { + unrelated: String + overlapping: Int +} +type B { + overlapping: Int! +} +type C { + overlapping: Int +} +union Union = A | B | C +type Query { + root: Union +} + +# There is a conflicting type due to `B.overlapping` and `C.overlapping` both being selected +# to `root.overlapping`, but neither overlap with the initial selection set for `root`. + +{ + root { + ... on A { unrelated } + } + root { + ... on B { overlapping } + } + root { + ... on C { overlapping } + } +} diff --git a/crates/apollo-compiler/test_data/diagnostics/0113_partially_overlapping_fragments.txt b/crates/apollo-compiler/test_data/diagnostics/0113_partially_overlapping_fragments.txt new file mode 100644 index 00000000..0c38fc60 --- /dev/null +++ b/crates/apollo-compiler/test_data/diagnostics/0113_partially_overlapping_fragments.txt @@ -0,0 +1,12 @@ +Error: operation must not select different types using the same name `overlapping` + ╭─[0113_partially_overlapping_fragments.graphql:27:16] + β”‚ + 24 β”‚ ... on B { overlapping } + β”‚ ─────┬───── + β”‚ ╰─────── `overlapping` is selected from `B.overlapping: Int!` here + β”‚ + 27 β”‚ ... on C { overlapping } + β”‚ ─────┬───── + β”‚ ╰─────── `overlapping` is selected from `C.overlapping: Int` here +────╯ + diff --git a/crates/apollo-compiler/test_data/serializer/diagnostics/0113_partially_overlapping_fragments.graphql b/crates/apollo-compiler/test_data/serializer/diagnostics/0113_partially_overlapping_fragments.graphql new file mode 100644 index 00000000..db59b1d8 --- /dev/null +++ b/crates/apollo-compiler/test_data/serializer/diagnostics/0113_partially_overlapping_fragments.graphql @@ -0,0 +1,36 @@ +type A { + unrelated: String + overlapping: Int +} + +type B { + overlapping: Int! +} + +type C { + overlapping: Int +} + +union Union = A | B | C + +type Query { + root: Union +} + +query { + root { + ... on A { + unrelated + } + } + root { + ... on B { + overlapping + } + } + root { + ... on C { + overlapping + } + } +} diff --git a/crates/apollo-compiler/tests/validation/field_merging.rs b/crates/apollo-compiler/tests/validation/field_merging.rs new file mode 100644 index 00000000..9c73f144 --- /dev/null +++ b/crates/apollo-compiler/tests/validation/field_merging.rs @@ -0,0 +1,1394 @@ +//! Ported from graphql-js, 2024-02-01 +//! https://github.com/graphql/graphql-js/blob/9c90a23dd430ba7b9db3d566b084e9f66aded346/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts +use apollo_compiler::validation::Valid; +use apollo_compiler::ExecutableDocument; +use apollo_compiler::Schema; +use expect_test::expect; +use expect_test::Expect; +use std::sync::OnceLock; +use unindent::unindent; + +const GRAPHQL_JS_TEST_SCHEMA: &str = r#" + interface Mammal { + mother: Mammal + father: Mammal + } + + interface Pet { + name(surname: Boolean): String + } + + interface Canine implements Mammal { + name(surname: Boolean): String + mother: Canine + father: Canine + } + + enum DogCommand { + SIT + HEEL + DOWN + } + + type Dog implements Pet & Mammal & Canine { + name(surname: Boolean): String + nickname: String + barkVolume: Int + barks: Boolean + doesKnowCommand(dogCommand: DogCommand): Boolean + isHouseTrained(atOtherHomes: Boolean = true): Boolean + isAtLocation(x: Int, y: Int): Boolean + mother: Dog + father: Dog + } + + type Cat implements Pet { + name(surname: Boolean): String + nickname: String + meows: Boolean + meowsVolume: Int + furColor: FurColor + } + + union CatOrDog = Cat | Dog + + type Human { + name(surname: Boolean): String + pets: [Pet] + relatives: [Human]! + } + + enum FurColor { + BROWN + BLACK + TAN + SPOTTED + NO_FUR + UNKNOWN + } + + input ComplexInput { + requiredField: Boolean! + nonNullField: Boolean! = false + intField: Int + stringField: String + booleanField: Boolean + stringListField: [String] + } + + # TODO oneOf not supported in apollo-rs + input OneOfInput { # @oneOf + stringField: String + intField: Int + } + + type ComplicatedArgs { + # TODO List + # TODO Coercion + # TODO NotNulls + intArgField(intArg: Int): String + nonNullIntArgField(nonNullIntArg: Int!): String + stringArgField(stringArg: String): String + booleanArgField(booleanArg: Boolean): String + enumArgField(enumArg: FurColor): String + floatArgField(floatArg: Float): String + idArgField(idArg: ID): String + stringListArgField(stringListArg: [String]): String + stringListNonNullArgField(stringListNonNullArg: [String!]): String + complexArgField(complexArg: ComplexInput): String + oneOfArgField(oneOfArg: OneOfInput): String + multipleReqs(req1: Int!, req2: Int!): String + nonNullFieldWithDefault(arg: Int! = 0): String + multipleOpts(opt1: Int = 0, opt2: Int = 0): String + multipleOptAndReq(req1: Int!, req2: Int!, opt1: Int = 0, opt2: Int = 0): String + } + + type QueryRoot { + human(id: ID): Human + dog: Dog + cat: Cat + pet: Pet + catOrDog: CatOrDog + complicatedArgs: ComplicatedArgs + } + + schema { + query: QueryRoot + } + + directive @onField on FIELD +"#; + +fn test_schema() -> &'static Valid { + static SCHEMA: OnceLock> = OnceLock::new(); + + SCHEMA.get_or_init(|| { + Schema::parse_and_validate(unindent(GRAPHQL_JS_TEST_SCHEMA), "schema.graphql").unwrap() + }) +} + +#[track_caller] +fn expect_valid(query: &'static str) { + let schema = test_schema(); + + ExecutableDocument::parse_and_validate(schema, unindent(query), "query.graphql").unwrap(); +} + +fn expect_errors(query: &'static str, expect: Expect) { + let schema = test_schema(); + + let errors = ExecutableDocument::parse_and_validate(schema, unindent(query), "query.graphql") + .expect_err("should have errors") + .errors; + expect.assert_eq(&errors.to_string()); +} + +#[test] +fn unique_fields() { + expect_valid( + r#" + fragment uniqueFields on Dog { + name + nickname + } + + { dog { ...uniqueFields } } + "#, + ); +} + +#[test] +fn identical_fields() { + expect_valid( + r#" + fragment mergeIdenticalFields on Dog { + name + name + } + + { dog { ...mergeIdenticalFields } } + "#, + ); +} + +#[test] +fn identical_fields_with_identical_args() { + expect_valid( + r#" + fragment mergeIdenticalFieldsWithIdenticalArgs on Dog { + doesKnowCommand(dogCommand: SIT) + doesKnowCommand(dogCommand: SIT) + } + + { dog { ...mergeIdenticalFieldsWithIdenticalArgs } } + "#, + ); +} + +#[test] +fn identical_fields_with_identical_directives() { + expect_valid( + r#" + fragment mergeSameFieldsWithSameDirectives on Dog { + name @include(if: true) + name @include(if: true) + } + + { dog { ...mergeSameFieldsWithSameDirectives } } + "#, + ); +} + +#[test] +fn different_args_with_different_aliases() { + expect_valid( + r#" + fragment differentArgsWithDifferentAliases on Dog { + knowsSit: doesKnowCommand(dogCommand: SIT) + knowsDown: doesKnowCommand(dogCommand: DOWN) + } + + { dog { ...differentArgsWithDifferentAliases } } + "#, + ); +} + +#[test] +fn different_directives_with_different_aliases() { + expect_valid( + r#" + fragment differentDirectivesWithDifferentAliases on Dog { + nameIfTrue: name @include(if: true) + nameIfFalse: name @include(if: false) + } + + { dog { ...differentDirectivesWithDifferentAliases } } + "#, + ); +} + +#[test] +fn different_skip_include_directives() { + expect_valid( + r#" + fragment differentDirectivesWithDifferentAliases on Dog { + name @include(if: true) + name @include(if: false) + } + + { dog { ...differentDirectivesWithDifferentAliases } } + "#, + ); +} + +/* @stream tests snipped here -- not supported in apollo-rs */ + +#[test] +fn same_aliases_with_different_field_targets() { + expect_errors( + r#" + fragment sameAliasesWithDifferentFieldTargets on Dog { + fido: name + fido: nickname + } + + { dog { ...sameAliasesWithDifferentFieldTargets } } + "#, + expect![[r#" + Error: cannot select different fields into the same alias `fido` + ╭─[query.graphql:3:3] + β”‚ + 2 β”‚ fido: name + β”‚ ─────┬──── + β”‚ ╰────── `fido` is selected from `Dog.name` here + 3 β”‚ fido: nickname + β”‚ ───────┬────── + β”‚ ╰──────── `fido` is selected from `Dog.nickname` here + β”‚ + β”‚ Help: Both fields may be present on the schema type, so it's not clear which one should be used to fill the response + ───╯ + "#]], + ); +} + +#[test] +fn same_aliases_on_non_overlapping_fields() { + expect_valid( + r#" + fragment sameAliasesWithDifferentFieldTargets on Pet { + ... on Dog { + name + } + ... on Cat { + name: nickname + } + } + + { pet { ...sameAliasesWithDifferentFieldTargets } } + "#, + ); +} + +#[test] +fn alias_masking_direct_field_access() { + expect_errors( + r#" + fragment aliasMaskingDirectFieldAccess on Dog { + name: nickname + name + } + + { dog { ...aliasMaskingDirectFieldAccess } } + "#, + expect![[r#" + Error: cannot select different fields into the same alias `name` + ╭─[query.graphql:3:3] + β”‚ + 2 β”‚ name: nickname + β”‚ ───────┬────── + β”‚ ╰──────── `name` is selected from `Dog.nickname` here + 3 β”‚ name + β”‚ ──┬─ + β”‚ ╰─── `name` is selected from `Dog.name` here + β”‚ + β”‚ Help: Both fields may be present on the schema type, so it's not clear which one should be used to fill the response + ───╯ + "#]], + ); +} + +#[test] +fn different_args_second_adds_argument() { + expect_errors( + r#" + fragment conflictingArgs on Dog { + doesKnowCommand + doesKnowCommand(dogCommand: HEEL) + } + + { dog { ...conflictingArgs } } + "#, + expect![[r#" + Error: operation must not provide conflicting field arguments for the same name `doesKnowCommand` + ╭─[query.graphql:3:3] + β”‚ + 2 β”‚ doesKnowCommand + β”‚ ───────┬─────── + β”‚ ╰───────── but argument `dogCommand` is not provided here + 3 β”‚ doesKnowCommand(dogCommand: HEEL) + β”‚ ────────────────┬──────────────── + β”‚ ╰────────────────── `doesKnowCommand` is selected with argument `dogCommand` here + β”‚ + β”‚ Help: The same name cannot be selected multiple times with different arguments, because it's not clear which set of arguments should be used to fill the response. If you intend to use diverging arguments, consider adding an alias to differentiate + ───╯ + "#]], + ); +} + +#[test] +fn different_args_second_missess_argument() { + expect_errors( + r#" + fragment conflictingArgs on Dog { + doesKnowCommand(dogCommand: SIT) + doesKnowCommand + } + + { dog { ...conflictingArgs } } + "#, + expect![[r#" + Error: operation must not provide conflicting field arguments for the same name `doesKnowCommand` + ╭─[query.graphql:3:3] + β”‚ + 2 β”‚ doesKnowCommand(dogCommand: SIT) + β”‚ ────────────────┬─────────────── + β”‚ ╰───────────────── `doesKnowCommand` is selected with argument `dogCommand` here + 3 β”‚ doesKnowCommand + β”‚ ───────┬─────── + β”‚ ╰───────── but argument `dogCommand` is not provided here + β”‚ + β”‚ Help: The same name cannot be selected multiple times with different arguments, because it's not clear which set of arguments should be used to fill the response. If you intend to use diverging arguments, consider adding an alias to differentiate + ───╯ + "#]], + ); +} + +#[test] +fn conflicting_arg_values() { + expect_errors( + r#" + fragment conflictingArgs on Dog { + doesKnowCommand(dogCommand: SIT) + doesKnowCommand(dogCommand: HEEL) + } + + { dog { ...conflictingArgs } } + "#, + expect![[r#" + Error: operation must not provide conflicting field arguments for the same name `doesKnowCommand` + ╭─[query.graphql:3:3] + β”‚ + 2 β”‚ doesKnowCommand(dogCommand: SIT) + β”‚ ────────────────┬─────────────── + β”‚ ╰───────────────── `Dog.doesKnowCommand(dogCommand:)` is used with one argument value here + 3 β”‚ doesKnowCommand(dogCommand: HEEL) + β”‚ ────────────────┬──────────────── + β”‚ ╰────────────────── but a different value here + β”‚ + β”‚ Help: The same name cannot be selected multiple times with different arguments, because it's not clear which set of arguments should be used to fill the response. If you intend to use diverging arguments, consider adding an alias to differentiate + ───╯ + "#]], + ); +} + +#[test] +fn conflicting_arg_names() { + expect_errors( + r#" + fragment conflictingArgs on Dog { + isAtLocation(x: 0) + isAtLocation(y: 0) + } + + { dog { ...conflictingArgs } } + "#, + expect![[r#" + Error: operation must not provide conflicting field arguments for the same name `isAtLocation` + ╭─[query.graphql:3:3] + β”‚ + 2 β”‚ isAtLocation(x: 0) + β”‚ ─────────┬──────── + β”‚ ╰────────── `isAtLocation` is selected with argument `x` here + 3 β”‚ isAtLocation(y: 0) + β”‚ ─────────┬──────── + β”‚ ╰────────── but argument `x` is not provided here + β”‚ + β”‚ Help: The same name cannot be selected multiple times with different arguments, because it's not clear which set of arguments should be used to fill the response. If you intend to use diverging arguments, consider adding an alias to differentiate + ───╯ + "#]], + ); +} + +#[test] +fn different_non_conflicting_args() { + expect_valid( + r#" + fragment conflictingArgs on Pet { + ... on Dog { + name(surname: true) + } + ... on Cat { + name + } + } + + { pet { ...conflictingArgs } } + "#, + ); +} + +#[test] +fn different_order_args() { + expect_valid( + r#" + { + dog { + isAtLocation(x: 0, y: 1) + isAtLocation(y: 1, x: 0) + } + } + "#, + ); +} + +#[test] +fn different_order_input_args() { + expect_valid( + r#" + { + complicatedArgs { + complexArgField(complexArg: { intField: 1, requiredField: true }) + complexArgField(complexArg: { requiredField: true, intField: 1 }) + } + } + "#, + ); +} + +mod field_conflicts { + use apollo_compiler::validation::Valid; + use apollo_compiler::ExecutableDocument; + use apollo_compiler::Schema; + use expect_test::expect; + use expect_test::Expect; + use std::sync::OnceLock; + use unindent::unindent; + + const CONFLICT_TEST_SCHEMA: &str = r#" + type Type { + a: Int + b: Int + c: Int + d: Int + } + + type T { + a: Boolean + b: Boolean + c: Boolean + d: Boolean + + deepField: Type + } + + type Query { + f1: Type + f2: Type + f3: Type + + field: T + } + "#; + + fn test_schema() -> &'static Valid { + static SCHEMA: OnceLock> = OnceLock::new(); + + SCHEMA.get_or_init(|| { + Schema::parse_and_validate(unindent(CONFLICT_TEST_SCHEMA), "schema.graphql").unwrap() + }) + } + + fn expect_errors(query: &'static str, expect: Expect) { + let schema = test_schema(); + + let errors = + ExecutableDocument::parse_and_validate(schema, unindent(query), "query.graphql") + .expect_err("should have errors") + .errors; + expect.assert_eq(&errors.to_string()); + } + + #[test] + fn conflicts_in_fragments() { + expect_errors( + r#" + { + f1 { + ...A + ...B + } + } + fragment A on Type { + x: a + } + fragment B on Type { + x: b + } + "#, + expect![[r#" + Error: cannot select different fields into the same alias `x` + ╭─[query.graphql:11:3] + β”‚ + 8 β”‚ x: a + β”‚ ──┬─ + β”‚ ╰─── `x` is selected from `Type.a` here + β”‚ + 11 β”‚ x: b + β”‚ ──┬─ + β”‚ ╰─── `x` is selected from `Type.b` here + β”‚ + β”‚ Help: Both fields may be present on the schema type, so it's not clear which one should be used to fill the response + ────╯ + "#]], + ); + } + + #[test] + fn dedupe_conflicts() { + expect_errors( + r#" + { + f1 { + ...A + ...B + } + f2 { + ...B + ...A + } + f3 { + ...A + ...B + x: c + } + } + fragment A on Type { + x: a + } + fragment B on Type { + x: b + } + "#, + expect![[r#" + Error: cannot select different fields into the same alias `x` + ╭─[query.graphql:17:3] + β”‚ + 17 β”‚ x: a + β”‚ ──┬─ + β”‚ ╰─── `x` is selected from `Type.a` here + β”‚ + 20 β”‚ x: b + β”‚ ──┬─ + β”‚ ╰─── `x` is selected from `Type.b` here + β”‚ + β”‚ Help: Both fields may be present on the schema type, so it's not clear which one should be used to fill the response + ────╯ + Error: cannot select different fields into the same alias `x` + ╭─[query.graphql:17:3] + β”‚ + 13 β”‚ x: c + β”‚ ──┬─ + β”‚ ╰─── `x` is selected from `Type.c` here + β”‚ + 17 β”‚ x: a + β”‚ ──┬─ + β”‚ ╰─── `x` is selected from `Type.a` here + β”‚ + β”‚ Help: Both fields may be present on the schema type, so it's not clear which one should be used to fill the response + ────╯ + Error: cannot select different fields into the same alias `x` + ╭─[query.graphql:20:3] + β”‚ + 17 β”‚ x: a + β”‚ ──┬─ + β”‚ ╰─── `x` is selected from `Type.a` here + β”‚ + 20 β”‚ x: b + β”‚ ──┬─ + β”‚ ╰─── `x` is selected from `Type.b` here + β”‚ + β”‚ Help: Both fields may be present on the schema type, so it's not clear which one should be used to fill the response + ────╯ + Error: cannot select different fields into the same alias `x` + ╭─[query.graphql:20:3] + β”‚ + 13 β”‚ x: c + β”‚ ──┬─ + β”‚ ╰─── `x` is selected from `Type.c` here + β”‚ + 20 β”‚ x: b + β”‚ ──┬─ + β”‚ ╰─── `x` is selected from `Type.b` here + β”‚ + β”‚ Help: Both fields may be present on the schema type, so it's not clear which one should be used to fill the response + ────╯ + "#]], + ); + } + + #[test] + fn deep_conflict() { + expect_errors( + r#" + { + f1 { + x: a + }, + f1 { + x: b + } + } + "#, + expect![[r#" + Error: cannot select different fields into the same alias `x` + ╭─[query.graphql:6:5] + β”‚ + 3 β”‚ x: a + β”‚ ──┬─ + β”‚ ╰─── `x` is selected from `Type.a` here + β”‚ + 6 β”‚ x: b + β”‚ ──┬─ + β”‚ ╰─── `x` is selected from `Type.b` here + β”‚ + β”‚ Help: Both fields may be present on the schema type, so it's not clear which one should be used to fill the response + ───╯ + "#]], + ); + } + + #[test] + fn deep_conflict_multiple_issues() { + expect_errors( + r#" + { + f1 { + x: a + y: c + }, + f1 { + x: b + y: d + } + } + "#, + expect![[r#" + Error: cannot select different fields into the same alias `x` + ╭─[query.graphql:7:5] + β”‚ + 3 β”‚ x: a + β”‚ ──┬─ + β”‚ ╰─── `x` is selected from `Type.a` here + β”‚ + 7 β”‚ x: b + β”‚ ──┬─ + β”‚ ╰─── `x` is selected from `Type.b` here + β”‚ + β”‚ Help: Both fields may be present on the schema type, so it's not clear which one should be used to fill the response + ───╯ + Error: cannot select different fields into the same alias `y` + ╭─[query.graphql:8:5] + β”‚ + 4 β”‚ y: c + β”‚ ──┬─ + β”‚ ╰─── `y` is selected from `Type.c` here + β”‚ + 8 β”‚ y: d + β”‚ ──┬─ + β”‚ ╰─── `y` is selected from `Type.d` here + β”‚ + β”‚ Help: Both fields may be present on the schema type, so it's not clear which one should be used to fill the response + ───╯ + "#]], + ); + } + + #[test] + fn very_deep_conflict() { + expect_errors( + r#" + { + field { + deepField { + x: a + } + }, + field { + deepField { + x: b + } + } + } + "#, + expect![[r#" + Error: cannot select different fields into the same alias `x` + ╭─[query.graphql:9:7] + β”‚ + 4 β”‚ x: a + β”‚ ──┬─ + β”‚ ╰─── `x` is selected from `Type.a` here + β”‚ + 9 β”‚ x: b + β”‚ ──┬─ + β”‚ ╰─── `x` is selected from `Type.b` here + β”‚ + β”‚ Help: Both fields may be present on the schema type, so it's not clear which one should be used to fill the response + ───╯ + "#]], + ); + } + + #[test] + fn deep_conflict_in_fragments() { + expect_errors( + r#" + { + field { + ...F + } + field { + ...I + } + } + fragment F on T { + x: a + ...G + } + fragment G on T { + y: c + } + fragment I on T { + y: d + ...J + } + fragment J on T { + x: b + } + "#, + expect![[r#" + Error: cannot select different fields into the same alias `y` + ╭─[query.graphql:14:3] + β”‚ + 14 β”‚ y: c + β”‚ ──┬─ + β”‚ ╰─── `y` is selected from `T.c` here + β”‚ + 17 β”‚ y: d + β”‚ ──┬─ + β”‚ ╰─── `y` is selected from `T.d` here + β”‚ + β”‚ Help: Both fields may be present on the schema type, so it's not clear which one should be used to fill the response + ────╯ + Error: cannot select different fields into the same alias `x` + ╭─[query.graphql:21:3] + β”‚ + 10 β”‚ x: a + β”‚ ──┬─ + β”‚ ╰─── `x` is selected from `T.a` here + β”‚ + 21 β”‚ x: b + β”‚ ──┬─ + β”‚ ╰─── `x` is selected from `T.b` here + β”‚ + β”‚ Help: Both fields may be present on the schema type, so it's not clear which one should be used to fill the response + ────╯ + "#]], + ); + } +} + +mod return_types { + use apollo_compiler::validation::Valid; + use apollo_compiler::ExecutableDocument; + use apollo_compiler::Schema; + use expect_test::expect; + use expect_test::Expect; + use std::sync::OnceLock; + use unindent::unindent; + + const RETURN_TYPES_TEST_SCHEMA: &str = r#" + interface SomeBox { + deepBox: SomeBox + unrelatedField: String + } + + type StringBox implements SomeBox { + scalar: String + deepBox: StringBox + unrelatedField: String + listStringBox: [StringBox] + stringBox: StringBox + intBox: IntBox + } + + type IntBox implements SomeBox { + scalar: Int + deepBox: IntBox + unrelatedField: String + listStringBox: [StringBox] + stringBox: StringBox + intBox: IntBox + } + + interface NonNullStringBox1 { + scalar: String! + } + + type NonNullStringBox1Impl implements SomeBox & NonNullStringBox1 { + scalar: String! + unrelatedField: String + deepBox: SomeBox + } + + interface NonNullStringBox2 { + scalar: String! + } + + type NonNullStringBox2Impl implements SomeBox & NonNullStringBox2 { + scalar: String! + unrelatedField: String + deepBox: SomeBox + } + + type Connection { + edges: [Edge] + } + + type Edge { + node: Node + } + + type Node { + id: ID + name: String + } + + type Query { + someBox: SomeBox + connection: Connection + } + "#; + + fn test_schema() -> &'static Valid { + static SCHEMA: OnceLock> = OnceLock::new(); + + SCHEMA.get_or_init(|| { + Schema::parse_and_validate(unindent(RETURN_TYPES_TEST_SCHEMA), "schema.graphql") + .unwrap() + }) + } + + #[track_caller] + fn expect_valid(query: &'static str) { + let schema = test_schema(); + + ExecutableDocument::parse_and_validate(schema, unindent(query), "query.graphql").unwrap(); + } + + fn expect_errors(query: &'static str, expect: Expect) { + let schema = test_schema(); + + let errors = + ExecutableDocument::parse_and_validate(schema, unindent(query), "query.graphql") + .expect_err("should have errors") + .errors; + expect.assert_eq(&errors.to_string()); + } + + #[test] + fn conflicting_return_types_with_potential_overlap() { + expect_errors( + r#" + { + someBox { + ...on IntBox { + scalar + } + ...on NonNullStringBox1 { + scalar + } + } + } + "#, + expect![[r#" + Error: operation must not select different types using the same name `scalar` + ╭─[query.graphql:7:7] + β”‚ + 4 β”‚ scalar + β”‚ ───┬── + β”‚ ╰──── `scalar` is selected from `IntBox.scalar: Int` here + β”‚ + 7 β”‚ scalar + β”‚ ───┬── + β”‚ ╰──── `scalar` is selected from `NonNullStringBox1.scalar: String!` here + ───╯ + "#]], + ); + } + + #[test] + fn compatible_return_shapes_on_different_return_types() { + expect_valid( + r#" + { + someBox { + ... on SomeBox { + deepBox { + unrelatedField + } + } + ... on StringBox { + deepBox { + unrelatedField + } + } + } + } + "#, + ); + } + + #[test] + fn no_differing_return_types_despite_no_overlap() { + expect_errors( + r#" + { + someBox { + ... on IntBox { + scalar + } + ... on StringBox { + scalar + } + } + } + "#, + expect![[r#" + Error: operation must not select different types using the same name `scalar` + ╭─[query.graphql:7:7] + β”‚ + 4 β”‚ scalar + β”‚ ───┬── + β”‚ ╰──── `scalar` is selected from `IntBox.scalar: Int` here + β”‚ + 7 β”‚ scalar + β”‚ ───┬── + β”‚ ╰──── `scalar` is selected from `StringBox.scalar: String` here + ───╯ + "#]], + ); + } + + #[test] + fn non_exclusive_follows_exclusive() { + expect_errors( + r#" + { + someBox { + ... on IntBox { + deepBox { + ...X + } + } + } + someBox { + ... on StringBox { + deepBox { + ...Y + } + } + } + memoed: someBox { + ... on IntBox { + deepBox { + ...X + } + } + } + memoed: someBox { + ... on StringBox { + deepBox { + ...Y + } + } + } + other: someBox { + ...X + } + other: someBox { + ...Y + } + } + fragment X on SomeBox { + scalar + } + fragment Y on SomeBox { + scalar: unrelatedField + } + "#, + expect![[r#" + Error: type `SomeBox` does not have a field `scalar` + ╭─[query.graphql:38:3] + β”‚ + 38 β”‚ scalar + β”‚ ───┬── + β”‚ ╰──── field `scalar` selected here + β”‚ + β”œβ”€[schema.graphql:1:11] + β”‚ + 1 β”‚ interface SomeBox { + β”‚ ───┬─── + β”‚ ╰───── type `SomeBox` defined here + β”‚ + β”‚ Note: path to the field: `fragment X β†’ scalar` + ────╯ + "#]], + ); + } + + #[test] + fn no_differing_nullability_despite_no_overlap() { + expect_errors( + r#" + { + someBox { + ... on NonNullStringBox1 { + scalar + } + ... on StringBox { + scalar + } + } + } + "#, + expect![[r#" + Error: operation must not select different types using the same name `scalar` + ╭─[query.graphql:7:7] + β”‚ + 4 β”‚ scalar + β”‚ ───┬── + β”‚ ╰──── `scalar` is selected from `NonNullStringBox1.scalar: String!` here + β”‚ + 7 β”‚ scalar + β”‚ ───┬── + β”‚ ╰──── `scalar` is selected from `StringBox.scalar: String` here + ───╯ + "#]], + ); + } + + #[test] + fn no_differing_list_despite_no_overlap() { + expect_errors( + r#" + { + someBox { + ... on IntBox { + box: listStringBox { + scalar + } + } + ... on StringBox { + box: stringBox { + scalar + } + } + } + } + "#, + expect![[r#" + Error: operation must not select different types using the same name `box` + ╭─[query.graphql:9:7] + β”‚ + 4 β”‚ ╭───▢ box: listStringBox { + ┆ ┆ + 6 β”‚ β”œβ”€β”€β”€β–Ά } + β”‚ β”‚ + β”‚ ╰─────────────── `box` is selected from `IntBox.listStringBox: [StringBox]` here + β”‚ + 9 β”‚ ╭─▢ box: stringBox { + ┆ ┆ + 11 β”‚ β”œβ”€β–Ά } + β”‚ β”‚ + β”‚ ╰───────────── `box` is selected from `StringBox.stringBox: StringBox` here + ────╯ + "#]], + ); + + expect_errors( + r#" + { + someBox { + ... on IntBox { + box: stringBox { + scalar + } + } + ... on StringBox { + box: listStringBox { + scalar + } + } + } + } + "#, + expect![[r#" + Error: operation must not select different types using the same name `box` + ╭─[query.graphql:9:7] + β”‚ + 4 β”‚ ╭─▢ box: stringBox { + ┆ ┆ + 6 β”‚ β”œβ”€β–Ά } + β”‚ β”‚ + β”‚ ╰───────────── `box` is selected from `IntBox.stringBox: StringBox` here + β”‚ + 9 β”‚ ╭───▢ box: listStringBox { + ┆ ┆ + 11 β”‚ β”œβ”€β”€β”€β–Ά } + β”‚ β”‚ + β”‚ ╰─────────────── `box` is selected from `StringBox.listStringBox: [StringBox]` here + ────╯ + "#]], + ); + } + + #[test] + fn differing_sub_fields() { + expect_errors( + r#" + { + someBox { + ... on IntBox { + box: stringBox { + val: scalar + val: unrelatedField + } + } + ... on StringBox { + box: stringBox { + val: scalar + } + } + } + } + "#, + expect![[r#" + Error: cannot select different fields into the same alias `val` + ╭─[query.graphql:6:9] + β”‚ + 5 β”‚ val: scalar + β”‚ ─────┬───── + β”‚ ╰─────── `val` is selected from `StringBox.scalar` here + 6 β”‚ val: unrelatedField + β”‚ ─────────┬───────── + β”‚ ╰─────────── `val` is selected from `StringBox.unrelatedField` here + β”‚ + β”‚ Help: Both fields may be present on the schema type, so it's not clear which one should be used to fill the response + ───╯ + "#]], + ); + } + + #[test] + fn differing_deep_return_types() { + expect_errors( + r#" + { + someBox { + ... on IntBox { + box: stringBox { + scalar + } + } + ... on StringBox { + box: intBox { + scalar + } + } + } + } + "#, + expect![[r#" + Error: operation must not select different types using the same name `scalar` + ╭─[query.graphql:10:9] + β”‚ + 5 β”‚ scalar + β”‚ ───┬── + β”‚ ╰──── `scalar` is selected from `StringBox.scalar: String` here + β”‚ + 10 β”‚ scalar + β”‚ ───┬── + β”‚ ╰──── `scalar` is selected from `IntBox.scalar: Int` here + ────╯ + "#]], + ); + } + + #[test] + fn non_conflicting_overlapping_types() { + expect_valid( + r#" + { + someBox { + ... on IntBox { + scalar: unrelatedField + } + ... on StringBox { + scalar + } + } + } + "#, + ); + } + + #[test] + fn same_scalars() { + expect_valid( + r#" + { + someBox { + ...on NonNullStringBox1 { + scalar + } + ...on NonNullStringBox2 { + scalar + } + } + } + "#, + ); + } + + #[test] + fn deep_types_including_list() { + expect_errors( + r#" + { + connection { + ...edgeID + edges { + node { + id: name + } + } + } + } + + fragment edgeID on Connection { + edges { + node { + id + } + } + } + "#, + expect![[r#" + Error: operation must not select different types using the same name `id` + ╭─[query.graphql:15:7] + β”‚ + 6 β”‚ id: name + β”‚ ────┬─── + β”‚ ╰───── `id` is selected from `Node.name: String` here + β”‚ + 15 β”‚ id + β”‚ ─┬ + β”‚ ╰── `id` is selected from `Node.id: ID` here + ────╯ + Error: cannot select different fields into the same alias `id` + ╭─[query.graphql:15:7] + β”‚ + 6 β”‚ id: name + β”‚ ────┬─── + β”‚ ╰───── `id` is selected from `Node.name` here + β”‚ + 15 β”‚ id + β”‚ ─┬ + β”‚ ╰── `id` is selected from `Node.id` here + β”‚ + β”‚ Help: Both fields may be present on the schema type, so it's not clear which one should be used to fill the response + ────╯ + "#]], + ); + } + + #[test] + fn unknown_types() { + // The important part is that it doesn't emit a field merging error. + expect_errors( + r#" + someBox { + ...on UnknownType { + scalar + } + ...on NonNullStringBox2 { + scalar + } + } + } + "#, + expect![[r#" + Error: syntax error: expected definition + ╭─[query.graphql:1:3] + β”‚ + 1 β”‚ someBox { + β”‚ ───┬─── + β”‚ ╰───── expected definition + ───╯ + Error: type condition `UnknownType` of inline fragment is not a type defined in the schema + ╭─[query.graphql:2:11] + β”‚ + 2 β”‚ ...on UnknownType { + β”‚ ─────┬───── + β”‚ ╰─────── type condition here + β”‚ + β”‚ Note: path to the inline fragment: `query β†’ ...` + ───╯ + Error: inline fragment with type condition `NonNullStringBox2` cannot be applied to `Query` + ╭─[query.graphql:5:5] + β”‚ + 5 β”‚ ╭─▢ ...on NonNullStringBox2 { + ┆ ┆ + 7 β”‚ β”œβ”€β–Ά } + β”‚ β”‚ + β”‚ ╰─────────── inline fragment cannot be applied + β”‚ + β”œβ”€[schema.graphql:57:1] + β”‚ + 57 β”‚ ╭─▢ type Query { + ┆ ┆ + 60 β”‚ β”œβ”€β–Ά } + β”‚ β”‚ + β”‚ ╰─────── type condition `NonNullStringBox2` is not assignable to this type + ────╯ + Error: syntax error: expected a StringValue, Name or OperationDefinition + ╭─[query.graphql:9:1] + β”‚ + 9 β”‚ } + β”‚ ┬ + β”‚ ╰── expected a StringValue, Name or OperationDefinition + ───╯ + "#]], + ); + } +} diff --git a/crates/apollo-compiler/tests/validation/mod.rs b/crates/apollo-compiler/tests/validation/mod.rs index ee3b14d2..df561654 100644 --- a/crates/apollo-compiler/tests/validation/mod.rs +++ b/crates/apollo-compiler/tests/validation/mod.rs @@ -1,3 +1,4 @@ +mod field_merging; mod interface; mod object; mod operation; diff --git a/crates/apollo-compiler/tests/validation/recursion.rs b/crates/apollo-compiler/tests/validation/recursion.rs index e4b1b8d6..b4935898 100644 --- a/crates/apollo-compiler/tests/validation/recursion.rs +++ b/crates/apollo-compiler/tests/validation/recursion.rs @@ -1,3 +1,5 @@ +use expect_test::expect; + fn build_fragment_chain(size: usize) -> String { let mut query = r#" query Introspection{ @@ -89,6 +91,16 @@ fn build_input_object_chain(size: usize) -> String { schema } +/// Returns a deeply nested selection set with a conflict in its leaf fields. +fn build_nested_selection(depth: usize) -> String { + format!( + "query {}{}{}", + "{ recur\n".repeat(depth), + "{ leaf(arg: true) leaf(arg: false) }", + "\n}".repeat(depth), + ) +} + #[test] fn long_fragment_chains_do_not_overflow_stack() { // Build a query that applies 1K fragments @@ -105,6 +117,7 @@ fn long_fragment_chains_do_not_overflow_stack() { .expect_err("must have recursion errors"); let expected = expect_test::expect![[r#" + Error: too much recursion Error: too much recursion Error: `typeFragment1` contains too much nesting ╭─[overflow.graphql:11:11] @@ -122,7 +135,7 @@ fn not_long_enough_fragment_chain_applies_correctly() { // Stay just under the recursion limit let query = build_fragment_chain(99); - let _ = apollo_compiler::parse_mixed_validate( + apollo_compiler::parse_mixed_validate( format!( "type Query {{ a: Int }} {query}" @@ -176,3 +189,37 @@ fn not_long_enough_input_object_chain_applies_correctly() { let _schema = apollo_compiler::Schema::parse_and_validate(schema, "input_objects.graphql") .expect("must not have recursion errors"); } + +#[test] +fn deeply_nested_selection_bails_out() { + // This test relies on implementation details to ensure that the correct recursion limit is + // tested. + // The parser has a recursion limit that applies to parsing nested selection sets. So we build + // a nested selection set that is below this limit. + // Field merging validation has its own recursion limit that is much lower. We test this limit + // by writing a conflicting field selection at a nesting level that is too deep for the field + // merging validation, but not deep enough to cause a parser bailout. + // If this test fails, it may be due to a change in the hardcoded limits, and removing or + // replacing this test may be appropriate. + + let schema = r#" +type Recur { + recur: Recur + leaf(arg: Boolean): Int +} +type Query { + recur: Recur +} + "#; + + let query = build_nested_selection(400); + + let errors = + apollo_compiler::parse_mixed_validate(format!("{schema}\n{query}"), "selection.graphql") + .expect_err("must have validation error"); + + expect![[r#" + Error: too much recursion + "#]] + .assert_eq(&errors.to_string()); +}