Fix fed1 schema upgraded when @external is on a type. (#2343)

For historical reasons, `@external` was legal to put on an object type
since about forever (even in federation 1) but it used to be completely
ignored. We "fixed" this support in #2214, but to limit surprises on
upgrades from fed1, the schema upgrader had been modified to drop any
`@external` on an object type (since again, those were legal but ignored
in fed1). However, the code for adding @Shareable in that same schema
upgrader was not updated correctly and so it _was_ taking `@external`
on types into account, and thus it ended up _not_ adding `@shareable`
in some cases where it should have.

.changeset/flat-pandas-cross.md

@@ -0,0 +1,7 @@
+---
+"@apollo/federation-internals": patch
+---
+
+Fix unexpected composition error about @shareable field when `@external` is on a type in a fed1 schema (one without
+`@link`)
+

gateway-js/CHANGELOG.md

@@ -4,6 +4,8 @@ This CHANGELOG pertains only to Apollo Federation packages in the 2.x range. The
 
 ## vNext
 
+- Fix unexpected composition error about `@shareable` field when `@external` is on a type in a fed1 schema (one without `@link`) [PR #2343](https://github.com/apollographql/federation/pull/2343).
+
 ## 2.3.0-beta.3
 - Rewrites gateway response post-processing to avoid `@interfaceObject` related issues [PR 2335](https://github.com/apollographql/federation/pull/2335).
 - Generates correct response error paths for errors thrown during entity fetches [PR #2304](https://github.com/apollographql/federation/pull/2304).

internals-js/CHANGELOG.md

@@ -2,6 +2,10 @@
 
 ## vNext
 
+## 2.3.0
+
+- Fix incorrect handling of `@external` on a type when dealing when adding `@shareable` during fed1 schema upgrades [PR #2343](https://github.com/apollographql/federation/pull/2343).
+
 ## 2.2.1
 
 - Fix federation spec always being expanded to the last version [PR #2274](https://github.com/apollographql/federation/pull/2274).

internals-js/src/__tests__/schemaUpgrader.test.ts

@@ -245,3 +245,43 @@ test('reject @interfaceObject usage if not all subgraphs are fed2', () => {
     + '@interfaceObject is used in subgraph "s1" but subgraph "s2" is not a federation 2 subgraph schema.'
   ]);
 })
+
+test('handles the addition of @shareable when an @external is used on a type', () => {
+  const s1 = `
+    type Query {
+      t1: T
+    }
+
+    type T @key(fields: "id") {
+      id: String
+      x: Int
+    }
+  `;
+
+  const s2 = `
+    type Query {
+      t2: T
+    }
+
+    type T @external {
+      x: Int
+    }
+  `;
+
+  const subgraphs = new Subgraphs();
+  subgraphs.add(buildSubgraph('s1', 'http://s1', s1));
+  subgraphs.add(buildSubgraph('s2', 'http://s2', s2));
+  const res = upgradeSubgraphsIfNecessary(subgraphs);
+  expect(res.errors).toBeUndefined();
+
+  // 2 things must happen here:
+  // 1. the @external on type `T` in s2 should be removed, as @external on types were no-ops in fed1 (but not in fed2 anymore, hence the removal)
+  // 2. field `T.x` in s1 must be marked @shareable since it is resolved by s2 (since again, it's @external annotation is ignored).
+
+  const s2Upgraded = res.subgraphs?.get('s2')!;
+  expect(s2Upgraded.schema.type('T')?.hasAppliedDirective('external')).toBe(false);
+
+  const s1Upgraded = res.subgraphs?.get('s1')!;
+  expect((s1Upgraded.schema.type('T') as ObjectType).field('x')?.hasAppliedDirective('shareable')).toBe(true);
+
+})

internals-js/src/federation.ts

@@ -577,7 +577,7 @@ export class FederationMetadata {
 
   private externalTester(): ExternalTester {
     if (!this._externalTester) {
-      this._externalTester = new ExternalTester(this.schema);
+      this._externalTester = new ExternalTester(this.schema, this.isFed2Schema());
     }
     return this._externalTester;
   }
@@ -1783,7 +1783,7 @@ class ExternalTester {
   private readonly externalDirective: DirectiveDefinition<{}>;
   private readonly externalFieldsOnType = new Set<string>();
 
-  constructor(readonly schema: Schema) {
+  constructor(readonly schema: Schema, private readonly isFed2Schema: boolean) {
     this.externalDirective = this.metadata().externalDirective();
     this.collectFakeExternals();
     this.collectProvidedFields();
@@ -1827,6 +1827,14 @@ class ExternalTester {
   }
 
   private collectExternalsOnType() {
+    // We do not collect @external on types for fed1 schema since those will be discarded by the schema upgrader.
+    // The schema upgrader, through calls to `isExternal`, relies on the populated `externalFieldsOnType` object to
+    // inform when @shareable should be automatically added. In the fed1 case, if the map is populated then @shareable won't
+    // be added in places where it should have.
+    if (!this.isFed2Schema) {
+      return;
+    }
+
     for (const type of this.schema.objectTypes()) {
       if (type.hasAppliedDirective(this.externalDirective)) {
         for (const field of type.fields()) {