From 9050f03a43871acc56d29212508aed0e40f2fa8a Mon Sep 17 00:00:00 2001 From: Chris Lenfest Date: Fri, 1 Apr 2022 10:41:54 -0500 Subject: [PATCH] Creation of @override directive along with relevant tests (#1484) * Adding @override for fields * only put external on overridden field if it is used (i.e. referenced by @provides, @requires) or referenced by an @key * if overridden directive does not have external, we want to filter it from consideration rather than allowing it to be used in the join * adding FieldMergeContext rather than filtering or adding external to subgraphs directly * adding lots of relevant tests Co-authored-by: Sylvain Lebresne --- composition-js/CHANGELOG.md | 1 + composition-js/src/__tests__/compose.test.ts | 10 +- .../__tests__/composeFed1Subgraphs.test.ts | 2 +- composition-js/src/__tests__/hints.test.ts | 127 ++- .../src/__tests__/override.compose.test.ts | 760 ++++++++++++++++++ composition-js/src/hints.ts | 18 + composition-js/src/merging/merge.ts | 265 +++++- docs/source/errors.md | 3 + .../src/fixtures/accounts.ts | 1 + .../src/fixtures/books.ts | 1 + .../src/__tests__/buildQueryPlan.test.ts | 55 +- .../__tests__/gateway/lifecycle-hooks.test.ts | 2 +- internals-js/CHANGELOG.md | 1 + .../src/__tests__/subgraphValidation.test.ts | 8 +- internals-js/src/definitions.ts | 2 +- internals-js/src/error.ts | 18 + .../src/extractSubgraphsFromSupergraph.ts | 6 + internals-js/src/federation.ts | 87 +- internals-js/src/federationSpec.ts | 14 + internals-js/src/index.ts | 1 + internals-js/src/joinSpec.ts | 12 +- internals-js/src/operations.ts | 15 + .../src/{sharing.ts => precompute.ts} | 3 +- internals-js/src/schemaUpgrader.ts | 11 +- internals-js/src/suggestions.ts | 2 +- query-graphs-js/src/graphPath.ts | 67 +- query-graphs-js/src/querygraph.ts | 2 +- query-planner-js/CHANGELOG.md | 1 + subgraph-js/CHANGELOG.md | 1 + .../src/__tests__/printSubgraphSchema.test.ts | 5 +- subgraph-js/src/directives.ts | 11 + 31 files changed, 1416 insertions(+), 96 deletions(-) create mode 100644 composition-js/src/__tests__/override.compose.test.ts rename internals-js/src/{sharing.ts => precompute.ts} (99%) diff --git a/composition-js/CHANGELOG.md b/composition-js/CHANGELOG.md index bf69d80d25..84af2b74d1 100644 --- a/composition-js/CHANGELOG.md +++ b/composition-js/CHANGELOG.md @@ -10,6 +10,7 @@ This CHANGELOG pertains only to Apollo Federation packages in the 2.x range. The - Optimize composition validation when many entities spans many subgraphs [PR #1653](https://github.com/apollographql/federation/pull/1653). - Support for Node 17 [PR #1541](https://github.com/apollographql/federation/pull/1541). - Adds Support for `@tag/v0.2`, which allows the `@tag` directive to be additionally placed on arguments, scalars, enums, enum values, input objects, and input object fields. [PR #1652](https://github.com/apollographql/federation/pull/1652). +- Adds support for the `@override` directive on fields to indicate that a field should be moved from one subgraph to another. [PR #1484](https://github.com/apollographql/federation/pull/1484) ## v2.0.0-preview.8 diff --git a/composition-js/src/__tests__/compose.test.ts b/composition-js/src/__tests__/compose.test.ts index 65bfe70519..72cab9f908 100644 --- a/composition-js/src/__tests__/compose.test.ts +++ b/composition-js/src/__tests__/compose.test.ts @@ -4,18 +4,18 @@ import gql from 'graphql-tag'; import './matchers'; import { print } from 'graphql'; -function assertCompositionSuccess(r: CompositionResult): asserts r is CompositionSuccess { +export function assertCompositionSuccess(r: CompositionResult): asserts r is CompositionSuccess { if (r.errors) { throw new Error(`Expected composition to succeed but got errors:\n${r.errors.join('\n\n')}`); } } -function errors(r: CompositionResult): [string, string][] { +export function errors(r: CompositionResult): [string, string][] { return r.errors?.map(e => [e.extensions.code as string, e.message]) ?? []; } // Returns [the supergraph schema, its api schema, the extracted subgraphs] -function schemas(result: CompositionSuccess): [Schema, Schema, Subgraphs] { +export function schemas(result: CompositionSuccess): [Schema, Schema, Subgraphs] { // Note that we could user `result.schema`, but reparsing to ensure we don't lose anything with printing/parsing. const schema = buildSchema(result.supergraphSdl); expect(schema.isCoreSchema()).toBeTruthy(); @@ -74,7 +74,7 @@ describe('composition', () => { query: Query } - directive @join__field(graph: join__Graph!, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION + directive @join__field(graph: join__Graph!, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION directive @join__graph(name: String!, url: String!) on ENUM_VALUE @@ -2025,7 +2025,7 @@ describe('composition', () => { query: Query } - directive @join__field(graph: join__Graph!, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION + directive @join__field(graph: join__Graph!, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION directive @join__graph(name: String!, url: String!) on ENUM_VALUE diff --git a/composition-js/src/__tests__/composeFed1Subgraphs.test.ts b/composition-js/src/__tests__/composeFed1Subgraphs.test.ts index 56c8ec715a..815ac922e7 100644 --- a/composition-js/src/__tests__/composeFed1Subgraphs.test.ts +++ b/composition-js/src/__tests__/composeFed1Subgraphs.test.ts @@ -15,7 +15,7 @@ function errors(r: CompositionResult): [string, string][] { // Returns [the supergraph schema, its api schema, the extracted subgraphs] function schemas(result: CompositionSuccess): [Schema, Schema, Subgraphs] { - // Note that we could user `result.schema`, but reparsing to ensure we don't lose anything with printing/parsing. + // Note that we could user `result.schema`, but re-parsing to ensure we don't lose anything with printing/parsing. const schema = buildSchema(result.supergraphSdl); expect(schema.isCoreSchema()).toBeTruthy(); return [schema, schema.toAPISchema(), extractSubgraphsFromSupergraph(schema)]; diff --git a/composition-js/src/__tests__/hints.test.ts b/composition-js/src/__tests__/hints.test.ts index 6193616a02..c73eb38c82 100644 --- a/composition-js/src/__tests__/hints.test.ts +++ b/composition-js/src/__tests__/hints.test.ts @@ -20,6 +20,9 @@ import { hintInconsistentExecutionDirectiveLocations, hintInconsistentArgumentPresence, hintInconsistentDescription, + hintFromSubgraphDoesNotExist, + hintOverrideDirectiveCanBeRemoved, + hintOverriddenFieldCanBeRemoved, } from '../hints'; import { MergeResult, mergeSubgraphs } from '../merging'; @@ -604,4 +607,126 @@ test('hints on inconsistent description for field', () => { + ' I don\'t know what I\'m doing\n' + ' """' ); -}) +}); + +describe('hint tests related to the @override directive', () => { + it('hint when from subgraph does not exist', () => { + const subgraph1 = gql` + type Query { + a: Int + } + + type T @key(fields: "id"){ + id: Int + f: Int @override(from: "Subgraph3") + } + `; + + const subgraph2 = gql` + type T @key(fields: "id"){ + id: Int + } + `; + const result = mergeDocuments(subgraph1, subgraph2); + expect(result).toRaiseHint( + hintFromSubgraphDoesNotExist, + `Source subgraph "Subgraph3" for field "T.f" on subgraph "Subgraph1" does not exist. Did you mean "Subgraph1" or "Subgraph2"?`, + ); + }); + + it('hint when @override directive can be removed', () => { + const subgraph1 = gql` + type Query { + a: Int + } + + type T @key(fields: "id"){ + id: Int + f: Int @override(from: "Subgraph2") + } + `; + + const subgraph2 = gql` + type T @key(fields: "id"){ + id: Int + } + `; + const result = mergeDocuments(subgraph1, subgraph2); + expect(result).toRaiseHint( + hintOverrideDirectiveCanBeRemoved, + `Field "T.f" on subgraph "Subgraph1" no longer exists in the from subgraph. The @override directive can be removed.`, + ); + }); + + it('hint overridden field can be removed', () => { + const subgraph1 = gql` + type Query { + a: Int + } + + type T @key(fields: "id"){ + id: Int + f: Int @override(from: "Subgraph2") + } + `; + + const subgraph2 = gql` + type T @key(fields: "id"){ + id: Int + f: Int + } + `; + const result = mergeDocuments(subgraph1, subgraph2); + expect(result).toRaiseHint( + hintOverriddenFieldCanBeRemoved, + `Field "T.f" on subgraph "Subgraph2" is overridden. Consider removing it.`, + ); + }); + + it('hint overridden field can be made external', () => { + const subgraph1 = gql` + type Query { + a: Int + } + + type T @key(fields: "id"){ + id: Int @override(from: "Subgraph2") + } + `; + + const subgraph2 = gql` + type T @key(fields: "id"){ + id: Int + } + `; + const result = mergeDocuments(subgraph1, subgraph2); + expect(result).toRaiseHint( + hintOverriddenFieldCanBeRemoved, + `Field "T.id" on subgraph "Subgraph2" is overridden. It is still used in some federation directive(s) (@key, @requires, and/or @provides) and/or to satisfy interface constraint(s), but consider marking it @external explicitly or removing it along with its references.`, + ); + }); + + it('hint when @override directive can be removed because overridden field has been marked external', () => { + const subgraph1 = gql` + type Query { + a: Int + } + + type T @key(fields: "id"){ + id: Int @override(from: "Subgraph2") + f: Int + } + `; + + const subgraph2 = gql` + type T @key(fields: "id"){ + id: Int @external + } + `; + const result = mergeDocuments(subgraph1, subgraph2); + expect(result).toRaiseHint( + hintOverrideDirectiveCanBeRemoved, + `Field "T.id" on subgraph "Subgraph1" is not resolved anymore by the from subgraph (it is marked "@external" in "Subgraph2"). The @override directive can be removed.`, + ); + }); +}); diff --git a/composition-js/src/__tests__/override.compose.test.ts b/composition-js/src/__tests__/override.compose.test.ts new file mode 100644 index 0000000000..c3f94cc158 --- /dev/null +++ b/composition-js/src/__tests__/override.compose.test.ts @@ -0,0 +1,760 @@ +import { printSchema, printType } from "@apollo/federation-internals"; +import gql from "graphql-tag"; +import "./matchers"; +import { + assertCompositionSuccess, + schemas, + errors, + composeAsFed2Subgraphs, +} from "./compose.test"; + +describe("composition involving @override directive", () => { + it.skip("@override whole type", () => { + const subgraph1 = { + name: "Subgraph1", + url: "https://Subgraph1", + typeDefs: gql` + type Query { + t: T + } + + type T @key(fields: "k") @override(from: "Subgraph2") { + k: ID + a: Int + b: Int + } + `, + }; + + const subgraph2 = { + name: "Subgraph2", + url: "https://Subgraph2", + typeDefs: gql` + type T @key(fields: "k") { + k: ID + a: Int + c: Int + } + `, + }; + + const result = composeAsFed2Subgraphs([subgraph1, subgraph2]); + assertCompositionSuccess(result); + + const typeT = result.schema.type("T"); + expect(printType(typeT!)).toMatchInlineSnapshot(` + "type T + @join__type(graph: SUBGRAPH1, key: \\"k\\") + @join__type(graph: SUBGRAPH2, key: \\"k\\") + { + k: ID + a: Int + b: Int + }" + `); + }); + + it("@override single field.", () => { + const subgraph1 = { + name: "Subgraph1", + url: "https://Subgraph1", + typeDefs: gql` + type Query { + t: T + } + + type T @key(fields: "k") { + k: ID + a: Int @override(from: "Subgraph2") + } + `, + }; + + const subgraph2 = { + name: "Subgraph2", + url: "https://Subgraph2", + typeDefs: gql` + type T @key(fields: "k") { + k: ID + a: Int + b: Int + } + `, + }; + + const result = composeAsFed2Subgraphs([subgraph1, subgraph2]); + assertCompositionSuccess(result); + + const typeT = result.schema.type("T"); + expect(printType(typeT!)).toMatchInlineSnapshot(` + "type T + @join__type(graph: SUBGRAPH1, key: \\"k\\") + @join__type(graph: SUBGRAPH2, key: \\"k\\") + { + k: ID + a: Int @join__field(graph: SUBGRAPH1, override: \\"Subgraph2\\") + b: Int @join__field(graph: SUBGRAPH2) + }" + `); + + const [_, api] = schemas(result); + expect(printSchema(api)).toMatchString(` + type Query { + t: T + } + + type T { + k: ID + a: Int + b: Int + } + `); + }); + + it("override field in a @provides", () => { + const subgraph1 = { + name: "Subgraph1", + url: "https://Subgraph1", + typeDefs: gql` + type Query { + t: T + } + type T @key(fields: "k") { + k: ID + a: A @shareable + } + type A @key(fields: "id") { + id: ID! + b: B @override(from: "Subgraph2") + } + type B @key(fields: "id") { + id: ID! + v: String @shareable + } + `, + }; + + // Note @provides is only allowed on fields that the subgraph does not resolve, but + // because of nesting, this doesn't equate to all fields in a @provides being + // external. But it does mean that for an overriden field to be in a @provides, + // some nesting has to be involved. + const subgraph2 = { + name: "Subgraph2", + url: "https://Subgraph2", + typeDefs: gql` + type T @key(fields: "k") { + k: ID + a: A @shareable @provides(fields: "b { v }") + } + type A @key(fields: "id") { + id: ID! + b: B + } + type B @key(fields: "id") { + id: ID! + v: String @external + } + `, + }; + + const result = composeAsFed2Subgraphs([subgraph1, subgraph2]); + assertCompositionSuccess(result); + + // Ensures `A.b` is marked external in Subgraph2 since it's overridden but there is still a provides mentioning it. + const typeA = result.schema.type("A"); + expect(printType(typeA!)).toMatchInlineSnapshot(` + "type A + @join__type(graph: SUBGRAPH1, key: \\"id\\") + @join__type(graph: SUBGRAPH2, key: \\"id\\") + { + id: ID! + b: B @join__field(graph: SUBGRAPH1, override: \\"Subgraph2\\") @join__field(graph: SUBGRAPH2, usedOverridden: true) + }" + `); + + // Ensuring the provides is still here. + const typeT = result.schema.type("T"); + expect(printType(typeT!)).toMatchInlineSnapshot(` + "type T + @join__type(graph: SUBGRAPH1, key: \\"k\\") + @join__type(graph: SUBGRAPH2, key: \\"k\\") + { + k: ID + a: A @join__field(graph: SUBGRAPH1) @join__field(graph: SUBGRAPH2, provides: \\"b { v }\\") + }" + `); + }); + + it("override field in a @requires", () => { + const subgraph1 = { + name: "Subgraph1", + url: "https://Subgraph1", + typeDefs: gql` + type Query { + t: T + } + type T @key(fields: "k") { + k: ID + a: A @shareable + } + type A @key(fields: "id") { + id: ID! + b: B @override(from: "Subgraph2") + } + type B @key(fields: "id") { + id: ID! + v: String @shareable + } + `, + }; + + const subgraph2 = { + name: "Subgraph2", + url: "https://Subgraph2", + typeDefs: gql` + type T @key(fields: "k") { + k: ID + a: A @shareable + x: Int @requires(fields: "a { b { v } }") + } + type A @key(fields: "id") { + id: ID! + b: B + } + type B @key(fields: "id") { + id: ID! + v: String @external + } + `, + }; + + const result = composeAsFed2Subgraphs([subgraph1, subgraph2]); + assertCompositionSuccess(result); + + // Ensures `A.b` is marked external in Subgraph2 since it's overridden but there is still a requires mentioning it. + const typeA = result.schema.type("A"); + expect(printType(typeA!)).toMatchInlineSnapshot(` + "type A + @join__type(graph: SUBGRAPH1, key: \\"id\\") + @join__type(graph: SUBGRAPH2, key: \\"id\\") + { + id: ID! + b: B @join__field(graph: SUBGRAPH1, override: \\"Subgraph2\\") @join__field(graph: SUBGRAPH2, usedOverridden: true) + }" + `); + + // Ensuring the requires is still here. + const typeT = result.schema.type("T"); + expect(printType(typeT!)).toMatchInlineSnapshot(` + "type T + @join__type(graph: SUBGRAPH1, key: \\"k\\") + @join__type(graph: SUBGRAPH2, key: \\"k\\") + { + k: ID + a: A + x: Int @join__field(graph: SUBGRAPH2, requires: \\"a { b { v } }\\") + }" + `); + }); + + it("override field that is necessary for an interface", () => { + const subgraph1 = { + name: "Subgraph1", + url: "https://Subgraph1", + typeDefs: gql` + type Query { + t: T + } + + interface I { + x: Int + } + + type T implements I @key(fields: "k") { + k: ID + x: Int + } + `, + }; + + const subgraph2 = { + name: "Subgraph2", + url: "https://Subgraph2", + typeDefs: gql` + type T @key(fields: "k") { + k: ID + x: Int @override(from: "Subgraph1") + } + `, + }; + + const result = composeAsFed2Subgraphs([subgraph1, subgraph2]); + assertCompositionSuccess(result); + + // Ensures `T.x` is marked external in Subgraph1 since it's overridden but still required by interface I. + const typeT = result.schema.type("T"); + expect(printType(typeT!)).toMatchInlineSnapshot(` + "type T implements I + @join__implements(graph: SUBGRAPH1, interface: \\"I\\") + @join__type(graph: SUBGRAPH1, key: \\"k\\") + @join__type(graph: SUBGRAPH2, key: \\"k\\") + { + k: ID + x: Int @join__field(graph: SUBGRAPH1, usedOverridden: true) @join__field(graph: SUBGRAPH2, override: \\"Subgraph1\\") + }" + `); + }); + + it("override from self error", () => { + const subgraph1 = { + name: "Subgraph1", + url: "https://Subgraph1", + typeDefs: gql` + type Query { + t: T + } + + type T @key(fields: "k") { + k: ID + a: Int @override(from: "Subgraph1") + } + `, + }; + + const subgraph2 = { + name: "Subgraph2", + url: "https://Subgraph2", + typeDefs: gql` + type T @key(fields: "k") { + k: ID + } + `, + }; + + const result = composeAsFed2Subgraphs([subgraph1, subgraph2]); + expect(result.errors?.length).toBe(1); + expect(result.errors).toBeDefined(); + expect(errors(result)).toStrictEqual([ + [ + "OVERRIDE_FROM_SELF_ERROR", + `Source and destination subgraphs "Subgraph1" are the same for overridden field "T.a"`, + ], + ]); + }); + + it.skip("override in both type and field error", () => { + const subgraph1 = { + name: "Subgraph1", + url: "https://Subgraph1", + typeDefs: gql` + type Query { + t: T + } + + type T @key(fields: "k") @override(from: "Subgraph2") { + k: ID + a: Int @override(from: "Subgraph2") + } + `, + }; + + const subgraph2 = { + name: "Subgraph2", + url: "https://Subgraph2", + typeDefs: gql` + type T @key(fields: "k") { + k: ID + a: Int + } + `, + }; + + const result = composeAsFed2Subgraphs([subgraph1, subgraph2]); + expect(result.errors?.length).toBe(1); + expect(result.errors).toBeDefined(); + expect(errors(result)).toStrictEqual([ + [ + "OVERRIDE_ON_BOTH_FIELD_AND_TYPE", + `Field "T.a" on subgraph "Subgraph1" is marked with @override directive on both the field and the type`, + ], + ]); + }); + + it("multiple override error", () => { + const subgraph1 = { + name: "Subgraph1", + url: "https://Subgraph1", + typeDefs: gql` + type Query { + t: T + } + + type T @key(fields: "k") { + k: ID + a: Int @override(from: "Subgraph2") + } + `, + }; + + const subgraph2 = { + name: "Subgraph2", + url: "https://Subgraph2", + typeDefs: gql` + type T @key(fields: "k") { + k: ID + a: Int @override(from: "Subgraph1") + } + `, + }; + + const result = composeAsFed2Subgraphs([subgraph1, subgraph2]); + // TODO: This test really should not cause the shareable error to be raised, but to fix it would be a bit of a pain, so punting + // for now + expect(result.errors?.length).toBe(3); + expect(result.errors).toBeDefined(); + expect(errors(result)).toStrictEqual([ + [ + "OVERRIDE_SOURCE_HAS_OVERRIDE", + `Field "T.a" on subgraph "Subgraph1" is also marked with directive @override in subgraph "Subgraph2". Only one @override directive is allowed per field.`, + ], + [ + "OVERRIDE_SOURCE_HAS_OVERRIDE", + `Field "T.a" on subgraph "Subgraph2" is also marked with directive @override in subgraph "Subgraph1". Only one @override directive is allowed per field.`, + ], + [ + "INVALID_FIELD_SHARING", + `Non-shareable field "T.a" is resolved from multiple subgraphs: it is resolved from subgraphs "Subgraph1" and "Subgraph2" and defined as non-shareable in all of them`, + ], + ]); + }); + + it("override @key field", () => { + const subgraph1 = { + name: "Subgraph1", + url: "https://Subgraph1", + typeDefs: gql` + type Query { + t: T + } + + type T @key(fields: "k") { + k: ID @override(from: "Subgraph2") + a: Int + } + `, + }; + + const subgraph2 = { + name: "Subgraph2", + url: "https://Subgraph2", + typeDefs: gql` + type T @key(fields: "k") { + k: ID + b: Int + } + `, + }; + + const result = composeAsFed2Subgraphs([subgraph1, subgraph2]); + assertCompositionSuccess(result); + + const typeT = result.schema.type("T"); + expect(printType(typeT!)).toMatchInlineSnapshot(` + "type T + @join__type(graph: SUBGRAPH1, key: \\"k\\") + @join__type(graph: SUBGRAPH2, key: \\"k\\") + { + k: ID @join__field(graph: SUBGRAPH1, override: \\"Subgraph2\\") @join__field(graph: SUBGRAPH2, usedOverridden: true) + a: Int @join__field(graph: SUBGRAPH1) + b: Int @join__field(graph: SUBGRAPH2) + }" + `); + }); + + it("override @key field that breaks composition validation", () => { + const subgraph1 = { + name: "Subgraph1", + url: "https://Subgraph1", + typeDefs: gql` + type Query { + t: T + } + type T @key(fields: "k") { + k: ID @override(from: "Subgraph2") + a: Int + } + `, + }; + + const subgraph2 = { + name: "Subgraph2", + url: "https://Subgraph2", + typeDefs: gql` + type Query { + otherT: T + } + type T @key(fields: "k") { + k: ID + b: Int + } + `, + }; + + const result = composeAsFed2Subgraphs([subgraph1, subgraph2]); + expect(result.errors).toBeDefined(); + expect(result.errors?.map(e => e.message)).toMatchStringArray([ + ` + The following supergraph API query: + { + otherT { + k + } + } + cannot be satisfied by the subgraphs because: + - from subgraph "Subgraph2": + - field "T.k" is not resolvable because it is overridden by subgraph "Subgraph1". + - cannot move to subgraph "Subgraph1" using @key(fields: "k") of "T", the key field(s) cannot be resolved from subgraph "Subgraph2" (note that some of those key fields are overridden in "Subgraph2"). + `, + ` + The following supergraph API query: + { + otherT { + a + } + } + cannot be satisfied by the subgraphs because: + - from subgraph "Subgraph2": + - cannot find field "T.a". + - cannot move to subgraph "Subgraph1" using @key(fields: "k") of "T", the key field(s) cannot be resolved from subgraph "Subgraph2" (note that some of those key fields are overridden in "Subgraph2"). + ` + ]); + }); + + it("override field with change to type definition", () => { + const subgraph1 = { + name: "Subgraph1", + url: "https://Subgraph1", + typeDefs: gql` + type Query { + t: T + } + + type T @key(fields: "k") { + k: ID + a: Int @override(from: "Subgraph2") + } + `, + }; + + const subgraph2 = { + name: "Subgraph2", + url: "https://Subgraph2", + typeDefs: gql` + type T @key(fields: "k") { + k: ID + a: String + } + `, + }; + + const result = composeAsFed2Subgraphs([subgraph1, subgraph2]); + expect(result.errors?.length).toBe(1); + expect(result.errors).toBeDefined(); + expect(errors(result)).toStrictEqual([ + [ + 'FIELD_TYPE_MISMATCH', + 'Field "T.a" has incompatible types across subgraphs: it has type "Int" in subgraph "Subgraph1" but type "String" in subgraph "Subgraph2"' + ] + ]); + }); + + it("override field that is a key in another type", () => { + const subgraph1 = { + name: "Subgraph1", + url: "https://Subgraph1", + typeDefs: gql` + type Query { + t: T + } + + type T @key(fields: "e { k }") { + e: E + } + + type E { + k: ID @override(from: "Subgraph2") + a: Int + } + `, + }; + + const subgraph2 = { + name: "Subgraph2", + url: "https://Subgraph2", + typeDefs: gql` + type T @key(fields: "e { k }") { + e: E + x: Int + } + + type E { + k: ID + b: Int + } + `, + }; + + const result = composeAsFed2Subgraphs([subgraph1, subgraph2]); + assertCompositionSuccess(result); + + const typeE = result.schema.type("E"); + expect(printType(typeE!)).toMatchInlineSnapshot(` + "type E + @join__type(graph: SUBGRAPH1) + @join__type(graph: SUBGRAPH2) + { + k: ID @join__field(graph: SUBGRAPH1, override: \\"Subgraph2\\") @join__field(graph: SUBGRAPH2, usedOverridden: true) + a: Int @join__field(graph: SUBGRAPH1) + b: Int @join__field(graph: SUBGRAPH2) + }" + `); + + const typeT = result.schema.type("T"); + expect(printType(typeT!)).toMatchInlineSnapshot(` + "type T + @join__type(graph: SUBGRAPH1, key: \\"e { k }\\") + @join__type(graph: SUBGRAPH2, key: \\"e { k }\\") + { + e: E + x: Int @join__field(graph: SUBGRAPH2) + }" + `); + }); + + it("override with @provides on overridden field", () => { + const subgraph1 = { + name: "Subgraph1", + url: "https://Subgraph1", + typeDefs: gql` + type Query { + t: T + } + + type T @key(fields: "k") { + k: ID + u: U @override(from: "Subgraph2") + } + + type U @key(fields: "id") { + id: ID + name: String + } + `, + }; + + const subgraph2 = { + name: "Subgraph2", + url: "https://Subgraph2", + typeDefs: gql` + type T @key(fields: "k") { + k: ID + u: U @provides(fields: "name") + } + + extend type U @key(fields: "id") { + id: ID + name: String @external + } + `, + }; + + const result = composeAsFed2Subgraphs([subgraph1, subgraph2]); + // expect(result.errors?.length).toBe(1); + expect(result.errors).toBeDefined(); + expect(errors(result)).toContainEqual([ + "OVERRIDE_COLLISION_WITH_ANOTHER_DIRECTIVE", + `@override cannot be used on field "T.u" on subgraph "Subgraph1" since "T.u" on "Subgraph1" is marked with directive "@provides"`, + ]); + }); + + it("override with @requires on overridden field", () => { + const subgraph1 = { + name: "Subgraph1", + url: "https://Subgraph1", + typeDefs: gql` + type Query { + t: T + } + + type T @key(fields: "k") { + k: ID + id: ID + u: U @override(from: "Subgraph2") + } + + type U @key(fields: "id") { + id: ID + } + `, + }; + + const subgraph2 = { + name: "Subgraph2", + url: "https://Subgraph2", + typeDefs: gql` + type T @key(fields: "k") { + k: ID + id: ID @external + u: U @requires(fields: "id") + } + + extend type U @key(fields: "id") { + id: ID + } + `, + }; + + const result = composeAsFed2Subgraphs([subgraph1, subgraph2]); + // expect(result.errors?.length).toBe(1); + expect(result.errors).toBeDefined(); + expect(errors(result)).toContainEqual([ + "OVERRIDE_COLLISION_WITH_ANOTHER_DIRECTIVE", + `@override cannot be used on field "T.u" on subgraph "Subgraph1" since "T.u" on "Subgraph1" is marked with directive "@requires"`, + ]); + }); + + it("override with @external on overriding field", () => { + const subgraph1 = { + name: "Subgraph1", + url: "https://Subgraph1", + typeDefs: gql` + type Query { + t: T + } + + type T @key(fields: "k") { + k: ID @override(from: "Subgraph2") @external + a: Int + } + `, + }; + + const subgraph2 = { + name: "Subgraph2", + url: "https://Subgraph2", + typeDefs: gql` + type T @key(fields: "k") { + k: ID + b: Int + } + `, + }; + + const result = composeAsFed2Subgraphs([subgraph1, subgraph2]); + expect(result.errors).toBeDefined(); + expect(errors(result)).toContainEqual([ + "OVERRIDE_COLLISION_WITH_ANOTHER_DIRECTIVE", + `@override cannot be used on field "T.k" on subgraph "Subgraph1" since "T.k" on "Subgraph1" is marked with directive "@external"`, + ]); + }); +}); diff --git a/composition-js/src/hints.ts b/composition-js/src/hints.ts index bf6fabf435..3c413bcc0d 100644 --- a/composition-js/src/hints.ts +++ b/composition-js/src/hints.ts @@ -122,6 +122,24 @@ export const hintInconsistentArgumentPresence = new HintID( 'the argument with mismatched types' ); +export const hintFromSubgraphDoesNotExist = new HintID( + 'FromSubgraphDoesNotExist', + 'Source subgraph specified by @override directive does not exist', + 'the argument with non-existent subgraph' +); + +export const hintOverriddenFieldCanBeRemoved = new HintID( + 'OverriddenFieldCanBeRemoved', + 'Field has been overridden by another subgraph. Consider removing.', + 'the overridden field' +); + +export const hintOverrideDirectiveCanBeRemoved = new HintID( + 'OverrideDirectiveCanBeRemoved', + 'Field with @override directive no longer exists in source subgraph, the directive can be safely removed', + 'the field with the override directive' +); + export class CompositionHint { public readonly nodes?: readonly SubgraphASTNode[]; diff --git a/composition-js/src/merging/merge.ts b/composition-js/src/merging/merge.ts index 352c32ccd6..25b65295c5 100644 --- a/composition-js/src/merging/merge.ts +++ b/composition-js/src/merging/merge.ts @@ -61,6 +61,8 @@ import { Subgraph, errorCode, withModifiedErrorNodes, + didYouMean, + suggestionList, } from "@apollo/federation-internals"; import { ASTNode, GraphQLError, DirectiveLocation } from "graphql"; import { @@ -81,9 +83,14 @@ import { hintInconsistentExecutionDirectiveLocations, hintInconsistentArgumentPresence, hintInconsistentDescription, + hintFromSubgraphDoesNotExist, + hintOverrideDirectiveCanBeRemoved, + hintOverriddenFieldCanBeRemoved, } from "../hints"; const linkSpec = LINK_VERSIONS.latest(); +type FieldOrUndefinedArray = (FieldDefinition | undefined)[]; + const joinSpec = JOIN_VERSIONS.latest(); export type MergeResult = MergeSuccess | MergeFailure; @@ -93,6 +100,35 @@ export type CompositionOptions = { allowedFieldTypeMergingSubtypingRules?: SubtypingRule[] } +// for each source, specify additional properties that validate functions can set +class FieldMergeContext { + _props: { usedOverridden: boolean, unusedOverridden: boolean }[]; + + constructor(sources: unknown[]) { + this._props = (new Array(sources.length)).fill(true).map(_ => ({ usedOverridden: false, unusedOverridden: false })); + } + + isUsedOverridden(idx: number) { + return this._props[idx].usedOverridden; + } + + isUnusedOverridden(idx: number) { + return this._props[idx].unusedOverridden; + } + + setUsedOverriden(idx: number) { + this._props[idx].usedOverridden = true; + } + + setUnusedOverridden(idx: number) { + this._props[idx].unusedOverridden = true; + } + + some(predicate: ({ usedOverridden, unusedOverridden }: { usedOverridden: boolean, unusedOverridden: boolean }) => boolean): boolean { + return this._props.some(predicate); + } +} + // TODO:" we currently cannot allow "list upgrades", meaning a subgraph returning `String` // and another returning `[String]`. To support it, we would need the execution code to // recognize situation and "coerce" results from the first subgraph (the one returning @@ -752,8 +788,10 @@ class Merger { this.hintOnInconsistentValueTypeField(sources, dest, destField); } const subgraphFields = sources.map(t => t?.field(destField.name)); - this.mergeField(subgraphFields, destField); - this.validateFieldSharing(subgraphFields, destField); + const mergeContext = this.validateOverride(subgraphFields, destField); + + this.mergeField(subgraphFields, destField, mergeContext); + this.validateFieldSharing(subgraphFields, destField, mergeContext); } } } @@ -886,7 +924,7 @@ class Merger { return filtered; } - private hasExternal(sources: (FieldDefinition | undefined)[]): boolean { + private hasExternal(sources: FieldOrUndefinedArray): boolean { return sources.some((s, i) => s !== undefined && this.isExternal(i, s)); } @@ -894,7 +932,166 @@ class Merger { return this.metadata(sourceIdx).isFieldShareable(field); } - private mergeField(sources: (FieldDefinition | undefined)[], dest: FieldDefinition) { + private getOverrideDirective(sourceIdx: number, field: FieldDefinition): Directive | undefined { + // Check the directive on the field, then on the enclosing type. + const metadata = this.metadata(sourceIdx); + const overrideDirective = metadata.isFed2Schema() ? metadata.overrideDirective() : undefined; + const allFieldOverrides = overrideDirective ? field.appliedDirectivesOf(overrideDirective) : []; + return allFieldOverrides[0]; // if array is empty, will return undefined + } + + private overrideConflictsWithOtherDirective({ + idx, + field, + subgraphName, + fromIdx, + fromField, + }: { + idx: number; + field: FieldDefinition | undefined; + subgraphName: string; + fromIdx: number; + fromField: FieldDefinition | undefined; + }): { result: boolean, conflictingDirective?: DirectiveDefinition, subgraph?: string } { + const fromMetadata = this.metadata(fromIdx); + for (const directive of [fromMetadata.requiresDirective(), fromMetadata.providesDirective()]) { + if (fromField?.hasAppliedDirective(directive)) { + return { + result: true, + conflictingDirective: directive, + subgraph: subgraphName, + }; + } + } + if (field && this.isExternal(idx, field)) { + return { + result: true, + conflictingDirective: fromMetadata.externalDirective(), + subgraph: subgraphName, + }; + } + return { result: false }; + } + + /** + * Validates whether or not the use of the @override directive is correct. + * return value is a list of fields that has been filtered to ignore overridden fields + */ + private validateOverride(sources: FieldOrUndefinedArray, { coordinate }: FieldDefinition): FieldMergeContext { + const result = new FieldMergeContext(sources); + + // For any field, we can't have more than one @override directive + type MappedValue = { + idx: number, + name: string, + overrideDirective: Directive> | undefined, + }; + + type ReduceResultType = { + subgraphsWithOverride: string[], + subgraphMap: { [key: string]: MappedValue }, + }; + + // convert sources to a map so we don't have to keep scanning through the array to find a source + const { subgraphsWithOverride, subgraphMap } = sources.map((source, idx) => { + if (!source) { + return undefined; + } + return { + idx, + name: this.names[idx], + overrideDirective: this.getOverrideDirective(idx, source), + }; + }).reduce((acc: ReduceResultType, elem) => { + if (elem !== undefined) { + acc.subgraphMap[elem.name] = elem; + if (elem.overrideDirective !== undefined) { + acc.subgraphsWithOverride.push(elem.name); + } + } + return acc; + }, { subgraphsWithOverride: [], subgraphMap: {} }); + + // for each subgraph that has an @override directive, check to see if any errors or hints should be surfaced + subgraphsWithOverride.forEach((subgraphName) => { + const { overrideDirective } = subgraphMap[subgraphName]; + const sourceSubgraphName = overrideDirective?.arguments()?.from; + if (!this.names.includes(sourceSubgraphName)) { + const suggestions = suggestionList(sourceSubgraphName, this.names); + const extraMsg = didYouMean(suggestions); + this.hints.push(new CompositionHint( + hintFromSubgraphDoesNotExist, + `Source subgraph "${sourceSubgraphName}" for field "${coordinate}" on subgraph "${subgraphName}" does not exist.${extraMsg}`, + coordinate, + )); + } else if (sourceSubgraphName === subgraphName) { + this.errors.push(ERRORS.OVERRIDE_FROM_SELF_ERROR.err({ + message: `Source and destination subgraphs "${sourceSubgraphName}" are the same for overridden field "${coordinate}"`, + nodes: overrideDirective?.sourceAST, + })); + } else if (subgraphsWithOverride.includes(sourceSubgraphName)) { + this.errors.push(ERRORS.OVERRIDE_SOURCE_HAS_OVERRIDE.err({ + message: `Field "${coordinate}" on subgraph "${subgraphName}" is also marked with directive @override in subgraph "${sourceSubgraphName}". Only one @override directive is allowed per field.`, + nodes: sourceASTs(overrideDirective, subgraphMap[sourceSubgraphName].overrideDirective) + })); + } else if (subgraphMap[sourceSubgraphName] === undefined) { + this.hints.push(new CompositionHint( + hintOverrideDirectiveCanBeRemoved, + `Field "${coordinate}" on subgraph "${subgraphName}" no longer exists in the from subgraph. The @override directive can be removed.`, + coordinate, + )); + } else { + // check to make sure that there is no conflicting @provides, @requires, or @external directives + const fromIdx = this.names.indexOf(sourceSubgraphName); + const fromField = sources[fromIdx]; + const { result: hasIncompatible, conflictingDirective, subgraph } = this.overrideConflictsWithOtherDirective({ + idx: subgraphMap[subgraphName].idx, + field: sources[subgraphMap[subgraphName].idx], + subgraphName, + fromIdx: this.names.indexOf(sourceSubgraphName), + fromField: sources[fromIdx], + }); + if (hasIncompatible) { + assert(conflictingDirective !== undefined, 'conflictingDirective should not be undefined'); + this.errors.push(ERRORS.OVERRIDE_COLLISION_WITH_ANOTHER_DIRECTIVE.err({ + message: `@override cannot be used on field "${fromField?.coordinate}" on subgraph "${subgraphName}" since "${fromField?.coordinate}" on "${subgraph}" is marked with directive "@${conflictingDirective.name}"`, + nodes: sourceASTs(overrideDirective, conflictingDirective) + })); + } else { + // if we get here, then the @override directive is valid + // if the field being overridden is used, then we need to add an @external directive + assert(fromField, 'fromField should not be undefined'); + if (this.isExternal(fromIdx, fromField)) { + // The from field is explcitely marked external by the user (which means it is "used" and cannot be completely + // removed) so the @override can be removed. + this.hints.push(new CompositionHint( + hintOverrideDirectiveCanBeRemoved, + `Field "${coordinate}" on subgraph "${subgraphName}" is not resolved anymore by the from subgraph (it is marked "@external" in "${sourceSubgraphName}"). The @override directive can be removed.`, + coordinate, + )); + } else if (this.metadata(fromIdx).isFieldUsed(fromField)) { + result.setUsedOverriden(fromIdx); + this.hints.push(new CompositionHint( + hintOverriddenFieldCanBeRemoved, + `Field "${coordinate}" on subgraph "${sourceSubgraphName}" is overridden. It is still used in some federation directive(s) (@key, @requires, and/or @provides) and/or to satisfy interface constraint(s), but consider marking it @external explicitly or removing it along with its references.`, + coordinate, + )); + } else { + result.setUnusedOverridden(fromIdx); + this.hints.push(new CompositionHint( + hintOverriddenFieldCanBeRemoved, + `Field "${coordinate}" on subgraph "${sourceSubgraphName}" is overridden. Consider removing it.`, + coordinate, + )); + } + } + } + }); + + return result; + } + + private mergeField(sources: FieldOrUndefinedArray, dest: FieldDefinition, mergeContext: FieldMergeContext = new FieldMergeContext(sources)) { if (sources.every((s, i) => s === undefined || this.isExternal(i, s))) { const definingSubgraphs = sources.map((source, i) => source ? this.names[i] : undefined).filter(s => s !== undefined) as string[]; const nodes = sources.map(source => source?.sourceAST).filter(s => s !== undefined) as ASTNode[]; @@ -920,16 +1117,16 @@ class Merger { if (this.hasExternal(sources)) { this.validateExternalFields(sources, dest, allTypesEqual); } - - this.addJoinField(sources, dest, allTypesEqual); + this.addJoinField({ sources, dest, allTypesEqual, mergeContext }); } - private validateFieldSharing(sources: (FieldDefinition | undefined)[], dest: FieldDefinition) { + private validateFieldSharing(sources: FieldOrUndefinedArray, dest: FieldDefinition, mergeContext: FieldMergeContext) { const shareableSources: number[] = []; const nonShareableSources: number[] = []; const allResolving: FieldDefinition[] = []; for (const [i, source] of sources.entries()) { - if (!source || this.isFullyExternal(i, source)) { + const overridden = mergeContext.isUsedOverridden(i) || mergeContext.isUnusedOverridden(i); + if (!source || this.isFullyExternal(i, source) || overridden) { continue; } @@ -953,7 +1150,7 @@ class Merger { } } - private validateExternalFields(sources: (FieldDefinition | undefined)[], dest: FieldDefinition, allTypesEqual: boolean) { + private validateExternalFields(sources: FieldOrUndefinedArray, dest: FieldDefinition, allTypesEqual: boolean) { let hasInvalidTypes = false; const invalidArgsPresence = new Set(); const invalidArgsTypes = new Set(); @@ -1031,22 +1228,32 @@ class Merger { } } - private needsJoinField | InputFieldDefinition>( + private needsJoinField | InputFieldDefinition>({ + sources, + parentName, + allTypesEqual, + mergeContext, + }: { sources: (T | undefined)[], parentName: string, - allTypesEqual: boolean - ): boolean { + allTypesEqual: boolean, + mergeContext: FieldMergeContext, + }): boolean { // If not all the types are equal, then we need to put a join__field to preserve the proper type information. if (!allTypesEqual) { return true; } + if (mergeContext.some(({ usedOverridden }) => usedOverridden)) { + return true; + } // We can avoid the join__field if: // 1) the field exists in all sources having the field parent type, // 2) none of the field instance has a @requires or @provides. // 3) none of the field is @external. for (const [idx, source] of sources.entries()) { - if (source) { + const overridden = mergeContext.isUnusedOverridden(idx); + if (source && !overridden) { const sourceMeta = this.subgraphs.values()[idx].metadata(); if (this.isExternal(idx, source) || source.hasAppliedDirective(sourceMeta.providesDirective()) @@ -1061,20 +1268,35 @@ class Merger { } } } + return false; } private addJoinField | InputFieldDefinition>( - sources: (T | undefined)[], - dest: T, - allTypesEqual: boolean - ) { - if (!this.needsJoinField(sources, dest.parent.name, allTypesEqual)) { + { + sources, + dest, + allTypesEqual, + mergeContext, + }: { + sources: (T | undefined)[], + dest: T, + allTypesEqual: boolean, + mergeContext: FieldMergeContext, + }) { + if (!this.needsJoinField({ + sources, + parentName: dest.parent.name, + allTypesEqual, + mergeContext, + })) { return; } const joinFieldDirective = joinSpec.fieldDirective(this.merged); for (const [idx, source] of sources.entries()) { - if (!source) { + const usedOverridden = mergeContext.isUsedOverridden(idx); + const unusedOverridden = mergeContext.isUnusedOverridden(idx); + if (!source || unusedOverridden) { continue; } @@ -1085,8 +1307,10 @@ class Merger { graph: name, requires: this.getFieldSet(source, sourceMeta.requiresDirective()), provides: this.getFieldSet(source, sourceMeta.providesDirective()), + override: source.appliedDirectivesOf(sourceMeta.overrideDirective()).pop()?.arguments()?.from, type: allTypesEqual ? undefined : source.type?.toString(), external: external ? true : undefined, + usedOverridden: usedOverridden ? true : undefined, }); } } @@ -1407,7 +1631,8 @@ class Merger { this.mergeDescription(sources, dest); this.mergeAppliedDirectives(sources, dest); const allTypesEqual = this.mergeTypeReference(sources, dest, true); - this.addJoinField(sources, dest, allTypesEqual); + const mergeContext = new FieldMergeContext(sources); + this.addJoinField({ sources, dest, allTypesEqual, mergeContext }); this.mergeDefaultValue(sources, dest, 'Input field'); } diff --git a/docs/source/errors.md b/docs/source/errors.md index 8feb7d04f2..424825474d 100644 --- a/docs/source/errors.md +++ b/docs/source/errors.md @@ -46,6 +46,9 @@ The following errors may be raised by composition: | `MERGED_DIRECTIVE_APPLICATION_ON_EXTERNAL` | In a subgraph, a field is both marked @external and has a merged directive applied to it | 2.0.0 | | | `NO_QUERIES` | None of the composed subgraphs expose any query. | 2.0.0 | | | `NON_REPEATABLE_DIRECTIVE_ARGUMENTS_MISMATCH` | A non-repeatable directive is applied to a schema element in different subgraphs but with arguments that are different. | 2.0.0 | | +| `OVERRIDE_COLLISION_WITH_ANOTHER_DIRECTIVE` | The @override directive cannot be used on external fields, nor to override fields with either @external, @provides, or @requires. | 2.0.0 | | +| `OVERRIDE_FROM_SELF_ERROR` | Field with `@override` directive has "from" location that references its own subgraph. | 2.0.0 | | +| `OVERRIDE_SOURCE_HAS_OVERRIDE` | Field which is overridden to another subgraph is also marked @override. | 2.0.0 | | | `PROVIDES_FIELDS_HAS_ARGS` | The `fields` argument of a `@provides` directive includes a field defined with arguments (which is not currently supported). | 2.0.0 | | | `PROVIDES_FIELDS_MISSING_EXTERNAL` | The `fields` argument of a `@provides` directive includes a field that is not marked as `@external`. | 0.x | | | `PROVIDES_INVALID_FIELDS_TYPE` | The value passed to the `fields` argument of a `@provides` directive is not a string. | 2.0.0 | | diff --git a/federation-integration-testsuite-js/src/fixtures/accounts.ts b/federation-integration-testsuite-js/src/fixtures/accounts.ts index eb9b763d09..3f5b41f695 100644 --- a/federation-integration-testsuite-js/src/fixtures/accounts.ts +++ b/federation-integration-testsuite-js/src/fixtures/accounts.ts @@ -83,6 +83,7 @@ export const typeDefs = gql` id: ID! name: String @external userAccount(id: ID! = "1"): User @requires(fields: "name") + description: String @override(from: "books") } `; diff --git a/federation-integration-testsuite-js/src/fixtures/books.ts b/federation-integration-testsuite-js/src/fixtures/books.ts index 7890d1616d..7c991b8acb 100644 --- a/federation-integration-testsuite-js/src/fixtures/books.ts +++ b/federation-integration-testsuite-js/src/fixtures/books.ts @@ -28,6 +28,7 @@ export const typeDefs = gql` type Library @key(fields: "id") { id: ID! name: String + description: String } # FIXME: turn back on when unions are supported in composition diff --git a/gateway-js/src/__tests__/buildQueryPlan.test.ts b/gateway-js/src/__tests__/buildQueryPlan.test.ts index 99677842d2..74fdac1c12 100644 --- a/gateway-js/src/__tests__/buildQueryPlan.test.ts +++ b/gateway-js/src/__tests__/buildQueryPlan.test.ts @@ -1,4 +1,7 @@ -import { astSerializer, queryPlanSerializer } from 'apollo-federation-integration-testsuite'; +import { + astSerializer, + queryPlanSerializer, +} from 'apollo-federation-integration-testsuite'; import { getFederatedTestingSchema } from './execution-utils'; import { QueryPlan, QueryPlanner } from '@apollo/query-planner'; import { Schema, parseOperation } from '@apollo/federation-internals'; @@ -6,14 +9,13 @@ import { Schema, parseOperation } from '@apollo/federation-internals'; expect.addSnapshotSerializer(astSerializer); expect.addSnapshotSerializer(queryPlanSerializer); - describe('buildQueryPlan', () => { let schema: Schema; let queryPlanner: QueryPlanner; const buildPlan = (operation: string): QueryPlan => { return queryPlanner.buildQueryPlan(parseOperation(schema, operation)); - } + }; beforeEach(() => { expect( @@ -515,7 +517,7 @@ describe('buildQueryPlan', () => { describe(`when requesting a composite field with subfields from another service`, () => { it(`should add key fields to the parent selection set and use a dependent fetch`, () => { - const operationString = `#graphql + const operationString = `#graphql query { topReviews { body @@ -1171,4 +1173,49 @@ describe('buildQueryPlan', () => { `); }); }); + + describe('overridden fields and type', () => { + it(`query plan of overridden field`, () => { + const operationString = `#graphql + query { + library (id: "3") { + name + description + } + } + `; + + const queryPlan = buildPlan(operationString); + expect(queryPlan).toMatchInlineSnapshot(` + QueryPlan { + Sequence { + Fetch(service: "books") { + { + library(id: 3) { + __typename + id + name + } + } + }, + Flatten(path: "library") { + Fetch(service: "accounts") { + { + ... on Library { + __typename + id + } + } => + { + ... on Library { + description + } + } + }, + }, + }, + } + `); + }); + }); }); diff --git a/gateway-js/src/__tests__/gateway/lifecycle-hooks.test.ts b/gateway-js/src/__tests__/gateway/lifecycle-hooks.test.ts index 6598dc21f9..7d27a67207 100644 --- a/gateway-js/src/__tests__/gateway/lifecycle-hooks.test.ts +++ b/gateway-js/src/__tests__/gateway/lifecycle-hooks.test.ts @@ -147,7 +147,7 @@ describe('lifecycle hooks', () => { // the supergraph (even just formatting differences), this ID will change // and this test will have to updated. expect(secondCall[0]!.compositionId).toEqual( - '3a799fc2690d4e0e2766002f8c583b4b796d2e6732f12da9bfafab782fc01240', + '730a2fe4036db8e2c847096ba2a62f78ff8f3c08c9ee092a5b1b37e1aa00ef5a', ); // second call should have previous info in the second arg expect(secondCall[1]!.compositionId).toEqual(expectedFirstId); diff --git a/internals-js/CHANGELOG.md b/internals-js/CHANGELOG.md index a07856e45a..8e449271bf 100644 --- a/internals-js/CHANGELOG.md +++ b/internals-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. - Adds Support for `@tag/v0.2`, which allows the `@tag` directive to be additionally placed on arguments, scalars, enums, enum values, input objects, and input object fields. [PR #1652](https://github.com/apollographql/federation/pull/1652). - Add missing `includeDeprecated` argument for `args` and `inputFields` when defining introspection fields [PR #1584](https://github.com/apollographql/federation/pull/1584) +- Adds support for the `@override` directive on fields to indicate that a field should be moved from one subgraph to another. [PR #1484](https://github.com/apollographql/federation/pull/1484) ## v2.0.0-preview.8 diff --git a/internals-js/src/__tests__/subgraphValidation.test.ts b/internals-js/src/__tests__/subgraphValidation.test.ts index bef0814a7f..bdfc5480f2 100644 --- a/internals-js/src/__tests__/subgraphValidation.test.ts +++ b/internals-js/src/__tests__/subgraphValidation.test.ts @@ -549,7 +549,7 @@ describe('@core/@link handling', () => { directive @federation__provides(fields: federation__FieldSet!) on FIELD_DEFINITION - directive @federation__external on OBJECT | FIELD_DEFINITION + directive @federation__external(reason: String) on OBJECT | FIELD_DEFINITION directive @federation__tag(name: String!) repeatable on FIELD_DEFINITION | OBJECT | INTERFACE | UNION | ARGUMENT_DEFINITION | SCALAR | ENUM | ENUM_VALUE | INPUT_OBJECT | INPUT_FIELD_DEFINITION @@ -559,6 +559,8 @@ describe('@core/@link handling', () => { directive @federation__inaccessible on FIELD_DEFINITION | OBJECT | INTERFACE | UNION + directive @federation__override(from: String!) on FIELD_DEFINITION + type T @key(fields: "k") { @@ -678,7 +680,7 @@ describe('@core/@link handling', () => { k: ID! } - directive @federation__external on OBJECT | FIELD_DEFINITION + directive @federation__external(reason: String) on OBJECT | FIELD_DEFINITION `, ]; @@ -775,7 +777,7 @@ describe('@core/@link handling', () => { k: ID! } - directive @federation__external on OBJECT | FIELD_DEFINITION | SCHEMA + directive @federation__external(reason: String) on OBJECT | FIELD_DEFINITION | SCHEMA `; // @external is not allowed on 'schema' and likely never will. diff --git a/internals-js/src/definitions.ts b/internals-js/src/definitions.ts index 89287c0e2f..c148f4645a 100644 --- a/internals-js/src/definitions.ts +++ b/internals-js/src/definitions.ts @@ -384,7 +384,7 @@ export class DirectiveTargetElement> { } export function sourceASTs(...elts: ({ sourceAST?: ASTNode } | undefined)[]): ASTNode[] { - return elts.map(elt => elt?.sourceAST).filter(elt => elt !== undefined) as ASTNode[]; + return elts.map(elt => elt?.sourceAST).filter((elt): elt is ASTNode => elt !== undefined); } // Not exposed: mostly about avoid code duplication between SchemaElement and Directive (which is not a SchemaElement as it can't diff --git a/internals-js/src/error.ts b/internals-js/src/error.ts index d4d277c634..7151de174b 100644 --- a/internals-js/src/error.ts +++ b/internals-js/src/error.ts @@ -359,6 +359,21 @@ const SATISFIABILITY_ERROR = makeCodeDefinition( 'Subgraphs can be merged, but the resulting supergraph API would have queries that cannot be satisfied by those subgraphs.', ); +const OVERRIDE_FROM_SELF_ERROR = makeCodeDefinition( + 'OVERRIDE_FROM_SELF_ERROR', + 'Field with `@override` directive has "from" location that references its own subgraph.', +); + +const OVERRIDE_SOURCE_HAS_OVERRIDE = makeCodeDefinition( + 'OVERRIDE_SOURCE_HAS_OVERRIDE', + 'Field which is overridden to another subgraph is also marked @override.', +); + +const OVERRIDE_COLLISION_WITH_ANOTHER_DIRECTIVE = makeCodeDefinition( + 'OVERRIDE_COLLISION_WITH_ANOTHER_DIRECTIVE', + 'The @override directive cannot be used on external fields, nor to override fields with either @external, @provides, or @requires.', +); + export const ERROR_CATEGORIES = { DIRECTIVE_FIELDS_MISSING_EXTERNAL, DIRECTIVE_UNSUPPORTED_ON_INTERFACE, @@ -418,6 +433,9 @@ export const ERRORS = { REFERENCED_INACCESSIBLE, REQUIRED_ARGUMENT_MISSING_IN_SOME_SUBGRAPH, SATISFIABILITY_ERROR, + OVERRIDE_COLLISION_WITH_ANOTHER_DIRECTIVE, + OVERRIDE_FROM_SELF_ERROR, + OVERRIDE_SOURCE_HAS_OVERRIDE, }; const codeDefByCode = Object.values(ERRORS).reduce((obj: {[code: string]: ErrorCodeDefinition}, codeDef: ErrorCodeDefinition) => { obj[codeDef.code] = codeDef; return obj; }, {}); diff --git a/internals-js/src/extractSubgraphsFromSupergraph.ts b/internals-js/src/extractSubgraphsFromSupergraph.ts index 2e4e76c479..1532ead25a 100644 --- a/internals-js/src/extractSubgraphsFromSupergraph.ts +++ b/internals-js/src/extractSubgraphsFromSupergraph.ts @@ -181,6 +181,12 @@ export function extractSubgraphsFromSupergraph(supergraph: Schema): Subgraphs { if (args.external) { subgraphField.applyDirective(subgraph.metadata().externalDirective()); } + if (args.usedOverridden) { + subgraphField.applyDirective(subgraph.metadata().externalDirective(), {'reason': '[overridden]'}); + } + if (args.override) { + subgraphField.applyDirective(subgraph.metadata().overrideDirective(), {'from': args.override}); + } } } } diff --git a/internals-js/src/federation.ts b/internals-js/src/federation.ts index 1f1fe9914e..d4c539614b 100644 --- a/internals-js/src/federation.ts +++ b/internals-js/src/federation.ts @@ -52,7 +52,7 @@ import { ERRORS, withModifiedErrorMessage, } from "./error"; -import { computeShareables } from "./sharing"; +import { computeShareables } from "./precompute"; import { CoreSpecDefinition, FeatureVersion, @@ -71,6 +71,7 @@ import { externalDirectiveSpec, extendsDirectiveSpec, shareableDirectiveSpec, + overrideDirectiveSpec, FEDERATION2_SPEC_DIRECTIVES, ALL_FEDERATION_DIRECTIVES_DEFAULT_NAMES, FEDERATION2_ONLY_SPEC_DIRECTIVES, @@ -262,53 +263,49 @@ function validateAllFieldSet>( } } -export function collectUsedExternalFieldsCoordinates(metadata: FederationMetadata): Set { - const usedExternalCoordinates = new Set(); +export function collectUsedFields(metadata: FederationMetadata): Set> { + const usedFields = new Set>(); // Collects all external fields used by a key, requires or provides - collectUsedExternaFieldsForDirective( - metadata, + collectUsedFieldsForDirective( metadata.keyDirective(), type => type, - usedExternalCoordinates, + usedFields, ); - collectUsedExternaFieldsForDirective>( - metadata, + collectUsedFieldsForDirective>( metadata.requiresDirective(), field => field.parent!, - usedExternalCoordinates, + usedFields, ); - collectUsedExternaFieldsForDirective>( - metadata, + collectUsedFieldsForDirective>( metadata.providesDirective(), field => { const type = baseType(field.type!); return isCompositeType(type) ? type : undefined; }, - usedExternalCoordinates, + usedFields, ); - // Collects all external fields used to satisfy an interface constraint + // Collects all fields used to satisfy an interface constraint for (const itfType of metadata.schema.types('InterfaceType')) { const runtimeTypes = itfType.possibleRuntimeTypes(); for (const field of itfType.fields()) { for (const runtimeType of runtimeTypes) { const implemField = runtimeType.field(field.name); - if (implemField && metadata.isFieldExternal(implemField)) { - usedExternalCoordinates.add(implemField.coordinate); + if (implemField) { + usedFields.add(implemField); } } } } - return usedExternalCoordinates; + return usedFields; } -function collectUsedExternaFieldsForDirective>( - metadata: FederationMetadata, +function collectUsedFieldsForDirective>( definition: DirectiveDefinition<{fields: any}>, targetTypeExtractor: (element: TParent) => CompositeType | undefined, - usedExternalCoordinates: Set + usedFieldDefs: Set> ) { for (const application of definition.applications()) { const type = targetTypeExtractor(application.parent! as TParent); @@ -326,8 +323,7 @@ function collectUsedExternaFieldsForDirective, includeInterfaceFieldsImplementations: true, validate: false, - }).filter((field) => metadata.isFieldExternal(field)) - .forEach((field) => usedExternalCoordinates.add(field.coordinate)); + }).forEach((field) => usedFieldDefs.add(field)); } } @@ -336,23 +332,20 @@ function collectUsedExternaFieldsForDirective): boolean { - return field.parent.interfaces().some(itf => itf.field(field.name)); -} - /** * Register errors when, for an interface field, some of the implementations of that field are @external * _and_ not all of those field implementation have the same type (which otherwise allowed because field @@ -425,6 +414,7 @@ function formatFieldsToReturnType([type, implems]: [string, FieldDefinition) => boolean; + private _fieldUsedPredicate?: (field: FieldDefinition) => boolean; private _isFed2Schema?: boolean; constructor(readonly schema: Schema) { @@ -434,6 +424,7 @@ export class FederationMetadata { this._externalTester = undefined; this._sharingPredicate = undefined; this._isFed2Schema = undefined; + this._fieldUsedPredicate = undefined; } isFed2Schema(): boolean { @@ -462,6 +453,18 @@ export class FederationMetadata { return this._sharingPredicate; } + private fieldUsedPredicate(): (field: FieldDefinition) => boolean { + if (!this._fieldUsedPredicate) { + const usedFields = collectUsedFields(this); + this._fieldUsedPredicate = (field: FieldDefinition) => !!usedFields.has(field); + } + return this._fieldUsedPredicate; + } + + isFieldUsed(field: FieldDefinition): boolean { + return this.fieldUsedPredicate()(field); + } + isFieldExternal(field: FieldDefinition | InputFieldDefinition) { return this.externalTester().isExternal(field); } @@ -533,11 +536,15 @@ export class FederationMetadata { return this.getFederationDirective(keyDirectiveSpec.name); } + overrideDirective(): DirectiveDefinition<{from: string}> { + return this.getFederationDirective(overrideDirectiveSpec.name); + } + extendsDirective(): DirectiveDefinition> { return this.getFederationDirective(extendsDirectiveSpec.name); } - externalDirective(): DirectiveDefinition> { + externalDirective(): DirectiveDefinition<{reason: string}> { return this.getFederationDirective(externalDirectiveSpec.name); } @@ -562,7 +569,7 @@ export class FederationMetadata { } allFederationDirectives(): DirectiveDefinition[] { - const baseDirectives = [ + const baseDirectives: DirectiveDefinition[] = [ this.keyDirective(), this.externalDirective(), this.requiresDirective(), @@ -571,7 +578,7 @@ export class FederationMetadata { this.extendsDirective(), ]; return this.isFed2Schema() - ? baseDirectives.concat(this.shareableDirective(), this.inaccessibleDirective()) + ? baseDirectives.concat(this.shareableDirective(), this.inaccessibleDirective(), this.overrideDirective()) : baseDirectives; } @@ -804,7 +811,7 @@ export class FederationBlueprint extends SchemaBlueprint { } } else { return withModifiedErrorMessage( - error, + error, `${error.message} If you meant the "@${unknownDirectiveName}" federation 2 directive, note that this schema is a federation 1 schema. To be a federation 2 schema, it needs to @link to the federation specifcation v2.` ); } @@ -813,7 +820,7 @@ export class FederationBlueprint extends SchemaBlueprint { const suggestions = suggestionList(unknownDirectiveName, FEDERATION2_ONLY_SPEC_DIRECTIVES.map((spec) => spec.name)); if (suggestions.length > 0) { return withModifiedErrorMessage( - error, + error, `${error.message}${didYouMean(suggestions.map((s) => '@' + s))} If so, note that ${suggestions.length === 1 ? 'it is a federation 2 directive' : 'they are federation 2 directives'} but this schema is a federation 1 one. To be a federation 2 schema, it needs to @link to the federation specifcation v2.` ); } @@ -876,7 +883,7 @@ export function setSchemaAsFed2Subgraph(schema: Schema) { // This is the full @link declaration as added by `asFed2SubgraphDocument`. It's here primarily for uses by tests that print and match // subgraph schema to avoid having to update 20+ tests every time we use a new directive or the order of import changes ... -export const FEDERATION2_LINK_WTH_FULL_IMPORTS = '@link(url: "https://specs.apollo.dev/federation/v2.0", import: ["@key", "@requires", "@provides", "@external", "@tag", "@extends", "@shareable", "@inaccessible"])'; +export const FEDERATION2_LINK_WTH_FULL_IMPORTS = '@link(url: "https://specs.apollo.dev/federation/v2.0", import: ["@key", "@requires", "@provides", "@external", "@tag", "@extends", "@shareable", "@inaccessible", "@override"])'; export function asFed2SubgraphDocument(document: DocumentNode): DocumentNode { const fed2LinkExtension: SchemaExtensionNode = { diff --git a/internals-js/src/federationSpec.ts b/internals-js/src/federationSpec.ts index 98be4039cf..10dfa70859 100644 --- a/internals-js/src/federationSpec.ts +++ b/internals-js/src/federationSpec.ts @@ -41,6 +41,10 @@ export const extendsDirectiveSpec = createDirectiveSpecification({ export const externalDirectiveSpec = createDirectiveSpecification({ name:'external', locations: [DirectiveLocation.OBJECT, DirectiveLocation.FIELD_DEFINITION], + argumentFct: (schema) => ({ + args: [{ name: 'reason', type: schema.stringType() }], + errors: [], + }), }); export const requiresDirectiveSpec = createDirectiveSpecification({ @@ -66,6 +70,15 @@ export const shareableDirectiveSpec = createDirectiveSpecification({ locations: [DirectiveLocation.OBJECT, DirectiveLocation.FIELD_DEFINITION], }); +export const overrideDirectiveSpec = createDirectiveSpecification({ + name: 'override', + locations: [DirectiveLocation.FIELD_DEFINITION], + argumentFct: (schema) => ({ + args: [{ name: 'from', type: new NonNullType(schema.stringType()) }], + errors: [], + }), +}); + function fieldsArgument(schema: Schema): ArgumentSpecification { return { name: 'fields', type: fieldSetType(schema) }; } @@ -79,6 +92,7 @@ function fieldSetType(schema: Schema): InputType { export const FEDERATION2_ONLY_SPEC_DIRECTIVES = [ shareableDirectiveSpec, inaccessibleDirectiveSpec, + overrideDirectiveSpec, ]; // Note that this is only used for federation 2+ (federation 1 adds the same directive, but not through a core spec). diff --git a/internals-js/src/index.ts b/internals-js/src/index.ts index 42d1b518e1..7ff400d4d7 100644 --- a/internals-js/src/index.ts +++ b/internals-js/src/index.ts @@ -16,3 +16,4 @@ export * from './supergraphs'; export * from './extractSubgraphsFromSupergraph'; export * from './error'; export * from './schemaUpgrader'; +export * from './suggestions'; diff --git a/internals-js/src/joinSpec.ts b/internals-js/src/joinSpec.ts index e47dc6e17e..934a125f3d 100644 --- a/internals-js/src/joinSpec.ts +++ b/internals-js/src/joinSpec.ts @@ -74,6 +74,8 @@ export class JoinSpecDefinition extends FeatureDefinition { if (!this.isV01()) { joinField.addArgument('type', schema.stringType()); joinField.addArgument('external', schema.booleanType()); + joinField.addArgument('override', schema.stringType()); + joinField.addArgument('usedOverridden', schema.booleanType()); } if (!this.isV01()) { @@ -159,7 +161,15 @@ export class JoinSpecDefinition extends FeatureDefinition { return this.directive(schema, 'implements'); } - fieldDirective(schema: Schema): DirectiveDefinition<{graph: string, requires?: string, provides?: string, type?: string, external?: boolean}> { + fieldDirective(schema: Schema): DirectiveDefinition<{ + graph: string, + requires?: string, + provides?: string, + override?: string, + type?: string, + external?: boolean, + usedOverridden?: boolean, + }> { return this.directive(schema, 'field')!; } diff --git a/internals-js/src/operations.ts b/internals-js/src/operations.ts index a9a5637d91..39d770a4a8 100644 --- a/internals-js/src/operations.ts +++ b/internals-js/src/operations.ts @@ -862,6 +862,21 @@ export class SelectionSet { } } +export function allFieldDefinitionsInSelectionSet(selection: SelectionSet): FieldDefinition[] { + const stack = Array.from(selection.selections()); + const allFields: FieldDefinition[] = []; + while (stack.length > 0) { + const selection = stack.pop()!; + if (selection.kind === 'FieldSelection') { + allFields.push(selection.field.definition); + } + if (selection.selectionSet) { + stack.push(...selection.selectionSet.selections()); + } + } + return allFields; +} + export function selectionSetOfElement(element: OperationElement, subSelection?: SelectionSet): SelectionSet { const selectionSet = new SelectionSet(element.parentType); selectionSet.add(selectionOfElement(element, subSelection)); diff --git a/internals-js/src/sharing.ts b/internals-js/src/precompute.ts similarity index 99% rename from internals-js/src/sharing.ts rename to internals-js/src/precompute.ts index 780c9684a2..8a7e511d09 100644 --- a/internals-js/src/sharing.ts +++ b/internals-js/src/precompute.ts @@ -7,7 +7,7 @@ import { collectTargetFields, InterfaceType, ObjectType, - Schema + Schema, } from "."; export function computeShareables(schema: Schema): (field: FieldDefinition) => boolean { @@ -65,4 +65,3 @@ export function computeShareables(schema: Schema): (field: FieldDefinition shareableFields.has(field.coordinate); } - diff --git a/internals-js/src/schemaUpgrader.ts b/internals-js/src/schemaUpgrader.ts index d09687281c..f5c32d830d 100644 --- a/internals-js/src/schemaUpgrader.ts +++ b/internals-js/src/schemaUpgrader.ts @@ -24,7 +24,6 @@ import { import { addSubgraphToError, collectTargetFields, - collectUsedExternalFieldsCoordinates, federationMetadata, FederationMetadata, printSubgraphNames, @@ -43,7 +42,7 @@ type UpgradeChanges = MultiMap; export type UpgradeSuccess = { subgraphs: Subgraphs, changes: Map, - errors?: never, + errors?: never, } export type UpgradeFailure = { @@ -235,7 +234,7 @@ export function upgradeSubgraphsIfNecessary(inputs: Subgraphs): UpgradeResult { * 2. do not have a definition for the same type in the same subgraph (this is a GraphQL extension otherwise). * * Not that type extensions in federation 1 generally have a @key but in really the code consider something a type extension even without - * it (which I'd argue is a unintended bug of fed1 since this leads to various problems) so we don't check for the presence of @key here. + * it (which I'd argue is a unintended bug of fed1 since this leads to various problems) so we don't check for the presence of @key here. */ function isFederationTypeExtension(type: NamedType): boolean { const metadata = federationMetadata(type.schema()); @@ -522,7 +521,7 @@ class SchemaUpgrader { continue; } assert(isCompositeType(typeInOther), () => `Type ${type} is of kind ${type.kind} in ${this.subgraph.name} but ${typeInOther.kind} in ${other.name}`); - const keysInOther = typeInOther.appliedDirectivesOf(other.metadata().keyDirective()); + const keysInOther = typeInOther.appliedDirectivesOf(other.metadata().keyDirective()); if (keysInOther.length === 0) { continue; } @@ -564,14 +563,12 @@ class SchemaUpgrader { } private removeUnusedExternals() { - const usedExternalFieldsCoordinates = collectUsedExternalFieldsCoordinates(this.metadata); - for (const type of this.schema.types()) { if (!isObjectType(type) && !isInterfaceType(type)) { continue; } for (const field of type.fields()) { - if (this.metadata.isFieldExternal(field) && !usedExternalFieldsCoordinates.has(field.coordinate)) { + if (this.metadata.isFieldExternal(field) && !this.metadata.isFieldUsed(field)) { this.addChange(new UnusedExternalRemoval(field.coordinate)); field.remove(); } diff --git a/internals-js/src/suggestions.ts b/internals-js/src/suggestions.ts index b02840acff..9eb901819c 100644 --- a/internals-js/src/suggestions.ts +++ b/internals-js/src/suggestions.ts @@ -5,7 +5,7 @@ import { mapKeys } from './utils'; * Given an invalid input string and a list of valid options, returns a filtered * list of valid options sorted based on their similarity with the input. */ -export function suggestionList(input: string, options: string[]): string[] { +export function suggestionList(input: string, options: readonly string[]): string[] { const optionsByDistance = new Map(); const threshold = Math.floor(input.length * 0.4) + 1; diff --git a/query-graphs-js/src/graphPath.ts b/query-graphs-js/src/graphPath.ts index 5aca54112c..1a2dfb2e57 100644 --- a/query-graphs-js/src/graphPath.ts +++ b/query-graphs-js/src/graphPath.ts @@ -23,9 +23,12 @@ import { federationMetadata, isSchemaRootType, Directive, + FieldDefinition, + printSubgraphNames, + allFieldDefinitionsInSelectionSet, } from "@apollo/federation-internals"; import { OpPathTree, traversePathTree } from "./pathTree"; -import { Vertex, QueryGraph, Edge, RootVertex, isRootVertex, isFederatedGraphRootType } from "./querygraph"; +import { Vertex, QueryGraph, Edge, RootVertex, isRootVertex, isFederatedGraphRootType, FEDERATED_GRAPH_ROOT_SOURCE } from "./querygraph"; import { Transition } from "./transition"; import { PathContext, emptyContext } from "./pathContext"; @@ -1146,11 +1149,17 @@ function advancePathWithNonCollectingAndTypePreservingTransitions { + // The subtlety here is that the definition of the fields in the condition are not the one of the subgraph we care + // about here in general, because the conditions on key edge are those of the destination of the edge, and here + // we want to check if the field is overridden in the source of the edge. Hence us getting the matching + // definition in the input schema. + const typeInSource = schema.type(field.parent.name) + const fieldInSource = typeInSource && isObjectType(typeInSource) && typeInSource.field(field.name); + return fieldInSource && fieldInSource.appliedDirectivesOf(externalDirective)?.pop()?.arguments().reason === '[overridden]'; + }); +} + function hasValidDirectKeyEdge( graph: QueryGraph, from: Vertex, @@ -1238,11 +1260,22 @@ function advancePathWithDirectTransition( if (fieldInSubgraph) { // the subgraph has the field but no correspond edge. This should only happen if the field is external. + const externalDirective = fieldInSubgraph.appliedDirectivesOf(federationMetadata(fieldInSubgraph.schema())!.externalDirective()).pop(); assert( - fieldInSubgraph.hasAppliedDirective('external'), + externalDirective, () => `${fieldInSubgraph.coordinate} in ${subgraph} is not external but there is no corresponding edge (edges from ${path} = [${path.nextEdges().join(', ')}])` ); - details = `field "${transition.definition.coordinate}" is not resolvable because marked @external`; + // but the field is external in the "subgraph-extracted-from-the-supergraph", but it might have been forced to an external + // due to being a used-overriden field, in which case we want to amend the message to avoid confusing the user. + // Note that the subgraph extraction marks such "forced external due to being overriden" by setting the "reason" to "[overridden]". + const overriddingSources = externalDirective.arguments().reason === '[overridden]' + ? findOverriddingSourcesIfOverridden(fieldInSubgraph, subgraph, path.graph.sources) + : []; + if (overriddingSources.length > 0) { + details = `field "${transition.definition.coordinate}" is not resolvable because it is overridden by ${printSubgraphNames(overriddingSources)}`; + } else { + details = `field "${transition.definition.coordinate}" is not resolvable because marked @external`; + } } else { details = `cannot find field "${transition.definition.coordinate}"`; } @@ -1259,6 +1292,28 @@ function advancePathWithDirectTransition( } } +function findOverriddingSourcesIfOverridden( + field: FieldDefinition, + fieldSource: string, + sources: ReadonlyMap, +): string[] { + return [...sources.entries()] + .map(([name, schema]) => { + if (name === FEDERATED_GRAPH_ROOT_SOURCE || name === fieldSource) { + return undefined; + } + const sourceMetadata = federationMetadata(schema)!; + const typeInSource = schema.type(field.parent.name); + if (!typeInSource || !isObjectType(typeInSource)) { + return undefined; + } + const fieldInSource = typeInSource.field(field.name); + const isOverriddingSource = fieldInSource?.appliedDirectivesOf(sourceMetadata.overrideDirective())?.pop()?.arguments()?.from === fieldSource; + return isOverriddingSource ? name : undefined; + }) + .filter((name) => !!name) as string[]; +} + function warnOnKeyFieldsMarkedExternal(type: CompositeType): string { // Because fed 1 used to (somewhat wrongly) require @external on key fields of type extension and because fed 2 allows you // to avoid type extensions, users upgrading might try to remove `extend` from their schema, but forgot to remove the @external diff --git a/query-graphs-js/src/querygraph.ts b/query-graphs-js/src/querygraph.ts index 1aaeb38583..77752d1d8b 100644 --- a/query-graphs-js/src/querygraph.ts +++ b/query-graphs-js/src/querygraph.ts @@ -40,7 +40,7 @@ import { preComputeNonTrivialFollowupEdges } from './nonTrivialEdgePrecomputing' // We use our federation reserved subgraph name to avoid risk of conflict with other subgraph names (wouldn't be a huge // deal, but safer that way). Using something short like `_` is also on purpose: it makes it stand out in debug messages // without taking space. -const FEDERATED_GRAPH_ROOT_SOURCE = FEDERATION_RESERVED_SUBGRAPH_NAME; +export const FEDERATED_GRAPH_ROOT_SOURCE = FEDERATION_RESERVED_SUBGRAPH_NAME; const FEDERATED_GRAPH_ROOT_SCHEMA = new Schema(); export function federatedGraphRootTypeName(rootKind: SchemaRootKind): string { diff --git a/query-planner-js/CHANGELOG.md b/query-planner-js/CHANGELOG.md index 107497c12c..06aaab8add 100644 --- a/query-planner-js/CHANGELOG.md +++ b/query-planner-js/CHANGELOG.md @@ -6,6 +6,7 @@ This CHANGELOG pertains only to Apollo Federation packages in the 2.x range. The > 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. - Adds Support for `@tag/v0.2`, which allows the `@tag` directive to be additionally placed on arguments, scalars, enums, enum values, input objects, and input object fields. [PR #1652](https://github.com/apollographql/federation/pull/1652). +- Adds support for the `@override` directive on fields to indicate that a field should be moved from one subgraph to another. [PR #1484](https://github.com/apollographql/federation/pull/1484) ## v2.0.0-preview.8 diff --git a/subgraph-js/CHANGELOG.md b/subgraph-js/CHANGELOG.md index 3a21712a57..c12ea5b85c 100644 --- a/subgraph-js/CHANGELOG.md +++ b/subgraph-js/CHANGELOG.md @@ -6,6 +6,7 @@ This CHANGELOG pertains only to Apollo Federation packages in the 2.x range. The > 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. - Adds Support for `@tag/v0.2`, which allows the `@tag` directive to be additionally placed on arguments, scalars, enums, enum values, input objects, and input object fields. [PR #1652](https://github.com/apollographql/federation/pull/1652). +- Adds support for the `@override` directive on fields to indicate that a field should be moved from one subgraph to another. [PR #1484](https://github.com/apollographql/federation/pull/1484) ## v2.0.0-preview.8 diff --git a/subgraph-js/src/__tests__/printSubgraphSchema.test.ts b/subgraph-js/src/__tests__/printSubgraphSchema.test.ts index 4141d0df3a..edc4c5a2a6 100644 --- a/subgraph-js/src/__tests__/printSubgraphSchema.test.ts +++ b/subgraph-js/src/__tests__/printSubgraphSchema.test.ts @@ -12,7 +12,7 @@ describe('printSubgraphSchema', () => { mutation: Mutation } - extend schema @link(url: \\"https://specs.apollo.dev/federation/v2.0\\", import: [\\"@key\\", \\"@requires\\", \\"@provides\\", \\"@external\\", \\"@tag\\", \\"@extends\\", \\"@shareable\\", \\"@inaccessible\\"]) + extend schema @link(url: \\"https://specs.apollo.dev/federation/v2.0\\", import: [\\"@key\\", \\"@requires\\", \\"@provides\\", \\"@external\\", \\"@tag\\", \\"@extends\\", \\"@shareable\\", \\"@inaccessible\\", \\"@override\\"]) directive @stream on FIELD @@ -73,6 +73,7 @@ describe('printSubgraphSchema', () => { id: ID! name: String @external userAccount(id: ID! = 1): User @requires(fields: \\"name\\") + description: String @override(from: \\"books\\") } " `); @@ -97,7 +98,7 @@ describe('printSubgraphSchema', () => { it('prints reviews subgraph correctly', () => { const schema = buildSubgraphSchema(fixtures[5].typeDefs); expect(printSubgraphSchema(schema)).toMatchInlineSnapshot(` - "extend schema @link(url: \\"https://specs.apollo.dev/federation/v2.0\\", import: [\\"@key\\", \\"@requires\\", \\"@provides\\", \\"@external\\", \\"@tag\\", \\"@extends\\", \\"@shareable\\", \\"@inaccessible\\"]) + "extend schema @link(url: \\"https://specs.apollo.dev/federation/v2.0\\", import: [\\"@key\\", \\"@requires\\", \\"@provides\\", \\"@external\\", \\"@tag\\", \\"@extends\\", \\"@shareable\\", \\"@inaccessible\\", \\"@override\\"]) directive @stream on FIELD diff --git a/subgraph-js/src/directives.ts b/subgraph-js/src/directives.ts index 663e3e31a8..b20c64149d 100644 --- a/subgraph-js/src/directives.ts +++ b/subgraph-js/src/directives.ts @@ -120,6 +120,16 @@ export const InaccessibleDirective = new GraphQLDirective({ ], }); +export const OverrideDirective = new GraphQLDirective({ + name: 'override', + locations: [DirectiveLocation.FIELD_DEFINITION], + args: { + from: { + type: new GraphQLNonNull(GraphQLString), + }, + }, +}); + export const federationDirectives = [ KeyDirective, ExtendsDirective, @@ -130,6 +140,7 @@ export const federationDirectives = [ LinkDirective, TagDirective, InaccessibleDirective, + OverrideDirective, ]; export function isFederationDirective(directive: GraphQLDirective): boolean {