Repository: apollographql/apollo-rs
Pull request #524 (2023-04-18 08:39:46)

feat(compiler): validate object type extensions
Closes https://github.com/apollographql/apollo-rs/issues/476

Progress:
* [x] The named type must already be defined
* [x] the already defined type must be an Object type.
* [x] The fields of an Object type extension must have unique names; no two fields may share the same name.
* [x] Any fields of an Object type extension must not be already defined on the original Object type.
* [x] Any non-repeatable directives provided must not already apply to the original Object type.
* [x] Any interfaces provided must not be already implemented by the original Object type.
* [x] The resulting extended object type must be a super-set of all interfaces it implements.

[spec](https://spec.graphql.org/October2021/#sec-Object-Extensions.Type-Validation)

goto-bus-stop: > Any interfaces provided must not be already implemented by the original Object type.

graphql-js doesnt seem to validate this 🤔 and at first glance it doesn't seem like a great rule to me. but maybe i misunderstand it. I'll check their testsuite if they have any tests for this.

I assume it's meant to report on this:
```graphql
type Object implements Intf {
  a: Int
}
extend type Object implements Intf {
}
```


Patch:
diff --git a/crates/apollo-compiler/src/diagnostics.rs b/crates/apollo-compiler/src/diagnostics.rs
index 335b605..7c5a6a5 100644
--- a/crates/apollo-compiler/src/diagnostics.rs
+++ b/crates/apollo-compiler/src/diagnostics.rs
@@ -221,6 +221,8 @@ pub enum DiagnosticData {
     },
     #[error("the required argument `{name}` is not provided")]
     RequiredArgument { name: String },
+    #[error("type `{ty}` can only implement interface `{interface}` once")]
+    DuplicateImplementsInterface { ty: String, interface: String },
     #[error(
         "Transitively implemented interfaces must also be defined on an implementing interface or object"
     )]
diff --git a/crates/apollo-compiler/src/validation/interface.rs b/crates/apollo-compiler/src/validation/interface.rs
index 58be140..934e376 100644
--- a/crates/apollo-compiler/src/validation/interface.rs
+++ b/crates/apollo-compiler/src/validation/interface.rs
@@ -1,4 +1,7 @@
-use std::{collections::HashSet, sync::Arc};
+use std::{
+    collections::{HashMap, HashSet},
+    sync::Arc,
+};
 
 use crate::{
     diagnostics::{ApolloDiagnostic, DiagnosticData, Label},
@@ -70,9 +73,10 @@ pub fn validate_interface_definition(
     diagnostics.extend(db.validate_field_definitions(interface_def.self_fields().to_vec()));
 
     // Implements Interfaceds validation.
-    diagnostics.extend(
-        db.validate_implements_interfaces(interface_def.self_implements_interfaces().to_vec()),
-    );
+    diagnostics.extend(db.validate_implements_interfaces(
+        interface_def.name().to_string(),
+        interface_def.self_implements_interfaces().to_vec(),
+    ));
 
     // When defining an interface that implements another interface, the
     // implementing interface must define each field that is specified by
@@ -131,6 +135,7 @@ pub fn validate_interface_definition(
 
 pub fn validate_implements_interfaces(
     db: &dyn ValidationDatabase,
+    implementor_name: String,
     impl_interfaces: Vec<ImplementsInterface>,
 ) -> Vec<ApolloDiagnostic> {
     let mut diagnostics = Vec::new();
@@ -216,6 +221,33 @@ pub fn validate_implements_interfaces(
         );
     }
 
+    let mut seen = HashMap::<&str, &ImplementsInterface>::new();
+    for impl_interface in &impl_interfaces {
+        let name = impl_interface.interface();
+        if let Some(original) = seen.get(&name) {
+            diagnostics.push(
+                ApolloDiagnostic::new(
+                    db,
+                    impl_interface.loc().into(),
+                    DiagnosticData::DuplicateImplementsInterface {
+                        ty: implementor_name.clone(),
+                        interface: name.to_string(),
+                    },
+                )
+                .label(Label::new(
+                    original.loc(),
+                    format!("`{name}` interface implementation previously declared here"),
+                ))
+                .label(Label::new(
+                    impl_interface.loc(),
+                    format!("`{name}` interface implementation declared again here"),
+                )),
+            );
+        } else {
+            seen.insert(name, impl_interface);
+        }
+    }
+
     diagnostics
 }
 
diff --git a/crates/apollo-compiler/src/validation/object.rs b/crates/apollo-compiler/src/validation/object.rs
index 429d82e..7a2c047 100644
--- a/crates/apollo-compiler/src/validation/object.rs
+++ b/crates/apollo-compiler/src/validation/object.rs
@@ -18,6 +18,18 @@ pub fn validate_object_type_definitions(db: &dyn ValidationDatabase) -> Vec<Apol
     diagnostics
 }
 
+fn collect_nodes<'a, Item: Clone, Ext>(
+    base: &'a [Item],
+    extensions: &'a [Arc<Ext>],
+    method: impl Fn(&'a Ext) -> &'a [Item],
+) -> Vec<Item> {
+    let mut nodes = base.to_vec();
+    for ext in extensions {
+        nodes.extend(method(ext).iter().cloned());
+    }
+    nodes
+}
+
 pub fn validate_object_type_definition(
     db: &dyn ValidationDatabase,
     object: Arc<hir::ObjectTypeDefinition>,
@@ -29,31 +41,42 @@ pub fn validate_object_type_definition(
         hir::DirectiveLocation::Object,
     ));
 
+    // Collect all fields, including duplicates
+    let field_definitions = collect_nodes(
+        object.self_fields(),
+        object.extensions(),
+        hir::ObjectTypeExtension::fields_definition,
+    );
+    let fields: HashSet<ValidationSet> = field_definitions
+        .iter()
+        .map(|field| ValidationSet {
+            name: field.name().into(),
+            loc: field.loc(),
+        })
+        .collect();
+
     // Object Type field validations.
-    diagnostics.extend(db.validate_field_definitions(object.self_fields().to_vec()));
+    diagnostics.extend(db.validate_field_definitions(field_definitions));
 
     // Implements Interfaces validation.
-    diagnostics
-        .extend(db.validate_implements_interfaces(object.self_implements_interfaces().to_vec()));
+    let implements_interfaces = collect_nodes(
+        object.self_implements_interfaces(),
+        object.extensions(),
+        hir::ObjectTypeExtension::implements_interfaces,
+    );
+    diagnostics.extend(
+        db.validate_implements_interfaces(object.name().to_string(), implements_interfaces),
+    );
 
     // When defining an interface that implements another interface, the
     // implementing interface must define each field that is specified by
     // the implemented interface.
     //
     // Returns a Missing Field error.
-    let fields: HashSet<ValidationSet> = object
-        .self_fields()
-        .iter()
-        .map(|field| ValidationSet {
-            name: field.name().into(),
-            loc: field.loc(),
-        })
-        .collect();
-    for implements_interface in object.self_implements_interfaces().iter() {
+    for implements_interface in object.implements_interfaces() {
         if let Some(interface) = implements_interface.interface_definition(db.upcast()) {
             let implements_interface_fields: HashSet<ValidationSet> = interface
-                .self_fields()
-                .iter()
+                .fields()
                 .map(|field| ValidationSet {
                     name: field.name().into(),
                     loc: field.loc(),
diff --git a/crates/apollo-compiler/src/validation/validation_db.rs b/crates/apollo-compiler/src/validation/validation_db.rs
index 7285f83..f2c8fdd 100644
--- a/crates/apollo-compiler/src/validation/validation_db.rs
+++ b/crates/apollo-compiler/src/validation/validation_db.rs
@@ -79,6 +79,7 @@ pub trait ValidationDatabase:
     #[salsa::invoke(interface::validate_implements_interfaces)]
     fn validate_implements_interfaces(
         &self,
+        implementor_name: String,
         impl_interfaces: Vec<ImplementsInterface>,
     ) -> Vec<ApolloDiagnostic>;
 
diff --git a/crates/apollo-parser/src/lexer/mod.rs b/crates/apollo-parser/src/lexer/mod.rs
index 0cbc60c..f6bac5c 100644
--- a/crates/apollo-parser/src/lexer/mod.rs
+++ b/crates/apollo-parser/src/lexer/mod.rs
@@ -30,7 +30,7 @@ pub struct Lexer<'a> {
     input: &'a str,
     index: usize,
     finished: bool,
-    limit: Option<LimitTracker>,
+    pub(crate) limit_tracker: LimitTracker,
 }
 
 impl<'a> Lexer<'a> {
@@ -56,12 +56,12 @@ impl<'a> Lexer<'a> {
             input,
             index: 0,
             finished: false,
-            limit: None,
+            limit_tracker: LimitTracker::new(usize::MAX),
         }
     }
 
     pub fn with_limit(mut self, limit: usize) -> Self {
-        self.limit = Some(LimitTracker::new(limit));
+        self.limit_tracker.limit = limit;
         self
     }
 
@@ -96,15 +96,13 @@ impl<'a> Iterator for Lexer<'a> {
             return Some(Ok(eof));
         }
 
-        if let Some(limit) = &mut self.limit {
-            limit.consume();
-            if limit.limited() {
-                self.finished = true;
-                return Some(Err(Error::limit(
-                    "token limit reached, aborting lexing",
-                    self.index,
-                )));
-            }
+        self.limit_tracker.consume();
+        if self.limit_tracker.limited() {
+            self.finished = true;
+            return Some(Err(Error::limit(
+                "token limit reached, aborting lexing",
+                self.index,
+            )));
         }
 
         let mut c = Cursor::new(self.input);
diff --git a/crates/apollo-parser/src/parser/mod.rs b/crates/apollo-parser/src/parser/mod.rs
index 4a49d48..2e25a28 100644
--- a/crates/apollo-parser/src/parser/mod.rs
+++ b/crates/apollo-parser/src/parser/mod.rs
@@ -124,7 +124,7 @@ impl<'a> Parser<'a> {
         let builder = Rc::try_unwrap(self.builder)
             .expect("More than one reference to builder left")
             .into_inner();
-        builder.finish(self.errors, self.recursion_limit)
+        builder.finish(self.errors, self.recursion_limit, self.lexer.limit_tracker)
     }
 
     /// Check if the current token is `kind`.
@@ -587,4 +587,12 @@ mod tests {
             Definition::InterfaceTypeDefinition(_)
         ));
     }
+
+    #[test]
+    fn token_limit() {
+        let ast = Parser::new("type Query { a a a a a a a a a }")
+            .token_limit(100)
+            .parse();
+        assert_eq!(ast.token_limit().high, 25);
+    }
 }
diff --git a/crates/apollo-parser/src/parser/syntax_tree.rs b/crates/apollo-parser/src/parser/syntax_tree.rs
index e3f8320..3178497 100644
--- a/crates/apollo-parser/src/parser/syntax_tree.rs
+++ b/crates/apollo-parser/src/parser/syntax_tree.rs
@@ -43,6 +43,7 @@ pub struct SyntaxTree {
     pub(crate) green: GreenNode,
     pub(crate) errors: Vec<crate::Error>,
     pub(crate) recursion_limit: LimitTracker,
+    pub(crate) token_limit: LimitTracker,
 }
 
 impl SyntaxTree {
@@ -56,6 +57,11 @@ impl SyntaxTree {
         self.recursion_limit
     }
 
+    /// Get the syntax tree's token limit.
+    pub fn token_limit(&self) -> LimitTracker {
+        self.token_limit
+    }
+
     pub fn green(&self) -> GreenNode {
         self.green.clone()
     }
@@ -156,13 +162,19 @@ impl SyntaxTreeBuilder {
         self.builder.token(rowan::SyntaxKind(kind as u16), text);
     }
 
-    pub(crate) fn finish(self, errors: Vec<Error>, recursion_limit: LimitTracker) -> SyntaxTree {
+    pub(crate) fn finish(
+        self,
+        errors: Vec<Error>,
+        recursion_limit: LimitTracker,
+        token_limit: LimitTracker,
+    ) -> SyntaxTree {
         SyntaxTree {
             green: self.builder.finish(),
             // TODO: keep the errors in the builder rather than pass it in here?
             errors,
-            // TODO: keep the recursion limit in the builder rather than pass it in here?
+            // TODO: keep the recursion and token limits in the builder rather than pass it in here?
             recursion_limit,
+            token_limit,
         }
     }
 }


Test patch:
diff --git a/crates/apollo-compiler/test_data/diagnostics/0094_object_type_extensions.graphql b/crates/apollo-compiler/test_data/diagnostics/0094_object_type_extensions.graphql
new file mode 100644
index 0000000..4168538
--- /dev/null
+++ b/crates/apollo-compiler/test_data/diagnostics/0094_object_type_extensions.graphql
@@ -0,0 +1,43 @@
+type UniqueNames {
+  name: String
+}
+extend type UniqueNames {
+  age: Int
+  age: Int
+}
+
+type ConflictingNames {
+  name: String
+}
+extend type ConflictingNames {
+  name: String
+}
+
+directive @nonRepeatable on OBJECT
+type UniqueDirective @nonRepeatable {
+  a: Int!
+}
+extend type UniqueDirective @nonRepeatable {
+  b: Int
+}
+
+interface Base {
+  name: String
+}
+type UniqueInterfaces implements Base {
+  name: String
+  nickname: String
+}
+extend type UniqueInterfaces implements Base {
+  age: Int
+}
+
+interface ExtendedInterface {
+  name: String
+}
+type ImplementsBaseButNotExtension implements ExtendedInterface {
+  name: String
+}
+extend interface ExtendedInterface {
+  fail: Boolean
+}
diff --git a/crates/apollo-compiler/test_data/diagnostics/0094_object_type_extensions.txt b/crates/apollo-compiler/test_data/diagnostics/0094_object_type_extensions.txt
new file mode 100644
index 0000000..dbf33db
--- /dev/null
+++ b/crates/apollo-compiler/test_data/diagnostics/0094_object_type_extensions.txt
@@ -0,0 +1,246 @@
+[
+    ApolloDiagnostic {
+        cache: {
+            0: "built_in_types.graphql",
+            91: "0094_object_type_extensions.graphql",
+        },
+        location: DiagnosticLocation {
+            file_id: FileId {
+                id: 91,
+            },
+            offset: 64,
+            length: 8,
+        },
+        labels: [
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 91,
+                    },
+                    offset: 64,
+                    length: 8,
+                },
+                text: "previous definition of `age` here",
+            },
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 91,
+                    },
+                    offset: 75,
+                    length: 8,
+                },
+                text: "`age` redefined here",
+            },
+        ],
+        help: Some(
+            "`age` field must only be defined once in this input object definition.",
+        ),
+        data: UniqueField {
+            field: "age",
+            original_definition: DiagnosticLocation {
+                file_id: FileId {
+                    id: 91,
+                },
+                offset: 64,
+                length: 8,
+            },
+            redefined_definition: DiagnosticLocation {
+                file_id: FileId {
+                    id: 91,
+                },
+                offset: 75,
+                length: 8,
+            },
+        },
+    },
+    ApolloDiagnostic {
+        cache: {
+            0: "built_in_types.graphql",
+            91: "0094_object_type_extensions.graphql",
+        },
+        location: DiagnosticLocation {
+            file_id: FileId {
+                id: 91,
+            },
+            offset: 113,
+            length: 12,
+        },
+        labels: [
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 91,
+                    },
+                    offset: 113,
+                    length: 12,
+                },
+                text: "previous definition of `name` here",
+            },
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 91,
+                    },
+                    offset: 161,
+                    length: 12,
+                },
+                text: "`name` redefined here",
+            },
+        ],
+        help: Some(
+            "`name` field must only be defined once in this input object definition.",
+        ),
+        data: UniqueField {
+            field: "name",
+            original_definition: DiagnosticLocation {
+                file_id: FileId {
+                    id: 91,
+                },
+                offset: 113,
+                length: 12,
+            },
+            redefined_definition: DiagnosticLocation {
+                file_id: FileId {
+                    id: 91,
+                },
+                offset: 161,
+                length: 12,
+            },
+        },
+    },
+    ApolloDiagnostic {
+        cache: {
+            0: "built_in_types.graphql",
+            91: "0094_object_type_extensions.graphql",
+        },
+        location: DiagnosticLocation {
+            file_id: FileId {
+                id: 91,
+            },
+            offset: 290,
+            length: 14,
+        },
+        labels: [
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 91,
+                    },
+                    offset: 233,
+                    length: 14,
+                },
+                text: "directive nonRepeatable first called here",
+            },
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 91,
+                    },
+                    offset: 290,
+                    length: 14,
+                },
+                text: "directive nonRepeatable called again here",
+            },
+        ],
+        help: None,
+        data: UniqueDirective {
+            name: "nonRepeatable",
+            original_call: DiagnosticLocation {
+                file_id: FileId {
+                    id: 91,
+                },
+                offset: 233,
+                length: 14,
+            },
+            conflicting_call: DiagnosticLocation {
+                file_id: FileId {
+                    id: 91,
+                },
+                offset: 290,
+                length: 14,
+            },
+        },
+    },
+    ApolloDiagnostic {
+        cache: {
+            0: "built_in_types.graphql",
+            91: "0094_object_type_extensions.graphql",
+        },
+        location: DiagnosticLocation {
+            file_id: FileId {
+                id: 91,
+            },
+            offset: 469,
+            length: 4,
+        },
+        labels: [
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 91,
+                    },
+                    offset: 386,
+                    length: 4,
+                },
+                text: "`Base` interface implementation previously declared here",
+            },
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 91,
+                    },
+                    offset: 469,
+                    length: 4,
+                },
+                text: "`Base` interface implementation declared again here",
+            },
+        ],
+        help: None,
+        data: DuplicateImplementsInterface {
+            ty: "UniqueInterfaces",
+            interface: "Base",
+        },
+    },
+    ApolloDiagnostic {
+        cache: {
+            0: "built_in_types.graphql",
+            91: "0094_object_type_extensions.graphql",
+        },
+        location: DiagnosticLocation {
+            file_id: FileId {
+                id: 91,
+            },
+            offset: 537,
+            length: 82,
+        },
+        labels: [
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 91,
+                    },
+                    offset: 537,
+                    length: 82,
+                },
+                text: "add `fail` field to this object",
+            },
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 91,
+                    },
+                    offset: 659,
+                    length: 13,
+                },
+                text: "`fail` was originally defined here",
+            },
+        ],
+        help: Some(
+            "An object must provide all fields required by the interfaces it implements",
+        ),
+        data: MissingField {
+            field: "fail",
+        },
+    },
+]
diff --git a/crates/apollo-compiler/test_data/diagnostics/0095_interface_implementation_declared_once.graphql b/crates/apollo-compiler/test_data/diagnostics/0095_interface_implementation_declared_once.graphql
new file mode 100644
index 0000000..0458afa
--- /dev/null
+++ b/crates/apollo-compiler/test_data/diagnostics/0095_interface_implementation_declared_once.graphql
@@ -0,0 +1,22 @@
+interface Intf {
+  field: String
+}
+
+type Object implements Intf & Intf {
+  field: String
+}
+
+type Extended implements Intf {
+  field: String
+}
+extend type Extended implements Intf
+
+interface SubIntf implements Intf & Intf {
+  field: String
+}
+
+# TODO(@goto-bus-stop): support interface extensions in validation
+# interface ExtendedIntf implements Intf {
+#   field: String
+# }
+# extend interface ExtendedIntf implements Intf
diff --git a/crates/apollo-compiler/test_data/diagnostics/0095_interface_implementation_declared_once.txt b/crates/apollo-compiler/test_data/diagnostics/0095_interface_implementation_declared_once.txt
new file mode 100644
index 0000000..099cfae
--- /dev/null
+++ b/crates/apollo-compiler/test_data/diagnostics/0095_interface_implementation_declared_once.txt
@@ -0,0 +1,122 @@
+[
+    ApolloDiagnostic {
+        cache: {
+            0: "built_in_types.graphql",
+            92: "0095_interface_implementation_declared_once.graphql",
+        },
+        location: DiagnosticLocation {
+            file_id: FileId {
+                id: 92,
+            },
+            offset: 216,
+            length: 4,
+        },
+        labels: [
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 92,
+                    },
+                    offset: 209,
+                    length: 4,
+                },
+                text: "`Intf` interface implementation previously declared here",
+            },
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 92,
+                    },
+                    offset: 216,
+                    length: 4,
+                },
+                text: "`Intf` interface implementation declared again here",
+            },
+        ],
+        help: None,
+        data: DuplicateImplementsInterface {
+            ty: "SubIntf",
+            interface: "Intf",
+        },
+    },
+    ApolloDiagnostic {
+        cache: {
+            0: "built_in_types.graphql",
+            92: "0095_interface_implementation_declared_once.graphql",
+        },
+        location: DiagnosticLocation {
+            file_id: FileId {
+                id: 92,
+            },
+            offset: 66,
+            length: 4,
+        },
+        labels: [
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 92,
+                    },
+                    offset: 59,
+                    length: 4,
+                },
+                text: "`Intf` interface implementation previously declared here",
+            },
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 92,
+                    },
+                    offset: 66,
+                    length: 4,
+                },
+                text: "`Intf` interface implementation declared again here",
+            },
+        ],
+        help: None,
+        data: DuplicateImplementsInterface {
+            ty: "Object",
+            interface: "Intf",
+        },
+    },
+    ApolloDiagnostic {
+        cache: {
+            0: "built_in_types.graphql",
+            92: "0095_interface_implementation_declared_once.graphql",
+        },
+        location: DiagnosticLocation {
+            file_id: FileId {
+                id: 92,
+            },
+            offset: 174,
+            length: 4,
+        },
+        labels: [
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 92,
+                    },
+                    offset: 117,
+                    length: 4,
+                },
+                text: "`Intf` interface implementation previously declared here",
+            },
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 92,
+                    },
+                    offset: 174,
+                    length: 4,
+                },
+                text: "`Intf` interface implementation declared again here",
+            },
+        ],
+        help: None,
+        data: DuplicateImplementsInterface {
+            ty: "Extended",
+            interface: "Intf",
+        },
+    },
+]