From 22005403b8d641ab5d1b998788bce24db6abb7f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Thu, 25 Jan 2024 16:40:29 +0100 Subject: [PATCH 01/42] Add failing test for partially non-overlapping selections This should warn about the `root.overlapping` selection being both `Int` and `Int!` at the same time, but it does not because the validation only compares each selection to the *first* selection. Neither `B` nor `C` conflict with the selection on `A`, so the current implementation considers this valid. --- ...13_partially_overlapping_fragments.graphql | 29 +++++++++++++++ ...13_partially_overlapping_fragments.graphql | 36 +++++++++++++++++++ 2 files changed, 65 insertions(+) create mode 100644 crates/apollo-compiler/test_data/diagnostics/0113_partially_overlapping_fragments.graphql create mode 100644 crates/apollo-compiler/test_data/serializer/diagnostics/0113_partially_overlapping_fragments.graphql 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/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 + } + } +} From d9d9b25dc0362f30b4f8e462ddb6b25b51a9b502 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Thu, 25 Jan 2024 16:43:19 +0100 Subject: [PATCH 02/42] Fix field merging validation when more than 2 fields are in play --- .../src/validation/selection.rs | 46 +++++++++++++------ .../0113_partially_overlapping_fragments.txt | 12 +++++ 2 files changed, 44 insertions(+), 14 deletions(-) create mode 100644 crates/apollo-compiler/test_data/diagnostics/0113_partially_overlapping_fragments.txt diff --git a/crates/apollo-compiler/src/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs index d1a07023..bf288a39 100644 --- a/crates/apollo-compiler/src/validation/selection.rs +++ b/crates/apollo-compiler/src/validation/selection.rs @@ -5,7 +5,15 @@ 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 + +/// Return all possible unordered combinations of 2 elements from a slice. +fn pair_combinations(slice: &[T]) -> impl Iterator { + slice + .iter() + .enumerate() + // Final element will zip with the empty slice and produce no result. + .flat_map(|(index, element)| std::iter::repeat(element).zip(&slice[index + 1..])) +} /// A field and the type it selects from. #[derive(Debug, Clone, Copy)] @@ -188,12 +196,9 @@ pub(crate) fn same_response_shape( )); let grouped_by_name = group_fields_by_name(merged_set); - for (_, fields_for_name) in grouped_by_name { + for fields_for_name in grouped_by_name.values() { // 9. Given each pair of members subfieldA and subfieldB in fieldsForName: - let Some((subfield_a, rest)) = fields_for_name.split_first() else { - continue; - }; - for subfield_b in rest { + for (subfield_a, subfield_b) in pair_combinations(&fields_for_name) { // 9a. If SameResponseShape(subfieldA, subfieldB) is false, return false. same_response_shape(db, file_id, *subfield_a, *subfield_b)?; } @@ -305,15 +310,12 @@ pub(crate) fn fields_in_set_can_merge( let mut diagnostics = vec![]; 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 - }; - // 2. Given each pair of members fieldA and fieldB in fieldsForName: - for field_b in rest { + for (field_a, field_b) in pair_combinations(&fields_for_name) { + 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 + }; + // 2a. SameResponseShape(fieldA, fieldB) must be true. if let Err(diagnostic) = same_response_shape(db, file_id, *field_a, *field_b) { diagnostics.push(diagnostic); @@ -439,3 +441,19 @@ pub(crate) fn validate_selections( diagnostics } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn pair_combinations_test() { + let pairs = pair_combinations::(&[1, 2, 3, 4]).collect::>(); + assert_eq!( + pairs, + &[(&1, &2), (&1, &3), (&1, &4), (&2, &3), (&2, &4), (&3, &4)] + ); + let pairs = pair_combinations(&["a", "a", "a"]).collect::>(); + assert_eq!(pairs, &[(&"a", &"a"), (&"a", &"a"), (&"a", &"a")]); + } +} 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..ca252c79 --- /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 field name `overlapping` + ╭─[0113_partially_overlapping_fragments.graphql:27:16] + │ + 24 │ ... on B { overlapping } + │ ─────┬───── + │ ╰─────── `overlapping` has type `Int!` here + │ + 27 │ ... on C { overlapping } + │ ─────┬───── + │ ╰─────── but the same field name has type `Int` here +────╯ + From 344c7e343a0d10b74dc28c632d953fc5a7152688 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Thu, 25 Jan 2024 16:49:59 +0100 Subject: [PATCH 03/42] clippy --- crates/apollo-compiler/src/validation/selection.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/apollo-compiler/src/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs index bf288a39..3a73138f 100644 --- a/crates/apollo-compiler/src/validation/selection.rs +++ b/crates/apollo-compiler/src/validation/selection.rs @@ -198,7 +198,7 @@ pub(crate) fn same_response_shape( for fields_for_name in grouped_by_name.values() { // 9. Given each pair of members subfieldA and subfieldB in fieldsForName: - for (subfield_a, subfield_b) in pair_combinations(&fields_for_name) { + for (subfield_a, subfield_b) in pair_combinations(fields_for_name) { // 9a. If SameResponseShape(subfieldA, subfieldB) is false, return false. same_response_shape(db, file_id, *subfield_a, *subfield_b)?; } From 54d3dd06ca99806c506b77af7aa9878274578dc5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Fri, 26 Jan 2024 16:02:37 +0100 Subject: [PATCH 04/42] Pull some field merging code into smaller functions --- .../src/validation/selection.rs | 211 +++++++++--------- 1 file changed, 111 insertions(+), 100 deletions(-) diff --git a/crates/apollo-compiler/src/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs index 3a73138f..32f89194 100644 --- a/crates/apollo-compiler/src/validation/selection.rs +++ b/crates/apollo-compiler/src/validation/selection.rs @@ -76,43 +76,28 @@ pub(crate) fn operation_fields<'a>( ) } -/// 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, +fn is_composite(ty: &schema::ExtendedType) -> bool { + use schema::ExtendedType::*; + matches!(ty, Object(_) | Interface(_) | Union(_)) +} + +/// Check if the output types of two fields match. +fn same_output_type_shape( + schema: &schema::Schema, field_a: FieldAgainstType<'_>, + mut type_a: &ast::Type, field_b: FieldAgainstType<'_>, + mut type_b: &ast::Type, ) -> 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(), + original_type: type_a.clone(), redefined_selection: field_b.field.location(), - redefined_type: full_type_b.ty.clone(), + redefined_type: type_b.clone(), }, ) }; @@ -153,11 +138,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. ( @@ -171,27 +151,63 @@ pub(crate) fn same_response_shape( Err(mismatching_type_diagnostic()) } } + // 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()), + } +} + +/// 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() +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 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 type_b = &full_type_b.ty; + + // Covers steps 3-5 of the spec algorithm. + same_output_type_shape(&schema, field_a, type_a, field_b, type_b)?; + + let (Some(def_a), Some(def_b)) = ( + schema.types.get(type_a.inner_named_type()), + schema.types.get(type_b.inner_named_type()), + ) else { + return Ok(()); // Cannot do much if we don't know the type + }; + + match (def_a, def_b) { // 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(()); - }; let named_fragments = db.ast_named_fragments(file_id); let mut merged_set = operation_fields( &named_fragments, - &subfield_a_type.name, + &full_type_a.name, &field_a.field.selection_set, ); merged_set.extend(operation_fields( &named_fragments, - &subfield_b_type.name, + &full_type_b.name, &field_b.field.selection_set, )); let grouped_by_name = group_fields_by_name(merged_set); @@ -228,61 +244,68 @@ fn group_fields_by_name( 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, +/// Check if two fields on the same type are the same, so the fields can be merged. +fn same_field_selection( + field_a: FieldAgainstType<'_>, + field_b: FieldAgainstType<'_>, ) -> Result<(), ValidationError> { - let args_a = &field_a.arguments; - let args_b = &field_b.arguments; + debug_assert_eq!(field_a.against_type, field_b.against_type); - let loc_a = field_a.location(); - let loc_b = field_b.location(); + // 2bi. fieldA and fieldB must have identical field names. + if field_a.field.name != field_b.field.name { + return Err(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(), + }, + )); + } + + // 2bii. fieldA and fieldB must have identical sets of arguments. + let args_a = &field_a.field.arguments; + let args_b = &field_b.field.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(); + + let data = DiagnosticData::ConflictingFieldArgument { + // field_a and field_b have the same name so we can use either one. + field: field_b.field.name.clone(), + argument: arg.name.clone(), + original_selection: field_a.field.location(), + original_value: original_arg.map(|arg| (*arg.value).clone()), + redefined_selection: field_b.field.location(), + redefined_value: redefined_arg.map(|arg| (*arg.value).clone()), + }; + ValidationError::new(field_b.field.location(), data) + }; // 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, - }, - )); + return Err(conflicting_field_argument(Some(arg), None)); }; 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()), - }, - )); + return Err(conflicting_field_argument(Some(arg), Some(other_arg))); } } // 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()), - }, - )); + return Err(conflicting_field_argument(None, Some(arg))); }; } @@ -294,7 +317,7 @@ fn identical_arguments( /// If the fields cannot be merged, returns an `Err` containing diagnostic information. /// /// Spec: https://spec.graphql.org/October2021/#FieldsInSetCanMerge() -pub(crate) fn fields_in_set_can_merge( +fn fields_in_set_can_merge( db: &dyn ValidationDatabase, file_id: FileId, named_fragments: &HashMap>, @@ -321,24 +344,12 @@ pub(crate) fn fields_in_set_can_merge( 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) { + if let Err(diagnostic) = same_field_selection(*field_a, *field_b) { diagnostics.push(diagnostic); continue; } From 68d09c43e551f42ec45e654fcc485e5556355dcc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Fri, 26 Jan 2024 18:21:56 +0100 Subject: [PATCH 05/42] Use high-level representation for field merging validation --- .../src/executable/validation.rs | 29 +- .../src/validation/selection.rs | 518 ++++++++---------- .../0074_merge_identical_fields.txt | 12 +- .../0075_merge_conflicting_args.txt | 60 -- .../0076_merge_differing_responses.txt | 11 - .../0078_merge_conflict_nested_fragments.txt | 6 +- 6 files changed, 275 insertions(+), 361 deletions(-) diff --git a/crates/apollo-compiler/src/executable/validation.rs b/crates/apollo-compiler/src/executable/validation.rs index caaad8b0..345e9f66 100644 --- a/crates/apollo-compiler/src/executable/validation.rs +++ b/crates/apollo-compiler/src/executable/validation.rs @@ -30,11 +30,32 @@ 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 compiler_diagnostics = vec![]; + for operation in document.all_operations() { + crate::validation::selection::fields_in_set_can_merge( + schema, + document, + &operation.selection_set, + &mut compiler_diagnostics, + ); + } + + for fragment in document.fragments.values() { + crate::validation::selection::fields_in_set_can_merge( + schema, + document, + &fragment.selection_set, + &mut compiler_diagnostics, + ); + } + + for diagnostic in compiler_diagnostics { + errors.push(diagnostic.location, Details::CompilerDiagnostic(diagnostic)) + } } pub(crate) fn validate_with_or_without_schema( diff --git a/crates/apollo-compiler/src/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs index 32f89194..7a131687 100644 --- a/crates/apollo-compiler/src/validation/selection.rs +++ b/crates/apollo-compiler/src/validation/selection.rs @@ -1,10 +1,10 @@ -use std::collections::{hash_map::Entry, HashMap}; - +use super::operation::OperationValidationConfig; use crate::validation::diagnostics::{DiagnosticData, ValidationError}; use crate::validation::{FileId, ValidationDatabase}; -use crate::{ast, schema, Node}; - -use super::operation::OperationValidationConfig; +use crate::{ast, executable, schema, Node}; +use indexmap::IndexMap; +use std::collections::{hash_map::Entry, HashMap}; +use std::collections::{HashSet, VecDeque}; /// Return all possible unordered combinations of 2 elements from a slice. fn pair_combinations(slice: &[T]) -> impl Iterator { @@ -81,300 +81,277 @@ fn is_composite(ty: &schema::ExtendedType) -> bool { matches!(ty, Object(_) | Interface(_) | Union(_)) } -/// Check if the output types of two fields match. -fn same_output_type_shape( +/// https://tech.new-work.se/graphql-overlapping-fields-can-be-merged-fast-ea6e92e0a01 +pub(crate) fn fields_in_set_can_merge( schema: &schema::Schema, - field_a: FieldAgainstType<'_>, - mut type_a: &ast::Type, - field_b: FieldAgainstType<'_>, - mut type_b: &ast::Type, -) -> Result<(), ValidationError> { - 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: type_a.clone(), - redefined_selection: field_b.field.location(), - redefined_type: type_b.clone(), - }, - ) - }; - - // Steps 3 and 4 of the spec text unwrap both types simultaneously down to the named type. - // The apollo-rs representation of NonNull and Lists makes it tricky to follow the steps - // exactly. - // - // Instead we unwrap lists and non-null lists first, which leaves just a named type or a - // non-null named type... - while !type_a.is_named() || !type_b.is_named() { - // 4. If typeA or typeB is List. - // 4a. If typeA or typeB is not List, return false. - // 4b. Let typeA be the item type of typeA - // 4c. Let typeB be the item type of typeB - (type_a, type_b) = match (type_a, type_b) { - (ast::Type::List(type_a), ast::Type::List(type_b)) - | (ast::Type::NonNullList(type_a), ast::Type::NonNullList(type_b)) => { - (type_a.as_ref(), type_b.as_ref()) - } - (ast::Type::List(_), _) - | (_, ast::Type::List(_)) - | (ast::Type::NonNullList(_), _) - | (_, ast::Type::NonNullList(_)) => return Err(mismatching_type_diagnostic()), - // Now it's a named type. - (type_a, type_b) => (type_a, type_b), - }; + document: &executable::ExecutableDocument, + selection_set: &executable::SelectionSet, + diagnostics: &mut Vec, +) { + let fields = expand_selections(&document.fragments, &[selection_set]); + + same_response_shape_by_name(schema, &document.fragments, &fields, diagnostics); + same_for_common_parents_by_name(schema, &document.fragments, &fields, diagnostics); + + /// Represents a field selected against a parent type. + #[derive(Debug, Clone)] + struct FieldSelection { + /// The type of the selection set this field selection is part of. + parent_type: ast::NamedType, + field: Node, } - // Now we are down to two named types, we can check that they have the same nullability... - let (type_a, type_b) = match (type_a, type_b) { - (ast::Type::NonNullNamed(a), ast::Type::NonNullNamed(b)) => (a, b), - (ast::Type::Named(a), ast::Type::Named(b)) => (a, b), - _ => return Err(mismatching_type_diagnostic()), - }; - - let (Some(def_a), Some(def_b)) = (schema.types.get(type_a), schema.types.get(type_b)) else { - return Ok(()); // Cannot do much if we don't know the type - }; - - match (def_a, def_b) { - // 5. If typeA or typeB is Scalar or Enum. - ( - def_a @ (schema::ExtendedType::Scalar(_) | schema::ExtendedType::Enum(_)), - def_b @ (schema::ExtendedType::Scalar(_) | schema::ExtendedType::Enum(_)), - ) => { - // 5a. If typeA and typeB are the same type return true, otherwise return false. - if def_a == def_b { - Ok(()) - } else { - Err(mismatching_type_diagnostic()) + fn same_response_shape_by_name( + schema: &schema::Schema, + fragments: &IndexMap>, + fields: &[FieldSelection], + diagnostics: &mut Vec, + ) { + for fields_for_name in group_selections_by_output_name(fields.iter().cloned()).values() { + for (field_a, field_b) in pair_combinations(fields_for_name) { + // Covers steps 3-5 of the spec algorithm. + if let Err(err) = same_output_type_shape(&schema, field_a.clone(), field_b.clone()) + { + diagnostics.push(err); + continue; + } + + let merged_set = expand_selections( + fragments, + &[&field_a.field.selection_set, &field_b.field.selection_set], + ); + same_response_shape_by_name(schema, fragments, &merged_set, diagnostics); } } - // 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()), } -} -/// 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() -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 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 type_b = &full_type_b.ty; - - // Covers steps 3-5 of the spec algorithm. - same_output_type_shape(&schema, field_a, type_a, field_b, type_b)?; - - let (Some(def_a), Some(def_b)) = ( - schema.types.get(type_a.inner_named_type()), - schema.types.get(type_b.inner_named_type()), - ) else { - return Ok(()); // Cannot do much if we don't know the type - }; - - match (def_a, def_b) { - // 6. Assert: typeA and typeB are both composite types. - (def_a, def_b) if is_composite(def_a) && is_composite(def_b) => { - let named_fragments = db.ast_named_fragments(file_id); - let mut merged_set = operation_fields( - &named_fragments, - &full_type_a.name, - &field_a.field.selection_set, - ); - merged_set.extend(operation_fields( - &named_fragments, - &full_type_b.name, - &field_b.field.selection_set, - )); - let grouped_by_name = group_fields_by_name(merged_set); + fn same_for_common_parents_by_name( + schema: &schema::Schema, + fragments: &IndexMap>, + fields: &[FieldSelection], + diagnostics: &mut Vec, + ) { + for fields_for_name in group_selections_by_output_name(fields.iter().cloned()).values() { + for (field_a, field_b) in pair_combinations(fields_for_name) { + if field_a.parent_type == field_b.parent_type { + // 2bi. fieldA and fieldB must have identical field names. + // 2bii. fieldA and fieldB must have identical sets of arguments. + if let Err(diagnostic) = same_field_selection(field_a, field_b) { + diagnostics.push(diagnostic); + continue; + } - for fields_for_name in grouped_by_name.values() { - // 9. Given each pair of members subfieldA and subfieldB in fieldsForName: - for (subfield_a, subfield_b) in pair_combinations(fields_for_name) { - // 9a. If SameResponseShape(subfieldA, subfieldB) is false, return false. - same_response_shape(db, file_id, *subfield_a, *subfield_b)?; + let merged_set = expand_selections( + fragments, + &[&field_a.field.selection_set, &field_b.field.selection_set], + ); + same_for_common_parents_by_name(schema, fragments, &merged_set, diagnostics); } } - - Ok(()) } - (_, _) => 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]); + /// Expand one or more selection sets to a list of all fields selected. + fn expand_selections( + fragments: &IndexMap>, + selection_sets: &[&executable::SelectionSet], + ) -> Vec { + let mut selections = vec![]; + let mut queue: VecDeque<&executable::SelectionSet> = + selection_sets.iter().copied().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.clone(), + field: field.clone(), + }), + 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); + } + } + executable::Selection::FragmentSpread(_) => { + // Already seen + } + } } } + + selections } - map -} -/// Check if two fields on the same type are the same, so the fields can be merged. -fn same_field_selection( - field_a: FieldAgainstType<'_>, - field_b: FieldAgainstType<'_>, -) -> Result<(), ValidationError> { - debug_assert_eq!(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 { - return Err(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(), - }, - )); + fn group_selections_by_output_name( + selections: impl Iterator, + ) -> HashMap> { + let mut map = HashMap::new(); + for selection in selections { + match map.entry(selection.field.response_key().clone()) { + Entry::Vacant(entry) => { + entry.insert(vec![selection]); + } + Entry::Occupied(mut entry) => { + entry.get_mut().push(selection); + } + } + } + map } - // 2bii. fieldA and fieldB must have identical sets of arguments. - let args_a = &field_a.field.arguments; - let args_b = &field_b.field.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(); - - let data = DiagnosticData::ConflictingFieldArgument { - // field_a and field_b have the same name so we can use either one. - field: field_b.field.name.clone(), - argument: arg.name.clone(), - original_selection: field_a.field.location(), - original_value: original_arg.map(|arg| (*arg.value).clone()), - redefined_selection: field_b.field.location(), - redefined_value: redefined_arg.map(|arg| (*arg.value).clone()), + fn same_output_type_shape( + schema: &schema::Schema, + selection_a: FieldSelection, + selection_b: FieldSelection, + ) -> Result<(), ValidationError> { + 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 = || { + ValidationError::new( + selection_b.field.location(), + DiagnosticData::ConflictingFieldType { + field: selection_a.field.response_key().clone(), + original_selection: selection_a.field.location(), + original_type: field_a.ty.clone(), + redefined_selection: selection_b.field.location(), + redefined_type: field_b.ty.clone(), + }, + ) + }; + + // Steps 3 and 4 of the spec text unwrap both types simultaneously down to the named type. + // The apollo-rs representation of NonNull and Lists makes it tricky to follow the steps + // exactly. + // + // Instead we unwrap lists and non-null lists first, which leaves just a named type or a + // non-null named type... + while !type_a.is_named() || !type_b.is_named() { + // 4. If typeA or typeB is List. + // 4a. If typeA or typeB is not List, return false. + // 4b. Let typeA be the item type of typeA + // 4c. Let typeB be the item type of typeB + (type_a, type_b) = match (type_a, type_b) { + (ast::Type::List(type_a), ast::Type::List(type_b)) + | (ast::Type::NonNullList(type_a), ast::Type::NonNullList(type_b)) => { + (type_a.as_ref(), type_b.as_ref()) + } + (ast::Type::List(_), _) + | (_, ast::Type::List(_)) + | (ast::Type::NonNullList(_), _) + | (_, ast::Type::NonNullList(_)) => return Err(mismatching_type_diagnostic()), + // Now it's a named type. + (type_a, type_b) => (type_a, type_b), }; - ValidationError::new(field_b.field.location(), data) + } + + // Now we are down to two named types, we can check that they have the same nullability... + let (type_a, type_b) = match (type_a, type_b) { + (ast::Type::NonNullNamed(a), ast::Type::NonNullNamed(b)) => (a, b), + (ast::Type::Named(a), ast::Type::Named(b)) => (a, b), + _ => return Err(mismatching_type_diagnostic()), }; - // 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(conflicting_field_argument(Some(arg), None)); + let (Some(def_a), Some(def_b)) = (schema.types.get(type_a), schema.types.get(type_b)) + else { + return Ok(()); // Cannot do much if we don't know the type }; - if other_arg.value != arg.value { - return Err(conflicting_field_argument(Some(arg), Some(other_arg))); + match (def_a, def_b) { + // 5. If typeA or typeB is Scalar or Enum. + ( + def_a @ (schema::ExtendedType::Scalar(_) | schema::ExtendedType::Enum(_)), + def_b @ (schema::ExtendedType::Scalar(_) | schema::ExtendedType::Enum(_)), + ) => { + // 5a. If typeA and typeB are the same type return true, otherwise return false. + if def_a == def_b { + Ok(()) + } else { + Err(mismatching_type_diagnostic()) + } + } + // 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()), } } - // 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(conflicting_field_argument(None, Some(arg))); - }; - } - - Ok(()) -} - -/// Check if the fields in a given selection set can be merged. -/// -/// If the fields cannot be merged, returns an `Err` containing diagnostic information. -/// -/// Spec: https://spec.graphql.org/October2021/#FieldsInSetCanMerge() -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(); - - // 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); - let mut diagnostics = vec![]; + /// Check if two field selections from the same type are the same, so the fields can be merged. + fn same_field_selection( + field_a: &FieldSelection, + field_b: &FieldSelection, + ) -> Result<(), ValidationError> { + debug_assert_eq!(field_a.parent_type, field_b.parent_type); + + // 2bi. fieldA and fieldB must have identical field names. + if field_a.field.name != field_b.field.name { + return Err(ValidationError::new( + field_b.field.location(), + DiagnosticData::ConflictingFieldName { + field: field_a.field.response_key().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(), + }, + )); + } - for (_, fields_for_name) in grouped_by_name { - // 2. Given each pair of members fieldA and fieldB in fieldsForName: - for (field_a, field_b) in pair_combinations(&fields_for_name) { - 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 + // 2bii. fieldA and fieldB must have identical sets of arguments. + let args_a = &field_a.field.arguments; + let args_b = &field_b.field.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(); + + let data = DiagnosticData::ConflictingFieldArgument { + // field_a and field_b have the same name so we can use either one. + field: field_b.field.name.clone(), + argument: arg.name.clone(), + original_selection: field_a.field.location(), + original_value: original_arg.map(|arg| (*arg.value).clone()), + redefined_selection: field_b.field.location(), + redefined_value: redefined_arg.map(|arg| (*arg.value).clone()), + }; + ValidationError::new(field_b.field.location(), data) }; - // 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; - } + // 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(conflicting_field_argument(Some(arg), None)); + }; - // 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. - // 2bii. fieldA and fieldB must have identical sets of arguments. - if let Err(diagnostic) = same_field_selection(*field_a, *field_b) { - 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; - } + if 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 args_b { + if !args_a.iter().any(|other_arg| other_arg.name == arg.name) { + return Err(conflicting_field_argument(None, Some(arg))); + }; + } - if diagnostics.is_empty() { Ok(()) - } else { - Err(diagnostics) } } @@ -387,19 +364,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..2194f163 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 @@ -8,15 +8,15 @@ Error: operation must not select different types using the same field name `name │ ──┬─ │ ╰─── but the same field name has type `String!` here ────╯ -Error: operation must not select different types using the same field name `name` +Error: operation must not select different fields to the same alias `name` ╭─[0074_merge_identical_fields.graphql:19:3] │ 18 │ name: nickname │ ───────┬────── - │ ╰──────── `name` has type `String` here + │ ╰──────── field `name` is selected from field `nickname` here 19 │ name │ ──┬─ - │ ╰─── but the same field name has type `String!` here + │ ╰─── but the same field `name` is also selected from field `name` here ────╯ Error: operation must not select different types using the same field name `fido` ╭─[0074_merge_identical_fields.graphql:24:3] @@ -28,14 +28,14 @@ Error: operation must not select different types using the same field name `fido │ ───────┬────── │ ╰──────── but the same field name has type `String` here ────╯ -Error: operation must not select different types using the same field name `fido` +Error: operation must not select different fields to the same alias `fido` ╭─[0074_merge_identical_fields.graphql:24:3] │ 23 │ fido: name │ ─────┬──── - │ ╰────── `fido` has type `String!` here + │ ╰────── field `fido` is selected from field `name` here 24 │ fido: nickname │ ───────┬────── - │ ╰──────── but the same field name has type `String` here + │ ╰──────── but the same field `fido` is also selected from field `nickname` here ────╯ 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..af396110 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 @@ -10,30 +10,6 @@ Error: operation must not provide conflicting field arguments for the same field │ │ 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: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` - ╭─[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` ╭─[0075_merge_conflicting_args.graphql:52:3] │ @@ -58,30 +34,6 @@ Error: operation must not provide conflicting field arguments for the same field │ │ 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: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` ╭─[0075_merge_conflicting_args.graphql:62:3] │ @@ -106,16 +58,4 @@ Error: operation must not provide conflicting field arguments for the same field │ │ 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. -────╯ 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..6538a2bc 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 @@ -9,15 +9,4 @@ Error: operation must not select different types using the same field name `some │ ──────────┬────────── │ ╰──────────── 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 -────╯ 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..9334264a 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,13 +1,13 @@ Error: operation must not select different fields to the same alias `y` - ╭─[0078_merge_conflict_nested_fragments.graphql:28:3] + ╭─[0078_merge_conflict_nested_fragments.graphql:25:3] │ 25 │ y: c │ ──┬─ - │ ╰─── field `y` is selected from field `c` here + │ ╰─── but the same field `y` is also selected from field `c` here │ 28 │ y: d │ ──┬─ - │ ╰─── but the same field `y` is also selected from field `d` here + │ ╰─── field `y` is selected from field `d` here ────╯ Error: operation must not select different fields to the same alias `x` ╭─[0078_merge_conflict_nested_fragments.graphql:32:3] From ee6c8ffdd59788a34bf42a05ab9fcaf52d21f621 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Tue, 30 Jan 2024 14:01:27 +0100 Subject: [PATCH 06/42] Bail out of field merging validation if selection set contains fragment cycles --- .../src/validation/selection.rs | 51 ++++++++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-) diff --git a/crates/apollo-compiler/src/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs index 7a131687..380b007b 100644 --- a/crates/apollo-compiler/src/validation/selection.rs +++ b/crates/apollo-compiler/src/validation/selection.rs @@ -1,6 +1,6 @@ use super::operation::OperationValidationConfig; use crate::validation::diagnostics::{DiagnosticData, ValidationError}; -use crate::validation::{FileId, ValidationDatabase}; +use crate::validation::{CycleError, FileId, RecursionGuard, RecursionStack, ValidationDatabase}; use crate::{ast, executable, schema, Node}; use indexmap::IndexMap; use std::collections::{hash_map::Entry, HashMap}; @@ -81,6 +81,51 @@ fn is_composite(ty: &schema::ExtendedType) -> bool { matches!(ty, Object(_) | Interface(_) | Union(_)) } +/// Check if a selection set contains a fragment cycle, meaning we can't run recursive +/// validations on it. +fn contains_any_fragment_cycle( + fragments: &IndexMap>, + selection_set: &executable::SelectionSet, +) -> bool { + let mut visited = RecursionStack::new().with_limit(100); + + return detect_fragment_cycle_inner(fragments, selection_set, &mut visited.guard()).is_err(); + + fn detect_fragment_cycle_inner( + fragments: &IndexMap>, + selection_set: &executable::SelectionSet, + visited: &mut RecursionGuard<'_>, + ) -> Result<(), CycleError<()>> { + for selection in &selection_set.selections { + match selection { + executable::Selection::FragmentSpread(spread) => { + if visited.contains(&spread.fragment_name) { + if visited.first() == Some(&spread.fragment_name) { + return Err(CycleError::Recursed(vec![])); + } + continue; + } + + if let Some(fragment) = fragments.get(&spread.fragment_name) { + detect_fragment_cycle_inner( + fragments, + &fragment.selection_set, + &mut visited.push(&fragment.name)?, + )?; + } + } + executable::Selection::InlineFragment(inline) => { + detect_fragment_cycle_inner(fragments, &inline.selection_set, visited)?; + } + executable::Selection::Field(field) => { + detect_fragment_cycle_inner(fragments, &field.selection_set, visited)?; + } + } + } + Ok(()) + } +} + /// https://tech.new-work.se/graphql-overlapping-fields-can-be-merged-fast-ea6e92e0a01 pub(crate) fn fields_in_set_can_merge( schema: &schema::Schema, @@ -88,6 +133,10 @@ pub(crate) fn fields_in_set_can_merge( selection_set: &executable::SelectionSet, diagnostics: &mut Vec, ) { + if contains_any_fragment_cycle(&document.fragments, selection_set) { + return; + } + let fields = expand_selections(&document.fragments, &[selection_set]); same_response_shape_by_name(schema, &document.fragments, &fields, diagnostics); From a3b9bff1c459a36522b4fc5714a9fa3f826a8161 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Tue, 30 Jan 2024 16:48:13 +0100 Subject: [PATCH 07/42] Make same response shape less mega quadratic recursive --- crates/apollo-compiler/src/validation/selection.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/crates/apollo-compiler/src/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs index 380b007b..e2ce2853 100644 --- a/crates/apollo-compiler/src/validation/selection.rs +++ b/crates/apollo-compiler/src/validation/selection.rs @@ -164,13 +164,15 @@ pub(crate) fn fields_in_set_can_merge( diagnostics.push(err); continue; } - - let merged_set = expand_selections( - fragments, - &[&field_a.field.selection_set, &field_b.field.selection_set], - ); - same_response_shape_by_name(schema, fragments, &merged_set, diagnostics); } + + let nested_selection_sets = fields_for_name + .iter() + .map(|selection| &selection.field.selection_set) + .filter(|set| !set.selections.is_empty()) + .collect::>(); + let merged_set = expand_selections(fragments, &nested_selection_sets); + same_response_shape_by_name(schema, fragments, &merged_set, diagnostics); } } From 0895bc06d257b26af9c42009cc3957b1fed58b43 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Wed, 31 Jan 2024 11:42:56 +0100 Subject: [PATCH 08/42] Make same field selection check less mega quadratic recursive --- crates/apollo-compiler/src/executable/mod.rs | 5 ++ .../src/validation/selection.rs | 80 ++++++++++++++----- .../0074_merge_identical_fields.txt | 40 ++++++++++ .../0075_merge_conflicting_args.txt | 60 ++++++++++++++ .../0076_merge_differing_responses.txt | 11 +++ 5 files changed, 177 insertions(+), 19 deletions(-) diff --git a/crates/apollo-compiler/src/executable/mod.rs b/crates/apollo-compiler/src/executable/mod.rs index 38108862..9cf894dd 100644 --- a/crates/apollo-compiler/src/executable/mod.rs +++ b/crates/apollo-compiler/src/executable/mod.rs @@ -591,6 +591,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/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs index e2ce2853..c00d5519 100644 --- a/crates/apollo-compiler/src/validation/selection.rs +++ b/crates/apollo-compiler/src/validation/selection.rs @@ -182,24 +182,69 @@ pub(crate) fn fields_in_set_can_merge( fields: &[FieldSelection], diagnostics: &mut Vec, ) { - for fields_for_name in group_selections_by_output_name(fields.iter().cloned()).values() { - for (field_a, field_b) in pair_combinations(fields_for_name) { - if field_a.parent_type == field_b.parent_type { - // 2bi. fieldA and fieldB must have identical field names. - // 2bii. fieldA and fieldB must have identical sets of arguments. - if let Err(diagnostic) = same_field_selection(field_a, field_b) { + for (_, fields_for_name) in + group_selections_by_output_name(fields.iter().cloned()).into_iter() + { + for fields_for_parents in + group_selections_by_common_parents(schema, fields_for_name.into_iter()) + { + // 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_a, field_b) in std::iter::repeat(field_a).zip(rest.iter()) { + if let Err(diagnostic) = same_name_and_arguments(field_a, field_b) { diagnostics.push(diagnostic); continue; } + } + + let nested_selection_sets = fields_for_parents + .iter() + .map(|selection| &selection.field.selection_set) + .filter(|set| !set.selections.is_empty()) + .collect::>(); + let merged_set = expand_selections(fragments, &nested_selection_sets); + same_for_common_parents_by_name(schema, fragments, &merged_set, diagnostics); + } + } + } - let merged_set = expand_selections( - fragments, - &[&field_a.field.selection_set, &field_b.field.selection_set], - ); - same_for_common_parents_by_name(schema, fragments, &merged_set, diagnostics); + fn group_selections_by_common_parents( + schema: &schema::Schema, + selections: impl Iterator, + ) -> Vec> { + let mut abstract_parents = vec![]; + let mut concrete_parents = HashMap::<_, Vec<_>>::new(); + for selection in 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 concrete_parents.is_empty() { + vec![abstract_parents] + } else { + concrete_parents + .into_iter() + .map(|(_name, mut group)| { + group.extend(abstract_parents.iter().cloned()); + group + }) + .collect() + } } /// Expand one or more selection sets to a list of all fields selected. @@ -338,7 +383,7 @@ pub(crate) fn fields_in_set_can_merge( } /// Check if two field selections from the same type are the same, so the fields can be merged. - fn same_field_selection( + fn same_name_and_arguments( field_a: &FieldSelection, field_b: &FieldSelection, ) -> Result<(), ValidationError> { @@ -359,9 +404,6 @@ pub(crate) fn fields_in_set_can_merge( } // 2bii. fieldA and fieldB must have identical sets of arguments. - let args_a = &field_a.field.arguments; - let args_b = &field_b.field.arguments; - let conflicting_field_argument = |original_arg: Option<&Node>, redefined_arg: Option<&Node>| { @@ -386,8 +428,8 @@ pub(crate) fn fields_in_set_can_merge( }; // 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 { + for arg in &field_a.field.arguments { + let Some(other_arg) = field_b.field.argument_by_name(&arg.name) else { return Err(conflicting_field_argument(Some(arg), None)); }; @@ -396,8 +438,8 @@ pub(crate) fn fields_in_set_can_merge( } } // 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) { + for arg in &field_b.field.arguments { + if field_a.field.argument_by_name(&arg.name).is_none() { return Err(conflicting_field_argument(None, Some(arg))); }; } 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 2194f163..28a8e828 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 @@ -18,6 +18,46 @@ Error: operation must not select different fields to the same alias `name` │ ──┬─ │ ╰─── but the same field `name` is also selected from field `name` here ────╯ +Error: operation must not select different types using the same field name `name` + ╭─[0074_merge_identical_fields.graphql:19:3] + │ + 18 │ name: nickname + │ ───────┬────── + │ ╰──────── `name` has type `String` here + 19 │ name + │ ──┬─ + │ ╰─── but the same field name has type `String!` here +────╯ +Error: operation must not select different fields to the same alias `name` + ╭─[0074_merge_identical_fields.graphql:19:3] + │ + 18 │ name: nickname + │ ───────┬────── + │ ╰──────── field `name` is selected from field `nickname` here + 19 │ name + │ ──┬─ + │ ╰─── but the same field `name` is also selected from field `name` here +────╯ +Error: operation must not select different types using the same field name `fido` + ╭─[0074_merge_identical_fields.graphql:24:3] + │ + 23 │ fido: name + │ ─────┬──── + │ ╰────── `fido` has type `String!` here + 24 │ fido: nickname + │ ───────┬────── + │ ╰──────── but the same field name has type `String` here +────╯ +Error: operation must not select different fields to the same alias `fido` + ╭─[0074_merge_identical_fields.graphql:24:3] + │ + 23 │ fido: name + │ ─────┬──── + │ ╰────── field `fido` is selected from field `name` here + 24 │ fido: nickname + │ ───────┬────── + │ ╰──────── but the same field `fido` is also selected from field `nickname` here +────╯ Error: operation must not select different types using the same field name `fido` ╭─[0074_merge_identical_fields.graphql:24:3] │ 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 af396110..41588a57 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 @@ -10,6 +10,30 @@ Error: operation must not provide conflicting field arguments for the same field │ │ 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: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` + ╭─[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` ╭─[0075_merge_conflicting_args.graphql:52:3] │ @@ -34,6 +58,30 @@ Error: operation must not provide conflicting field arguments for the same field │ │ 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: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` ╭─[0075_merge_conflicting_args.graphql:62:3] │ @@ -58,4 +106,16 @@ Error: operation must not provide conflicting field arguments for the same field │ │ 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. +────╯ 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 6538a2bc..6e34797a 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 @@ -9,4 +9,15 @@ Error: operation must not select different types using the same field name `some │ ──────────┬────────── │ ╰──────────── 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 +────╯ From b59b3f177b8fc7cc2b6dddefdf8ea7e4c4dce84a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Wed, 31 Jan 2024 12:10:58 +0100 Subject: [PATCH 09/42] Port subscription validation that uses field selection expansion --- crates/apollo-compiler/src/executable/mod.rs | 16 ++ .../src/executable/validation.rs | 5 + .../src/validation/operation.rs | 54 +++---- .../src/validation/selection.rs | 138 +++++------------- 4 files changed, 86 insertions(+), 127 deletions(-) diff --git a/crates/apollo-compiler/src/executable/mod.rs b/crates/apollo-compiler/src/executable/mod.rs index 9cf894dd..64bbacb5 100644 --- a/crates/apollo-compiler/src/executable/mod.rs +++ b/crates/apollo-compiler/src/executable/mod.rs @@ -327,10 +327,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 { diff --git a/crates/apollo-compiler/src/executable/validation.rs b/crates/apollo-compiler/src/executable/validation.rs index 345e9f66..dc893ac1 100644 --- a/crates/apollo-compiler/src/executable/validation.rs +++ b/crates/apollo-compiler/src/executable/validation.rs @@ -36,6 +36,11 @@ fn validate_with_schema( ) { let mut compiler_diagnostics = vec![]; for operation in document.all_operations() { + crate::validation::operation::validate_subscription( + document, + operation, + &mut compiler_diagnostics, + ); crate::validation::selection::fields_in_set_can_merge( schema, document, diff --git a/crates/apollo-compiler/src/validation/operation.rs b/crates/apollo-compiler/src/validation/operation.rs index f0c4a3f0..35d89776 100644 --- a/crates/apollo-compiler/src/validation/operation.rs +++ b/crates/apollo-compiler/src/validation/operation.rs @@ -1,6 +1,6 @@ use crate::validation::diagnostics::{DiagnosticData, ValidationError}; 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,31 +10,14 @@ 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 Vec, +) { + if operation.is_subscription() { + let fields = + super::selection::expand_selections(&document.fragments, &[&operation.selection_set]); if fields.len() > 1 { diagnostics.push(ValidationError::new( @@ -57,7 +40,7 @@ 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( field.location(), @@ -68,6 +51,23 @@ pub(crate) fn validate_operation( )); } } +} + +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 c00d5519..6031053b 100644 --- a/crates/apollo-compiler/src/validation/selection.rs +++ b/crates/apollo-compiler/src/validation/selection.rs @@ -15,65 +15,49 @@ fn pair_combinations(slice: &[T]) -> impl Iterator { .flat_map(|(index, element)| std::iter::repeat(element).zip(&slice[index + 1..])) } -/// 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, +/// Represents a field selected against a parent type. +#[derive(Debug, Clone)] +pub(crate) struct FieldSelection { + /// The type of the selection set this field selection is part of. + pub parent_type: ast::NamedType, + pub field: Node, } -// 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, - 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![]; +/// Expand one or more selection sets to a list of all fields selected. +pub(crate) fn expand_selections( + fragments: &IndexMap>, + selection_sets: &[&executable::SelectionSet], +) -> Vec { + let mut selections = vec![]; + let mut queue: VecDeque<&executable::SelectionSet> = selection_sets.iter().copied().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.clone(), + field: field.clone(), + }), + 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() } - }) - .collect() + executable::Selection::FragmentSpread(_) => { + // Already seen + } + } + } } - inner( - named_fragments, - &mut Default::default(), - against_type, - selections, - ) + + selections } fn is_composite(ty: &schema::ExtendedType) -> bool { @@ -142,14 +126,6 @@ pub(crate) fn fields_in_set_can_merge( same_response_shape_by_name(schema, &document.fragments, &fields, diagnostics); same_for_common_parents_by_name(schema, &document.fragments, &fields, diagnostics); - /// Represents a field selected against a parent type. - #[derive(Debug, Clone)] - struct FieldSelection { - /// The type of the selection set this field selection is part of. - parent_type: ast::NamedType, - field: Node, - } - fn same_response_shape_by_name( schema: &schema::Schema, fragments: &IndexMap>, @@ -247,44 +223,6 @@ pub(crate) fn fields_in_set_can_merge( } } - /// Expand one or more selection sets to a list of all fields selected. - fn expand_selections( - fragments: &IndexMap>, - selection_sets: &[&executable::SelectionSet], - ) -> Vec { - let mut selections = vec![]; - let mut queue: VecDeque<&executable::SelectionSet> = - selection_sets.iter().copied().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.clone(), - field: field.clone(), - }), - 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); - } - } - executable::Selection::FragmentSpread(_) => { - // Already seen - } - } - } - } - - selections - } - fn group_selections_by_output_name( selections: impl Iterator, ) -> HashMap> { From c1a558f23754b43c63833c5574674072a260c2dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Wed, 31 Jan 2024 12:23:45 +0100 Subject: [PATCH 10/42] Migrate subscription validation errors to use BuildError --- crates/apollo-compiler/src/executable/mod.rs | 28 ++++++++++ .../src/executable/validation.rs | 6 +-- .../src/validation/diagnostics.rs | 51 ++----------------- crates/apollo-compiler/src/validation/mod.rs | 36 +++++++++++++ .../src/validation/operation.rs | 17 ++++--- 5 files changed, 79 insertions(+), 59 deletions(-) diff --git a/crates/apollo-compiler/src/executable/mod.rs b/crates/apollo-compiler/src/executable/mod.rs index 64bbacb5..4983caf2 100644 --- a/crates/apollo-compiler/src/executable/mod.rs +++ b/crates/apollo-compiler/src/executable/mod.rs @@ -161,6 +161,34 @@ pub(crate) enum BuildError { field_name: Name, path: SelectionPath, }, + + #[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, + }, +} + +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)] diff --git a/crates/apollo-compiler/src/executable/validation.rs b/crates/apollo-compiler/src/executable/validation.rs index dc893ac1..0f1d1bbc 100644 --- a/crates/apollo-compiler/src/executable/validation.rs +++ b/crates/apollo-compiler/src/executable/validation.rs @@ -36,11 +36,7 @@ fn validate_with_schema( ) { let mut compiler_diagnostics = vec![]; for operation in document.all_operations() { - crate::validation::operation::validate_subscription( - document, - operation, - &mut compiler_diagnostics, - ); + crate::validation::operation::validate_subscription(document, operation, errors); crate::validation::selection::fields_in_set_can_merge( schema, document, diff --git a/crates/apollo-compiler/src/validation/diagnostics.rs b/crates/apollo-compiler/src/validation/diagnostics.rs index 528e4d97..67442df3 100644 --- a/crates/apollo-compiler/src/validation/diagnostics.rs +++ b/crates/apollo-compiler/src/validation/diagnostics.rs @@ -51,14 +51,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,16 +205,6 @@ 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, @@ -380,17 +362,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 +616,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, @@ -832,10 +797,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 +822,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 459d3b78..0c80655f 100644 --- a/crates/apollo-compiler/src/validation/mod.rs +++ b/crates/apollo-compiler/src/validation/mod.rs @@ -409,6 +409,23 @@ 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"), + ); + } }, } } @@ -624,3 +641,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 35d89776..5c4c9ab9 100644 --- a/crates/apollo-compiler/src/validation/operation.rs +++ b/crates/apollo-compiler/src/validation/operation.rs @@ -1,4 +1,5 @@ -use crate::validation::diagnostics::{DiagnosticData, ValidationError}; +use crate::validation::diagnostics::ValidationError; +use crate::validation::DiagnosticList; use crate::validation::FileId; use crate::{ast, executable, Node, ValidationDatabase}; @@ -13,23 +14,23 @@ pub(crate) struct OperationValidationConfig<'vars> { pub(crate) fn validate_subscription( document: &executable::ExecutableDocument, operation: &Node, - diagnostics: &mut Vec, + diagnostics: &mut DiagnosticList, ) { if operation.is_subscription() { let fields = super::selection::expand_selections(&document.fragments, &[&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 @@ -42,13 +43,13 @@ pub(crate) fn validate_subscription( }) .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(), }, - )); + ); } } } From 8db8671712cabbdb05a92b58c8bd8a99cd44c6e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Wed, 31 Jan 2024 12:25:04 +0100 Subject: [PATCH 11/42] clippy --- crates/apollo-compiler/src/validation/selection.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/crates/apollo-compiler/src/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs index 6031053b..f31ecd67 100644 --- a/crates/apollo-compiler/src/validation/selection.rs +++ b/crates/apollo-compiler/src/validation/selection.rs @@ -135,8 +135,7 @@ pub(crate) fn fields_in_set_can_merge( for fields_for_name in group_selections_by_output_name(fields.iter().cloned()).values() { for (field_a, field_b) in pair_combinations(fields_for_name) { // Covers steps 3-5 of the spec algorithm. - if let Err(err) = same_output_type_shape(&schema, field_a.clone(), field_b.clone()) - { + if let Err(err) = same_output_type_shape(schema, field_a.clone(), field_b.clone()) { diagnostics.push(err); continue; } @@ -214,8 +213,8 @@ pub(crate) fn fields_in_set_can_merge( vec![abstract_parents] } else { concrete_parents - .into_iter() - .map(|(_name, mut group)| { + .into_values() + .map(|mut group| { group.extend(abstract_parents.iter().cloned()); group }) @@ -315,7 +314,7 @@ pub(crate) fn fields_in_set_can_merge( } } // 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(()), + (def_a, def_b) if is_composite(def_a) && is_composite(def_b) => Ok(()), _ => Err(mismatching_type_diagnostic()), } } From 91f10dbfa03dfc57700537681d48e6c30ccabbe4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Wed, 31 Jan 2024 12:33:33 +0100 Subject: [PATCH 12/42] Skip some unnecessary arc clones --- .../src/validation/selection.rs | 50 +++++++++---------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/crates/apollo-compiler/src/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs index f31ecd67..6c327b31 100644 --- a/crates/apollo-compiler/src/validation/selection.rs +++ b/crates/apollo-compiler/src/validation/selection.rs @@ -16,18 +16,18 @@ fn pair_combinations(slice: &[T]) -> impl Iterator { } /// Represents a field selected against a parent type. -#[derive(Debug, Clone)] -pub(crate) struct FieldSelection { +#[derive(Debug, Clone, Copy)] +pub(crate) struct FieldSelection<'a> { /// The type of the selection set this field selection is part of. - pub parent_type: ast::NamedType, - pub field: Node, + pub parent_type: &'a ast::NamedType, + pub field: &'a Node, } /// Expand one or more selection sets to a list of all fields selected. -pub(crate) fn expand_selections( - fragments: &IndexMap>, - selection_sets: &[&executable::SelectionSet], -) -> Vec { +pub(crate) fn expand_selections<'doc>( + fragments: &'doc IndexMap>, + selection_sets: &[&'doc executable::SelectionSet], +) -> Vec> { let mut selections = vec![]; let mut queue: VecDeque<&executable::SelectionSet> = selection_sets.iter().copied().collect(); let mut seen_fragments = HashSet::new(); @@ -36,8 +36,8 @@ pub(crate) fn expand_selections( for selection in &next_set.selections { match selection { executable::Selection::Field(field) => selections.push(FieldSelection { - parent_type: next_set.ty.clone(), - field: field.clone(), + parent_type: &next_set.ty, + field, }), executable::Selection::InlineFragment(spread) => { queue.push_back(&spread.selection_set) @@ -132,7 +132,7 @@ pub(crate) fn fields_in_set_can_merge( fields: &[FieldSelection], diagnostics: &mut Vec, ) { - for fields_for_name in group_selections_by_output_name(fields.iter().cloned()).values() { + for fields_for_name in group_selections_by_output_name(fields.iter().copied()).values() { for (field_a, field_b) in pair_combinations(fields_for_name) { // Covers steps 3-5 of the spec algorithm. if let Err(err) = same_output_type_shape(schema, field_a.clone(), field_b.clone()) { @@ -158,7 +158,7 @@ pub(crate) fn fields_in_set_can_merge( diagnostics: &mut Vec, ) { for (_, fields_for_name) in - group_selections_by_output_name(fields.iter().cloned()).into_iter() + group_selections_by_output_name(fields.iter().copied()).into_iter() { for fields_for_parents in group_selections_by_common_parents(schema, fields_for_name.into_iter()) @@ -171,7 +171,7 @@ pub(crate) fn fields_in_set_can_merge( continue; }; for (field_a, field_b) in std::iter::repeat(field_a).zip(rest.iter()) { - if let Err(diagnostic) = same_name_and_arguments(field_a, field_b) { + if let Err(diagnostic) = same_name_and_arguments(*field_a, *field_b) { diagnostics.push(diagnostic); continue; } @@ -188,14 +188,14 @@ pub(crate) fn fields_in_set_can_merge( } } - fn group_selections_by_common_parents( + fn group_selections_by_common_parents<'doc>( schema: &schema::Schema, - selections: impl Iterator, - ) -> Vec> { + selections: impl Iterator>, + ) -> Vec>> { let mut abstract_parents = vec![]; let mut concrete_parents = HashMap::<_, Vec<_>>::new(); for selection in selections { - match schema.types.get(&selection.parent_type) { + match schema.types.get(selection.parent_type) { Some(schema::ExtendedType::Object(object)) => { concrete_parents .entry(object.name.clone()) @@ -215,16 +215,16 @@ pub(crate) fn fields_in_set_can_merge( concrete_parents .into_values() .map(|mut group| { - group.extend(abstract_parents.iter().cloned()); + group.extend(abstract_parents.iter().copied()); group }) .collect() } } - fn group_selections_by_output_name( - selections: impl Iterator, - ) -> HashMap> { + fn group_selections_by_output_name<'doc>( + selections: impl Iterator>, + ) -> HashMap>> { let mut map = HashMap::new(); for selection in selections { match map.entry(selection.field.response_key().clone()) { @@ -241,8 +241,8 @@ pub(crate) fn fields_in_set_can_merge( fn same_output_type_shape( schema: &schema::Schema, - selection_a: FieldSelection, - selection_b: FieldSelection, + selection_a: FieldSelection<'_>, + selection_b: FieldSelection<'_>, ) -> Result<(), ValidationError> { let field_a = &selection_a.field.definition; let field_b = &selection_b.field.definition; @@ -321,8 +321,8 @@ pub(crate) fn fields_in_set_can_merge( /// Check if two field selections from the same type are the same, so the fields can be merged. fn same_name_and_arguments( - field_a: &FieldSelection, - field_b: &FieldSelection, + field_a: FieldSelection<'_>, + field_b: FieldSelection<'_>, ) -> Result<(), ValidationError> { debug_assert_eq!(field_a.parent_type, field_b.parent_type); From 763878de49f97cf884574967cdfee6736c158ff7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Wed, 31 Jan 2024 16:09:33 +0100 Subject: [PATCH 13/42] deref --- crates/apollo-compiler/src/validation/selection.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/apollo-compiler/src/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs index 6c327b31..e75ca335 100644 --- a/crates/apollo-compiler/src/validation/selection.rs +++ b/crates/apollo-compiler/src/validation/selection.rs @@ -135,7 +135,7 @@ pub(crate) fn fields_in_set_can_merge( for fields_for_name in group_selections_by_output_name(fields.iter().copied()).values() { for (field_a, field_b) in pair_combinations(fields_for_name) { // Covers steps 3-5 of the spec algorithm. - if let Err(err) = same_output_type_shape(schema, field_a.clone(), field_b.clone()) { + if let Err(err) = same_output_type_shape(schema, *field_a, *field_b) { diagnostics.push(err); continue; } From ddcc80634b1306884cdef5e3b00f8a35f731bce2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Wed, 31 Jan 2024 16:25:26 +0100 Subject: [PATCH 14/42] The individual checks can now both be implemented linearly --- .../src/validation/selection.rs | 30 +++---------------- 1 file changed, 4 insertions(+), 26 deletions(-) diff --git a/crates/apollo-compiler/src/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs index e75ca335..ac9462a9 100644 --- a/crates/apollo-compiler/src/validation/selection.rs +++ b/crates/apollo-compiler/src/validation/selection.rs @@ -6,15 +6,6 @@ use indexmap::IndexMap; use std::collections::{hash_map::Entry, HashMap}; use std::collections::{HashSet, VecDeque}; -/// Return all possible unordered combinations of 2 elements from a slice. -fn pair_combinations(slice: &[T]) -> impl Iterator { - slice - .iter() - .enumerate() - // Final element will zip with the empty slice and produce no result. - .flat_map(|(index, element)| std::iter::repeat(element).zip(&slice[index + 1..])) -} - /// Represents a field selected against a parent type. #[derive(Debug, Clone, Copy)] pub(crate) struct FieldSelection<'a> { @@ -133,7 +124,10 @@ pub(crate) fn fields_in_set_can_merge( diagnostics: &mut Vec, ) { for fields_for_name in group_selections_by_output_name(fields.iter().copied()).values() { - for (field_a, field_b) in pair_combinations(fields_for_name) { + let Some((field_a, rest)) = fields_for_name.split_first() else { + continue; + }; + for (field_a, field_b) in std::iter::repeat(field_a).zip(rest.iter()) { // Covers steps 3-5 of the spec algorithm. if let Err(err) = same_output_type_shape(schema, *field_a, *field_b) { diagnostics.push(err); @@ -446,19 +440,3 @@ pub(crate) fn validate_selections( diagnostics } - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn pair_combinations_test() { - let pairs = pair_combinations::(&[1, 2, 3, 4]).collect::>(); - assert_eq!( - pairs, - &[(&1, &2), (&1, &3), (&1, &4), (&2, &3), (&2, &4), (&3, &4)] - ); - let pairs = pair_combinations(&["a", "a", "a"]).collect::>(); - assert_eq!(pairs, &[(&"a", &"a"), (&"a", &"a"), (&"a", &"a")]); - } -} From 6b9b778565a0b7203ed13a51187d828fd4386552 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Wed, 31 Jan 2024 16:26:31 +0100 Subject: [PATCH 15/42] Add many repeated fields validation benchmark --- .../benches/fields_validation.rs | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 crates/apollo-compiler/benches/fields_validation.rs diff --git a/crates/apollo-compiler/benches/fields_validation.rs b/crates/apollo-compiler/benches/fields_validation.rs new file mode 100644 index 00000000..9c1967b2 --- /dev/null +++ b/crates/apollo-compiler/benches/fields_validation.rs @@ -0,0 +1,40 @@ +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); + }); + }); +} + +criterion_group!(benches, bench_many_same_field, bench_many_same_nested_field); +criterion_main!(benches); From 7d9c58b462a6703ae7d8d4f5a9d4f138c12c4fa0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Thu, 1 Feb 2024 16:10:15 +0100 Subject: [PATCH 16/42] field(a:1, b:2) and field(b:2, a:1) should compare equal --- .../src/validation/selection.rs | 32 ++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/crates/apollo-compiler/src/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs index ac9462a9..4b01ee87 100644 --- a/crates/apollo-compiler/src/validation/selection.rs +++ b/crates/apollo-compiler/src/validation/selection.rs @@ -364,7 +364,7 @@ pub(crate) fn fields_in_set_can_merge( return Err(conflicting_field_argument(Some(arg), None)); }; - if other_arg.value != arg.value { + if !same_value(&other_arg.value, &arg.value) { return Err(conflicting_field_argument(Some(arg), Some(other_arg))); } } @@ -377,6 +377,36 @@ pub(crate) fn fields_in_set_can_merge( 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)) + }) + } + _ => false, + } + } } pub(crate) fn validate_selection_set( From 29d15db656d3f346bf07c2267a90a1daee2d28f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Thu, 1 Feb 2024 16:12:58 +0100 Subject: [PATCH 17/42] Remove now-invalid assertion This function can take one field from an object type, and another field from an interface type--then their parent types are not equal. --- crates/apollo-compiler/src/validation/selection.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/crates/apollo-compiler/src/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs index 4b01ee87..acbcd691 100644 --- a/crates/apollo-compiler/src/validation/selection.rs +++ b/crates/apollo-compiler/src/validation/selection.rs @@ -318,8 +318,6 @@ pub(crate) fn fields_in_set_can_merge( field_a: FieldSelection<'_>, field_b: FieldSelection<'_>, ) -> Result<(), ValidationError> { - debug_assert_eq!(field_a.parent_type, field_b.parent_type); - // 2bi. fieldA and fieldB must have identical field names. if field_a.field.name != field_b.field.name { return Err(ValidationError::new( From 03c2eb7d7334da5896ff0c42e37e999688bb6759 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Thu, 1 Feb 2024 16:17:52 +0100 Subject: [PATCH 18/42] Port field merging tests from graphql-js --- .../tests/validation/field_merging.rs | 1490 +++++++++++++++++ .../apollo-compiler/tests/validation/mod.rs | 1 + 2 files changed, 1491 insertions(+) create mode 100644 crates/apollo-compiler/tests/validation/field_merging.rs 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..43b6b376 --- /dev/null +++ b/crates/apollo-compiler/tests/validation/field_merging.rs @@ -0,0 +1,1490 @@ +//! 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: operation must not select different fields to the same alias `fido` + ╭─[query.graphql:3:3] + │ + 2 │ fido: name + │ ─────┬──── + │ ╰────── field `fido` is selected from field `name` here + 3 │ fido: nickname + │ ───────┬────── + │ ╰──────── but the same field `fido` is also selected from field `nickname` here + ───╯ + Error: operation must not select different fields to the same alias `fido` + ╭─[query.graphql:3:3] + │ + 2 │ fido: name + │ ─────┬──── + │ ╰────── field `fido` is selected from field `name` here + 3 │ fido: nickname + │ ───────┬────── + │ ╰──────── but the same field `fido` is also selected from field `nickname` here + ───╯ + "#]], + ); +} + +#[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: operation must not select different fields to the same alias `name` + ╭─[query.graphql:3:3] + │ + 2 │ name: nickname + │ ───────┬────── + │ ╰──────── field `name` is selected from field `nickname` here + 3 │ name + │ ──┬─ + │ ╰─── but the same field `name` is also selected from field `name` here + ───╯ + Error: operation must not select different fields to the same alias `name` + ╭─[query.graphql:3:3] + │ + 2 │ name: nickname + │ ───────┬────── + │ ╰──────── field `name` is selected from field `nickname` here + 3 │ name + │ ──┬─ + │ ╰─── but the same field `name` is also selected from field `name` here + ───╯ + "#]], + ); +} + +#[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 field name `doesKnowCommand` + ╭─[query.graphql:3:3] + │ + 2 │ doesKnowCommand + │ ───────┬─────── + │ ╰───────── but argument `dogCommand` is not provided here + 3 │ doesKnowCommand(dogCommand: HEEL) + │ ────────────────┬──────────────── + │ ╰────────────────── field `doesKnowCommand` is selected with argument `dogCommand` 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` + ╭─[query.graphql:3:3] + │ + 2 │ doesKnowCommand + │ ───────┬─────── + │ ╰───────── but argument `dogCommand` is not provided here + 3 │ doesKnowCommand(dogCommand: HEEL) + │ ────────────────┬──────────────── + │ ╰────────────────── field `doesKnowCommand` is selected with argument `dogCommand` 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. + ───╯ + "#]], + ); +} + +#[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 field name `doesKnowCommand` + ╭─[query.graphql:3:3] + │ + 2 │ doesKnowCommand(dogCommand: SIT) + │ ────────────────┬─────────────── + │ ╰───────────────── field `doesKnowCommand` is selected with argument `dogCommand` here + 3 │ 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` + ╭─[query.graphql:3:3] + │ + 2 │ doesKnowCommand(dogCommand: SIT) + │ ────────────────┬─────────────── + │ ╰───────────────── field `doesKnowCommand` is selected with argument `dogCommand` here + 3 │ 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. + ───╯ + "#]], + ); +} + +#[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 field name `doesKnowCommand` + ╭─[query.graphql:3:3] + │ + 2 │ doesKnowCommand(dogCommand: SIT) + │ ────────────────┬─────────────── + │ ╰───────────────── field `doesKnowCommand` provides one argument value here + 3 │ 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` + ╭─[query.graphql:3:3] + │ + 2 │ doesKnowCommand(dogCommand: SIT) + │ ────────────────┬─────────────── + │ ╰───────────────── field `doesKnowCommand` provides one argument value here + 3 │ 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. + ───╯ + "#]], + ); +} + +#[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 field name `isAtLocation` + ╭─[query.graphql:3:3] + │ + 2 │ isAtLocation(x: 0) + │ ─────────┬──────── + │ ╰────────── field `isAtLocation` is selected with argument `x` here + 3 │ 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. + ───╯ + Error: operation must not provide conflicting field arguments for the same field name `isAtLocation` + ╭─[query.graphql:3:3] + │ + 2 │ isAtLocation(x: 0) + │ ─────────┬──────── + │ ╰────────── field `isAtLocation` is selected with argument `x` here + 3 │ 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. + ───╯ + "#]], + ); +} + +#[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 }) + } + } + "#, + ); +} + +#[test] +fn conflicts_in_fragments() { + expect_errors( + r#" + { + ...A + ...B + } + fragment A on Type { + x: a + } + fragment B on Type { + x: b + } + "#, + expect![[r#" + Error: cannot find fragment `A` in this document + ╭─[query.graphql:2:3] + │ + 2 │ ...A + │ ──┬─ + │ ╰─── fragment `A` is not defined + ───╯ + Error: cannot find fragment `B` in this document + ╭─[query.graphql:3:3] + │ + 3 │ ...B + │ ──┬─ + │ ╰─── fragment `B` is not defined + ───╯ + Error: type condition `Type` of fragment `A` is not a type defined in the schema + ╭─[query.graphql:5:15] + │ + 5 │ fragment A on Type { + │ ──┬─ + │ ╰─── type condition here + ───╯ + Error: type condition `Type` of fragment `B` is not a type defined in the schema + ╭─[query.graphql:8:15] + │ + 8 │ fragment B on Type { + │ ──┬─ + │ ╰─── type condition here + ───╯ + "#]], + ); +} + +#[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: type `QueryRoot` does not have a field `f1` + ╭─[query.graphql:2:3] + │ + 2 │ f1 { + │ ─┬ + │ ╰── field `f1` selected here + │ + ├─[schema.graphql:95:6] + │ + 95 │ type QueryRoot { + │ ────┬──── + │ ╰────── type `QueryRoot` defined here + │ + │ Note: path to the field: `query → f1` + ────╯ + Error: type `QueryRoot` does not have a field `f2` + ╭─[query.graphql:6:3] + │ + 6 │ f2 { + │ ─┬ + │ ╰── field `f2` selected here + │ + ├─[schema.graphql:95:6] + │ + 95 │ type QueryRoot { + │ ────┬──── + │ ╰────── type `QueryRoot` defined here + │ + │ Note: path to the field: `query → f2` + ────╯ + Error: type `QueryRoot` does not have a field `f3` + ╭─[query.graphql:10:3] + │ + 10 │ f3 { + │ ─┬ + │ ╰── field `f3` selected here + │ + ├─[schema.graphql:95:6] + │ + 95 │ type QueryRoot { + │ ────┬──── + │ ╰────── type `QueryRoot` defined here + │ + │ Note: path to the field: `query → f3` + ────╯ + Error: type condition `Type` of fragment `A` is not a type defined in the schema + ╭─[query.graphql:16:15] + │ + 16 │ fragment A on Type { + │ ──┬─ + │ ╰─── type condition here + ────╯ + Error: type condition `Type` of fragment `B` is not a type defined in the schema + ╭─[query.graphql:19:15] + │ + 19 │ fragment B on Type { + │ ──┬─ + │ ╰─── type condition here + ────╯ + "#]], + ); +} + +#[test] +fn deep_conflict() { + expect_errors( + r#" + { + field { + x: a + }, + field { + x: b + } + } + "#, + expect![[r#" + Error: type `QueryRoot` does not have a field `field` + ╭─[query.graphql:2:3] + │ + 2 │ field { + │ ──┬── + │ ╰──── field `field` selected here + │ + ├─[schema.graphql:95:6] + │ + 95 │ type QueryRoot { + │ ────┬──── + │ ╰────── type `QueryRoot` defined here + │ + │ Note: path to the field: `query → field` + ────╯ + Error: type `QueryRoot` does not have a field `field` + ╭─[query.graphql:5:3] + │ + 5 │ field { + │ ──┬── + │ ╰──── field `field` selected here + │ + ├─[schema.graphql:95:6] + │ + 95 │ type QueryRoot { + │ ────┬──── + │ ╰────── type `QueryRoot` defined here + │ + │ Note: path to the field: `query → field` + ────╯ + "#]], + ); +} + +#[test] +fn deep_conflict_multiple_issues() { + expect_errors( + r#" + { + field { + x: a + y: c + }, + field { + x: b + y: d + } + } + "#, + expect![[r#" + Error: type `QueryRoot` does not have a field `field` + ╭─[query.graphql:2:3] + │ + 2 │ field { + │ ──┬── + │ ╰──── field `field` selected here + │ + ├─[schema.graphql:95:6] + │ + 95 │ type QueryRoot { + │ ────┬──── + │ ╰────── type `QueryRoot` defined here + │ + │ Note: path to the field: `query → field` + ────╯ + Error: type `QueryRoot` does not have a field `field` + ╭─[query.graphql:6:3] + │ + 6 │ field { + │ ──┬── + │ ╰──── field `field` selected here + │ + ├─[schema.graphql:95:6] + │ + 95 │ type QueryRoot { + │ ────┬──── + │ ╰────── type `QueryRoot` defined here + │ + │ Note: path to the field: `query → field` + ────╯ + "#]], + ); +} + +#[test] +fn very_deep_conflict() { + expect_errors( + r#" + { + field { + deepField { + x: a + } + }, + field { + deepField { + x: b + } + } + } + "#, + expect![[r#" + Error: type `QueryRoot` does not have a field `field` + ╭─[query.graphql:2:3] + │ + 2 │ field { + │ ──┬── + │ ╰──── field `field` selected here + │ + ├─[schema.graphql:95:6] + │ + 95 │ type QueryRoot { + │ ────┬──── + │ ╰────── type `QueryRoot` defined here + │ + │ Note: path to the field: `query → field` + ────╯ + Error: type `QueryRoot` does not have a field `field` + ╭─[query.graphql:7:3] + │ + 7 │ field { + │ ──┬── + │ ╰──── field `field` selected here + │ + ├─[schema.graphql:95:6] + │ + 95 │ type QueryRoot { + │ ────┬──── + │ ╰────── type `QueryRoot` defined here + │ + │ Note: path to the field: `query → field` + ────╯ + "#]], + ); +} + +#[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: type `QueryRoot` does not have a field `field` + ╭─[query.graphql:2:3] + │ + 2 │ field { + │ ──┬── + │ ╰──── field `field` selected here + │ + ├─[schema.graphql:95:6] + │ + 95 │ type QueryRoot { + │ ────┬──── + │ ╰────── type `QueryRoot` defined here + │ + │ Note: path to the field: `query → field` + ────╯ + Error: type `QueryRoot` does not have a field `field` + ╭─[query.graphql:5:3] + │ + 5 │ field { + │ ──┬── + │ ╰──── field `field` selected here + │ + ├─[schema.graphql:95:6] + │ + 95 │ type QueryRoot { + │ ────┬──── + │ ╰────── type `QueryRoot` defined here + │ + │ Note: path to the field: `query → field` + ────╯ + Error: type condition `T` of fragment `F` is not a type defined in the schema + ╭─[query.graphql:9:15] + │ + 9 │ fragment F on T { + │ ┬ + │ ╰── type condition here + ───╯ + Error: type condition `T` of fragment `G` is not a type defined in the schema + ╭─[query.graphql:13:15] + │ + 13 │ fragment G on T { + │ ┬ + │ ╰── type condition here + ────╯ + Error: type condition `T` of fragment `I` is not a type defined in the schema + ╭─[query.graphql:16:15] + │ + 16 │ fragment I on T { + │ ┬ + │ ╰── type condition here + ────╯ + Error: type condition `T` of fragment `J` is not a type defined in the schema + ╭─[query.graphql:20:15] + │ + 20 │ fragment J on T { + │ ┬ + │ ╰── type condition here + ────╯ + "#]], + ); +} + +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 field name `scalar` + ╭─[query.graphql:7:7] + │ + 4 │ scalar + │ ───┬── + │ ╰──── `scalar` has type `Int` here + │ + 7 │ scalar + │ ───┬── + │ ╰──── but the same field name has type `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 field name `scalar` + ╭─[query.graphql:7:7] + │ + 4 │ scalar + │ ───┬── + │ ╰──── `scalar` has type `Int` here + │ + 7 │ scalar + │ ───┬── + │ ╰──── but the same field name has type `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 field name `scalar` + ╭─[query.graphql:7:7] + │ + 4 │ scalar + │ ───┬── + │ ╰──── `scalar` has type `String!` here + │ + 7 │ scalar + │ ───┬── + │ ╰──── but the same field name has type `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 field name `box` + ╭─[query.graphql:9:7] + │ + 4 │ ╭───▶ box: listStringBox { + ┆ ┆ + 6 │ ├───▶ } + │ │ + │ ╰─────────────── `box` has type `[StringBox]` here + │ + 9 │ ╭─▶ box: stringBox { + ┆ ┆ + 11 │ ├─▶ } + │ │ + │ ╰───────────── but the same field name has type `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 field name `box` + ╭─[query.graphql:9:7] + │ + 4 │ ╭─▶ box: stringBox { + ┆ ┆ + 6 │ ├─▶ } + │ │ + │ ╰───────────── `box` has type `StringBox` here + │ + 9 │ ╭───▶ box: listStringBox { + ┆ ┆ + 11 │ ├───▶ } + │ │ + │ ╰─────────────── but the same field name has type `[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: operation must not select different fields to the same alias `val` + ╭─[query.graphql:6:9] + │ + 5 │ val: scalar + │ ─────┬───── + │ ╰─────── field `val` is selected from field `scalar` here + 6 │ val: unrelatedField + │ ─────────┬───────── + │ ╰─────────── but the same field `val` is also selected from field `unrelatedField` here + ───╯ + "#]], + ); + } + + #[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 field name `scalar` + ╭─[query.graphql:10:9] + │ + 5 │ scalar + │ ───┬── + │ ╰──── `scalar` has type `String` here + │ + 10 │ scalar + │ ───┬── + │ ╰──── but the same field name has type `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 field name `id` + ╭─[query.graphql:15:7] + │ + 6 │ id: name + │ ────┬─── + │ ╰───── `id` has type `String` here + │ + 15 │ id + │ ─┬ + │ ╰── but the same field name has type `ID` here + ────╯ + Error: operation must not select different fields to the same alias `id` + ╭─[query.graphql:15:7] + │ + 6 │ id: name + │ ────┬─── + │ ╰───── field `id` is selected from field `name` here + │ + 15 │ id + │ ─┬ + │ ╰── but the same field `id` is also selected from field `id` here + ────╯ + "#]], + ); + } + + #[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; From fb50b0d44da1add4679c06bc449e5c67e9dc359b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Thu, 1 Feb 2024 16:33:45 +0100 Subject: [PATCH 19/42] Reduce duplicate validation errors from field merging Field merging validation was applied to operations and fragment definitions. Selection sets of fragment definitions must always be checked in the context of an operation or other fragment, and in the end it always leads to an operation. So we strictly only need to validate operations to find all issues in all reachable fragments. This can still output duplicate errors if a fragment contains a conflict internally, and is used in multiple operations. --- .../src/executable/validation.rs | 9 --- .../0074_merge_identical_fields.txt | 40 ----------- .../0075_merge_conflicting_args.txt | 60 ---------------- .../0076_merge_differing_responses.txt | 11 --- .../tests/validation/field_merging.rs | 68 ------------------- 5 files changed, 188 deletions(-) diff --git a/crates/apollo-compiler/src/executable/validation.rs b/crates/apollo-compiler/src/executable/validation.rs index 0f1d1bbc..960db8af 100644 --- a/crates/apollo-compiler/src/executable/validation.rs +++ b/crates/apollo-compiler/src/executable/validation.rs @@ -45,15 +45,6 @@ fn validate_with_schema( ); } - for fragment in document.fragments.values() { - crate::validation::selection::fields_in_set_can_merge( - schema, - document, - &fragment.selection_set, - &mut compiler_diagnostics, - ); - } - for diagnostic in compiler_diagnostics { errors.push(diagnostic.location, Details::CompilerDiagnostic(diagnostic)) } 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 28a8e828..2194f163 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 @@ -18,46 +18,6 @@ Error: operation must not select different fields to the same alias `name` │ ──┬─ │ ╰─── but the same field `name` is also selected from field `name` here ────╯ -Error: operation must not select different types using the same field name `name` - ╭─[0074_merge_identical_fields.graphql:19:3] - │ - 18 │ name: nickname - │ ───────┬────── - │ ╰──────── `name` has type `String` here - 19 │ name - │ ──┬─ - │ ╰─── but the same field name has type `String!` here -────╯ -Error: operation must not select different fields to the same alias `name` - ╭─[0074_merge_identical_fields.graphql:19:3] - │ - 18 │ name: nickname - │ ───────┬────── - │ ╰──────── field `name` is selected from field `nickname` here - 19 │ name - │ ──┬─ - │ ╰─── but the same field `name` is also selected from field `name` here -────╯ -Error: operation must not select different types using the same field name `fido` - ╭─[0074_merge_identical_fields.graphql:24:3] - │ - 23 │ fido: name - │ ─────┬──── - │ ╰────── `fido` has type `String!` here - 24 │ fido: nickname - │ ───────┬────── - │ ╰──────── but the same field name has type `String` here -────╯ -Error: operation must not select different fields to the same alias `fido` - ╭─[0074_merge_identical_fields.graphql:24:3] - │ - 23 │ fido: name - │ ─────┬──── - │ ╰────── field `fido` is selected from field `name` here - 24 │ fido: nickname - │ ───────┬────── - │ ╰──────── but the same field `fido` is also selected from field `nickname` here -────╯ Error: operation must not select different types using the same field name `fido` ╭─[0074_merge_identical_fields.graphql:24:3] │ 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..af396110 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 @@ -10,30 +10,6 @@ Error: operation must not provide conflicting field arguments for the same field │ │ 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: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` - ╭─[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` ╭─[0075_merge_conflicting_args.graphql:52:3] │ @@ -58,30 +34,6 @@ Error: operation must not provide conflicting field arguments for the same field │ │ 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: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` ╭─[0075_merge_conflicting_args.graphql:62:3] │ @@ -106,16 +58,4 @@ Error: operation must not provide conflicting field arguments for the same field │ │ 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. -────╯ 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..6538a2bc 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 @@ -9,15 +9,4 @@ Error: operation must not select different types using the same field name `some │ ──────────┬────────── │ ╰──────────── 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 -────╯ diff --git a/crates/apollo-compiler/tests/validation/field_merging.rs b/crates/apollo-compiler/tests/validation/field_merging.rs index 43b6b376..831b92da 100644 --- a/crates/apollo-compiler/tests/validation/field_merging.rs +++ b/crates/apollo-compiler/tests/validation/field_merging.rs @@ -265,16 +265,6 @@ fn same_aliases_with_different_field_targets() { │ ───────┬────── │ ╰──────── but the same field `fido` is also selected from field `nickname` here ───╯ - Error: operation must not select different fields to the same alias `fido` - ╭─[query.graphql:3:3] - │ - 2 │ fido: name - │ ─────┬──── - │ ╰────── field `fido` is selected from field `name` here - 3 │ fido: nickname - │ ───────┬────── - │ ╰──────── but the same field `fido` is also selected from field `nickname` here - ───╯ "#]], ); } @@ -319,16 +309,6 @@ fn alias_masking_direct_field_access() { │ ──┬─ │ ╰─── but the same field `name` is also selected from field `name` here ───╯ - Error: operation must not select different fields to the same alias `name` - ╭─[query.graphql:3:3] - │ - 2 │ name: nickname - │ ───────┬────── - │ ╰──────── field `name` is selected from field `nickname` here - 3 │ name - │ ──┬─ - │ ╰─── but the same field `name` is also selected from field `name` here - ───╯ "#]], ); } @@ -357,18 +337,6 @@ fn different_args_second_adds_argument() { │ │ 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` - ╭─[query.graphql:3:3] - │ - 2 │ doesKnowCommand - │ ───────┬─────── - │ ╰───────── but argument `dogCommand` is not provided here - 3 │ doesKnowCommand(dogCommand: HEEL) - │ ────────────────┬──────────────── - │ ╰────────────────── field `doesKnowCommand` is selected with argument `dogCommand` 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. - ───╯ "#]], ); } @@ -397,18 +365,6 @@ fn different_args_second_missess_argument() { │ │ 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` - ╭─[query.graphql:3:3] - │ - 2 │ doesKnowCommand(dogCommand: SIT) - │ ────────────────┬─────────────── - │ ╰───────────────── field `doesKnowCommand` is selected with argument `dogCommand` here - 3 │ 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. - ───╯ "#]], ); } @@ -437,18 +393,6 @@ fn conflicting_arg_values() { │ │ 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` - ╭─[query.graphql:3:3] - │ - 2 │ doesKnowCommand(dogCommand: SIT) - │ ────────────────┬─────────────── - │ ╰───────────────── field `doesKnowCommand` provides one argument value here - 3 │ 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. - ───╯ "#]], ); } @@ -477,18 +421,6 @@ fn conflicting_arg_names() { │ │ 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` - ╭─[query.graphql:3:3] - │ - 2 │ isAtLocation(x: 0) - │ ─────────┬──────── - │ ╰────────── field `isAtLocation` is selected with argument `x` here - 3 │ 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. - ───╯ "#]], ); } From 4828dcbb80c49919486b439d69202f16176918f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Thu, 1 Feb 2024 17:47:26 +0100 Subject: [PATCH 20/42] doc comments --- crates/apollo-compiler/src/validation/selection.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/crates/apollo-compiler/src/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs index acbcd691..3b56c5f3 100644 --- a/crates/apollo-compiler/src/validation/selection.rs +++ b/crates/apollo-compiler/src/validation/selection.rs @@ -117,6 +117,8 @@ pub(crate) fn fields_in_set_can_merge( same_response_shape_by_name(schema, &document.fragments, &fields, diagnostics); same_for_common_parents_by_name(schema, &document.fragments, &fields, diagnostics); + /// Given a set of fields, do all the fields that contribute to 1 output name have the same + /// shape? fn same_response_shape_by_name( schema: &schema::Schema, fragments: &IndexMap>, @@ -145,6 +147,8 @@ pub(crate) fn fields_in_set_can_merge( } } + /// Given a set of fields, do all the fields selecting from potentially overlapping types + /// select the same thing? fn same_for_common_parents_by_name( schema: &schema::Schema, fragments: &IndexMap>, @@ -182,6 +186,9 @@ pub(crate) fn fields_in_set_can_merge( } } + /// 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_selections_by_common_parents<'doc>( schema: &schema::Schema, selections: impl Iterator>, @@ -313,7 +320,7 @@ pub(crate) fn fields_in_set_can_merge( } } - /// Check if two field selections from the same type are the same, so the fields can be merged. + /// 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<'_>, From ee69e97b5066aa0313f89d9a8c52f40972beb654 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Fri, 2 Feb 2024 11:31:17 +0100 Subject: [PATCH 21/42] Take iterator as input for expand_selections --- .../src/validation/operation.rs | 6 ++++-- .../src/validation/selection.rs | 18 +++++++----------- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/crates/apollo-compiler/src/validation/operation.rs b/crates/apollo-compiler/src/validation/operation.rs index 5c4c9ab9..55279420 100644 --- a/crates/apollo-compiler/src/validation/operation.rs +++ b/crates/apollo-compiler/src/validation/operation.rs @@ -17,8 +17,10 @@ pub(crate) fn validate_subscription( diagnostics: &mut DiagnosticList, ) { if operation.is_subscription() { - let fields = - super::selection::expand_selections(&document.fragments, &[&operation.selection_set]); + let fields = super::selection::expand_selections( + &document.fragments, + std::iter::once(&operation.selection_set), + ); if fields.len() > 1 { diagnostics.push( diff --git a/crates/apollo-compiler/src/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs index 3b56c5f3..88aff3d4 100644 --- a/crates/apollo-compiler/src/validation/selection.rs +++ b/crates/apollo-compiler/src/validation/selection.rs @@ -17,10 +17,10 @@ pub(crate) struct FieldSelection<'a> { /// Expand one or more selection sets to a list of all fields selected. pub(crate) fn expand_selections<'doc>( fragments: &'doc IndexMap>, - selection_sets: &[&'doc executable::SelectionSet], + selection_sets: impl Iterator, ) -> Vec> { let mut selections = vec![]; - let mut queue: VecDeque<&executable::SelectionSet> = selection_sets.iter().copied().collect(); + let mut queue: VecDeque<&executable::SelectionSet> = selection_sets.collect(); let mut seen_fragments = HashSet::new(); while let Some(next_set) = queue.pop_front() { @@ -112,7 +112,7 @@ pub(crate) fn fields_in_set_can_merge( return; } - let fields = expand_selections(&document.fragments, &[selection_set]); + let fields = expand_selections(&document.fragments, std::iter::once(selection_set)); same_response_shape_by_name(schema, &document.fragments, &fields, diagnostics); same_for_common_parents_by_name(schema, &document.fragments, &fields, diagnostics); @@ -139,10 +139,8 @@ pub(crate) fn fields_in_set_can_merge( let nested_selection_sets = fields_for_name .iter() - .map(|selection| &selection.field.selection_set) - .filter(|set| !set.selections.is_empty()) - .collect::>(); - let merged_set = expand_selections(fragments, &nested_selection_sets); + .map(|selection| &selection.field.selection_set); + let merged_set = expand_selections(fragments, nested_selection_sets); same_response_shape_by_name(schema, fragments, &merged_set, diagnostics); } } @@ -177,10 +175,8 @@ pub(crate) fn fields_in_set_can_merge( let nested_selection_sets = fields_for_parents .iter() - .map(|selection| &selection.field.selection_set) - .filter(|set| !set.selections.is_empty()) - .collect::>(); - let merged_set = expand_selections(fragments, &nested_selection_sets); + .map(|selection| &selection.field.selection_set); + let merged_set = expand_selections(fragments, nested_selection_sets); same_for_common_parents_by_name(schema, fragments, &merged_set, diagnostics); } } From 862255e364231a0588c2757612077d25d53a0ea9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Mon, 5 Feb 2024 13:36:23 +0100 Subject: [PATCH 22/42] Add a cache for merged selection sets --- crates/apollo-compiler/Cargo.toml | 5 + .../benches/fields_validation.rs | 67 +- .../benches/testdata/supergraph_query.graphql | 9 +- crates/apollo-compiler/src/executable/mod.rs | 10 +- .../src/executable/validation.rs | 10 +- .../src/validation/selection.rs | 610 +++++++++++------- 6 files changed, 451 insertions(+), 260 deletions(-) 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 index 9c1967b2..b94ce37f 100644 --- a/crates/apollo-compiler/benches/fields_validation.rs +++ b/crates/apollo-compiler/benches/fields_validation.rs @@ -36,5 +36,68 @@ fn bench_many_same_nested_field(c: &mut Criterion) { }); } -criterion_group!(benches, bench_many_same_field, bench_many_same_nested_field); -criterion_main!(benches); +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_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 4983caf2..0f0659e6 100644 --- a/crates/apollo-compiler/src/executable/mod.rs +++ b/crates/apollo-compiler/src/executable/mod.rs @@ -69,20 +69,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,13 +93,13 @@ 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, diff --git a/crates/apollo-compiler/src/executable/validation.rs b/crates/apollo-compiler/src/executable/validation.rs index 960db8af..3b338276 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; @@ -35,14 +36,11 @@ fn validate_with_schema( document: &ExecutableDocument, ) { let mut compiler_diagnostics = vec![]; + + 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); - crate::validation::selection::fields_in_set_can_merge( - schema, - document, - &operation.selection_set, - &mut compiler_diagnostics, - ); + fields_in_set_can_merge.validate(&operation.selection_set, &mut compiler_diagnostics); } for diagnostic in compiler_diagnostics { diff --git a/crates/apollo-compiler/src/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs index 88aff3d4..51bf6262 100644 --- a/crates/apollo-compiler/src/validation/selection.rs +++ b/crates/apollo-compiler/src/validation/selection.rs @@ -3,11 +3,13 @@ use crate::validation::diagnostics::{DiagnosticData, ValidationError}; use crate::validation::{CycleError, FileId, RecursionGuard, RecursionStack, ValidationDatabase}; use crate::{ast, executable, schema, Node}; use indexmap::IndexMap; +use std::cell::OnceCell; use std::collections::{hash_map::Entry, HashMap}; use std::collections::{HashSet, VecDeque}; +use std::rc::Rc; /// Represents a field selected against a parent type. -#[derive(Debug, Clone, Copy)] +#[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, @@ -101,64 +103,269 @@ fn contains_any_fragment_cycle( } } -/// https://tech.new-work.se/graphql-overlapping-fields-can-be-merged-fast-ea6e92e0a01 -pub(crate) fn fields_in_set_can_merge( +/// 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<(), ValidationError> { + // 2bi. fieldA and fieldB must have identical field names. + if field_a.field.name != field_b.field.name { + return Err(ValidationError::new( + field_b.field.location(), + DiagnosticData::ConflictingFieldName { + field: field_a.field.response_key().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(), + }, + )); + } + + // 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(); + + let data = DiagnosticData::ConflictingFieldArgument { + // field_a and field_b have the same name so we can use either one. + field: field_b.field.name.clone(), + argument: arg.name.clone(), + original_selection: field_a.field.location(), + original_value: original_arg.map(|arg| (*arg.value).clone()), + redefined_selection: field_b.field.location(), + redefined_value: redefined_arg.map(|arg| (*arg.value).clone()), + }; + ValidationError::new(field_b.field.location(), data) + }; + + // Check if fieldB provides the same argument names and values as fieldA (order-independent). + for arg in &field_a.field.arguments { + let Some(other_arg) = field_b.field.argument_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 field_a.field.argument_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)) + }) + } + _ => false, + } +} + +fn same_output_type_shape( schema: &schema::Schema, - document: &executable::ExecutableDocument, - selection_set: &executable::SelectionSet, - diagnostics: &mut Vec, -) { - if contains_any_fragment_cycle(&document.fragments, selection_set) { - return; + selection_a: FieldSelection<'_>, + selection_b: FieldSelection<'_>, +) -> Result<(), ValidationError> { + 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 = || { + ValidationError::new( + selection_b.field.location(), + DiagnosticData::ConflictingFieldType { + field: selection_a.field.response_key().clone(), + original_selection: selection_a.field.location(), + original_type: field_a.ty.clone(), + redefined_selection: selection_b.field.location(), + redefined_type: field_b.ty.clone(), + }, + ) + }; + + // Steps 3 and 4 of the spec text unwrap both types simultaneously down to the named type. + // The apollo-rs representation of NonNull and Lists makes it tricky to follow the steps + // exactly. + // + // Instead we unwrap lists and non-null lists first, which leaves just a named type or a + // non-null named type... + while !type_a.is_named() || !type_b.is_named() { + // 4. If typeA or typeB is List. + // 4a. If typeA or typeB is not List, return false. + // 4b. Let typeA be the item type of typeA + // 4c. Let typeB be the item type of typeB + (type_a, type_b) = match (type_a, type_b) { + (ast::Type::List(type_a), ast::Type::List(type_b)) + | (ast::Type::NonNullList(type_a), ast::Type::NonNullList(type_b)) => { + (type_a.as_ref(), type_b.as_ref()) + } + (ast::Type::List(_), _) + | (_, ast::Type::List(_)) + | (ast::Type::NonNullList(_), _) + | (_, ast::Type::NonNullList(_)) => return Err(mismatching_type_diagnostic()), + // Now it's a named type. + (type_a, type_b) => (type_a, type_b), + }; } - let fields = expand_selections(&document.fragments, std::iter::once(selection_set)); + // Now we are down to two named types, we can check that they have the same nullability... + let (type_a, type_b) = match (type_a, type_b) { + (ast::Type::NonNullNamed(a), ast::Type::NonNullNamed(b)) => (a, b), + (ast::Type::Named(a), ast::Type::Named(b)) => (a, b), + _ => return Err(mismatching_type_diagnostic()), + }; + + let (Some(def_a), Some(def_b)) = (schema.types.get(type_a), schema.types.get(type_b)) else { + return Ok(()); // Cannot do much if we don't know the type + }; + + match (def_a, def_b) { + // 5. If typeA or typeB is Scalar or Enum. + ( + def_a @ (schema::ExtendedType::Scalar(_) | schema::ExtendedType::Enum(_)), + def_b @ (schema::ExtendedType::Scalar(_) | schema::ExtendedType::Enum(_)), + ) => { + // 5a. If typeA and typeB are the same type return true, otherwise return false. + if def_a == def_b { + Ok(()) + } else { + Err(mismatching_type_diagnostic()) + } + } + // 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()), + } +} - same_response_shape_by_name(schema, &document.fragments, &fields, diagnostics); - same_for_common_parents_by_name(schema, &document.fragments, &fields, diagnostics); +/// 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 check(&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( - schema: &schema::Schema, - fragments: &IndexMap>, - fields: &[FieldSelection], + &self, + validator: &mut FieldsInSetCanMerge<'_, 'doc>, diagnostics: &mut Vec, ) { - for fields_for_name in group_selections_by_output_name(fields.iter().copied()).values() { + // No need to do this if this field set has been checked before. + if self.same_response_shape_guard.check() { + return; + } + + for fields_for_name in self.group_by_output_name().values() { let Some((field_a, rest)) = fields_for_name.split_first() else { continue; }; for (field_a, field_b) in std::iter::repeat(field_a).zip(rest.iter()) { // Covers steps 3-5 of the spec algorithm. - if let Err(err) = same_output_type_shape(schema, *field_a, *field_b) { + if let Err(err) = same_output_type_shape(validator.schema, *field_a, *field_b) { diagnostics.push(err); continue; } } - let nested_selection_sets = fields_for_name + let mut nested_selection_sets = fields_for_name .iter() - .map(|selection| &selection.field.selection_set); - let merged_set = expand_selections(fragments, nested_selection_sets); - same_response_shape_by_name(schema, fragments, &merged_set, diagnostics); + .map(|selection| &selection.field.selection_set) + .filter(|set| !set.selections.is_empty()) + .peekable(); + // TODO cache + 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); + } } } /// Given a set of fields, do all the fields selecting from potentially overlapping types - /// select the same thing? + /// 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( - schema: &schema::Schema, - fragments: &IndexMap>, - fields: &[FieldSelection], + &self, + validator: &mut FieldsInSetCanMerge<'_, 'doc>, diagnostics: &mut Vec, ) { - for (_, fields_for_name) in - group_selections_by_output_name(fields.iter().copied()).into_iter() - { - for fields_for_parents in - group_selections_by_common_parents(schema, fields_for_name.into_iter()) - { + // No need to do this if this field set has been checked before. + if self.same_for_common_parents_guard.check() { + 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 @@ -173,240 +380,163 @@ pub(crate) fn fields_in_set_can_merge( } } - let nested_selection_sets = fields_for_parents + let mut nested_selection_sets = fields_for_parents .iter() - .map(|selection| &selection.field.selection_set); - let merged_set = expand_selections(fragments, nested_selection_sets); - same_for_common_parents_by_name(schema, fragments, &merged_set, diagnostics); + .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); + } } } } + fn group_by_output_name(&self) -> &HashMap>> { + self.grouped_by_output_names.get_or_init(|| { + let mut map = HashMap::new(); + for selection in &self.selections { + match map.entry(selection.field.response_key().clone()) { + Entry::Vacant(entry) => { + entry.insert(vec![*selection]); + } + Entry::Occupied(mut entry) => { + entry.get_mut().push(*selection); + } + } + } + map + }) + } + /// 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_selections_by_common_parents<'doc>( - schema: &schema::Schema, - selections: impl Iterator>, - ) -> Vec>> { - let mut abstract_parents = vec![]; - let mut concrete_parents = HashMap::<_, Vec<_>>::new(); - for selection in 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); + 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 = HashMap::<_, 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 concrete_parents.is_empty() { - vec![abstract_parents] - } else { - concrete_parents - .into_values() - .map(|mut group| { - group.extend(abstract_parents.iter().copied()); - group - }) - .collect() - } - } - - fn group_selections_by_output_name<'doc>( - selections: impl Iterator>, - ) -> HashMap>> { - let mut map = HashMap::new(); - for selection in selections { - match map.entry(selection.field.response_key().clone()) { - Entry::Vacant(entry) => { - entry.insert(vec![selection]); - } - Entry::Occupied(mut entry) => { - entry.get_mut().push(selection); - } + if concrete_parents.is_empty() { + vec![abstract_parents] + } else { + concrete_parents + .into_values() + .map(|mut group| { + group.extend(abstract_parents.iter().copied()); + group + }) + .collect() } - } - map + }) } +} - fn same_output_type_shape( - schema: &schema::Schema, - selection_a: FieldSelection<'_>, - selection_b: FieldSelection<'_>, - ) -> Result<(), ValidationError> { - 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 = || { - ValidationError::new( - selection_b.field.location(), - DiagnosticData::ConflictingFieldType { - field: selection_a.field.response_key().clone(), - original_selection: selection_a.field.location(), - original_type: field_a.ty.clone(), - redefined_selection: selection_b.field.location(), - redefined_type: field_b.ty.clone(), - }, - ) - }; - - // Steps 3 and 4 of the spec text unwrap both types simultaneously down to the named type. - // The apollo-rs representation of NonNull and Lists makes it tricky to follow the steps - // exactly. - // - // Instead we unwrap lists and non-null lists first, which leaves just a named type or a - // non-null named type... - while !type_a.is_named() || !type_b.is_named() { - // 4. If typeA or typeB is List. - // 4a. If typeA or typeB is not List, return false. - // 4b. Let typeA be the item type of typeA - // 4c. Let typeB be the item type of typeB - (type_a, type_b) = match (type_a, type_b) { - (ast::Type::List(type_a), ast::Type::List(type_b)) - | (ast::Type::NonNullList(type_a), ast::Type::NonNullList(type_b)) => { - (type_a.as_ref(), type_b.as_ref()) - } - (ast::Type::List(_), _) - | (_, ast::Type::List(_)) - | (ast::Type::NonNullList(_), _) - | (_, ast::Type::NonNullList(_)) => return Err(mismatching_type_diagnostic()), - // Now it's a named type. - (type_a, type_b) => (type_a, type_b), - }; - } - - // Now we are down to two named types, we can check that they have the same nullability... - let (type_a, type_b) = match (type_a, type_b) { - (ast::Type::NonNullNamed(a), ast::Type::NonNullNamed(b)) => (a, b), - (ast::Type::Named(a), ast::Type::Named(b)) => (a, b), - _ => return Err(mismatching_type_diagnostic()), - }; +/// 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()) + } +} - let (Some(def_a), Some(def_b)) = (schema.types.get(type_a), schema.types.get(type_b)) - else { - return Ok(()); // Cannot do much if we don't know the type - }; +/// Implements the `FieldsInSetCanMerge()` validation. +/// https://spec.graphql.org/draft/#sec-Field-Selection-Merging +/// +/// This uses the [validation algorithm described by Xing][0], which scales much better +/// with larger selection sets that may have many overlapping fields, and with widespread +/// use of fragments. +/// +/// [0]: 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>>, +} - match (def_a, def_b) { - // 5. If typeA or typeB is Scalar or Enum. - ( - def_a @ (schema::ExtendedType::Scalar(_) | schema::ExtendedType::Enum(_)), - def_b @ (schema::ExtendedType::Scalar(_) | schema::ExtendedType::Enum(_)), - ) => { - // 5a. If typeA and typeB are the same type return true, otherwise return false. - if def_a == def_b { - Ok(()) - } else { - Err(mismatching_type_diagnostic()) - } - } - // 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()), +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(), } } - /// 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<(), ValidationError> { - // 2bi. fieldA and fieldB must have identical field names. - if field_a.field.name != field_b.field.name { - return Err(ValidationError::new( - field_b.field.location(), - DiagnosticData::ConflictingFieldName { - field: field_a.field.response_key().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(), - }, - )); + pub(crate) fn validate( + &mut self, + root: &'doc executable::SelectionSet, + diagnostics: &mut Vec, + ) { + // We cannot safely check cyclical fragments + // TODO(@goto-bus-stop) - or maybe we can, given that a cycle will result in a cache hit? + if contains_any_fragment_cycle(&self.document.fragments, root) { + return; } - // 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(); - - let data = DiagnosticData::ConflictingFieldArgument { - // field_a and field_b have the same name so we can use either one. - field: field_b.field.name.clone(), - argument: arg.name.clone(), - original_selection: field_a.field.location(), - original_value: original_arg.map(|arg| (*arg.value).clone()), - redefined_selection: field_b.field.location(), - redefined_value: redefined_arg.map(|arg| (*arg.value).clone()), - }; - ValidationError::new(field_b.field.location(), data) - }; - - // Check if fieldB provides the same argument names and values as fieldA (order-independent). - for arg in &field_a.field.arguments { - let Some(other_arg) = field_b.field.argument_by_name(&arg.name) else { - return Err(conflicting_field_argument(Some(arg), None)); - }; + let fields = expand_selections(&self.document.fragments, std::iter::once(root)); + let set = self.lookup(fields); + set.same_response_shape_by_name(self, diagnostics); + set.same_for_common_parents_by_name(self, diagnostics); + } - 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 field_a.field.argument_by_name(&arg.name).is_none() { - return Err(conflicting_field_argument(None, Some(arg))); - }; - } + 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() + } - Ok(()) + fn same_for_common_parents_by_name( + &mut self, + selections: Vec>, + diagnostics: &mut Vec, + ) { + let field_set = self.lookup(selections); + field_set.same_for_common_parents_by_name(self, diagnostics); } - /// 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)) - }) - } - _ => false, - } + fn same_response_shape_by_name( + &mut self, + selections: Vec>, + diagnostics: &mut Vec, + ) { + let field_set = self.lookup(selections); + field_set.same_response_shape_by_name(self, diagnostics); } } From 82081de388ad2ff2ba3c2051d557b2ff7fbcde8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Mon, 5 Feb 2024 13:40:20 +0100 Subject: [PATCH 23/42] changelog --- crates/apollo-compiler/CHANGELOG.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/crates/apollo-compiler/CHANGELOG.md b/crates/apollo-compiler/CHANGELOG.md index 744a3add..3fe38b71 100644 --- a/crates/apollo-compiler/CHANGELOG.md +++ b/crates/apollo-compiler/CHANGELOG.md @@ -17,6 +17,17 @@ 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. + +[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 From 9fe6e38f78fe9884f52ea3b1ef10777e3f7bed68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Tue, 6 Feb 2024 09:42:55 +0100 Subject: [PATCH 24/42] Rework the ConflictingFieldNames diagnostic --- .../src/validation/diagnostics.rs | 30 +++++++++-------- .../src/validation/selection.rs | 20 +++++++++--- .../0074_merge_identical_fields.txt | 16 ++++++---- .../diagnostics/0077_merge_conflict_deep.txt | 8 +++-- .../0078_merge_conflict_nested_fragments.txt | 16 ++++++---- .../tests/validation/field_merging.rs | 32 ++++++++++++------- 6 files changed, 76 insertions(+), 46 deletions(-) diff --git a/crates/apollo-compiler/src/validation/diagnostics.rs b/crates/apollo-compiler/src/validation/diagnostics.rs index 67442df3..8a9811f9 100644 --- a/crates/apollo-compiler/src/validation/diagnostics.rs +++ b/crates/apollo-compiler/src/validation/diagnostics.rs @@ -231,14 +231,14 @@ pub(crate) enum DiagnosticData { redefined_selection: Option, redefined_value: Option, }, - #[error("operation must not select different fields to the same alias `{field}`")] + #[error("cannot select multiple fields into the same alias `{alias}`")] ConflictingFieldName { /// Name of the non-unique field. - field: Name, - original_selection: Option, - original_name: Name, - redefined_selection: Option, - redefined_name: Name, + alias: Name, + original_location: Option, + original_selection: TypeAttributeCoordinate, + conflicting_location: Option, + conflicting_selection: TypeAttributeCoordinate, }, #[error( "{} must have a composite type in its type condition", @@ -682,20 +682,22 @@ impl ValidationError { 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, + alias: field, original_selection, - original_name, - redefined_selection, - redefined_name, + original_location, + conflicting_selection, + conflicting_location, } => { report.with_label_opt( - *original_selection, - format_args!("field `{field}` is selected from field `{original_name}` here"), + *original_location, + format_args!("`{field}` is selected from `{original_selection}` here"), ); report.with_label_opt( - *redefined_selection, - format_args!("but the same field `{field}` is also selected from field `{redefined_name}` here"), + *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"); } DiagnosticData::InvalidFragmentTarget { name: _, ty } => { report.with_label_opt( diff --git a/crates/apollo-compiler/src/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs index 51bf6262..c0e7035b 100644 --- a/crates/apollo-compiler/src/validation/selection.rs +++ b/crates/apollo-compiler/src/validation/selection.rs @@ -1,4 +1,5 @@ use super::operation::OperationValidationConfig; +use crate::coordinate::TypeAttributeCoordinate; use crate::validation::diagnostics::{DiagnosticData, ValidationError}; use crate::validation::{CycleError, FileId, RecursionGuard, RecursionStack, ValidationDatabase}; use crate::{ast, executable, schema, Node}; @@ -16,6 +17,15 @@ pub(crate) struct FieldSelection<'a> { pub field: &'a Node, } +impl FieldSelection<'_> { + pub fn coordinate(&self) -> TypeAttributeCoordinate { + TypeAttributeCoordinate { + ty: self.parent_type.clone(), + attribute: self.field.name.clone(), + } + } +} + /// Expand one or more selection sets to a list of all fields selected. pub(crate) fn expand_selections<'doc>( fragments: &'doc IndexMap>, @@ -113,11 +123,11 @@ fn same_name_and_arguments( return Err(ValidationError::new( field_b.field.location(), DiagnosticData::ConflictingFieldName { - field: field_a.field.response_key().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(), + 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(), }, )); } 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 2194f163..cedc36a4 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 @@ -8,15 +8,17 @@ Error: operation must not select different types using the same field name `name │ ──┬─ │ ╰─── but the same field name has type `String!` here ────╯ -Error: operation must not select different fields to the same alias `name` +Error: cannot select multiple fields into the same alias `name` ╭─[0074_merge_identical_fields.graphql:19:3] │ 18 │ name: nickname │ ───────┬────── - │ ╰──────── field `name` is selected from field `nickname` here + │ ╰──────── `name` is selected from `Dog.nickname` here 19 │ name │ ──┬─ - │ ╰─── but the same field `name` is also selected from field `name` 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` ╭─[0074_merge_identical_fields.graphql:24:3] @@ -28,14 +30,16 @@ Error: operation must not select different types using the same field name `fido │ ───────┬────── │ ╰──────── but the same field name has type `String` here ────╯ -Error: operation must not select different fields to the same alias `fido` +Error: cannot select multiple fields into the same alias `fido` ╭─[0074_merge_identical_fields.graphql:24:3] │ 23 │ fido: name │ ─────┬──── - │ ╰────── field `fido` is selected from field `name` here + │ ╰────── `fido` is selected from `Dog.name` here 24 │ fido: nickname │ ───────┬────── - │ ╰──────── but the same field `fido` is also selected from field `nickname` 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/0077_merge_conflict_deep.txt b/crates/apollo-compiler/test_data/diagnostics/0077_merge_conflict_deep.txt index b5fd3782..6fb7fddb 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 multiple 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 9334264a..ecbf432e 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` +Error: cannot select multiple fields into the same alias `y` ╭─[0078_merge_conflict_nested_fragments.graphql:25:3] │ 25 │ y: c │ ──┬─ - │ ╰─── but the same field `y` is also selected from field `c` here + │ ╰─── `y` is selected from `T.c` here │ 28 │ y: d │ ──┬─ - │ ╰─── field `y` is 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 multiple 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/tests/validation/field_merging.rs b/crates/apollo-compiler/tests/validation/field_merging.rs index 831b92da..30af954d 100644 --- a/crates/apollo-compiler/tests/validation/field_merging.rs +++ b/crates/apollo-compiler/tests/validation/field_merging.rs @@ -255,15 +255,17 @@ fn same_aliases_with_different_field_targets() { { dog { ...sameAliasesWithDifferentFieldTargets } } "#, expect![[r#" - Error: operation must not select different fields to the same alias `fido` + Error: cannot select multiple fields into the same alias `fido` ╭─[query.graphql:3:3] │ 2 │ fido: name │ ─────┬──── - │ ╰────── field `fido` is selected from field `name` here + │ ╰────── `fido` is selected from `Dog.name` here 3 │ fido: nickname │ ───────┬────── - │ ╰──────── but the same field `fido` is also selected from field `nickname` 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 ───╯ "#]], ); @@ -299,15 +301,17 @@ fn alias_masking_direct_field_access() { { dog { ...aliasMaskingDirectFieldAccess } } "#, expect![[r#" - Error: operation must not select different fields to the same alias `name` + Error: cannot select multiple fields into the same alias `name` ╭─[query.graphql:3:3] │ 2 │ name: nickname │ ───────┬────── - │ ╰──────── field `name` is selected from field `nickname` here + │ ╰──────── `name` is selected from `Dog.nickname` here 3 │ name │ ──┬─ - │ ╰─── but the same field `name` is also selected from field `name` 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 ───╯ "#]], ); @@ -1225,15 +1229,17 @@ mod return_types { } "#, expect![[r#" - Error: operation must not select different fields to the same alias `val` + Error: cannot select multiple fields into the same alias `val` ╭─[query.graphql:6:9] │ 5 │ val: scalar │ ─────┬───── - │ ╰─────── field `val` is selected from field `scalar` here + │ ╰─────── `val` is selected from `StringBox.scalar` here 6 │ val: unrelatedField │ ─────────┬───────── - │ ╰─────────── but the same field `val` is also selected from field `unrelatedField` here + │ ╰─────────── `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 ───╯ "#]], ); @@ -1345,16 +1351,18 @@ mod return_types { │ ─┬ │ ╰── but the same field name has type `ID` here ────╯ - Error: operation must not select different fields to the same alias `id` + Error: cannot select multiple fields into the same alias `id` ╭─[query.graphql:15:7] │ 6 │ id: name │ ────┬─── - │ ╰───── field `id` is selected from field `name` here + │ ╰───── `id` is selected from `Node.name` here │ 15 │ id │ ─┬ - │ ╰── but the same field `id` is also selected from field `id` here + │ ╰── `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 ────╯ "#]], ); From aac0192bf6e72ed4280fdc4308932834b41f08ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Tue, 6 Feb 2024 09:51:28 +0100 Subject: [PATCH 25/42] mention fragment definitions validation change --- crates/apollo-compiler/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/apollo-compiler/CHANGELOG.md b/crates/apollo-compiler/CHANGELOG.md index 3fe38b71..51210aaa 100644 --- a/crates/apollo-compiler/CHANGELOG.md +++ b/crates/apollo-compiler/CHANGELOG.md @@ -23,6 +23,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - **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 From f3e808d94f3ee17ddbb168079d99e1215ffad1bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Tue, 6 Feb 2024 13:42:29 +0100 Subject: [PATCH 26/42] Tweak error reports for conflicting types and arguments --- .../src/validation/diagnostics.rs | 88 +++++++++++-------- .../src/validation/selection.rs | 21 +++-- .../0074_merge_identical_fields.txt | 12 +-- .../0075_merge_conflicting_args.txt | 30 +++---- .../0076_merge_differing_responses.txt | 6 +- .../0113_partially_overlapping_fragments.txt | 6 +- .../tests/validation/field_merging.rs | 66 +++++++------- 7 files changed, 121 insertions(+), 108 deletions(-) diff --git a/crates/apollo-compiler/src/validation/diagnostics.rs b/crates/apollo-compiler/src/validation/diagnostics.rs index 8a9811f9..d1b01874 100644 --- a/crates/apollo-compiler/src/validation/diagnostics.rs +++ b/crates/apollo-compiler/src/validation/diagnostics.rs @@ -3,6 +3,7 @@ use crate::ast::DirectiveLocation; use crate::ast::Name; use crate::ast::Type; use crate::ast::Value; +use crate::coordinate::FieldArgumentCoordinate; use crate::coordinate::SchemaCoordinate; use crate::coordinate::TypeAttributeCoordinate; use crate::diagnostic::CliReport; @@ -210,26 +211,27 @@ pub(crate) enum DiagnosticData { coordinate: TypeAttributeCoordinate, describe_type: &'static str, }, - #[error("operation must not select different types using the same field name `{field}`")] + #[error("operation must not select different types using the same name `{alias}`")] ConflictingFieldType { - /// Name of the non-unique field. - field: Name, - original_selection: Option, + /// Name or alias of the non-unique field. + alias: Name, + original_location: Option, + original_coordinate: TypeAttributeCoordinate, original_type: Type, - redefined_selection: Option, - redefined_type: Type, + conflicting_location: Option, + conflicting_coordinate: TypeAttributeCoordinate, + conflicting_type: Type, }, - #[error( - "operation must not provide conflicting field arguments for the same field name `{field}`" - )] + #[error("operation must not provide conflicting field arguments for the same name `{alias}`")] ConflictingFieldArgument { - /// Name of the non-unique field. - field: Name, - argument: Name, - original_selection: Option, + /// Name or alias of the non-unique field. + alias: Name, + original_location: Option, + original_coordinate: FieldArgumentCoordinate, original_value: Option, - redefined_selection: Option, - redefined_value: Option, + conflicting_location: Option, + conflicting_coordinate: FieldArgumentCoordinate, + conflicting_value: Option, }, #[error("cannot select multiple fields into the same alias `{alias}`")] ConflictingFieldName { @@ -626,60 +628,68 @@ impl ValidationError { ); } DiagnosticData::ConflictingFieldType { - field, - original_selection, + alias, + original_location, + original_coordinate, original_type, - redefined_selection, - redefined_type, + conflicting_location, + conflicting_coordinate, + conflicting_type, } => { report.with_label_opt( - *original_selection, - format_args!("`{field}` has type `{original_type}` here"), + *original_location, + format_args!( + "`{alias}` is selected from `{original_coordinate}: {original_type}` here" + ), ); report.with_label_opt( - *redefined_selection, - format_args!("but the same field name has type `{redefined_type}` here"), + *conflicting_location, + format_args!("`{alias}` is selected from `{conflicting_coordinate}: {conflicting_type}` here"), ); } DiagnosticData::ConflictingFieldArgument { - field, - argument, - original_selection, + alias, + original_location, + original_coordinate, original_value, - redefined_selection, - redefined_value, + conflicting_location, + conflicting_coordinate: _, + conflicting_value, } => { - match (original_value, redefined_value) { + let argument = &original_coordinate.argument; + match (original_value, conflicting_value) { (Some(_), Some(_)) => { report.with_label_opt( - *original_selection, - format_args!("field `{field}` provides one argument value here"), + *original_location, + format_args!( + "`{original_coordinate}` is used with one argument value here" + ), ); - report.with_label_opt(*redefined_selection, "but a different value here"); + report.with_label_opt(*conflicting_location, "but a different value here"); } (Some(_), None) => { report.with_label_opt( - *original_selection, - format!("field `{field}` is selected with argument `{argument}` here",), + *original_location, + format!("`{alias}` is selected with argument `{argument}` here",), ); report.with_label_opt( - *redefined_selection, + *conflicting_location, 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",), + *conflicting_location, + format!("`{alias}` is selected with argument `{argument}` here",), ); report.with_label_opt( - *original_selection, + *original_location, 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."); + 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"); } DiagnosticData::ConflictingFieldName { alias: field, diff --git a/crates/apollo-compiler/src/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs index c0e7035b..94b1684f 100644 --- a/crates/apollo-compiler/src/validation/selection.rs +++ b/crates/apollo-compiler/src/validation/selection.rs @@ -146,12 +146,13 @@ fn same_name_and_arguments( let data = DiagnosticData::ConflictingFieldArgument { // field_a and field_b have the same name so we can use either one. - field: field_b.field.name.clone(), - argument: arg.name.clone(), - original_selection: field_a.field.location(), + 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()), - redefined_selection: field_b.field.location(), - redefined_value: redefined_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()), }; ValidationError::new(field_b.field.location(), data) }; @@ -221,11 +222,13 @@ fn same_output_type_shape( ValidationError::new( selection_b.field.location(), DiagnosticData::ConflictingFieldType { - field: selection_a.field.response_key().clone(), - original_selection: selection_a.field.location(), + alias: selection_a.field.response_key().clone(), + original_location: selection_a.field.location(), + original_coordinate: selection_a.coordinate(), original_type: field_a.ty.clone(), - redefined_selection: selection_b.field.location(), - redefined_type: field_b.ty.clone(), + conflicting_location: selection_b.field.location(), + conflicting_coordinate: selection_b.coordinate(), + conflicting_type: field_b.ty.clone(), }, ) }; 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 cedc36a4..0c162a39 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,12 +1,12 @@ -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: cannot select multiple fields into the same alias `name` ╭─[0074_merge_identical_fields.graphql:19:3] @@ -20,15 +20,15 @@ Error: cannot select multiple fields into the same alias `name` │ │ 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: cannot select multiple fields into the same alias `fido` ╭─[0074_merge_identical_fields.graphql:24:3] 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 af396110..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,61 +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` +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` +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` +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. + │ 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 6538a2bc..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,12 +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 + │ ╰──────────── `someValue` is selected from `Cat.meowVolume: Int!` here ────╯ 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 index ca252c79..0c38fc60 100644 --- a/crates/apollo-compiler/test_data/diagnostics/0113_partially_overlapping_fragments.txt +++ b/crates/apollo-compiler/test_data/diagnostics/0113_partially_overlapping_fragments.txt @@ -1,12 +1,12 @@ -Error: operation must not select different types using the same field name `overlapping` +Error: operation must not select different types using the same name `overlapping` ╭─[0113_partially_overlapping_fragments.graphql:27:16] │ 24 │ ... on B { overlapping } │ ─────┬───── - │ ╰─────── `overlapping` has type `Int!` here + │ ╰─────── `overlapping` is selected from `B.overlapping: Int!` here │ 27 │ ... on C { overlapping } │ ─────┬───── - │ ╰─────── but the same field name has type `Int` here + │ ╰─────── `overlapping` is selected from `C.overlapping: Int` here ────╯ diff --git a/crates/apollo-compiler/tests/validation/field_merging.rs b/crates/apollo-compiler/tests/validation/field_merging.rs index 30af954d..6eaf5ef8 100644 --- a/crates/apollo-compiler/tests/validation/field_merging.rs +++ b/crates/apollo-compiler/tests/validation/field_merging.rs @@ -329,7 +329,7 @@ fn different_args_second_adds_argument() { { dog { ...conflictingArgs } } "#, expect![[r#" - 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` ╭─[query.graphql:3:3] │ 2 │ doesKnowCommand @@ -337,9 +337,9 @@ fn different_args_second_adds_argument() { │ ╰───────── but argument `dogCommand` is not provided here 3 │ doesKnowCommand(dogCommand: HEEL) │ ────────────────┬──────────────── - │ ╰────────────────── field `doesKnowCommand` is selected with argument `dogCommand` here + │ ╰────────────────── `doesKnowCommand` is selected with argument `dogCommand` 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 ───╯ "#]], ); @@ -357,17 +357,17 @@ fn different_args_second_missess_argument() { { dog { ...conflictingArgs } } "#, expect![[r#" - 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` ╭─[query.graphql:3:3] │ 2 │ doesKnowCommand(dogCommand: SIT) │ ────────────────┬─────────────── - │ ╰───────────────── field `doesKnowCommand` is selected with argument `dogCommand` here + │ ╰───────────────── `doesKnowCommand` is selected with argument `dogCommand` here 3 │ 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. + │ 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 ───╯ "#]], ); @@ -385,17 +385,17 @@ fn conflicting_arg_values() { { dog { ...conflictingArgs } } "#, expect![[r#" - 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` ╭─[query.graphql:3:3] │ 2 │ doesKnowCommand(dogCommand: SIT) │ ────────────────┬─────────────── - │ ╰───────────────── field `doesKnowCommand` provides one argument value here + │ ╰───────────────── `Dog.doesKnowCommand(dogCommand:)` is used with one argument value here 3 │ 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 ───╯ "#]], ); @@ -413,17 +413,17 @@ fn conflicting_arg_names() { { dog { ...conflictingArgs } } "#, expect![[r#" - 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` ╭─[query.graphql:3:3] │ 2 │ isAtLocation(x: 0) │ ─────────┬──────── - │ ╰────────── field `isAtLocation` is selected with argument `x` here + │ ╰────────── `isAtLocation` is selected with argument `x` here 3 │ 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 ───╯ "#]], ); @@ -967,16 +967,16 @@ mod return_types { } "#, expect![[r#" - Error: operation must not select different types using the same field name `scalar` + Error: operation must not select different types using the same name `scalar` ╭─[query.graphql:7:7] │ 4 │ scalar │ ───┬── - │ ╰──── `scalar` has type `Int` here + │ ╰──── `scalar` is selected from `IntBox.scalar: Int` here │ 7 │ scalar │ ───┬── - │ ╰──── but the same field name has type `String!` here + │ ╰──── `scalar` is selected from `NonNullStringBox1.scalar: String!` here ───╯ "#]], ); @@ -1020,16 +1020,16 @@ mod return_types { } "#, expect![[r#" - Error: operation must not select different types using the same field name `scalar` + Error: operation must not select different types using the same name `scalar` ╭─[query.graphql:7:7] │ 4 │ scalar │ ───┬── - │ ╰──── `scalar` has type `Int` here + │ ╰──── `scalar` is selected from `IntBox.scalar: Int` here │ 7 │ scalar │ ───┬── - │ ╰──── but the same field name has type `String` here + │ ╰──── `scalar` is selected from `StringBox.scalar: String` here ───╯ "#]], ); @@ -1118,16 +1118,16 @@ mod return_types { } "#, expect![[r#" - Error: operation must not select different types using the same field name `scalar` + Error: operation must not select different types using the same name `scalar` ╭─[query.graphql:7:7] │ 4 │ scalar │ ───┬── - │ ╰──── `scalar` has type `String!` here + │ ╰──── `scalar` is selected from `NonNullStringBox1.scalar: String!` here │ 7 │ scalar │ ───┬── - │ ╰──── but the same field name has type `String` here + │ ╰──── `scalar` is selected from `StringBox.scalar: String` here ───╯ "#]], ); @@ -1153,20 +1153,20 @@ mod return_types { } "#, expect![[r#" - Error: operation must not select different types using the same field name `box` + Error: operation must not select different types using the same name `box` ╭─[query.graphql:9:7] │ 4 │ ╭───▶ box: listStringBox { ┆ ┆ 6 │ ├───▶ } │ │ - │ ╰─────────────── `box` has type `[StringBox]` here + │ ╰─────────────── `box` is selected from `IntBox.listStringBox: [StringBox]` here │ 9 │ ╭─▶ box: stringBox { ┆ ┆ 11 │ ├─▶ } │ │ - │ ╰───────────── but the same field name has type `StringBox` here + │ ╰───────────── `box` is selected from `StringBox.stringBox: StringBox` here ────╯ "#]], ); @@ -1189,20 +1189,20 @@ mod return_types { } "#, expect![[r#" - Error: operation must not select different types using the same field name `box` + Error: operation must not select different types using the same name `box` ╭─[query.graphql:9:7] │ 4 │ ╭─▶ box: stringBox { ┆ ┆ 6 │ ├─▶ } │ │ - │ ╰───────────── `box` has type `StringBox` here + │ ╰───────────── `box` is selected from `IntBox.stringBox: StringBox` here │ 9 │ ╭───▶ box: listStringBox { ┆ ┆ 11 │ ├───▶ } │ │ - │ ╰─────────────── but the same field name has type `[StringBox]` here + │ ╰─────────────── `box` is selected from `StringBox.listStringBox: [StringBox]` here ────╯ "#]], ); @@ -1265,16 +1265,16 @@ mod return_types { } "#, expect![[r#" - Error: operation must not select different types using the same field name `scalar` + Error: operation must not select different types using the same name `scalar` ╭─[query.graphql:10:9] │ 5 │ scalar │ ───┬── - │ ╰──── `scalar` has type `String` here + │ ╰──── `scalar` is selected from `StringBox.scalar: String` here │ 10 │ scalar │ ───┬── - │ ╰──── but the same field name has type `Int` here + │ ╰──── `scalar` is selected from `IntBox.scalar: Int` here ────╯ "#]], ); @@ -1340,16 +1340,16 @@ mod return_types { } "#, expect![[r#" - Error: operation must not select different types using the same field name `id` + Error: operation must not select different types using the same name `id` ╭─[query.graphql:15:7] │ 6 │ id: name │ ────┬─── - │ ╰───── `id` has type `String` here + │ ╰───── `id` is selected from `Node.name: String` here │ 15 │ id │ ─┬ - │ ╰── but the same field name has type `ID` here + │ ╰── `id` is selected from `Node.id: ID` here ────╯ Error: cannot select multiple fields into the same alias `id` ╭─[query.graphql:15:7] From 3832078e1dfc3725ac3ac698b45ec889f22d018b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Tue, 6 Feb 2024 15:04:24 +0100 Subject: [PATCH 27/42] Fix field name conflict tests --- .../tests/validation/field_merging.rs | 540 ++++++++---------- 1 file changed, 252 insertions(+), 288 deletions(-) diff --git a/crates/apollo-compiler/tests/validation/field_merging.rs b/crates/apollo-compiler/tests/validation/field_merging.rs index 6eaf5ef8..0e321b4d 100644 --- a/crates/apollo-compiler/tests/validation/field_merging.rs +++ b/crates/apollo-compiler/tests/validation/field_merging.rs @@ -475,13 +475,68 @@ fn different_order_input_args() { ); } -#[test] -fn conflicts_in_fragments() { - expect_errors( - r#" +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#" { - ...A - ...B + f1 { + ...A + ...B + } } fragment A on Type { x: a @@ -490,43 +545,28 @@ fn conflicts_in_fragments() { x: b } "#, - expect![[r#" - Error: cannot find fragment `A` in this document - ╭─[query.graphql:2:3] - │ - 2 │ ...A - │ ──┬─ - │ ╰─── fragment `A` is not defined - ───╯ - Error: cannot find fragment `B` in this document - ╭─[query.graphql:3:3] - │ - 3 │ ...B - │ ──┬─ - │ ╰─── fragment `B` is not defined - ───╯ - Error: type condition `Type` of fragment `A` is not a type defined in the schema - ╭─[query.graphql:5:15] - │ - 5 │ fragment A on Type { - │ ──┬─ - │ ╰─── type condition here - ───╯ - Error: type condition `Type` of fragment `B` is not a type defined in the schema - ╭─[query.graphql:8:15] - │ - 8 │ fragment B on Type { - │ ──┬─ - │ ╰─── type condition here - ───╯ - "#]], - ); -} + expect![[r#" + Error: cannot select multiple 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#" + #[test] + fn dedupe_conflicts() { + expect_errors( + r#" { f1 { ...A @@ -549,172 +589,144 @@ fn dedupe_conflicts() { x: b } "#, - expect![[r#" - Error: type `QueryRoot` does not have a field `f1` - ╭─[query.graphql:2:3] - │ - 2 │ f1 { - │ ─┬ - │ ╰── field `f1` selected here - │ - ├─[schema.graphql:95:6] - │ - 95 │ type QueryRoot { - │ ────┬──── - │ ╰────── type `QueryRoot` defined here - │ - │ Note: path to the field: `query → f1` - ────╯ - Error: type `QueryRoot` does not have a field `f2` - ╭─[query.graphql:6:3] - │ - 6 │ f2 { - │ ─┬ - │ ╰── field `f2` selected here - │ - ├─[schema.graphql:95:6] - │ - 95 │ type QueryRoot { - │ ────┬──── - │ ╰────── type `QueryRoot` defined here - │ - │ Note: path to the field: `query → f2` - ────╯ - Error: type `QueryRoot` does not have a field `f3` - ╭─[query.graphql:10:3] - │ - 10 │ f3 { - │ ─┬ - │ ╰── field `f3` selected here - │ - ├─[schema.graphql:95:6] - │ - 95 │ type QueryRoot { - │ ────┬──── - │ ╰────── type `QueryRoot` defined here - │ - │ Note: path to the field: `query → f3` - ────╯ - Error: type condition `Type` of fragment `A` is not a type defined in the schema - ╭─[query.graphql:16:15] - │ - 16 │ fragment A on Type { - │ ──┬─ - │ ╰─── type condition here - ────╯ - Error: type condition `Type` of fragment `B` is not a type defined in the schema - ╭─[query.graphql:19:15] - │ - 19 │ fragment B on Type { - │ ──┬─ - │ ╰─── type condition here - ────╯ - "#]], - ); -} + expect![[r#" + Error: cannot select multiple 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 multiple 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 multiple 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 multiple 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#" + #[test] + fn deep_conflict() { + expect_errors( + r#" { - field { + f1 { x: a }, - field { + f1 { x: b } } "#, - expect![[r#" - Error: type `QueryRoot` does not have a field `field` - ╭─[query.graphql:2:3] - │ - 2 │ field { - │ ──┬── - │ ╰──── field `field` selected here - │ - ├─[schema.graphql:95:6] - │ - 95 │ type QueryRoot { - │ ────┬──── - │ ╰────── type `QueryRoot` defined here - │ - │ Note: path to the field: `query → field` - ────╯ - Error: type `QueryRoot` does not have a field `field` - ╭─[query.graphql:5:3] - │ - 5 │ field { - │ ──┬── - │ ╰──── field `field` selected here - │ - ├─[schema.graphql:95:6] - │ - 95 │ type QueryRoot { - │ ────┬──── - │ ╰────── type `QueryRoot` defined here - │ - │ Note: path to the field: `query → field` - ────╯ - "#]], - ); -} + expect![[r#" + Error: cannot select multiple 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#" + #[test] + fn deep_conflict_multiple_issues() { + expect_errors( + r#" { - field { + f1 { x: a y: c }, - field { + f1 { x: b y: d } } "#, - expect![[r#" - Error: type `QueryRoot` does not have a field `field` - ╭─[query.graphql:2:3] - │ - 2 │ field { - │ ──┬── - │ ╰──── field `field` selected here - │ - ├─[schema.graphql:95:6] - │ - 95 │ type QueryRoot { - │ ────┬──── - │ ╰────── type `QueryRoot` defined here - │ - │ Note: path to the field: `query → field` - ────╯ - Error: type `QueryRoot` does not have a field `field` - ╭─[query.graphql:6:3] - │ - 6 │ field { - │ ──┬── - │ ╰──── field `field` selected here - │ - ├─[schema.graphql:95:6] - │ - 95 │ type QueryRoot { - │ ────┬──── - │ ╰────── type `QueryRoot` defined here - │ - │ Note: path to the field: `query → field` - ────╯ - "#]], - ); -} + expect![[r#" + Error: cannot select multiple 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 multiple 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#" + #[test] + fn very_deep_conflict() { + expect_errors( + r#" { field { deepField { @@ -728,45 +740,28 @@ fn very_deep_conflict() { } } "#, - expect![[r#" - Error: type `QueryRoot` does not have a field `field` - ╭─[query.graphql:2:3] - │ - 2 │ field { - │ ──┬── - │ ╰──── field `field` selected here - │ - ├─[schema.graphql:95:6] - │ - 95 │ type QueryRoot { - │ ────┬──── - │ ╰────── type `QueryRoot` defined here - │ - │ Note: path to the field: `query → field` - ────╯ - Error: type `QueryRoot` does not have a field `field` - ╭─[query.graphql:7:3] - │ - 7 │ field { - │ ──┬── - │ ╰──── field `field` selected here - │ - ├─[schema.graphql:95:6] - │ - 95 │ type QueryRoot { - │ ────┬──── - │ ╰────── type `QueryRoot` defined here - │ - │ Note: path to the field: `query → field` - ────╯ - "#]], - ); -} + expect![[r#" + Error: cannot select multiple 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#" + #[test] + fn deep_conflict_in_fragments() { + expect_errors( + r#" { field { ...F @@ -790,67 +785,36 @@ fn deep_conflict_in_fragments() { x: b } "#, - expect![[r#" - Error: type `QueryRoot` does not have a field `field` - ╭─[query.graphql:2:3] - │ - 2 │ field { - │ ──┬── - │ ╰──── field `field` selected here - │ - ├─[schema.graphql:95:6] - │ - 95 │ type QueryRoot { - │ ────┬──── - │ ╰────── type `QueryRoot` defined here - │ - │ Note: path to the field: `query → field` - ────╯ - Error: type `QueryRoot` does not have a field `field` - ╭─[query.graphql:5:3] - │ - 5 │ field { - │ ──┬── - │ ╰──── field `field` selected here - │ - ├─[schema.graphql:95:6] - │ - 95 │ type QueryRoot { - │ ────┬──── - │ ╰────── type `QueryRoot` defined here - │ - │ Note: path to the field: `query → field` - ────╯ - Error: type condition `T` of fragment `F` is not a type defined in the schema - ╭─[query.graphql:9:15] - │ - 9 │ fragment F on T { - │ ┬ - │ ╰── type condition here - ───╯ - Error: type condition `T` of fragment `G` is not a type defined in the schema - ╭─[query.graphql:13:15] - │ - 13 │ fragment G on T { - │ ┬ - │ ╰── type condition here - ────╯ - Error: type condition `T` of fragment `I` is not a type defined in the schema - ╭─[query.graphql:16:15] - │ - 16 │ fragment I on T { - │ ┬ - │ ╰── type condition here - ────╯ - Error: type condition `T` of fragment `J` is not a type defined in the schema - ╭─[query.graphql:20:15] - │ - 20 │ fragment J on T { - │ ┬ - │ ╰── type condition here - ────╯ - "#]], - ); + expect![[r#" + Error: cannot select multiple 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 multiple 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 { From 930d43eaeb6ff9795cc4a0c9babeae47b2e85153 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Tue, 6 Feb 2024 15:27:06 +0100 Subject: [PATCH 28/42] Ensure consistent ordering of errors by using indexmap --- crates/apollo-compiler/src/validation/selection.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/apollo-compiler/src/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs index 94b1684f..736e8e58 100644 --- a/crates/apollo-compiler/src/validation/selection.rs +++ b/crates/apollo-compiler/src/validation/selection.rs @@ -3,10 +3,10 @@ use crate::coordinate::TypeAttributeCoordinate; use crate::validation::diagnostics::{DiagnosticData, ValidationError}; use crate::validation::{CycleError, FileId, RecursionGuard, RecursionStack, ValidationDatabase}; use crate::{ast, executable, schema, Node}; +use indexmap::map::Entry; use indexmap::IndexMap; use std::cell::OnceCell; -use std::collections::{hash_map::Entry, HashMap}; -use std::collections::{HashSet, VecDeque}; +use std::collections::{HashMap, HashSet, VecDeque}; use std::rc::Rc; /// Represents a field selected against a parent type. @@ -304,7 +304,7 @@ impl OnceBool { /// Represents a merged field set that may or may not be valid. struct MergedFieldSet<'doc> { selections: Vec>, - grouped_by_output_names: OnceCell>>>, + grouped_by_output_names: OnceCell>>>, grouped_by_common_parents: OnceCell>>>, same_response_shape_guard: OnceBool, same_for_common_parents_guard: OnceBool, @@ -407,9 +407,9 @@ impl<'doc> MergedFieldSet<'doc> { } } - fn group_by_output_name(&self) -> &HashMap>> { + fn group_by_output_name(&self) -> &IndexMap>> { self.grouped_by_output_names.get_or_init(|| { - let mut map = HashMap::new(); + let mut map = IndexMap::new(); for selection in &self.selections { match map.entry(selection.field.response_key().clone()) { Entry::Vacant(entry) => { @@ -430,7 +430,7 @@ impl<'doc> MergedFieldSet<'doc> { 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 = HashMap::<_, Vec<_>>::new(); + let mut concrete_parents = IndexMap::<_, Vec<_>>::new(); for selection in &self.selections { match schema.types.get(selection.parent_type) { Some(schema::ExtendedType::Object(object)) => { From 2214452bed1ab097e7d86aea72bc0db5c552dc14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Thu, 8 Feb 2024 09:25:38 +0100 Subject: [PATCH 29/42] Push field merging errors directly into DiagnosticList --- crates/apollo-compiler/src/executable/mod.rs | 38 +++++- .../src/executable/validation.rs | 8 +- .../src/validation/diagnostics.rs | 115 ------------------ crates/apollo-compiler/src/validation/mod.rs | 85 +++++++++++++ .../src/validation/selection.rs | 67 +++++----- 5 files changed, 153 insertions(+), 160 deletions(-) diff --git a/crates/apollo-compiler/src/executable/mod.rs b/crates/apollo-compiler/src/executable/mod.rs index 0f0659e6..2b17227b 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; @@ -106,7 +108,8 @@ pub struct InlineFragment { 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}")] @@ -162,6 +165,7 @@ pub(crate) enum BuildError { path: SelectionPath, }, + // Validation errors #[error( "{} can only have one root field", subscription_name_or_anonymous(name) @@ -181,6 +185,38 @@ pub(crate) enum BuildError { /// 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 multiple 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 + '_ { diff --git a/crates/apollo-compiler/src/executable/validation.rs b/crates/apollo-compiler/src/executable/validation.rs index 3b338276..299aa9fd 100644 --- a/crates/apollo-compiler/src/executable/validation.rs +++ b/crates/apollo-compiler/src/executable/validation.rs @@ -35,16 +35,10 @@ fn validate_with_schema( schema: &Schema, document: &ExecutableDocument, ) { - let mut compiler_diagnostics = vec![]; - 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.selection_set, &mut compiler_diagnostics); - } - - for diagnostic in compiler_diagnostics { - errors.push(diagnostic.location, Details::CompilerDiagnostic(diagnostic)) + fields_in_set_can_merge.validate(&operation.selection_set, errors); } } diff --git a/crates/apollo-compiler/src/validation/diagnostics.rs b/crates/apollo-compiler/src/validation/diagnostics.rs index d1b01874..30fb3246 100644 --- a/crates/apollo-compiler/src/validation/diagnostics.rs +++ b/crates/apollo-compiler/src/validation/diagnostics.rs @@ -2,8 +2,6 @@ use crate::ast; use crate::ast::DirectiveLocation; use crate::ast::Name; use crate::ast::Type; -use crate::ast::Value; -use crate::coordinate::FieldArgumentCoordinate; use crate::coordinate::SchemaCoordinate; use crate::coordinate::TypeAttributeCoordinate; use crate::diagnostic::CliReport; @@ -211,37 +209,6 @@ pub(crate) enum DiagnosticData { coordinate: TypeAttributeCoordinate, describe_type: &'static str, }, - #[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 multiple 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, - }, #[error( "{} must have a composite type in its type condition", fragment_name_or_inline(name) @@ -627,88 +594,6 @@ impl ValidationError { format_args!("{coordinate} is {describe_type} and must select fields"), ); } - DiagnosticData::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"), - ); - } - DiagnosticData::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"); - } - DiagnosticData::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"); - } DiagnosticData::InvalidFragmentTarget { name: _, ty } => { report.with_label_opt( self.location, diff --git a/crates/apollo-compiler/src/validation/mod.rs b/crates/apollo-compiler/src/validation/mod.rs index 0c80655f..6302520f 100644 --- a/crates/apollo-compiler/src/validation/mod.rs +++ b/crates/apollo-compiler/src/validation/mod.rs @@ -426,6 +426,91 @@ impl ToCliReport for DiagnosticData { 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"); + } }, } } diff --git a/crates/apollo-compiler/src/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs index 736e8e58..4665914a 100644 --- a/crates/apollo-compiler/src/validation/selection.rs +++ b/crates/apollo-compiler/src/validation/selection.rs @@ -1,6 +1,8 @@ -use super::operation::OperationValidationConfig; use crate::coordinate::TypeAttributeCoordinate; -use crate::validation::diagnostics::{DiagnosticData, ValidationError}; +use crate::executable::BuildError; +use crate::validation::diagnostics::ValidationError; +use crate::validation::operation::OperationValidationConfig; +use crate::validation::DiagnosticList; use crate::validation::{CycleError, FileId, RecursionGuard, RecursionStack, ValidationDatabase}; use crate::{ast, executable, schema, Node}; use indexmap::map::Entry; @@ -117,19 +119,16 @@ fn contains_any_fragment_cycle( fn same_name_and_arguments( field_a: FieldSelection<'_>, field_b: FieldSelection<'_>, -) -> Result<(), ValidationError> { +) -> Result<(), BuildError> { // 2bi. fieldA and fieldB must have identical field names. if field_a.field.name != field_b.field.name { - return Err(ValidationError::new( - field_b.field.location(), - DiagnosticData::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(), - }, - )); + 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. @@ -144,7 +143,7 @@ fn same_name_and_arguments( // 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(); - let data = DiagnosticData::ConflictingFieldArgument { + 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(), @@ -153,8 +152,7 @@ fn same_name_and_arguments( 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()), - }; - ValidationError::new(field_b.field.location(), data) + } }; // Check if fieldB provides the same argument names and values as fieldA (order-independent). @@ -211,26 +209,21 @@ fn same_output_type_shape( schema: &schema::Schema, selection_a: FieldSelection<'_>, selection_b: FieldSelection<'_>, -) -> Result<(), ValidationError> { +) -> 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 = || { - ValidationError::new( - selection_b.field.location(), - DiagnosticData::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(), - }, - ) + 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. @@ -328,7 +321,7 @@ impl<'doc> MergedFieldSet<'doc> { fn same_response_shape_by_name( &self, validator: &mut FieldsInSetCanMerge<'_, 'doc>, - diagnostics: &mut Vec, + diagnostics: &mut DiagnosticList, ) { // No need to do this if this field set has been checked before. if self.same_response_shape_guard.check() { @@ -342,7 +335,7 @@ impl<'doc> MergedFieldSet<'doc> { for (field_a, field_b) in std::iter::repeat(field_a).zip(rest.iter()) { // Covers steps 3-5 of the spec algorithm. if let Err(err) = same_output_type_shape(validator.schema, *field_a, *field_b) { - diagnostics.push(err); + diagnostics.push(field_b.field.location(), err); continue; } } @@ -369,7 +362,7 @@ impl<'doc> MergedFieldSet<'doc> { fn same_for_common_parents_by_name( &self, validator: &mut FieldsInSetCanMerge<'_, 'doc>, - diagnostics: &mut Vec, + diagnostics: &mut DiagnosticList, ) { // No need to do this if this field set has been checked before. if self.same_for_common_parents_guard.check() { @@ -388,7 +381,7 @@ impl<'doc> MergedFieldSet<'doc> { }; for (field_a, field_b) in std::iter::repeat(field_a).zip(rest.iter()) { if let Err(diagnostic) = same_name_and_arguments(*field_a, *field_b) { - diagnostics.push(diagnostic); + diagnostics.push(field_b.field.location(), diagnostic); continue; } } @@ -512,7 +505,7 @@ impl<'s, 'doc> FieldsInSetCanMerge<'s, 'doc> { pub(crate) fn validate( &mut self, root: &'doc executable::SelectionSet, - diagnostics: &mut Vec, + diagnostics: &mut DiagnosticList, ) { // We cannot safely check cyclical fragments // TODO(@goto-bus-stop) - or maybe we can, given that a cycle will result in a cache hit? @@ -537,7 +530,7 @@ impl<'s, 'doc> FieldsInSetCanMerge<'s, 'doc> { fn same_for_common_parents_by_name( &mut self, selections: Vec>, - diagnostics: &mut Vec, + diagnostics: &mut DiagnosticList, ) { let field_set = self.lookup(selections); field_set.same_for_common_parents_by_name(self, diagnostics); @@ -546,7 +539,7 @@ impl<'s, 'doc> FieldsInSetCanMerge<'s, 'doc> { fn same_response_shape_by_name( &mut self, selections: Vec>, - diagnostics: &mut Vec, + diagnostics: &mut DiagnosticList, ) { let field_set = self.lookup(selections); field_set.same_response_shape_by_name(self, diagnostics); From 90357cbe2b93c869ea48db9fffba9223561dea96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Thu, 8 Feb 2024 15:32:08 +0100 Subject: [PATCH 30/42] Remove unnecessary fragment recursion check --- .../src/validation/selection.rs | 53 +------------------ 1 file changed, 1 insertion(+), 52 deletions(-) diff --git a/crates/apollo-compiler/src/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs index 4665914a..9f27b893 100644 --- a/crates/apollo-compiler/src/validation/selection.rs +++ b/crates/apollo-compiler/src/validation/selection.rs @@ -3,7 +3,7 @@ use crate::executable::BuildError; use crate::validation::diagnostics::ValidationError; use crate::validation::operation::OperationValidationConfig; use crate::validation::DiagnosticList; -use crate::validation::{CycleError, FileId, RecursionGuard, RecursionStack, ValidationDatabase}; +use crate::validation::{FileId, ValidationDatabase}; use crate::{ast, executable, schema, Node}; use indexmap::map::Entry; use indexmap::IndexMap; @@ -70,51 +70,6 @@ fn is_composite(ty: &schema::ExtendedType) -> bool { matches!(ty, Object(_) | Interface(_) | Union(_)) } -/// Check if a selection set contains a fragment cycle, meaning we can't run recursive -/// validations on it. -fn contains_any_fragment_cycle( - fragments: &IndexMap>, - selection_set: &executable::SelectionSet, -) -> bool { - let mut visited = RecursionStack::new().with_limit(100); - - return detect_fragment_cycle_inner(fragments, selection_set, &mut visited.guard()).is_err(); - - fn detect_fragment_cycle_inner( - fragments: &IndexMap>, - selection_set: &executable::SelectionSet, - visited: &mut RecursionGuard<'_>, - ) -> Result<(), CycleError<()>> { - for selection in &selection_set.selections { - match selection { - executable::Selection::FragmentSpread(spread) => { - if visited.contains(&spread.fragment_name) { - if visited.first() == Some(&spread.fragment_name) { - return Err(CycleError::Recursed(vec![])); - } - continue; - } - - if let Some(fragment) = fragments.get(&spread.fragment_name) { - detect_fragment_cycle_inner( - fragments, - &fragment.selection_set, - &mut visited.push(&fragment.name)?, - )?; - } - } - executable::Selection::InlineFragment(inline) => { - detect_fragment_cycle_inner(fragments, &inline.selection_set, visited)?; - } - executable::Selection::Field(field) => { - detect_fragment_cycle_inner(fragments, &field.selection_set, visited)?; - } - } - } - Ok(()) - } -} - /// 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<'_>, @@ -507,12 +462,6 @@ impl<'s, 'doc> FieldsInSetCanMerge<'s, 'doc> { root: &'doc executable::SelectionSet, diagnostics: &mut DiagnosticList, ) { - // We cannot safely check cyclical fragments - // TODO(@goto-bus-stop) - or maybe we can, given that a cycle will result in a cache hit? - if contains_any_fragment_cycle(&self.document.fragments, root) { - return; - } - let fields = expand_selections(&self.document.fragments, std::iter::once(root)); let set = self.lookup(fields); set.same_response_shape_by_name(self, diagnostics); From 9d35fc84f87d0adc36c72a4cb4bb5d68c8b32488 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Fri, 9 Feb 2024 16:05:37 +0100 Subject: [PATCH 31/42] Use a map to optimize comparing large argument lists --- .../src/validation/selection.rs | 27 +++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/crates/apollo-compiler/src/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs index 9f27b893..5841baf8 100644 --- a/crates/apollo-compiler/src/validation/selection.rs +++ b/crates/apollo-compiler/src/validation/selection.rs @@ -110,9 +110,32 @@ fn same_name_and_arguments( } }; + enum ArgumentLookup<'a> { + Indexed(HashMap<&'a ast::Name, &'a Node>), + List(&'a [Node]), + } + impl<'a> ArgumentLookup<'a> { + fn new(list: &'a [Node]) -> Self { + if list.len() > 20 { + Self::Indexed(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::Indexed(map) => map.get(name).copied(), + Self::List(list) => list.iter().find(|arg| arg.name == *name), + } + } + } + // 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) = field_b.field.argument_by_name(&arg.name) else { + let Some(other_arg) = other_args.by_name(&arg.name) else { return Err(conflicting_field_argument(Some(arg), None)); }; @@ -122,7 +145,7 @@ fn same_name_and_arguments( } // Check if fieldB provides any arguments that fieldA does not provide. for arg in &field_b.field.arguments { - if field_a.field.argument_by_name(&arg.name).is_none() { + if self_args.by_name(&arg.name).is_none() { return Err(conflicting_field_argument(None, Some(arg))); }; } From 850d9b5490d5dbb6f3a112c5de9aa0f993c78013 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Fri, 9 Feb 2024 16:52:43 +0100 Subject: [PATCH 32/42] Benchmark checking a field with very many arguments --- .../benches/fields_validation.rs | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/crates/apollo-compiler/benches/fields_validation.rs b/crates/apollo-compiler/benches/fields_validation.rs index b94ce37f..2940bdb4 100644 --- a/crates/apollo-compiler/benches/fields_validation.rs +++ b/crates/apollo-compiler/benches/fields_validation.rs @@ -36,6 +36,27 @@ fn bench_many_same_nested_field(c: &mut Criterion) { }); } +fn bench_many_arguments(c: &mut Criterion) { + let schema = + Schema::parse_and_validate(format!("type Query {{ hello: String! }}"), "schema.graphql") + .unwrap(); + let field = format!( + "hello({})", + (0..2_000) + .map(|i| format!("arg{i}: {i}\n")) + .collect::() + ); + 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( " @@ -98,6 +119,7 @@ criterion_group!( fields, bench_many_same_field, bench_many_same_nested_field, + bench_many_arguments, bench_many_types, ); criterion_main!(fields); From 689e8513b066a65a9e5068747322b54df34f513e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Tue, 13 Feb 2024 16:40:05 +0100 Subject: [PATCH 33/42] update diagnostic message: multiple -> different --- crates/apollo-compiler/src/executable/mod.rs | 2 +- .../0074_merge_identical_fields.txt | 4 +-- .../diagnostics/0077_merge_conflict_deep.txt | 2 +- .../0078_merge_conflict_nested_fragments.txt | 4 +-- .../tests/validation/field_merging.rs | 30 +++++++++---------- 5 files changed, 21 insertions(+), 21 deletions(-) diff --git a/crates/apollo-compiler/src/executable/mod.rs b/crates/apollo-compiler/src/executable/mod.rs index 2b17227b..cfd1cd87 100644 --- a/crates/apollo-compiler/src/executable/mod.rs +++ b/crates/apollo-compiler/src/executable/mod.rs @@ -208,7 +208,7 @@ pub(crate) enum BuildError { conflicting_coordinate: FieldArgumentCoordinate, conflicting_value: Option, }, - #[error("cannot select multiple fields into the same alias `{alias}`")] + #[error("cannot select different fields into the same alias `{alias}`")] ConflictingFieldName { /// Name of the non-unique field. alias: Name, 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 0c162a39..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 @@ -8,7 +8,7 @@ Error: operation must not select different types using the same name `name` │ ──┬─ │ ╰─── `name` is selected from `Dog.name: String!` here ────╯ -Error: cannot select multiple fields into the same alias `name` +Error: cannot select different fields into the same alias `name` ╭─[0074_merge_identical_fields.graphql:19:3] │ 18 │ name: nickname @@ -30,7 +30,7 @@ Error: operation must not select different types using the same name `fido` │ ───────┬────── │ ╰──────── `fido` is selected from `Dog.nickname: String` here ────╯ -Error: cannot select multiple fields into the same alias `fido` +Error: cannot select different fields into the same alias `fido` ╭─[0074_merge_identical_fields.graphql:24:3] │ 23 │ fido: name 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 6fb7fddb..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,4 +1,4 @@ -Error: cannot select multiple fields into the same alias `x` +Error: cannot select different fields into the same alias `x` ╭─[0077_merge_conflict_deep.graphql:14:5] │ 11 │ x: a 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 ecbf432e..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,4 +1,4 @@ -Error: cannot select multiple fields into the same alias `y` +Error: cannot select different fields into the same alias `y` ╭─[0078_merge_conflict_nested_fragments.graphql:25:3] │ 25 │ y: c @@ -11,7 +11,7 @@ Error: cannot select multiple fields into the same alias `y` │ │ 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 multiple fields into 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 diff --git a/crates/apollo-compiler/tests/validation/field_merging.rs b/crates/apollo-compiler/tests/validation/field_merging.rs index 0e321b4d..9c73f144 100644 --- a/crates/apollo-compiler/tests/validation/field_merging.rs +++ b/crates/apollo-compiler/tests/validation/field_merging.rs @@ -255,7 +255,7 @@ fn same_aliases_with_different_field_targets() { { dog { ...sameAliasesWithDifferentFieldTargets } } "#, expect![[r#" - Error: cannot select multiple fields into the same alias `fido` + Error: cannot select different fields into the same alias `fido` ╭─[query.graphql:3:3] │ 2 │ fido: name @@ -301,7 +301,7 @@ fn alias_masking_direct_field_access() { { dog { ...aliasMaskingDirectFieldAccess } } "#, expect![[r#" - Error: cannot select multiple fields into the same alias `name` + Error: cannot select different fields into the same alias `name` ╭─[query.graphql:3:3] │ 2 │ name: nickname @@ -546,7 +546,7 @@ mod field_conflicts { } "#, expect![[r#" - Error: cannot select multiple fields into the same alias `x` + Error: cannot select different fields into the same alias `x` ╭─[query.graphql:11:3] │ 8 │ x: a @@ -590,7 +590,7 @@ mod field_conflicts { } "#, expect![[r#" - Error: cannot select multiple fields into the same alias `x` + Error: cannot select different fields into the same alias `x` ╭─[query.graphql:17:3] │ 17 │ x: a @@ -603,7 +603,7 @@ mod field_conflicts { │ │ 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 multiple fields into the same alias `x` + Error: cannot select different fields into the same alias `x` ╭─[query.graphql:17:3] │ 13 │ x: c @@ -616,7 +616,7 @@ mod field_conflicts { │ │ 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 multiple fields into the same alias `x` + Error: cannot select different fields into the same alias `x` ╭─[query.graphql:20:3] │ 17 │ x: a @@ -629,7 +629,7 @@ mod field_conflicts { │ │ 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 multiple fields into the same alias `x` + Error: cannot select different fields into the same alias `x` ╭─[query.graphql:20:3] │ 13 │ x: c @@ -660,7 +660,7 @@ mod field_conflicts { } "#, expect![[r#" - Error: cannot select multiple fields into the same alias `x` + Error: cannot select different fields into the same alias `x` ╭─[query.graphql:6:5] │ 3 │ x: a @@ -693,7 +693,7 @@ mod field_conflicts { } "#, expect![[r#" - Error: cannot select multiple fields into the same alias `x` + Error: cannot select different fields into the same alias `x` ╭─[query.graphql:7:5] │ 3 │ x: a @@ -706,7 +706,7 @@ mod field_conflicts { │ │ 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 multiple fields into the same alias `y` + Error: cannot select different fields into the same alias `y` ╭─[query.graphql:8:5] │ 4 │ y: c @@ -741,7 +741,7 @@ mod field_conflicts { } "#, expect![[r#" - Error: cannot select multiple fields into the same alias `x` + Error: cannot select different fields into the same alias `x` ╭─[query.graphql:9:7] │ 4 │ x: a @@ -786,7 +786,7 @@ mod field_conflicts { } "#, expect![[r#" - Error: cannot select multiple fields into the same alias `y` + Error: cannot select different fields into the same alias `y` ╭─[query.graphql:14:3] │ 14 │ y: c @@ -799,7 +799,7 @@ mod field_conflicts { │ │ 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 multiple fields into the same alias `x` + Error: cannot select different fields into the same alias `x` ╭─[query.graphql:21:3] │ 10 │ x: a @@ -1193,7 +1193,7 @@ mod return_types { } "#, expect![[r#" - Error: cannot select multiple fields into the same alias `val` + Error: cannot select different fields into the same alias `val` ╭─[query.graphql:6:9] │ 5 │ val: scalar @@ -1315,7 +1315,7 @@ mod return_types { │ ─┬ │ ╰── `id` is selected from `Node.id: ID` here ────╯ - Error: cannot select multiple fields into the same alias `id` + Error: cannot select different fields into the same alias `id` ╭─[query.graphql:15:7] │ 6 │ id: name From 4d8ee98fcfbbfe444097af295497791f8f7c0d4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Tue, 13 Feb 2024 16:50:51 +0100 Subject: [PATCH 34/42] remove outdated comment --- crates/apollo-compiler/src/validation/selection.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/apollo-compiler/src/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs index 5841baf8..bb38db08 100644 --- a/crates/apollo-compiler/src/validation/selection.rs +++ b/crates/apollo-compiler/src/validation/selection.rs @@ -323,7 +323,6 @@ impl<'doc> MergedFieldSet<'doc> { .map(|selection| &selection.field.selection_set) .filter(|set| !set.selections.is_empty()) .peekable(); - // TODO cache if nested_selection_sets.peek().is_some() { let merged_set = expand_selections(&validator.document.fragments, nested_selection_sets); From 9be0f6e6c92fd33b94c55ebb6d39db8562e949b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Tue, 13 Feb 2024 16:54:51 +0100 Subject: [PATCH 35/42] useless zip --- crates/apollo-compiler/src/validation/selection.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/apollo-compiler/src/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs index bb38db08..73fce8ff 100644 --- a/crates/apollo-compiler/src/validation/selection.rs +++ b/crates/apollo-compiler/src/validation/selection.rs @@ -310,7 +310,7 @@ impl<'doc> MergedFieldSet<'doc> { let Some((field_a, rest)) = fields_for_name.split_first() else { continue; }; - for (field_a, field_b) in std::iter::repeat(field_a).zip(rest.iter()) { + 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); @@ -356,7 +356,7 @@ impl<'doc> MergedFieldSet<'doc> { let Some((field_a, rest)) = fields_for_parents.split_first() else { continue; }; - for (field_a, field_b) in std::iter::repeat(field_a).zip(rest.iter()) { + 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; From 83a44281bfe909a6e460a232144b1c4a1281d4a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e?= Date: Tue, 13 Feb 2024 16:55:13 +0100 Subject: [PATCH 36/42] Use Entry::or_default Co-authored-by: Simon Sapin --- crates/apollo-compiler/src/validation/selection.rs | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/crates/apollo-compiler/src/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs index 73fce8ff..a9b493ee 100644 --- a/crates/apollo-compiler/src/validation/selection.rs +++ b/crates/apollo-compiler/src/validation/selection.rs @@ -381,14 +381,9 @@ impl<'doc> MergedFieldSet<'doc> { self.grouped_by_output_names.get_or_init(|| { let mut map = IndexMap::new(); for selection in &self.selections { - match map.entry(selection.field.response_key().clone()) { - Entry::Vacant(entry) => { - entry.insert(vec![*selection]); - } - Entry::Occupied(mut entry) => { - entry.get_mut().push(*selection); - } - } + map.entry(selection.field.response_key().clone()) + .or_default() + .push(*selection); } map }) From 453de795fbcc3008a292176627faa662a1fb6704 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Tue, 13 Feb 2024 17:11:40 +0100 Subject: [PATCH 37/42] make clippy happy with the benchmarking code --- .../apollo-compiler/benches/fields_validation.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/crates/apollo-compiler/benches/fields_validation.rs b/crates/apollo-compiler/benches/fields_validation.rs index 2940bdb4..e2b70a7e 100644 --- a/crates/apollo-compiler/benches/fields_validation.rs +++ b/crates/apollo-compiler/benches/fields_validation.rs @@ -38,14 +38,13 @@ fn bench_many_same_nested_field(c: &mut Criterion) { fn bench_many_arguments(c: &mut Criterion) { let schema = - Schema::parse_and_validate(format!("type Query {{ hello: String! }}"), "schema.graphql") - .unwrap(); - let field = format!( - "hello({})", - (0..2_000) - .map(|i| format!("arg{i}: {i}\n")) - .collect::() - ); + 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| { From f5f038043188a1b2487e591683e6b60dead62178 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Tue, 13 Feb 2024 17:52:36 +0100 Subject: [PATCH 38/42] Add a recursion limit while merging fields --- .../src/validation/selection.rs | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/crates/apollo-compiler/src/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs index a9b493ee..f4157f2c 100644 --- a/crates/apollo-compiler/src/validation/selection.rs +++ b/crates/apollo-compiler/src/validation/selection.rs @@ -5,7 +5,7 @@ use crate::validation::operation::OperationValidationConfig; use crate::validation::DiagnosticList; use crate::validation::{FileId, ValidationDatabase}; use crate::{ast, executable, schema, Node}; -use indexmap::map::Entry; +use apollo_parser::LimitTracker; use indexmap::IndexMap; use std::cell::OnceCell; use std::collections::{HashMap, HashSet, VecDeque}; @@ -379,7 +379,7 @@ impl<'doc> MergedFieldSet<'doc> { fn group_by_output_name(&self) -> &IndexMap>> { self.grouped_by_output_names.get_or_init(|| { - let mut map = IndexMap::new(); + let mut map = IndexMap::<_, Vec<_>>::new(); for selection in &self.selections { map.entry(selection.field.response_key().clone()) .or_default() @@ -444,6 +444,10 @@ impl FieldSelectionsId { } } +// 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 /// @@ -460,6 +464,9 @@ pub(crate) struct FieldsInSetCanMerge<'s, 'doc> { /// 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, } impl<'s, 'doc> FieldsInSetCanMerge<'s, 'doc> { @@ -471,6 +478,7 @@ impl<'s, 'doc> FieldsInSetCanMerge<'s, 'doc> { schema, document, cache: Default::default(), + recursion_limit: LimitTracker::new(FIELD_DEPTH_LIMIT), } } @@ -498,8 +506,12 @@ impl<'s, 'doc> FieldsInSetCanMerge<'s, 'doc> { 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(); } fn same_response_shape_by_name( @@ -507,8 +519,12 @@ impl<'s, 'doc> FieldsInSetCanMerge<'s, 'doc> { 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(); } } From a56fcca34fb0a7a7a5abf2a1da8a5d772a4bebf1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Tue, 13 Feb 2024 17:55:42 +0100 Subject: [PATCH 39/42] check -> already_done --- crates/apollo-compiler/src/validation/selection.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/apollo-compiler/src/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs index f4157f2c..6d0eec18 100644 --- a/crates/apollo-compiler/src/validation/selection.rs +++ b/crates/apollo-compiler/src/validation/selection.rs @@ -267,7 +267,7 @@ impl OnceBool { } /// Returns `false` the first time it is called, then returns `true` forever. - fn check(&self) -> bool { + fn already_done(&self) -> bool { self.0.replace(true) } } @@ -302,7 +302,7 @@ impl<'doc> MergedFieldSet<'doc> { diagnostics: &mut DiagnosticList, ) { // No need to do this if this field set has been checked before. - if self.same_response_shape_guard.check() { + if self.same_response_shape_guard.already_done() { return; } @@ -342,7 +342,7 @@ impl<'doc> MergedFieldSet<'doc> { diagnostics: &mut DiagnosticList, ) { // No need to do this if this field set has been checked before. - if self.same_for_common_parents_guard.check() { + if self.same_for_common_parents_guard.already_done() { return; } From 53db9626428d614e1a8cdfcb893a5e7892ba9009 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Wed, 14 Feb 2024 09:32:19 +0100 Subject: [PATCH 40/42] Move ArgumentLookup out of the function --- .../src/validation/selection.rs | 44 ++++++++++--------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/crates/apollo-compiler/src/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs index 6d0eec18..f4f94ebd 100644 --- a/crates/apollo-compiler/src/validation/selection.rs +++ b/crates/apollo-compiler/src/validation/selection.rs @@ -70,6 +70,29 @@ fn is_composite(ty: &schema::ExtendedType) -> bool { 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<'_>, @@ -110,27 +133,6 @@ fn same_name_and_arguments( } }; - enum ArgumentLookup<'a> { - Indexed(HashMap<&'a ast::Name, &'a Node>), - List(&'a [Node]), - } - impl<'a> ArgumentLookup<'a> { - fn new(list: &'a [Node]) -> Self { - if list.len() > 20 { - Self::Indexed(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::Indexed(map) => map.get(name).copied(), - Self::List(list) => list.iter().find(|arg| arg.name == *name), - } - } - } - // 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); From acdd46411ad94102f4bbb3fe4e8a74c733ebe462 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Wed, 14 Feb 2024 10:57:15 +0100 Subject: [PATCH 41/42] Emit an error when the new recursion limit is reached; add test --- .../src/executable/validation.rs | 2 +- crates/apollo-compiler/src/validation/mod.rs | 3 ++ .../src/validation/selection.rs | 13 +++-- .../tests/validation/recursion.rs | 49 ++++++++++++++++++- 4 files changed, 62 insertions(+), 5 deletions(-) diff --git a/crates/apollo-compiler/src/executable/validation.rs b/crates/apollo-compiler/src/executable/validation.rs index 299aa9fd..50d19203 100644 --- a/crates/apollo-compiler/src/executable/validation.rs +++ b/crates/apollo-compiler/src/executable/validation.rs @@ -38,7 +38,7 @@ fn validate_with_schema( 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.selection_set, errors); + fields_in_set_can_merge.validate_operation(operation, errors); } } diff --git a/crates/apollo-compiler/src/validation/mod.rs b/crates/apollo-compiler/src/validation/mod.rs index 9145fdc2..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 { @@ -512,6 +514,7 @@ impl ToCliReport for DiagnosticData { 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 => {} } } } diff --git a/crates/apollo-compiler/src/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs index f4f94ebd..ce9815f0 100644 --- a/crates/apollo-compiler/src/validation/selection.rs +++ b/crates/apollo-compiler/src/validation/selection.rs @@ -484,15 +484,22 @@ impl<'s, 'doc> FieldsInSetCanMerge<'s, 'doc> { } } - pub(crate) fn validate( + pub(crate) fn validate_operation( &mut self, - root: &'doc executable::SelectionSet, + operation: &'doc Node, diagnostics: &mut DiagnosticList, ) { - let fields = expand_selections(&self.document.fragments, std::iter::once(root)); + 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); + } } fn lookup(&mut self, selections: Vec>) -> Rc> { 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()); +} From 28f8fbf0a505bd2e654986c1c1771c37c1ac1d08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Wed, 14 Feb 2024 10:58:21 +0100 Subject: [PATCH 42/42] ref archived article --- crates/apollo-compiler/src/validation/selection.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/crates/apollo-compiler/src/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs index ce9815f0..672199f6 100644 --- a/crates/apollo-compiler/src/validation/selection.rs +++ b/crates/apollo-compiler/src/validation/selection.rs @@ -453,11 +453,12 @@ const FIELD_DEPTH_LIMIT: usize = 128; /// Implements the `FieldsInSetCanMerge()` validation. /// https://spec.graphql.org/draft/#sec-Field-Selection-Merging /// -/// This uses the [validation algorithm described by Xing][0], which scales much better -/// with larger selection sets that may have many overlapping fields, and with widespread -/// use of fragments. +/// 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. /// /// [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,