From 282c56331122a177f21231cea25a20736b5719b3 Mon Sep 17 00:00:00 2001 From: Sylvain Lebresne Date: Mon, 23 Jan 2023 10:36:14 +0100 Subject: [PATCH] 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 | 7 ++++ gateway-js/CHANGELOG.md | 2 + internals-js/CHANGELOG.md | 4 ++ .../src/__tests__/schemaUpgrader.test.ts | 40 +++++++++++++++++++ internals-js/src/federation.ts | 12 +++++- 5 files changed, 63 insertions(+), 2 deletions(-) create mode 100644 .changeset/flat-pandas-cross.md diff --git a/.changeset/flat-pandas-cross.md b/.changeset/flat-pandas-cross.md new file mode 100644 index 000000000..155615d37 --- /dev/null +++ b/.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`) + diff --git a/gateway-js/CHANGELOG.md b/gateway-js/CHANGELOG.md index 7669ebabf..259ce6e1d 100644 --- a/gateway-js/CHANGELOG.md +++ b/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). diff --git a/internals-js/CHANGELOG.md b/internals-js/CHANGELOG.md index 52db990bd..db6729bf5 100644 --- a/internals-js/CHANGELOG.md +++ b/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). diff --git a/internals-js/src/__tests__/schemaUpgrader.test.ts b/internals-js/src/__tests__/schemaUpgrader.test.ts index 0c4e82ee8..826ca4121 100644 --- a/internals-js/src/__tests__/schemaUpgrader.test.ts +++ b/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); + +}) diff --git a/internals-js/src/federation.ts b/internals-js/src/federation.ts index d3d27bfd6..08aabb9ae 100644 --- a/internals-js/src/federation.ts +++ b/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(); - 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()) {