Repository: apollographql/apollo-rs
Pull request #484 (2023-03-16 18:07:56)

feat(compiler): implicit extensions in HIR root operation APIs
A collection of loosely related changes, part of https://github.com/apollographql/apollo-rs/issues/468.

HIR no longer creates an implicit root operation if an explicit one exists in a schema extension, even if an object type named `Query`/`Mutation`/`Subscription` exists and the base schema definition doesn’t define an explicit root operation of that type.

`SchemaDefinition::root_operation_type_definition()` was renamed to `self_root_operations`.

`SchemaDefinition::root_operations() -> impl Iterator` was added. It includes root operations from extensions.

`SchemaDefinition::query()`, `mutation()`, and `subscription()` now account for schema extensions, and return `Option<&str>` with the name instead of the definition of the corresponding object type. That definition can be found at `compiler.db.objects().get(name)`. starting % with '%' will be ignored, and an empty message aborts the commit.

As a side effect of that last change these methods can return `Some(name)` for invalid documents where that name doesn’t have a type definition, or it’s not an object type. Previously they would return `None` since there is no object type definition to return. As a result, validation stopped emitting "`query` root operation type must be defined here" diagnostics for such documents. I believe this is correct, since a "not found in this scope" or "root operation type must be of an Object Type" diagnostic was and still is emitted separately.


Patch:
diff --git a/crates/apollo-compiler/src/database/hir.rs b/crates/apollo-compiler/src/database/hir.rs
index 38739cf..2c50607 100644
--- a/crates/apollo-compiler/src/database/hir.rs
+++ b/crates/apollo-compiler/src/database/hir.rs
@@ -446,11 +446,13 @@ impl OperationDefinition {
 
     /// Get operation's definition object type.
     pub fn object_type(&self, db: &dyn HirDatabase) -> Option<Arc<ObjectTypeDefinition>> {
-        match self.operation_ty {
-            OperationType::Query => db.schema().query(db),
-            OperationType::Mutation => db.schema().mutation(db),
-            OperationType::Subscription => db.schema().subscription(db),
-        }
+        let schema = db.schema();
+        let name = match self.operation_ty {
+            OperationType::Query => schema.query()?,
+            OperationType::Mutation => schema.mutation()?,
+            OperationType::Subscription => schema.subscription()?,
+        };
+        db.object_types().get(name).cloned()
     }
 
     /// Get a reference to the operation definition's variables.
@@ -554,13 +556,19 @@ impl OperationType {
     }
 }
 
+impl From<OperationType> for &'static str {
+    fn from(ty: OperationType) -> &'static str {
+        match ty {
+            OperationType::Query => "Query",
+            OperationType::Mutation => "Mutation",
+            OperationType::Subscription => "Subscription",
+        }
+    }
+}
+
 impl fmt::Display for OperationType {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        match self {
-            OperationType::Query => write!(f, "Query"),
-            OperationType::Mutation => write!(f, "Mutation"),
-            OperationType::Subscription => write!(f, "Subscription"),
-        }
+        f.write_str((*self).into())
     }
 }
 
@@ -1689,6 +1697,14 @@ pub struct SchemaDefinition {
     pub(crate) root_operation_type_definition: Arc<Vec<RootOperationTypeDefinition>>,
     pub(crate) loc: Option<HirNodeLocation>,
     pub(crate) extensions: Vec<Arc<SchemaExtension>>,
+    pub(crate) root_operation_names: RootOperationNames,
+}
+
+#[derive(Default, Clone, Debug, Hash, PartialEq, Eq)]
+pub(crate) struct RootOperationNames {
+    pub(crate) query: Option<String>,
+    pub(crate) mutation: Option<String>,
+    pub(crate) subscription: Option<String>,
 }
 
 impl SchemaDefinition {
@@ -1732,16 +1748,19 @@ impl SchemaDefinition {
             .filter(move |directive| directive.name() == name)
     }
 
-    /// Get a reference to the schema definition's root operation type definition.
-    pub fn root_operation_type_definition(&self) -> &[RootOperationTypeDefinition] {
+    /// Returns the root operations from this schema definition,
+    /// excluding those from schema extensions.
+    pub fn self_root_operations(&self) -> &[RootOperationTypeDefinition] {
         self.root_operation_type_definition.as_ref()
     }
 
-    /// Set the schema definition's root operation type definition.
-    pub(crate) fn set_root_operation_type_definition(&mut self, op: RootOperationTypeDefinition) {
-        Arc::get_mut(&mut self.root_operation_type_definition)
-            .unwrap()
-            .push(op)
+    /// Returns an iterator of root operations, from either on this schema defintion or its extensions.
+    pub fn root_operations(&self) -> impl Iterator<Item = &RootOperationTypeDefinition> {
+        self.self_root_operations().iter().chain(
+            self.extensions()
+                .iter()
+                .flat_map(|ext| ext.root_operations()),
+        )
     }
 
     /// Get the AST location information for this HIR node.
@@ -1754,40 +1773,31 @@ impl SchemaDefinition {
         &self.extensions
     }
 
-    // NOTE(@lrlna): potentially have the following fns on the database itself
-    // so they are memoised as well
-
-    /// Get Schema's query object type definition.
-    pub fn query(&self, db: &dyn HirDatabase) -> Option<Arc<ObjectTypeDefinition>> {
-        self.root_operation_type_definition().iter().find_map(|op| {
-            if op.operation_ty.is_query() {
-                op.object_type(db)
-            } else {
-                None
-            }
-        })
+    /// Returns the name of the object type for the `query` root operation,
+    /// defined either on this schema defintion or its extensions.
+    ///
+    /// The corresponding object type definition can be found
+    /// at [`compiler.db.object_types().get(name)`][HirDatabase::object_types].
+    pub fn query(&self) -> Option<&str> {
+        self.root_operation_names.query.as_deref()
     }
 
-    /// Get Schema's mutation object type definition.
-    pub fn mutation(&self, db: &dyn HirDatabase) -> Option<Arc<ObjectTypeDefinition>> {
-        self.root_operation_type_definition().iter().find_map(|op| {
-            if op.operation_ty.is_mutation() {
-                op.object_type(db)
-            } else {
-                None
-            }
-        })
+    /// Returns the name of the object type for the `mutation` root operation,
+    /// defined either on this schema defintion or its extensions.
+    ///
+    /// The corresponding object type definition can be found
+    /// at [`compiler.db.object_types().get(name)`][HirDatabase::object_types].
+    pub fn mutation(&self) -> Option<&str> {
+        self.root_operation_names.mutation.as_deref()
     }
 
-    /// Get Schema's subscription object type definition.
-    pub fn subscription(&self, db: &dyn HirDatabase) -> Option<Arc<ObjectTypeDefinition>> {
-        self.root_operation_type_definition().iter().find_map(|op| {
-            if op.operation_ty.is_subscription() {
-                op.object_type(db)
-            } else {
-                None
-            }
-        })
+    /// Returns the name of the object type for the `subscription` root operation,
+    /// defined either on this schema defintion or its extensions.
+    ///
+    /// The corresponding object type definition can be found
+    /// at [`compiler.db.object_types().get(name)`][HirDatabase::object_types].
+    pub fn subscription(&self) -> Option<&str> {
+        self.root_operation_names.subscription.as_deref()
     }
 }
 
@@ -2861,7 +2871,7 @@ impl SchemaExtension {
     }
 
     /// Get a reference to the schema definition's root operation type definition.
-    pub fn root_operation_type_definition(&self) -> &[RootOperationTypeDefinition] {
+    pub fn root_operations(&self) -> &[RootOperationTypeDefinition] {
         self.root_operation_type_definition.as_ref()
     }
 
@@ -3290,6 +3300,50 @@ mod tests {
         assert_eq!(default_values[3], "98765432109876540000");
     }
 
+    #[test]
+    fn root_operations() {
+        let mut compiler = ApolloCompiler::new();
+        let first = r#"
+            schema @core(feature: "https://specs.apollo.dev/core/v0.1")
+            type Query {
+                field: Int
+            }
+            type Subscription {
+                newsletter: [String]
+            }
+        "#;
+        let second = r#"
+            extend schema @core(feature: "https://specs.apollo.dev/join/v0.1") {
+                query: MyQuery
+            }
+            type MyQuery {
+                different_field: String
+            }
+        "#;
+        compiler.add_type_system(first, "first.graphql");
+        compiler.add_type_system(second, "second.graphql");
+
+        let schema = compiler.db.schema();
+        assert_eq!(
+            schema
+                .self_root_operations()
+                .iter()
+                .map(|op| op.named_type().name())
+                .collect::<Vec<_>>(),
+            ["Subscription"]
+        );
+        assert_eq!(
+            schema
+                .root_operations()
+                .map(|op| op.named_type().name())
+                .collect::<Vec<_>>(),
+            ["Subscription", "MyQuery"]
+        );
+        assert!(schema.mutation().is_none());
+        assert_eq!(schema.query().unwrap(), "MyQuery");
+        assert_eq!(schema.subscription().unwrap(), "Subscription");
+    }
+
     #[test]
     fn extensions() {
         let mut compiler = ApolloCompiler::new();
diff --git a/crates/apollo-compiler/src/database/hir_db.rs b/crates/apollo-compiler/src/database/hir_db.rs
index 1f48f5f..fec40b7 100644
--- a/crates/apollo-compiler/src/database/hir_db.rs
+++ b/crates/apollo-compiler/src/database/hir_db.rs
@@ -347,24 +347,11 @@ fn schema(db: &dyn HirDatabase) -> Arc<SchemaDefinition> {
         // Panics in `ApolloCompiler` methods ensure `type_definition_files().is_empty()`
         return precomputed.definitions.schema.clone();
     }
-    let mut schema_def = type_definitions(db, schema_definition)
-        .next()
-        .unwrap_or_default();
-
-    // NOTE(@lrlna):
-    //
-    // "Query", "Subscription", "Mutation" object type definitions do not need
-    // to be explicitly defined in a schema definition, but are implicitly
-    // added.
-    //
-    // There will be a time when we need to distinguish between implicit and
-    // explicit definitions for validation purposes.
-    let type_defs = add_object_type_id_to_schema(db);
-    type_defs
-        .iter()
-        .for_each(|type_def| schema_def.set_root_operation_type_definition(type_def.clone()));
-
-    Arc::new(schema_def)
+    Arc::new(
+        type_definitions(db, schema_definition)
+            .next()
+            .unwrap_or_else(|| implicit_schema_definition(db)),
+    )
 }
 
 macro_rules! by_name {
@@ -476,17 +463,13 @@ fn operation_definition(
     let name = op_def.name().map(|n| name_hir_node(n, file_id));
     let ty = operation_type(op_def.operation_type());
     let variables = variable_definitions(op_def.variable_definitions(), file_id);
-    let parent_object_ty = db
-        .schema()
-        .root_operation_type_definition()
-        .iter()
-        .find_map(|op| {
-            if op.operation_ty() == ty {
-                Some(op.named_type().name())
-            } else {
-                None
-            }
-        });
+    let parent_object_ty = db.schema().self_root_operations().iter().find_map(|op| {
+        if op.operation_ty() == ty {
+            Some(op.named_type().name())
+        } else {
+            None
+        }
+    });
     let selection_set = selection_set(db, op_def.selection_set(), parent_object_ty, file_id);
     let directives = directives(op_def.directives(), file_id);
     let loc = location(file_id, op_def.syntax());
@@ -536,33 +519,115 @@ fn schema_definition(
     schema_def: ast::SchemaDefinition,
     file_id: FileId,
 ) -> Option<SchemaDefinition> {
-    let extensions = type_definitions(db, |_db, def: ast::SchemaExtension, file_id| {
-        Some(Arc::new(SchemaExtension {
-            directives: directives(def.directives(), file_id),
-            root_operation_type_definition: root_operation_type_definition(
-                def.root_operation_type_definitions(),
-                file_id,
-            ),
-            loc: location(file_id, def.syntax()),
-        }))
-    })
-    .collect();
-
     let description = description(schema_def.description());
     let directives = directives(schema_def.directives(), file_id);
-    let root_operation_type_definition =
+    let mut operations =
         root_operation_type_definition(schema_def.root_operation_type_definitions(), file_id);
     let loc = location(file_id, schema_def.syntax());
+    let extensions = schema_extensions(db);
+    let mut root_operation_names = root_operations_names(&operations, &extensions);
+    add_implicit_operations(db, &mut operations, &mut root_operation_names);
 
     Some(SchemaDefinition {
         description,
         directives,
-        root_operation_type_definition,
+        root_operation_type_definition: Arc::new(operations),
         loc: Some(loc),
         extensions,
+        root_operation_names,
     })
 }
 
+fn implicit_schema_definition(db: &dyn HirDatabase) -> SchemaDefinition {
+    let extensions = schema_extensions(db);
+    let mut operations = Vec::new();
+    let mut root_operation_names = root_operations_names(&operations, &extensions);
+    add_implicit_operations(db, &mut operations, &mut root_operation_names);
+    SchemaDefinition {
+        description: None,
+        directives: Arc::new(Vec::new()),
+        root_operation_type_definition: Arc::new(operations),
+        loc: None,
+        extensions,
+        root_operation_names,
+    }
+}
+
+fn schema_extensions(db: &dyn HirDatabase) -> Vec<Arc<SchemaExtension>> {
+    type_definitions(db, |_db, def: ast::SchemaExtension, file_id| {
+        let directives = directives(def.directives(), file_id);
+        let root_operation_type_definition = Arc::new(root_operation_type_definition(
+            def.root_operation_type_definitions(),
+            file_id,
+        ));
+        let loc = location(file_id, def.syntax());
+        Some(Arc::new(SchemaExtension {
+            directives,
+            root_operation_type_definition,
+            loc,
+        }))
+    })
+    .collect()
+}
+
+fn root_operations_names(
+    root_operation_type_definition: &[RootOperationTypeDefinition],
+    extensions: &[Arc<SchemaExtension>],
+) -> RootOperationNames {
+    let mut names = RootOperationNames::default();
+    let mut add_operations = |ops: &[RootOperationTypeDefinition]| {
+        for op in ops {
+            let name_field = match op.operation_ty() {
+                OperationType::Query => &mut names.query,
+                OperationType::Mutation => &mut names.mutation,
+                OperationType::Subscription => &mut names.subscription,
+            };
+            if name_field.is_none() {
+                *name_field = Some(op.named_type().name());
+            }
+        }
+    };
+    add_operations(root_operation_type_definition);
+    for extension in extensions {
+        add_operations(extension.root_operations());
+    }
+    names
+}
+
+/// https://spec.graphql.org/October2021/#sec-Root-Operation-Types.Default-Root-Operation-Type-Names
+///
+/// To distinguish between implicit and explicit definitions for validation purposes,
+/// check `operation.loc.is_none()`.
+///
+/// NOTE(@lrlna):
+/// "Query", "Subscription", "Mutation" object type definitions do not need
+/// to be explicitly defined in a schema definition, but are implicitly
+/// added.
+fn add_implicit_operations(
+    db: &dyn HirDatabase,
+    operations: &mut Vec<RootOperationTypeDefinition>,
+    names: &mut RootOperationNames,
+) {
+    for (name_field, operation_ty) in [
+        (&mut names.query, OperationType::Query),
+        (&mut names.mutation, OperationType::Mutation),
+        (&mut names.subscription, OperationType::Subscription),
+    ] {
+        let name = operation_ty.into();
+        if name_field.is_none() && db.object_types().contains_key(name) {
+            *name_field = Some(name.to_owned());
+            operations.push(RootOperationTypeDefinition {
+                operation_ty,
+                named_type: Type::Named {
+                    name: name.to_owned(),
+                    loc: None,
+                },
+                loc: None,
+            })
+        }
+    }
+}
+
 fn object_type_definition(
     _db: &dyn HirDatabase,
     obj_def: ast::ObjectTypeDefinition,
@@ -913,38 +978,6 @@ fn extension(db: &dyn HirDatabase, def: ast::Definition, file_id: FileId) -> Opt
     }
 }
 
-fn add_object_type_id_to_schema(db: &dyn HirDatabase) -> Arc<Vec<RootOperationTypeDefinition>> {
-    // Schema Definition does not have to be present in the SDL if ObjectType name is
-    // - Query
-    // - Subscription
-    // - Mutation
-    //
-    // Compiler's internal schema, however, should have a reference to these
-    // object types if they are present
-    let type_defs: Vec<RootOperationTypeDefinition> = db
-        .object_types()
-        .values()
-        .filter_map(|obj_type| {
-            let obj_name = obj_type.name();
-            if matches!(obj_name, "Query" | "Subscription" | "Mutation") {
-                let operation_type = obj_name.into();
-                Some(RootOperationTypeDefinition {
-                    operation_ty: operation_type,
-                    named_type: Type::Named {
-                        name: obj_name.to_string(),
-                        loc: None,
-                    },
-                    loc: None,
-                })
-            } else {
-                None
-            }
-        })
-        .collect();
-
-    Arc::new(type_defs)
-}
-
 fn implements_interfaces(
     implements_interfaces: Option<ast::ImplementsInterfaces>,
     file_id: FileId,
@@ -1073,8 +1106,8 @@ fn default_value(
 fn root_operation_type_definition(
     root_type_def: AstChildren<ast::RootOperationTypeDefinition>,
     file_id: FileId,
-) -> Arc<Vec<RootOperationTypeDefinition>> {
-    let type_defs: Vec<RootOperationTypeDefinition> = root_type_def
+) -> Vec<RootOperationTypeDefinition> {
+    root_type_def
         .into_iter()
         .filter_map(|ty| {
             if let Some(named_ty) = ty.named_type() {
@@ -1091,9 +1124,7 @@ fn root_operation_type_definition(
                 None
             }
         })
-        .collect();
-
-    Arc::new(type_defs)
+        .collect()
 }
 
 fn operation_type(op_type: Option<ast::OperationType>) -> OperationType {
diff --git a/crates/apollo-compiler/src/validation/operation.rs b/crates/apollo-compiler/src/validation/operation.rs
index 840baaa..7630a1f 100644
--- a/crates/apollo-compiler/src/validation/operation.rs
+++ b/crates/apollo-compiler/src/validation/operation.rs
@@ -133,7 +133,7 @@ pub fn validate_subscription_operations(
     // defined schema root operation types.
     //
     //   * subscription operation - subscription root operation
-    if !subscriptions.is_empty() && db.schema().subscription(db.upcast()).is_none() {
+    if !subscriptions.is_empty() && db.schema().subscription().is_none() {
         let unsupported_ops: Vec<ApolloDiagnostic> = subscriptions
             .iter()
             .map(|op| {
@@ -162,7 +162,7 @@ pub fn validate_query_operations(
     // defined schema root operation types.
     //
     //   * query operation - query root operation
-    if !queries.is_empty() && db.schema().query(db.upcast()).is_none() {
+    if !queries.is_empty() && db.schema().query().is_none() {
         let unsupported_ops: Vec<ApolloDiagnostic> = queries
             .iter()
             .map(|op| {
@@ -202,7 +202,7 @@ pub fn validate_mutation_operations(
     // defined schema root operation types.
     //
     //   * mutation operation - mutation root operation
-    if !mutations.is_empty() && db.schema().mutation(db.upcast()).is_none() {
+    if !mutations.is_empty() && db.schema().mutation().is_none() {
         let unsupported_ops: Vec<ApolloDiagnostic> = mutations
             .iter()
             .map(|op| {
diff --git a/crates/apollo-compiler/src/validation/schema.rs b/crates/apollo-compiler/src/validation/schema.rs
index 86e0f38..4441485 100644
--- a/crates/apollo-compiler/src/validation/schema.rs
+++ b/crates/apollo-compiler/src/validation/schema.rs
@@ -12,7 +12,7 @@ pub fn validate_schema_definition(
     let mut diagnostics = Vec::new();
 
     // A GraphQL schema must have a Query root operation.
-    if schema_def.query(db.upcast()).is_none() {
+    if schema_def.query().is_none() {
         if let Some(loc) = db.schema().loc() {
             diagnostics.push(
                 ApolloDiagnostic::new(db, loc.into(), DiagnosticData::QueryRootOperationType)
@@ -23,11 +23,8 @@ pub fn validate_schema_definition(
             );
         }
     }
-    diagnostics.extend(
-        db.validate_root_operation_definitions(
-            schema_def.root_operation_type_definition().to_vec(),
-        ),
-    );
+    diagnostics
+        .extend(db.validate_root_operation_definitions(schema_def.self_root_operations().to_vec()));
 
     diagnostics.extend(db.validate_directives(
         schema_def.self_directives().to_vec(),


Test patch:
diff --git a/crates/apollo-compiler/test_data/diagnostics/0059_root_operation_object_type.txt b/crates/apollo-compiler/test_data/diagnostics/0059_root_operation_object_type.txt
index 1c15cce..1cc2507 100644
--- a/crates/apollo-compiler/test_data/diagnostics/0059_root_operation_object_type.txt
+++ b/crates/apollo-compiler/test_data/diagnostics/0059_root_operation_object_type.txt
@@ -1,30 +1,4 @@
 [
-    ApolloDiagnostic {
-        cache: {
-            58: "0059_root_operation_object_type.graphql",
-        },
-        location: DiagnosticLocation {
-            file_id: FileId {
-                id: 58,
-            },
-            offset: 0,
-            length: 36,
-        },
-        labels: [
-            Label {
-                location: DiagnosticLocation {
-                    file_id: FileId {
-                        id: 58,
-                    },
-                    offset: 0,
-                    length: 36,
-                },
-                text: "`query` root operation type must be defined here",
-            },
-        ],
-        help: None,
-        data: QueryRootOperationType,
-    },
     ApolloDiagnostic {
         cache: {
             58: "0059_root_operation_object_type.graphql",
diff --git a/crates/apollo-compiler/test_data/diagnostics/0060_root_operation_definition_undefined_operation_type.txt b/crates/apollo-compiler/test_data/diagnostics/0060_root_operation_definition_undefined_operation_type.txt
index c258582..fd936f5 100644
--- a/crates/apollo-compiler/test_data/diagnostics/0060_root_operation_definition_undefined_operation_type.txt
+++ b/crates/apollo-compiler/test_data/diagnostics/0060_root_operation_definition_undefined_operation_type.txt
@@ -1,30 +1,4 @@
 [
-    ApolloDiagnostic {
-        cache: {
-            59: "0060_root_operation_definition_undefined_operation_type.graphql",
-        },
-        location: DiagnosticLocation {
-            file_id: FileId {
-                id: 59,
-            },
-            offset: 0,
-            length: 31,
-        },
-        labels: [
-            Label {
-                location: DiagnosticLocation {
-                    file_id: FileId {
-                        id: 59,
-                    },
-                    offset: 0,
-                    length: 31,
-                },
-                text: "`query` root operation type must be defined here",
-            },
-        ],
-        help: None,
-        data: QueryRootOperationType,
-    },
     ApolloDiagnostic {
         cache: {
             59: "0060_root_operation_definition_undefined_operation_type.graphql",
diff --git a/crates/apollo-compiler/test_data/diagnostics/0061_root_operation_with_default_undefined_query.txt b/crates/apollo-compiler/test_data/diagnostics/0061_root_operation_with_default_undefined_query.txt
index 43ff2ab..e00cf83 100644
--- a/crates/apollo-compiler/test_data/diagnostics/0061_root_operation_with_default_undefined_query.txt
+++ b/crates/apollo-compiler/test_data/diagnostics/0061_root_operation_with_default_undefined_query.txt
@@ -1,30 +1,4 @@
 [
-    ApolloDiagnostic {
-        cache: {
-            60: "0061_root_operation_with_default_undefined_query.graphql",
-        },
-        location: DiagnosticLocation {
-            file_id: FileId {
-                id: 60,
-            },
-            offset: 0,
-            length: 27,
-        },
-        labels: [
-            Label {
-                location: DiagnosticLocation {
-                    file_id: FileId {
-                        id: 60,
-                    },
-                    offset: 0,
-                    length: 27,
-                },
-                text: "`query` root operation type must be defined here",
-            },
-        ],
-        help: None,
-        data: QueryRootOperationType,
-    },
     ApolloDiagnostic {
         cache: {
             60: "0061_root_operation_with_default_undefined_query.graphql",