feat(compiler): validate field merging using the Xing algorithm (#816)

* 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.

* Fix field merging validation when more than 2 fields are in play

* clippy

* Pull some field merging code into smaller functions

* Use high-level representation for field merging validation

* Bail out of field merging validation if selection set contains fragment cycles

* Make same response shape less mega quadratic recursive

* Make same field selection check less mega quadratic recursive

* Port subscription validation that uses field selection expansion

* Migrate subscription validation errors to use BuildError

* clippy

* Skip some unnecessary arc clones

* deref

* The individual checks can now both be implemented linearly

* Add many repeated fields validation benchmark

* field(a:1, b:2) and field(b:2, a:1) should compare equal

* 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.

* Port field merging tests from graphql-js

* 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.

* doc comments

* Take iterator as input for expand_selections

* Add a cache for merged selection sets

* changelog

* Rework the ConflictingFieldNames diagnostic

* mention fragment definitions validation change

* Tweak error reports for conflicting types and arguments

* Fix field name conflict tests

* Ensure consistent ordering of errors by using indexmap

* Push field merging errors directly into DiagnosticList

* Remove unnecessary fragment recursion check

* Use a map to optimize comparing large argument lists

* Benchmark checking a field with very many arguments

* update diagnostic message: multiple -> different

* remove outdated comment

* useless zip

* Use Entry::or_default

Co-authored-by: Simon Sapin <[email protected]>

* make clippy happy with the benchmarking code

* Add a recursion limit while merging fields

* check -> already_done

* Move ArgumentLookup out of the function

* Emit an error when the new recursion limit is reached; add test

* ref archived article

---------

Co-authored-by: Simon Sapin <[email protected]>

crates/apollo-compiler/CHANGELOG.md

@@ -17,6 +17,21 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 ## Maintenance
 ## Documentation-->
 
+# [x.x.x] (unreleased) - 2024-mm-dd
+
+## Features
+- **New field merging validation implementation - [goto-bus-stop], [pull/816]**
+  - Uses the much more scalable [Xing algorithm].
+  - This also fixes some invalid selections that were previously accepted by apollo-compiler.
+  - Selections in fragment definitions are now only validated in the context of the operations they
+    are used in, reducing duplicate error reports for invalid fragments. This means invalid *unused*
+    fragments do not produce field merging errors, but they will still raise a different error because
+    fragments being unused is also an error according to the spec.
+
+[goto-bus-stop]: https://github.com/goto-bus-stop]
+[pull/816]: https://github.com/apollographql/apollo-rs/pull/816
+[Xing algorithm]: https://tech.new-work.se/graphql-overlapping-fields-can-be-merged-fast-ea6e92e0a01
+
 # [1.0.0-beta.12](https://crates.io/crates/apollo-compiler/1.0.0-beta.12) - 2024-01-15
 
 ## BREAKING

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"
 

crates/apollo-compiler/benches/fields_validation.rs

@@ -0,0 +1,124 @@
+use apollo_compiler::ExecutableDocument;
+use apollo_compiler::Schema;
+use criterion::*;
+
+fn bench_many_same_field(c: &mut Criterion) {
+    let schema =
+        Schema::parse_and_validate("type Query { hello: String! }", "schema.graphql").unwrap();
+    let query = format!("{{ {} }}", "hello ".repeat(1_000));
+
+    c.bench_function("many_same_field", move |b| {
+        b.iter(|| {
+            let doc =
+                ExecutableDocument::parse_and_validate(&schema, &query, "query.graphql").unwrap();
+            black_box(doc);
+        });
+    });
+}
+
+fn bench_many_same_nested_field(c: &mut Criterion) {
+    let schema = Schema::parse_and_validate(
+        "
+        type Nested { hello: String! }
+        type Query { nested: Nested! }
+    ",
+        "schema.graphql",
+    )
+    .unwrap();
+    let query = format!("{{ {} }}", "nested { hello } ".repeat(1_000));
+
+    c.bench_function("many_same_nested_field", move |b| {
+        b.iter(|| {
+            let doc =
+                ExecutableDocument::parse_and_validate(&schema, &query, "query.graphql").unwrap();
+            black_box(doc);
+        });
+    });
+}
+
+fn bench_many_arguments(c: &mut Criterion) {
+    let schema =
+        Schema::parse_and_validate("type Query { hello: String! }", "schema.graphql").unwrap();
+    let args = (0..2_000).fold(String::new(), |mut acc, i| {
+        use std::fmt::Write;
+        _ = writeln!(&mut acc, "arg{i}: {i}");
+        acc
+    });
+    let field = format!("hello({args})");
+    let query = format!("{{ {field} {field} }}");
+
+    c.bench_function("many_arguments", move |b| {
+        b.iter(|| {
+            // Will return errors but that's cool
+            let doc = ExecutableDocument::parse_and_validate(&schema, &query, "query.graphql");
+            _ = black_box(doc);
+        });
+    });
+}
+
+fn bench_many_types(c: &mut Criterion) {
+    let schema = Schema::parse_and_validate(
+        "
+        interface Abstract {
+          field: Abstract
+          leaf: Int
+        }
+        interface Abstract1 {
+          field: Abstract
+          leaf: Int
+        }
+        interface Abstract2 {
+          field: Abstract
+          leaf: Int
+        }
+        type Concrete1 implements Abstract & Abstract1 {
+          field: Abstract
+          leaf: Int
+        }
+        type Concrete2 implements Abstract & Abstract2 {
+          field: Abstract
+          leaf: Int
+        }
+        type Query {
+            field: Abstract
+        }
+    ",
+        "schema.graphql",
+    )
+    .unwrap();
+    let query = format!(
+        "
+        fragment multiply on Abstract {{
+           field {{
+             ... on Abstract1 {{ field {{ leaf }} }}
+             ... on Abstract2 {{ field {{ leaf }} }}
+             ... on Concrete1 {{ field {{ leaf }} }}
+             ... on Concrete2 {{ field {{ leaf }} }}
+           }}
+        }}
+
+        query DeepAbstractConcrete {{
+            {open}{close}
+        }}
+    ",
+        open = "field { ...multiply ".repeat(100),
+        close = "}".repeat(100)
+    );
+
+    c.bench_function("many_types", move |b| {
+        b.iter(|| {
+            let doc =
+                ExecutableDocument::parse_and_validate(&schema, &query, "query.graphql").unwrap();
+            black_box(doc);
+        });
+    });
+}
+
+criterion_group!(
+    fields,
+    bench_many_same_field,
+    bench_many_same_nested_field,
+    bench_many_arguments,
+    bench_many_types,
+);
+criterion_main!(fields);

crates/apollo-compiler/benches/testdata/supergraph_query.graphql

@@ -1,10 +1,5 @@
 query Query {
-  allProducts {
-    id
+  topProducts {
     sku
-    createdBy {
-      email
-      totalProductsCreated
-    }
   }
-}
+}

crates/apollo-compiler/src/executable/mod.rs

@@ -2,6 +2,8 @@
 //! which can contain operations and fragments.
 
 use crate::ast;
+use crate::coordinate::FieldArgumentCoordinate;
+use crate::coordinate::TypeAttributeCoordinate;
 use crate::schema;
 use crate::Node;
 use crate::Parser;
@@ -69,20 +71,20 @@ pub struct Fragment {
     pub selection_set: SelectionSet,
 }
 
-#[derive(Debug, Clone, PartialEq, Eq)]
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
 pub struct SelectionSet {
     pub ty: NamedType,
     pub selections: Vec<Selection>,
 }
 
-#[derive(Debug, Clone, PartialEq, Eq)]
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
 pub enum Selection {
     Field(Node<Field>),
     FragmentSpread(Node<FragmentSpread>),
     InlineFragment(Node<InlineFragment>),
 }
 
-#[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<schema::FieldDefinition>,
@@ -93,20 +95,21 @@ pub struct Field {
     pub selection_set: SelectionSet,
 }
 
-#[derive(Debug, Clone, PartialEq, Eq)]
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
 pub struct FragmentSpread {
     pub fragment_name: Name,
     pub directives: DirectiveList,
 }
 
-#[derive(Debug, Clone, PartialEq, Eq)]
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
 pub struct InlineFragment {
     pub type_condition: Option<NamedType>,
     pub directives: DirectiveList,
     pub selection_set: SelectionSet,
 }
 
-/// AST node that has been skipped during conversion to `ExecutableDocument`
+/// Errors that can occur during conversion from AST to executable document or
+/// validation of an executable document.
 #[derive(thiserror::Error, Debug, Clone)]
 pub(crate) enum BuildError {
     #[error("an executable document must not contain {describe}")]
@@ -161,6 +164,67 @@ pub(crate) enum BuildError {
         field_name: Name,
         path: SelectionPath,
     },
+
+    // Validation errors
+    #[error(
+        "{} can only have one root field",
+        subscription_name_or_anonymous(name)
+    )]
+    SubscriptionUsesMultipleFields {
+        name: Option<Name>,
+        fields: Vec<Name>,
+    },
+
+    #[error(
+        "{} can not have an introspection field as a root field",
+        subscription_name_or_anonymous(name)
+    )]
+    SubscriptionUsesIntrospection {
+        /// Name of the operation
+        name: Option<Name>,
+        /// 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<NodeLocation>,
+        original_coordinate: TypeAttributeCoordinate,
+        original_type: Type,
+        conflicting_location: Option<NodeLocation>,
+        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<NodeLocation>,
+        original_coordinate: FieldArgumentCoordinate,
+        original_value: Option<Value>,
+        conflicting_location: Option<NodeLocation>,
+        conflicting_coordinate: FieldArgumentCoordinate,
+        conflicting_value: Option<Value>,
+    },
+    #[error("cannot select different fields into the same alias `{alias}`")]
+    ConflictingFieldName {
+        /// Name of the non-unique field.
+        alias: Name,
+        original_location: Option<NodeLocation>,
+        original_selection: TypeAttributeCoordinate,
+        conflicting_location: Option<NodeLocation>,
+        conflicting_selection: TypeAttributeCoordinate,
+    },
+}
+
+fn subscription_name_or_anonymous(name: &Option<Name>) -> impl std::fmt::Display + '_ {
+    crate::validation::diagnostics::NameOrAnon {
+        name: name.as_ref(),
+        if_some_prefix: "subscription",
+        if_none: "anonymous subscription",
+    }
 }
 
 #[derive(Debug, Clone, PartialEq, Eq)]
@@ -327,10 +391,26 @@ impl PartialEq for ExecutableDocument {
 }
 
 impl Operation {
+    /// Returns the name of the schema type this operation selects against.
     pub fn object_type(&self) -> &NamedType {
         &self.selection_set.ty
     }
 
+    /// Returns true if this is a query operation.
+    pub fn is_query(&self) -> bool {
+        self.operation_type == OperationType::Query
+    }
+
+    /// Returns true if this is a mutation operation.
+    pub fn is_mutation(&self) -> bool {
+        self.operation_type == OperationType::Mutation
+    }
+
+    /// Returns true if this is a subscription operation.
+    pub fn is_subscription(&self) -> bool {
+        self.operation_type == OperationType::Subscription
+    }
+
     /// Return whether this operation is a query that only selects introspection meta-fields:
     /// `__type`, `__schema`, and `__typename`
     pub fn is_introspection(&self, document: &ExecutableDocument) -> bool {
@@ -591,6 +671,11 @@ impl Field {
         schema.types.get(self.ty().inner_named_type())
     }
 
+    /// Returns the argument by a given name.
+    pub fn argument_by_name(&self, name: &str) -> Option<&'_ Node<Argument>> {
+        self.arguments.iter().find(|argument| argument.name == name)
+    }
+
     serialize_method!();
 }
 

crates/apollo-compiler/src/executable/validation.rs

@@ -1,5 +1,6 @@
 use super::FieldSet;
 use crate::ast;
+use crate::validation::selection::FieldsInSetCanMerge;
 use crate::validation::Details;
 use crate::validation::DiagnosticList;
 use crate::validation::FileId;
@@ -30,11 +31,15 @@ pub(crate) fn validate_standalone_executable(
 }
 
 fn validate_with_schema(
-    _errors: &mut DiagnosticList,
-    _schema: &Schema,
-    _document: &ExecutableDocument,
+    errors: &mut DiagnosticList,
+    schema: &Schema,
+    document: &ExecutableDocument,
 ) {
-    // TODO
+    let mut fields_in_set_can_merge = FieldsInSetCanMerge::new(schema, document);
+    for operation in document.all_operations() {
+        crate::validation::operation::validate_subscription(document, operation, errors);
+        fields_in_set_can_merge.validate_operation(operation, errors);
+    }
 }
 
 pub(crate) fn validate_with_or_without_schema(