Repository: apollographql/apollo-rs
Pull request #684 (2023-10-09 11:20:33)

Allow built-in directives to be redefined
Fixes https://github.com/apollographql/apollo-rs/issues/656

SimonSapin: The relevant code is completely different so that will have to be a manual backport

Patch:
diff --git a/crates/apollo-compiler/CHANGELOG.md b/crates/apollo-compiler/CHANGELOG.md
index 418bfd3..80b3825 100644
--- a/crates/apollo-compiler/CHANGELOG.md
+++ b/crates/apollo-compiler/CHANGELOG.md
@@ -17,28 +17,14 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 ## Maintenance
 ## Documentation-->
 
-<!--
 # [x.x.x] (unreleased) - 2023-mm-dd
 
-## Features
-- Add `validate_standalone_executable` function to validate an executable document without access to a schema, by [goto-bus-stop] in [pull/631], [issue/629]
+## Fixes
 
-  This runs just those validations that can be done on operations without knowing the types of things.
-  ```rust
-  let compiler = ApolloCompiler::new();
-  let file_id = compiler.add_executable(r#"
-  {
-    user { ...userData }
-  }
-  "#, "query.graphql");
-  let diagnostics = compiler.db.validate_standalone_executable(file_id);
-  // Complains about `userData` fragment not existing, but does not complain about `user` being an unknown query.
-  ```
+- Allow built-in directives to be redefined, by [SimonSapin] in [pull/684], [issue/656]
 
-[goto-bus-stop]: https://github.com/goto-bus-stop
-[pull/631]: https://github.com/apollographql/apollo-rs/pull/631
-[issue/629]: https://github.com/apollographql/apollo-rs/issues/629
--->
+[pull/684]: https://github.com/apollographql/apollo-rs/pull/684
+[issue/656]: https://github.com/apollographql/apollo-rs/issues/656
 
 # [1.0.0-beta.1](https://crates.io/crates/apollo-compiler/1.0.0-beta.1) - 2023-10-05
 
@@ -71,6 +57,25 @@ that provides structural sharing and copy-on-write semantics.
 
 # [0.11.2](https://crates.io/crates/apollo-compiler/0.11.2) - 2023-09-11
 
+## Features
+- Add `validate_standalone_executable` function to validate an executable document without access to a schema, by [goto-bus-stop] in [pull/631], [issue/629]
+
+  This runs just those validations that can be done on operations without knowing the types of things.
+  ```rust
+  let compiler = ApolloCompiler::new();
+  let file_id = compiler.add_executable(r#"
+  {
+    user { ...userData }
+  }
+  "#, "query.graphql");
+  let diagnostics = compiler.db.validate_standalone_executable(file_id);
+  // Complains about `userData` fragment not existing, but does not complain about `user` being an unknown query.
+  ```
+
+[goto-bus-stop]: https://github.com/goto-bus-stop
+[pull/631]: https://github.com/apollographql/apollo-rs/pull/631
+[issue/629]: https://github.com/apollographql/apollo-rs/issues/629
+
 ## Fixes
 - validate input value types, by [goto-bus-stop] in [pull/642]
 
diff --git a/crates/apollo-compiler/src/schema/from_ast.rs b/crates/apollo-compiler/src/schema/from_ast.rs
index 1e0ea08..49df2b4 100644
--- a/crates/apollo-compiler/src/schema/from_ast.rs
+++ b/crates/apollo-compiler/src/schema/from_ast.rs
@@ -95,13 +95,21 @@ impl SchemaBuilder {
                             def.clone()
                         })
                     {
-                        self.schema
-                            .build_errors
-                            .push(BuildError::DirectiveDefinitionCollision {
-                                location: def.name.location(),
-                                previous_location: previous.name.location(),
-                                name: def.name.clone(),
-                            })
+                        if previous.is_built_in() {
+                            // https://github.com/apollographql/apollo-rs/issues/656
+                            // Re-defining a built-in definition is allowed, but only once.
+                            // (`is_built_in` is based on file ID, not directive name,
+                            // so the new definition won’t be considered built-in.)
+                            *previous = def.clone()
+                        } else {
+                            self.schema.build_errors.push(
+                                BuildError::DirectiveDefinitionCollision {
+                                    location: def.name.location(),
+                                    previous_location: previous.name.location(),
+                                    name: def.name.clone(),
+                                },
+                            )
+                        }
                     }
                 }
                 ast::Definition::ScalarTypeDefinition(def) => {
@@ -899,13 +907,16 @@ fn insert_sticky<'map, V>(
     map: &'map mut IndexMap<Name, V>,
     key: &Name,
     make_value: impl FnOnce() -> V,
-) -> Result<(), (&'map Name, &'map V)> {
+) -> Result<(), (&'map Name, &'map mut V)> {
     match map.entry(key.clone()) {
         Entry::Vacant(entry) => {
             entry.insert(make_value());
             Ok(())
         }
-        Entry::Occupied(_) => Err(map.get_key_value(key).unwrap()),
+        Entry::Occupied(_) => {
+            let (_index, key, value) = map.get_full_mut(key).unwrap();
+            Err((key, value))
+        }
     }
 }
 


Test patch:
diff --git a/crates/apollo-compiler/test_data/diagnostics/0107_built_in_directive_double_redefinition.graphql b/crates/apollo-compiler/test_data/diagnostics/0107_built_in_directive_double_redefinition.graphql
new file mode 100644
index 0000000..760f8a6
--- /dev/null
+++ b/crates/apollo-compiler/test_data/diagnostics/0107_built_in_directive_double_redefinition.graphql
@@ -0,0 +1,24 @@
+schema
+  # No error here (the *first* redefinition is used, allowing SCHEMA location)
+  @deprecated(
+    reason: """
+    The person who got promoted by shipping this product has moved on to another team.
+    The service will shut down next month.
+    """
+  ) {
+  query: Query
+}
+
+type Query {
+  importantData: Int
+}
+
+# No error at first redefinition
+directive @deprecated(
+  reason: String = "No longer supported"
+) on FIELD_DEFINITION | ARGUMENT_DEFINITION | INPUT_FIELD_DEFINITION | ENUM_VALUE | SCHEMA
+
+# Error (name collision) at the second redefinition
+directive @deprecated(
+  reason: String = "No longer supported"
+) on FIELD_DEFINITION | ARGUMENT_DEFINITION | INPUT_FIELD_DEFINITION | ENUM_VALUE | OBJECT | INTERFACE | UNION | ENUM | INPUT_OBJECT
diff --git a/crates/apollo-compiler/test_data/diagnostics/0107_built_in_directive_double_redefinition.txt b/crates/apollo-compiler/test_data/diagnostics/0107_built_in_directive_double_redefinition.txt
new file mode 100644
index 0000000..c47e5cd
--- /dev/null
+++ b/crates/apollo-compiler/test_data/diagnostics/0107_built_in_directive_double_redefinition.txt
@@ -0,0 +1,14 @@
+Error: the directive `@deprecated` is defined multiple times in the schema
+    ╭─[0107_built_in_directive_double_redefinition.graphql:22:12]
+    │
+ 17 │ directive @deprecated(
+    │            ─────┬────  
+    │                 ╰────── previous definition of `@deprecated` here
+    │ 
+ 22 │ directive @deprecated(
+    │            ─────┬────  
+    │                 ╰────── `@deprecated` redefined here
+    │ 
+    │ Help: remove or rename one of the definitions
+────╯
+
diff --git a/crates/apollo-compiler/test_data/ok/0034_built_in_directive_redefinition.graphql b/crates/apollo-compiler/test_data/ok/0034_built_in_directive_redefinition.graphql
new file mode 100644
index 0000000..f07f191
--- /dev/null
+++ b/crates/apollo-compiler/test_data/ok/0034_built_in_directive_redefinition.graphql
@@ -0,0 +1,17 @@
+schema
+  @deprecated(
+    reason: """
+    The person who got promoted by shipping this product has moved on to another team.
+    The service will shut down next month.
+    """
+  ) {
+  query: Query
+}
+
+type Query {
+  importantData: Int
+}
+
+directive @deprecated(
+  reason: String = "No longer supported"
+) on FIELD_DEFINITION | ARGUMENT_DEFINITION | INPUT_FIELD_DEFINITION | ENUM_VALUE | SCHEMA
diff --git a/crates/apollo-compiler/test_data/ok/0034_built_in_directive_redefinition.txt b/crates/apollo-compiler/test_data/ok/0034_built_in_directive_redefinition.txt
new file mode 100644
index 0000000..490c6d4
--- /dev/null
+++ b/crates/apollo-compiler/test_data/ok/0034_built_in_directive_redefinition.txt
@@ -0,0 +1,67 @@
+0..198 @33 SchemaDefinition {
+    description: None,
+    directives: [
+        9..179 @33 Directive {
+            name: "deprecated",
+            arguments: [
+                26..175 @33 Argument {
+                    name: "reason",
+                    value: 34..175 @33 String(
+                        "The person who got promoted by shipping this product has moved on to another team.\nThe service will shut down next month.",
+                    ),
+                },
+            ],
+        },
+    ],
+    root_operations: [
+        184..196 @33 (
+            Query,
+            "Query",
+        ),
+    ],
+}
+200..235 @33 ObjectTypeDefinition {
+    description: None,
+    name: "Query",
+    implements_interfaces: [],
+    directives: [],
+    fields: [
+        215..233 @33 FieldDefinition {
+            description: None,
+            name: "importantData",
+            arguments: [],
+            ty: Named(
+                "Int",
+            ),
+            directives: [],
+        },
+    ],
+}
+237..391 @33 DirectiveDefinition {
+    description: None,
+    name: "deprecated",
+    arguments: [
+        262..300 @33 InputValueDefinition {
+            description: None,
+            name: "reason",
+            ty: 270..276 @33 Named(
+                "String",
+            ),
+            default_value: Some(
+                279..300 @33 String(
+                    "No longer supported",
+                ),
+            ),
+            directives: [],
+        },
+    ],
+    repeatable: false,
+    locations: [
+        "FIELD_DEFINITION",
+        "ARGUMENT_DEFINITION",
+        "INPUT_FIELD_DEFINITION",
+        "ENUM_VALUE",
+        "SCHEMA",
+    ],
+}
+
diff --git a/crates/apollo-compiler/test_data/serializer/diagnostics/0107_built_in_directive_double_redefinition.graphql b/crates/apollo-compiler/test_data/serializer/diagnostics/0107_built_in_directive_double_redefinition.graphql
new file mode 100644
index 0000000..b68798c
--- /dev/null
+++ b/crates/apollo-compiler/test_data/serializer/diagnostics/0107_built_in_directive_double_redefinition.graphql
@@ -0,0 +1,11 @@
+schema @deprecated(reason: "The person who got promoted by shipping this product has moved on to another team.\nThe service will shut down next month.") {
+  query: Query
+}
+
+type Query {
+  importantData: Int
+}
+
+directive @deprecated(reason: String = "No longer supported") on FIELD_DEFINITION | ARGUMENT_DEFINITION | INPUT_FIELD_DEFINITION | ENUM_VALUE | SCHEMA
+
+directive @deprecated(reason: String = "No longer supported") on FIELD_DEFINITION | ARGUMENT_DEFINITION | INPUT_FIELD_DEFINITION | ENUM_VALUE | OBJECT | INTERFACE | UNION | ENUM | INPUT_OBJECT
diff --git a/crates/apollo-compiler/test_data/serializer/ok/0034_built_in_directive_redefinition.graphql b/crates/apollo-compiler/test_data/serializer/ok/0034_built_in_directive_redefinition.graphql
new file mode 100644
index 0000000..3d8f307
--- /dev/null
+++ b/crates/apollo-compiler/test_data/serializer/ok/0034_built_in_directive_redefinition.graphql
@@ -0,0 +1,9 @@
+schema @deprecated(reason: "The person who got promoted by shipping this product has moved on to another team.\nThe service will shut down next month.") {
+  query: Query
+}
+
+type Query {
+  importantData: Int
+}
+
+directive @deprecated(reason: String = "No longer supported") on FIELD_DEFINITION | ARGUMENT_DEFINITION | INPUT_FIELD_DEFINITION | ENUM_VALUE | SCHEMA