Review feedback

apollographql · Jan 23, 2023 · d63fc8e · d63fc8e
1 parent 8d8c40d
commit d63fc8e
Show file tree

Hide file tree

Showing 4 changed files with 9 additions and 7 deletions.
diff --git a/.changeset/flat-pandas-cross.md b/.changeset/flat-pandas-cross.md
@@ -2,5 +2,6 @@
 "@apollo/federation-internals": patch
 ---
 
-Fix fed1 schema upgraded when `@external` is on a type.
-
+Fix unexpected composition error about @shareable field when `@external` is on a type in a fed1 schema (one without
+`@link`)
+
diff --git a/gateway-js/CHANGELOG.md b/gateway-js/CHANGELOG.md
@@ -4,7 +4,7 @@ This CHANGELOG pertains only to Apollo Federation packages in the 2.x range. The
 
 ## vNext
 
-- Fix fed1 schema upgraded when @external is on a type [PR #2343](https://github.com/apollographql/federation/pull/2343).
+- 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).

diff --git a/internals-js/CHANGELOG.md b/internals-js/CHANGELOG.md
@@ -4,7 +4,7 @@
 
 ## 2.3.0
 
-- Fix fed1 schema upgraded when @external is on a type [PR #2343](https://github.com/apollographql/federation/pull/2343).
+- 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
 

diff --git a/internals-js/src/federation.ts b/internals-js/src/federation.ts
@@ -1827,9 +1827,10 @@ class ExternalTester {
   }
 
   private collectExternalsOnType() {
-    // We do not collect @external on types for fed1 schema as those are ignored and will be discarded by the schema upgrader
-    // Do note however that the schema upgrader relying on the methods of this tester to figure out if a subgraph resolves
-    // a field in order to know when @shareable should be automatically added, hence the need for the short-cut here.
+    // 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;
     }