Make everything know their own name (#727)

* Make everything know their own name

Fixes #708

In a few places (but not consistently) a `name` field
was omitted from some structs used as map values
on the basis that it would have been redundant with the map key.
This reverts that decision,
making it the user’s responsibility when mutating documents to keep names consistent.

* Add a `pub name: Name` field to `executable::Fragment` as well as
  `ScalarType`, `ObjectType`, `InterfaceType`, `EnumType`, `UnionType`, and `InputObjectType`
  in `schema`.
* Add a `fn name(&self) -> &Name` method to the `schema::ExtendedType` enum
* Add a `pub name: Option<Name>` field to `executable::Operation`
* Remove `executable::OperationRef<'_>`
  (which was equivalent to `(Option<&Name>, &Node<Operation>)`),
  replacing its uses with `&Node<Operation>`

* clippy

crates/apollo-compiler/CHANGELOG.md

@@ -35,6 +35,30 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 [pull/725]: https://github.com/apollographql/apollo-rs/pull/725
 [pull/726]: https://github.com/apollographql/apollo-rs/pull/726
 
+# [x.x.x] (unreleased) - 2023-mm-dd
+
+## BREAKING
+
+- **Make everything know their own name - [SimonSapin], [pull/727] fixing [issue/708].**
+
+  In a few places (but not consistently) a `name` field
+  was omitted from some structs used as map values 
+  on the basis that it would have been redundant with the map key.
+  This reverts that decision,
+  making it the user’s responsibility when mutating documents to keep names consistent.
+
+  * Add a `pub name: Name` field to `executable::Fragment` as well as 
+    `ScalarType`, `ObjectType`, `InterfaceType`, `EnumType`, `UnionType`, and `InputObjectType`
+    in `schema`.
+  * Add a `fn name(&self) -> &Name` method to the `schema::ExtendedType` enum
+  * Add a `pub name: Option<Name>` field to `executable::Operation`
+  * Remove `executable::OperationRef<'_>` 
+    (which was equivalent to `(Option<&Name>, &Node<Operation>)`),
+    replacing its uses with `&Node<Operation>`
+
+[SimonSapin]: https://github.com/SimonSapin
+[issue/708]: https://github.com/apollographql/apollo-rs/issues/708
+[pull/727]: https://github.com/apollographql/apollo-rs/pull/727
 
 # [1.0.0-beta.4](https://crates.io/crates/apollo-compiler/1.0.0-beta.4) - 2023-10-16
 

crates/apollo-compiler/examples/extract_directives_used_by_query.rs

@@ -9,7 +9,7 @@ fn get_directives_used_in_query(doc: &ExecutableDocument) -> Vec<&Node<Directive
     // seed the stack with top-level fields
     let mut stack: Vec<_> = doc
         .all_operations()
-        .flat_map(|op| op.definition().selection_set.fields())
+        .flat_map(|op| op.selection_set.fields())
         .collect();
 
     let mut directives = vec![];

crates/apollo-compiler/examples/rename.rs

@@ -0,0 +1,57 @@
+//! This example shows how to rename a type definition
+
+use apollo_compiler::schema::ExtendedType;
+use apollo_compiler::schema::Name;
+use apollo_compiler::Schema;
+
+#[cfg(not(test))]
+fn main() {
+    print!("{}", renamed())
+}
+
+fn renamed() -> Schema {
+    let input = "type Query { field: Int }";
+    let mut schema = Schema::parse(input, "schema.graphql");
+    schema.validate().unwrap();
+
+    // 1. Remove the definition from the `types` map, using its old name as a key
+    let mut type_def = schema.types.remove("Query").unwrap();
+
+    // 2. Set the new name in the struct
+    let ExtendedType::Object(obj) = &mut type_def else {
+        panic!("expected an object type")
+    };
+    let new_name = Name::from("MyQuery");
+    obj.make_mut().name = new_name.clone();
+
+    // 3. Insert back into the map using the new name as the key
+    // WARNING: it’s your responsibility to make sure to use the same name as in the struct!
+    // Failing to do so make cause code elsewhere to behave incorrectly, or potentially panic.
+    schema.types.insert(new_name.clone(), type_def);
+
+    // 4. Update any existing reference to the old name
+    schema
+        .schema_definition
+        .make_mut()
+        .query
+        .as_mut()
+        .unwrap()
+        .node = new_name;
+
+    schema.validate().unwrap();
+    schema
+}
+
+#[test]
+fn test_renamed() {
+    let expected = expect_test::expect![[r#"
+        schema {
+          query: MyQuery
+        }
+
+        type MyQuery {
+          field: Int
+        }
+    "#]];
+    expected.assert_eq(&renamed().to_string());
+}

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

@@ -111,10 +111,11 @@ impl Operation {
         let mut selection_set = SelectionSet::new(ty);
         selection_set.extend_from_ast(schema, errors, &ast.selection_set);
         Some(Self {
-            selection_set,
             operation_type: ast.operation_type,
+            name: ast.name.clone(),
             variables: ast.variables.clone(),
             directives: ast.directives.clone(),
+            selection_set,
         })
     }
 }
@@ -140,8 +141,9 @@ impl Fragment {
         let mut selection_set = SelectionSet::new(ast.type_condition.clone());
         selection_set.extend_from_ast(schema, errors, &ast.selection_set);
         Some(Self {
-            selection_set,
+            name: ast.name.clone(),
             directives: ast.directives.clone(),
+            selection_set,
         })
     }
 }

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

@@ -61,18 +61,15 @@ pub struct FieldSet {
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub struct Operation {
     pub operation_type: OperationType,
+    pub name: Option<Name>,
     pub variables: Vec<Node<VariableDefinition>>,
     pub directives: Directives,
     pub selection_set: SelectionSet,
 }
 
-pub enum OperationRef<'a> {
-    Anonymous(&'a Node<Operation>),
-    Named(&'a Name, &'a Node<Operation>),
-}
-
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub struct Fragment {
+    pub name: Name,
     pub directives: Directives,
     pub selection_set: SelectionSet,
 }
@@ -237,16 +234,11 @@ impl ExecutableDocument {
     }
 
     /// Returns an iterator of operations, both anonymous and named
-    pub fn all_operations(&self) -> impl Iterator<Item = OperationRef<'_>> {
+    pub fn all_operations(&self) -> impl Iterator<Item = &'_ Node<Operation>> {
         self.anonymous_operation
             .as_ref()
             .into_iter()
-            .map(OperationRef::Anonymous)
-            .chain(
-                self.named_operations
-                    .iter()
-                    .map(|(name, op)| OperationRef::Named(name, op)),
-            )
+            .chain(self.named_operations.values())
     }
 
     /// Return the relevant operation for a request, or a request error
@@ -262,22 +254,19 @@ impl ExecutableDocument {
     pub fn get_operation(
         &self,
         name_request: Option<&str>,
-    ) -> Result<OperationRef<'_>, GetOperationError> {
+    ) -> Result<&Node<Operation>, GetOperationError> {
         if let Some(name) = name_request {
             // Honor the request
-            self.named_operations
-                .get_key_value(name)
-                .map(|(name, op)| OperationRef::Named(name, op))
+            self.named_operations.get(name)
         } else if let Some(op) = &self.anonymous_operation {
             // No name request, return the anonymous operation if it’s the only operation
-            self.named_operations
-                .is_empty()
-                .then_some(OperationRef::Anonymous(op))
+            self.named_operations.is_empty().then_some(op)
         } else {
             // No name request or anonymous operation, return a named operation if it’s the only one
-            self.named_operations.iter().next().and_then(|(name, op)| {
-                (self.named_operations.len() == 1).then_some(OperationRef::Named(name, op))
-            })
+            self.named_operations
+                .values()
+                .next()
+                .and_then(|op| (self.named_operations.len() == 1).then_some(op))
         }
         .ok_or(GetOperationError())
     }
@@ -326,29 +315,6 @@ impl PartialEq for ExecutableDocument {
     }
 }
 
-impl<'a> OperationRef<'a> {
-    pub fn name(&self) -> Option<&'a Name> {
-        match self {
-            OperationRef::Anonymous(_) => None,
-            OperationRef::Named(name, _) => Some(name),
-        }
-    }
-
-    pub fn definition(&self) -> &'a Node<Operation> {
-        match self {
-            OperationRef::Anonymous(def) | OperationRef::Named(_, def) => def,
-        }
-    }
-}
-
-impl std::ops::Deref for OperationRef<'_> {
-    type Target = Node<Operation>;
-
-    fn deref(&self) -> &Self::Target {
-        self.definition()
-    }
-}
-
 impl Operation {
     pub fn object_type(&self) -> &NamedType {
         &self.selection_set.ty

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

@@ -12,23 +12,23 @@ impl ExecutableDocument {
     pub(crate) fn to_ast(&self) -> ast::Document {
         let mut doc = ast::Document::new();
         if let Some(operation) = &self.anonymous_operation {
-            doc.definitions.push(operation.to_ast(None))
+            doc.definitions.push(operation.to_ast())
         }
-        for (name, operation) in &self.named_operations {
-            doc.definitions.push(operation.to_ast(Some(name)))
+        for operation in self.named_operations.values() {
+            doc.definitions.push(operation.to_ast())
         }
-        for (name, fragment) in &self.fragments {
-            doc.definitions.push(fragment.to_ast(name))
+        for fragment in self.fragments.values() {
+            doc.definitions.push(fragment.to_ast())
         }
         doc
     }
 }
 
 impl Node<Operation> {
-    fn to_ast(&self, name: Option<&Name>) -> ast::Definition {
+    fn to_ast(&self) -> ast::Definition {
         ast::Definition::OperationDefinition(self.same_location(ast::OperationDefinition {
             operation_type: self.operation_type,
-            name: name.cloned(),
+            name: self.name.clone(),
             variables: self.variables.clone(),
             directives: self.directives.clone(),
             selection_set: self.selection_set.to_ast(),
@@ -37,9 +37,9 @@ impl Node<Operation> {
 }
 
 impl Node<Fragment> {
-    fn to_ast(&self, name: &Name) -> ast::Definition {
+    fn to_ast(&self) -> ast::Definition {
         ast::Definition::FragmentDefinition(self.same_location(ast::FragmentDefinition {
-            name: name.clone(),
+            name: self.name.clone(),
             type_condition: self.selection_set.ty.clone(),
             directives: self.directives.clone(),
             selection_set: self.selection_set.to_ast(),

crates/apollo-compiler/src/schema/from_ast.rs

@@ -119,17 +119,16 @@ impl SchemaBuilder {
                             );
                             entry.insert(extended_def.into());
                         }
-                        Entry::Occupied(_) => {
-                            let (_index, prev_name, previous) =
-                                self.schema.types.get_full_mut(&$def.name).unwrap();
+                        Entry::Occupied(entry) => {
+                            let previous = entry.get();
                             let error = if $is_scalar && previous.is_built_in() {
                                 BuildError::BuiltInScalarTypeRedefinition {
                                     location: $def.location(),
                                 }
                             } else {
                                 BuildError::TypeDefinitionCollision {
                                     location: $def.name.location(),
-                                    previous_location: prev_name.location(),
+                                    previous_location: previous.name().location(),
                                     name: $def.name.clone(),
                                 }
                             };
@@ -140,7 +139,7 @@ impl SchemaBuilder {
             }
             macro_rules! type_extension {
                 ($ext: ident, $Kind: ident) => {
-                    if let Some((_, ty_name, ty)) = self.schema.types.get_full_mut(&$ext.name) {
+                    if let Some(ty) = self.schema.types.get_mut(&$ext.name) {
                         if let ExtendedType::$Kind(ty) = ty {
                             ty.make_mut()
                                 .extend_ast(&mut self.schema.build_errors, $ext)
@@ -151,7 +150,7 @@ impl SchemaBuilder {
                                     location: $ext.name.location(),
                                     name: $ext.name.clone(),
                                     describe_ext: definition.describe(),
-                                    def_location: ty_name.location(),
+                                    def_location: ty.name().location(),
                                     describe_def: ty.describe(),
                                 })
                         }
@@ -372,35 +371,42 @@ fn adopt_type_extensions(
             }
         };
     }
+    let name = type_name.clone();
     extend! {
         ast::Definition::ScalarTypeExtension => "a scalar type" ScalarType {
             description: Default::default(),
+            name,
             directives: Default::default(),
         }
         ast::Definition::ObjectTypeExtension => "an object type" ObjectType {
             description: Default::default(),
+            name,
             implements_interfaces: Default::default(),
             directives: Default::default(),
             fields: Default::default(),
         }
         ast::Definition::InterfaceTypeExtension => "an interface type" InterfaceType {
             description: Default::default(),
+            name,
             implements_interfaces: Default::default(),
             directives: Default::default(),
             fields: Default::default(),
         }
         ast::Definition::UnionTypeExtension => "a union type" UnionType {
             description: Default::default(),
+            name,
             directives: Default::default(),
             members: Default::default(),
         }
         ast::Definition::EnumTypeExtension => "an enum type" EnumType {
             description: Default::default(),
+            name,
             directives: Default::default(),
             values: Default::default(),
         }
         ast::Definition::InputObjectTypeExtension => "an input object type" InputObjectType {
             description: Default::default(),
+            name,
             directives: Default::default(),
             fields: Default::default(),
         }
@@ -479,6 +485,7 @@ impl ScalarType {
     ) -> Node<Self> {
         let mut ty = Self {
             description: definition.description.clone(),
+            name: definition.name.clone(),
             directives: definition
                 .directives
                 .iter()
@@ -516,6 +523,7 @@ impl ObjectType {
     ) -> Node<Self> {
         let mut ty = Self {
             description: definition.description.clone(),
+            name: definition.name.clone(),
             implements_interfaces: collect_sticky_set(
                 definition
                     .implements_interfaces
@@ -607,6 +615,7 @@ impl InterfaceType {
     ) -> Node<Self> {
         let mut ty = Self {
             description: definition.description.clone(),
+            name: definition.name.clone(),
             implements_interfaces: collect_sticky_set(
                 definition
                     .implements_interfaces
@@ -698,6 +707,7 @@ impl UnionType {
     ) -> Node<Self> {
         let mut ty = Self {
             description: definition.description.clone(),
+            name: definition.name.clone(),
             directives: definition
                 .directives
                 .iter()
@@ -762,6 +772,7 @@ impl EnumType {
     ) -> Node<Self> {
         let mut ty = Self {
             description: definition.description.clone(),
+            name: definition.name.clone(),
             directives: definition
                 .directives
                 .iter()
@@ -828,6 +839,7 @@ impl InputObjectType {
     ) -> Node<Self> {
         let mut ty = Self {
             description: definition.description.clone(),
+            name: definition.name.clone(),
             directives: definition
                 .directives
                 .iter()