fix(federation): Prevent @tag and @inaccessible usages on fields marked @external

docs/source/errors.md

@@ -53,6 +53,13 @@ If Apollo Gateway encounters an error, composition fails. This document lists co
 | `REQUIRES_FIELDS_MISSING_ON_BASE` | The `fields` argument of an entity field's `@requires` directive includes a field that is not defined in the entity's originating subgraph.`|
 | `REQUIRES_USED_ON_BASE` | An entity field is marked with `@requires` in the entity's originating subgraph, which is invalid. |
 
+## Non-federation directives
+
+| Code | Description |
+|---|---|
+| `TAG_USED_WITH_EXTERNAL` | Fields marked as `@external` cannot also have `@tag` usages.  |
+| `INACCESSIBLE_USED_WITH_EXTERNAL` | Fields marked as `@external` cannot also have `@inaccessible` usages.  |
+
 ## Custom directives
 
 | Code | Description |

federation-integration-testsuite-js/src/fixtures/reviews.ts

@@ -29,7 +29,7 @@ export const typeDefs = gql`
   }
 
   extend type User @key(fields: "id") {
-    id: ID! @external @tag(name: "reviews") @inaccessible
+    id: ID! @external
     username: String @external
     reviews: [Review]
     numberOfReviews: Int!

federation-js/CHANGELOG.md

@@ -5,6 +5,7 @@
 > The changes noted within this `vNEXT` section have not been released yet.  New PRs and commits which introduce changes should include an entry in this `vNEXT` section as part of their development.  When a release is being prepared, a new header will be (manually) created below and the appropriate changes within that release will be moved into the new section.
 
 - Skip missing types while iterating over field directive usages. It's possible to capture directive usages on fields whose types don't actually exist in the schema (due to invalid composition). See PR for more details. [PR #868](https://github.com/apollographql/federation/pull/868)
+- Prevent the usage of either @tag or @inaccessible on fields that are marked as @external. [PR #869](https://github.com/apollographql/federation/pull/869)
 
 ## v0.26.0
 

federation-js/src/composition/__tests__/composeAndValidate.test.ts

@@ -185,14 +185,14 @@ describe('unknown types', () => {
     assertCompositionFailure(compositionResult);
     expect(compositionResult.errors[0]).toMatchInlineSnapshot(`
       Object {
-        "code": "EXTENSION_WITH_NO_BASE",
+        "code": "TAG_USED_WITH_EXTERNAL",
         "locations": Array [
           Object {
-            "column": 1,
-            "line": 2,
+            "column": 21,
+            "line": 3,
           },
         ],
-        "message": "[inventory] Product -> \`Product\` is an extension type, but \`Product\` is not defined in any service",
+        "message": "[inventory] Product.id -> Found illegal use of @tag directive. @tag directives cannot currently be used in tandem with an @external directive.",
       }
     `);
   });

federation-js/src/composition/compose.ts

@@ -21,7 +21,10 @@ import {
   DirectiveNode,
 } from 'graphql';
 import { transformSchema } from 'apollo-graphql';
-import apolloTypeSystemDirectives, { appliedDirectives, federationDirectives } from '../directives';
+import apolloTypeSystemDirectives, {
+  otherKnownDirectiveDefinitions,
+  federationDirectives,
+} from '../directives';
 import {
   findDirectivesOnNode,
   isStringValueNode,
@@ -130,9 +133,10 @@ type FieldDirectivesMap = Map<string, DirectiveNode[]>;
 type TypeNameToFieldDirectivesMap = Map<string, FieldDirectivesMap>;
 
 /**
- * A set of directive names that have been used at least once
+ * A set of directive names that have been used at least once - specifically
+ * non-federation, Apollo-specific directives
  */
-type AppliedDirectiveUsages = Set<string>;
+type OtherKnownDirectiveUsages = Set<string>;
 
 /**
  * Loop over each service and process its typeDefs (`definitions`)
@@ -148,7 +152,7 @@ export function buildMapsFromServiceList(serviceList: ServiceDefinition[]) {
   const keyDirectivesMap: KeyDirectivesMap = Object.create(null);
   const valueTypes: ValueTypes = new Set();
   const typeNameToFieldDirectivesMap: TypeNameToFieldDirectivesMap = new Map();
-  const appliedDirectiveUsages: AppliedDirectiveUsages = new Set();
+  const otherKnownDirectiveUsages: OtherKnownDirectiveUsages = new Set();
 
   for (const { typeDefs, name: serviceName } of serviceList) {
     // Build a new SDL with @external fields removed, as well as information about
@@ -192,19 +196,15 @@ export function buildMapsFromServiceList(serviceList: ServiceDefinition[]) {
           }
         }
 
-        // Capture `@tag` and `@inaccessible` directive applications
+        // Capture `@tag` and `@inaccessible` directive usages
         for (const field of definition.fields ?? []) {
           const fieldName = field.name.value;
-
           const tagUsages = findDirectivesOnNode(field, 'tag');
-          const inaccessibleUsages = findDirectivesOnNode(
-            field,
-            'inaccessible',
-          );
+          const inaccessibleUsages = findDirectivesOnNode(field, 'inaccessible');
 
-          if (tagUsages.length > 0) appliedDirectiveUsages.add('tag');
+          if (tagUsages.length > 0) otherKnownDirectiveUsages.add('tag');
           if (inaccessibleUsages.length > 0)
-            appliedDirectiveUsages.add('inaccessible');
+            otherKnownDirectiveUsages.add('inaccessible');
 
           if (tagUsages.length > 0 || inaccessibleUsages.length > 0) {
             const fieldToDirectivesMap = mapGetOrSet(
@@ -352,31 +352,6 @@ export function buildMapsFromServiceList(serviceList: ServiceDefinition[]) {
     }
   }
 
-  // We need to capture applied directives from the external fields as well,
-  // which are stripped and excluded from the main loop over the typeDefs
-  for (const { parentTypeName, field } of externalFields) {
-    const tagDirectivesOnField = findDirectivesOnNode(field, 'tag');
-    const inaccessibleDirectivesOnField = findDirectivesOnNode(field, 'inaccessible');
-
-    const appliedDirectivesOnField = [
-      ...tagDirectivesOnField,
-      ...inaccessibleDirectivesOnField,
-    ];
-    if (appliedDirectivesOnField.length > 0) {
-      const fieldToDirectivesMap = mapGetOrSet(
-        typeNameToFieldDirectivesMap,
-        parentTypeName,
-        new Map(),
-      );
-      const directives = mapGetOrSet(
-        fieldToDirectivesMap,
-        field.name.value,
-        [],
-      );
-      directives.push(...appliedDirectivesOnField);
-    }
-  }
-
   // Since all Query/Mutation definitions in service schemas are treated as
   // extensions, we don't have a Query or Mutation DEFINITION in the definitions
   // list. Without a Query/Mutation definition, we can't _extend_ the type.
@@ -397,35 +372,36 @@ export function buildMapsFromServiceList(serviceList: ServiceDefinition[]) {
     keyDirectivesMap,
     valueTypes,
     typeNameToFieldDirectivesMap,
-    appliedDirectiveUsages,
+    otherKnownDirectiveUsages,
   };
 }
 
 export function buildSchemaFromDefinitionsAndExtensions({
   typeDefinitionsMap,
   typeExtensionsMap,
   directiveDefinitionsMap,
-  appliedDirectiveUsages,
+  otherKnownDirectiveUsages,
 }: {
   typeDefinitionsMap: TypeDefinitionsMap;
   typeExtensionsMap: TypeExtensionsMap;
   directiveDefinitionsMap: DirectiveDefinitionsMap;
-  appliedDirectiveUsages: AppliedDirectiveUsages;
+  otherKnownDirectiveUsages: OtherKnownDirectiveUsages;
 }) {
   let errors: GraphQLError[] | undefined = undefined;
 
-  // We only want to include the definitions of applied directives (currently
-  // just @tag and @include) if there are usages.
-  const appliedDirectivesToInclude = appliedDirectives.filter((directive) =>
-    appliedDirectiveUsages.has(directive.name),
-  );
+  // We only want to include the definitions of other known directives (currently
+  // just `@tag` and `@include`) if there are usages.
+  const otherKnownDirectiveDefinitionsToInclude =
+    otherKnownDirectiveDefinitions.filter((directive) =>
+      otherKnownDirectiveUsages.has(directive.name),
+    );
 
   let schema = new GraphQLSchema({
     query: undefined,
     directives: [
       ...specifiedDirectives,
       ...federationDirectives,
-      ...appliedDirectivesToInclude,
+      ...otherKnownDirectiveDefinitionsToInclude,
     ],
   });
 
@@ -686,25 +662,29 @@ export function addFederationMetadataToSchemaNodes({
 
     for (const [
       fieldName,
-      appliedDirectives,
+      otherKnownDirectiveUsages,
     ] of fieldsToDirectivesMap.entries()) {
       const field = type.getFields()[fieldName];
 
       const seenNonRepeatableDirectives: Record<string, boolean> = {};
-      const filteredDirectives = appliedDirectives.filter(directive => {
-        const name = directive.name.value;
-        const matchingDirective = apolloTypeSystemDirectives.find(d => d.name === name);
-        if (matchingDirective?.isRepeatable) return true;
-        if (seenNonRepeatableDirectives[name]) return false;
-        seenNonRepeatableDirectives[name] = true;
-        return true;
-      });
+      const filteredDirectives = otherKnownDirectiveUsages.filter(
+        (directive) => {
+          const name = directive.name.value;
+          const matchingDirective = apolloTypeSystemDirectives.find(
+            (d) => d.name === name,
+          );
+          if (matchingDirective?.isRepeatable) return true;
+          if (seenNonRepeatableDirectives[name]) return false;
+          seenNonRepeatableDirectives[name] = true;
+          return true;
+        },
+      );
 
       // TODO: probably don't need to recreate these objects
       const existingMetadata = getFederationMetadata(field);
       const fieldFederationMetadata: FederationField = {
         ...existingMetadata,
-        appliedDirectives: filteredDirectives,
+        otherKnownDirectiveUsages: filteredDirectives,
       };
 
       field.extensions = {
@@ -726,14 +706,14 @@ export function composeServices(services: ServiceDefinition[]): CompositionResul
     keyDirectivesMap,
     valueTypes,
     typeNameToFieldDirectivesMap,
-    appliedDirectiveUsages,
+    otherKnownDirectiveUsages,
   } = buildMapsFromServiceList(services);
 
   let { schema, errors } = buildSchemaFromDefinitionsAndExtensions({
     typeDefinitionsMap,
     typeExtensionsMap,
     directiveDefinitionsMap,
-    appliedDirectiveUsages,
+    otherKnownDirectiveUsages,
   });
 
   // TODO: We should fix this to take non-default operation root types in

federation-js/src/composition/types.ts

@@ -39,7 +39,7 @@ export interface FederationField {
   requires?: ReadonlyArray<SelectionNode>;
   provides?: ReadonlyArray<SelectionNode>;
   belongsToValueType?: boolean;
-  appliedDirectives?: DirectiveNode[]
+  otherKnownDirectiveUsages?: DirectiveNode[]
 }
 
 export interface FederationDirective {