fix(schema): building contracts with extend keyword (#6706)

Author: n1ru4l

.changeset/wicked-flowers-bake.md

@@ -0,0 +1,5 @@
+---
+'hive': patch
+---
+
+Improve contract schema building for subgraphs using the `extend` keyword.

integration-tests/tests/schema/contracts.spec.ts

@@ -392,3 +392,90 @@ test('type is marked as inaccessible if all fields are inaccessible and the type
     'type Brr @join__type(graph: FOO_GRAPHQL) @inaccessible {',
   );
 });
+
+test('inaccessible is only applied once per type', async () => {
+  const result = await client.composeAndValidate.mutate({
+    type: 'federation',
+    native: true,
+    schemas: [
+      {
+        raw: /* GraphQL */ `
+          extend schema
+            @link(url: "https://specs.apollo.dev/link/v1.0")
+            @link(url: "https://specs.apollo.dev/federation/v2.8", import: ["@tag"])
+
+          type Query {
+            hello: String @tag(name: "public")
+          }
+
+          type Brr {
+            a: String
+          }
+
+          extend type Brr {
+            b: String
+          }
+        `,
+        source: 'foo.graphql',
+        url: null,
+      },
+    ],
+    external: null,
+    contracts: [
+      {
+        id: 'foo',
+        filter: {
+          removeUnreachableTypesFromPublicApiSchema: true,
+          exclude: null,
+          include: ['public'],
+        },
+      },
+    ],
+  });
+
+  expect(result.contracts?.[0].errors).toEqual([]);
+});
+
+test('inaccessible is not applied on type if at least one type extension has a public field', async () => {
+  const result = await client.composeAndValidate.mutate({
+    type: 'federation',
+    native: true,
+    schemas: [
+      {
+        raw: /* GraphQL */ `
+          extend schema
+            @link(url: "https://specs.apollo.dev/link/v1.0")
+            @link(url: "https://specs.apollo.dev/federation/v2.8", import: ["@tag"])
+
+          type Query {
+            hello: String @tag(name: "public")
+          }
+
+          type Brr {
+            a: String
+          }
+
+          extend type Brr {
+            b: String @tag(name: "public")
+          }
+        `,
+        source: 'foo.graphql',
+        url: null,
+      },
+    ],
+    external: null,
+    contracts: [
+      {
+        id: 'foo',
+        filter: {
+          removeUnreachableTypesFromPublicApiSchema: false,
+          exclude: null,
+          include: ['public'],
+        },
+      },
+    ],
+  });
+
+  expect(result.contracts?.[0].errors).toEqual([]);
+  expect(result.contracts?.[0].sdl).toContain('type Brr {');
+});

packages/services/schema/src/lib/federation-tag-extraction.spec.ts

@@ -1818,7 +1818,7 @@ describe('applyTagFilterOnSubgraphs', () => {
         field1: String! @inaccessible
       }
 
-      extend type Type1 @inaccessible {
+      extend type Type1 {
         field2: String! @inaccessible
       }
     `);

packages/services/schema/src/lib/federation-tag-extraction.ts

@@ -93,6 +93,14 @@ function getRootTypeNamesFromDocumentNode(document: DocumentNode) {
   return names;
 }
 
+type ObjectLikeNode =
+  | ObjectTypeExtensionNode
+  | ObjectTypeDefinitionNode
+  | InterfaceTypeDefinitionNode
+  | InterfaceTypeExtensionNode
+  | InputObjectTypeDefinitionNode
+  | InputObjectTypeExtensionNode;
+
 /**
  * Takes a subgraph document node and a set of tag filters and transforms the document node to contain `@inaccessible` directives on all fields not included by the applied filter.
  * Note: you probably want to use `filterSubgraphs` instead, as it also applies the correct post step required after applying this.
@@ -102,8 +110,11 @@ export function applyTagFilterToInaccessibleTransformOnSubgraphSchema(
   filter: Federation2SubgraphDocumentNodeByTagsFilter,
 ): {
   typeDefs: DocumentNode;
+  /** Types within THIS subgraph where all fields are inaccessible */
   typesWithAllFieldsInaccessible: Map<string, boolean>;
   transformTagDirectives: ReturnType<typeof createTransformTagDirectives>;
+  /** Types in this subgraph that have the inaccessible directive applied */
+  typesWithInaccessibleApplied: Set<string>;
 } {
   const { resolveImportName } = extractLinkImplementations(documentNode);
   const inaccessibleDirectiveName = resolveImportName(
@@ -149,71 +160,119 @@ export function applyTagFilterToInaccessibleTransformOnSubgraphSchema(
     };
   }
 
-  function fieldLikeObjectHandler(
-    node:
-      | ObjectTypeExtensionNode
-      | ObjectTypeDefinitionNode
-      | InterfaceTypeDefinitionNode
-      | InterfaceTypeExtensionNode
-      | InputObjectTypeDefinitionNode
-      | InputObjectTypeExtensionNode,
-  ) {
-    const tagsOnNode = getTagsOnNode(node);
-
-    let isAllFieldsInaccessible = true;
-
-    const newNode = {
-      ...node,
-      fields: node.fields?.map(node => {
-        const tagsOnNode = getTagsOnNode(node);
-
-        if (node.kind === Kind.FIELD_DEFINITION) {
-          node = {
-            ...node,
-            arguments: node.arguments?.map(fieldArgumentHandler),
-          } as FieldDefinitionNode;
+  const definitionsBySchemaCoordinate = new Map<string, Array<ObjectLikeNode>>();
+
+  //
+  // A type can be defined multiple times within a subgraph and we need to find all implementations
+  // for determining whether the full type, or only some fields are part of the public contract schema
+  //
+  for (const definition of documentNode.definitions) {
+    switch (definition.kind) {
+      case Kind.OBJECT_TYPE_DEFINITION:
+      case Kind.OBJECT_TYPE_EXTENSION:
+      case Kind.INTERFACE_TYPE_DEFINITION:
+      case Kind.INTERFACE_TYPE_EXTENSION:
+      case Kind.INPUT_OBJECT_TYPE_DEFINITION:
+      case Kind.INPUT_OBJECT_TYPE_EXTENSION: {
+        let items = definitionsBySchemaCoordinate.get(definition.name.value);
+        if (!items) {
+          items = [];
+          definitionsBySchemaCoordinate.set(definition.name.value, items);
         }
+        items.push(definition);
+      }
+    }
+  }
+
+  // Tracking for which type already has `@inaccessible` applied (can only occur once)
+  const typesWithInaccessibleApplied = new Set<string>();
+  // These are later used within the visitor to actually replace the nodes.
+  const replacementTypeNodes = new Map<ObjectLikeNode, ObjectLikeNode>();
+
+  for (const [typeName, nodes] of definitionsBySchemaCoordinate) {
+    /** After processing all nodes implementing a type, we know whether all or only some fields are inaccessible */
+    let isSomeFieldsAccessible = false;
+    /** First node occurance record as stored within the `replacementTypeNodes` map.  */
+    let firstReplacementTypeNodeRecord: {
+      key: ObjectLikeNode;
+      value: ObjectLikeNode;
+    } | null = null;
+
+    for (const node of nodes) {
+      const tagsOnNode = getTagsOnNode(node);
+      let newNode = {
+        ...node,
+        fields: node.fields?.map(node => {
+          const tagsOnNode = getTagsOnNode(node);
+
+          if (node.kind === Kind.FIELD_DEFINITION) {
+            node = {
+              ...node,
+              arguments: node.arguments?.map(fieldArgumentHandler),
+            } as FieldDefinitionNode;
+          }
+
+          if (
+            (filter.include.size && !hasIntersection(tagsOnNode, filter.include)) ||
+            (filter.exclude.size && hasIntersection(tagsOnNode, filter.exclude))
+          ) {
+            return {
+              ...node,
+              directives: transformTagDirectives(node, true),
+            };
+          }
+
+          isSomeFieldsAccessible = true;
 
-        if (
-          (filter.include.size && !hasIntersection(tagsOnNode, filter.include)) ||
-          (filter.exclude.size && hasIntersection(tagsOnNode, filter.exclude))
-        ) {
           return {
             ...node,
-            directives: transformTagDirectives(node, true),
+            directives: transformTagDirectives(node),
           };
-        }
-
-        isAllFieldsInaccessible = false;
-
-        return {
-          ...node,
+        }),
+      } as ObjectLikeNode;
+
+      if (filter.exclude.size && hasIntersection(tagsOnNode, filter.exclude)) {
+        newNode = {
+          ...newNode,
+          directives: transformTagDirectives(
+            node,
+            typesWithInaccessibleApplied.has(typeName) ? false : true,
+          ),
+        } as ObjectLikeNode;
+        typesWithInaccessibleApplied.add(typeName);
+      } else {
+        newNode = {
+          ...newNode,
           directives: transformTagDirectives(node),
         };
-      }),
-    };
+      }
 
-    if (
-      !rootTypeNames.has(node.name.value) &&
-      filter.exclude.size &&
-      hasIntersection(tagsOnNode, filter.exclude)
-    ) {
-      return {
-        ...newNode,
-        directives: transformTagDirectives(node, true),
-      };
+      if (!firstReplacementTypeNodeRecord) {
+        firstReplacementTypeNodeRecord = {
+          key: node,
+          value: newNode,
+        };
+      }
+      replacementTypeNodes.set(node, newNode);
     }
 
-    if (isAllFieldsInaccessible) {
-      onAllFieldsInaccessible(node.name.value);
-    } else {
-      onSomeFieldsAccessible(node.name.value);
+    // If some fields are accessible, we continue with the next type
+    if (isSomeFieldsAccessible) {
+      onSomeFieldsAccessible(typeName);
+      continue;
     }
+    onAllFieldsInaccessible(typeName);
+  }
 
-    return {
-      ...newNode,
-      directives: transformTagDirectives(node),
-    };
+  function fieldLikeObjectHandler(node: ObjectLikeNode) {
+    const newNode = replacementTypeNodes.get(node);
+    if (!newNode) {
+      throw new Error(
+        `Found type without transformation mapping. ${node.name.value} ${node.name.kind}`,
+      );
+    }
+
+    return newNode;
   }
 
   function enumHandler(node: EnumTypeDefinitionNode | EnumTypeExtensionNode) {
@@ -304,6 +363,7 @@ export function applyTagFilterToInaccessibleTransformOnSubgraphSchema(
     typeDefs,
     typesWithAllFieldsInaccessible: typesWithAllFieldsInaccessibleTracker,
     transformTagDirectives,
+    typesWithInaccessibleApplied,
   };
 }
 
@@ -312,6 +372,8 @@ function makeTypesFromSetInaccessible(
   types: Set<string>,
   transformTagDirectives: ReturnType<typeof createTransformTagDirectives>,
 ) {
+  /** We can only apply @accessible once on each unique typename, otherwise we get a composition error */
+  const alreadyAppliedOnTypeNames = new Set<string>();
   function typeHandler(
     node:
       | ObjectTypeExtensionNode
@@ -323,9 +385,10 @@ function makeTypesFromSetInaccessible(
       | EnumTypeDefinitionNode
       | EnumTypeExtensionNode,
   ) {
-    if (types.has(node.name.value) === false) {
+    if (types.has(node.name.value) === false || alreadyAppliedOnTypeNames.has(node.name.value)) {
       return;
     }
+    alreadyAppliedOnTypeNames.add(node.name.value);
     return {
       ...node,
       directives: transformTagDirectives(node, true),
@@ -392,12 +455,26 @@ export function applyTagFilterOnSubgraphs<
     ...subgraph,
     typeDefs: makeTypesFromSetInaccessible(
       subgraph.typeDefs,
-      intersectionOfTypesWhereAllFieldsAreInaccessible,
+      /** We exclude the types that are already marked as inaccessible within the subgraph as we want to avoid `@inaccessible` applied more than once. */
+      difference(
+        intersectionOfTypesWhereAllFieldsAreInaccessible,
+        subgraph.typesWithInaccessibleApplied,
+      ),
       subgraph.transformTagDirectives,
     ),
   }));
 }
 
+function difference<$Type>(set1: Set<$Type>, set2: Set<$Type>): Set<$Type> {
+  const result = new Set<$Type>();
+  set1.forEach(item => {
+    if (!set2.has(item)) {
+      result.add(item);
+    }
+  });
+  return result;
+}
+
 export const extractTagsFromDocument = (
   documentNode: DocumentNode,
   tagStrategy: TagExtractionStrategy,