From 2d02a8590023efcc8a98bf88a49d73b030cf8c26 Mon Sep 17 00:00:00 2001 From: Chris Lenfest Date: Tue, 6 Feb 2024 17:11:55 -0600 Subject: [PATCH 1/3] Do not auto-upgrade federation subgraphs past fed 2.6. This is so that we don't inadvertently require a router that supports the latest join spec when it's not needed. --- .changeset/lemon-yaks-think.md | 6 +++ .../src/__tests__/schemaUpgrader.test.ts | 8 ++-- internals-js/src/federation.ts | 47 ++++++++++++++----- query-planner-js/src/__tests__/testHelper.ts | 14 ++++-- 4 files changed, 53 insertions(+), 22 deletions(-) create mode 100644 .changeset/lemon-yaks-think.md diff --git a/.changeset/lemon-yaks-think.md b/.changeset/lemon-yaks-think.md new file mode 100644 index 000000000..e55755377 --- /dev/null +++ b/.changeset/lemon-yaks-think.md @@ -0,0 +1,6 @@ +--- +"@apollo/query-planner": patch +"@apollo/federation-internals": patch +--- + +When auto-upgrading a subgraph (i.e. one that does not explicitly @link the federation spec) do not go past v2.6. This is so that subgraphs will not inadvertenly require the latest join spec (which cause the router or gateway not to start if running an older version). diff --git a/internals-js/src/__tests__/schemaUpgrader.test.ts b/internals-js/src/__tests__/schemaUpgrader.test.ts index 44c27abf6..de5dcd2bc 100644 --- a/internals-js/src/__tests__/schemaUpgrader.test.ts +++ b/internals-js/src/__tests__/schemaUpgrader.test.ts @@ -1,4 +1,4 @@ -import { FEDERATION2_LINK_WITH_AUTO_EXPANDED_IMPORTS, printSchema } from '..'; +import { FEDERATION2_LINK_WITH_AUTO_EXPANDED_IMPORTS_UPGRADED, printSchema } from '..'; import { ObjectType } from '../definitions'; import { buildSubgraph, Subgraphs } from '../federation'; import { UpgradeChangeID, UpgradeResult, upgradeSubgraphsIfNecessary } from '../schemaUpgrader'; @@ -92,7 +92,7 @@ test('upgrade complex schema', () => { expect(res.subgraphs?.get('s1')?.toString()).toMatchString(` schema - ${FEDERATION2_LINK_WITH_AUTO_EXPANDED_IMPORTS} + ${FEDERATION2_LINK_WITH_AUTO_EXPANDED_IMPORTS_UPGRADED} { query: Query } @@ -148,7 +148,7 @@ test('update federation directive non-string arguments', () => { expect(res.subgraphs?.get('s')?.toString()).toMatchString(` schema - ${FEDERATION2_LINK_WITH_AUTO_EXPANDED_IMPORTS} + ${FEDERATION2_LINK_WITH_AUTO_EXPANDED_IMPORTS_UPGRADED} { query: Query } @@ -320,7 +320,7 @@ test("fully upgrades a schema with no @link directive", () => { expect(printSchema(result.subgraphs!.get("subgraph")!.schema!)).toContain( `schema @link(url: "https://specs.apollo.dev/link/v1.0") - @link(url: "https://specs.apollo.dev/federation/v2.7", import: ["@key", "@requires", "@provides", "@external", "@tag", "@extends", "@shareable", "@inaccessible", "@override", "@composeDirective", "@interfaceObject"]) + @link(url: "https://specs.apollo.dev/federation/v2.6", import: ["@key", "@requires", "@provides", "@external", "@tag", "@extends", "@shareable", "@inaccessible", "@override", "@composeDirective", "@interfaceObject"]) { query: Query }` diff --git a/internals-js/src/federation.ts b/internals-js/src/federation.ts index d9541a3cb..e856fdb63 100644 --- a/internals-js/src/federation.ts +++ b/internals-js/src/federation.ts @@ -79,6 +79,7 @@ import { FederationTypeName, FEDERATION1_TYPES, FEDERATION1_DIRECTIVES, + FederationSpecDefinition, } from "./specs/federationSpec"; import { defaultPrintOptions, PrintOptions as PrintOptions, printSchema } from "./print"; import { createObjectTypeSpecification, createScalarTypeSpecification, createUnionTypeSpecification } from "./directiveAndTypeSpecification"; @@ -93,10 +94,21 @@ import { const linkSpec = LINK_VERSIONS.latest(); const tagSpec = TAG_VERSIONS.latest(); -const federationSpec = FEDERATION_VERSIONS.latest(); +const federationSpec = (version?: FeatureVersion): FederationSpecDefinition => { + if (!version) return FEDERATION_VERSIONS.latest(); + const spec = FEDERATION_VERSIONS.find(version); + assert(spec, `Federation spec version ${version} is not known`); + return spec; +}; + // Some users rely on auto-expanding fed v1 graphs with fed v2 directives. While technically we should only expand @tag // directive from v2 definitions, we will continue expanding other directives (up to v2.4) to ensure backwards compatibility. -const autoExpandedFederationSpec = FEDERATION_VERSIONS.find(new FeatureVersion(2, 4))!; +const autoExpandedFederationSpec = federationSpec(new FeatureVersion(2, 4)); + +// For subgraphs that don't explicitly specify a federation version, we need a version number to link in the upgraded schema. +const autoUpgradedFederationSpec = federationSpec(new FeatureVersion(2, 6)); + +const latestFederationSpec = federationSpec(); // We don't let user use this as a subgraph name. That allows us to use it in `query graphs` to name the source of roots // in the "federated query graph" without worrying about conflict (see `FEDERATED_GRAPH_ROOT_SOURCE` in `querygraph.ts`). @@ -601,7 +613,7 @@ export class FederationMetadata { } federationFeature(): CoreFeature | undefined { - return this.schema.coreFeatures?.getByIdentity(federationSpec.identity); + return this.schema.coreFeatures?.getByIdentity(latestFederationSpec.identity); } private externalTester(): ExternalTester { @@ -663,7 +675,7 @@ export class FederationMetadata { if (this.isFed2Schema()) { const coreFeatures = this.schema.coreFeatures; assert(coreFeatures, 'Schema should be a core schema'); - const federationFeature = coreFeatures.getByIdentity(federationSpec.identity); + const federationFeature = coreFeatures.getByIdentity(latestFederationSpec.identity); assert(federationFeature, 'Schema should have the federation feature'); return federationFeature.directiveNameInSchema(name); } else { @@ -685,7 +697,7 @@ export class FederationMetadata { if (this.isFed2Schema()) { const coreFeatures = this.schema.coreFeatures; assert(coreFeatures, 'Schema should be a core schema'); - const federationFeature = coreFeatures.getByIdentity(federationSpec.identity); + const federationFeature = coreFeatures.getByIdentity(latestFederationSpec.identity); assert(federationFeature, 'Schema should have the federation feature'); return federationFeature.typeNameInSchema(name); } else { @@ -1190,7 +1202,7 @@ function findUnusedNamedForLinkDirective(schema: Schema): string | undefined { } } -export function setSchemaAsFed2Subgraph(schema: Schema) { +export function setSchemaAsFed2Subgraph(schema: Schema, useLatest: boolean = false) { let core = schema.coreFeatures; let spec: CoreSpecDefinition; if (core) { @@ -1209,11 +1221,16 @@ export function setSchemaAsFed2Subgraph(schema: Schema) { assert(core, 'Schema should now be a core schema'); } - assert(!core.getByIdentity(federationSpec.identity), 'Schema already set as a federation subgraph'); + const fedSpec = useLatest ? latestFederationSpec : autoUpgradedFederationSpec; + + assert(!core.getByIdentity(fedSpec.identity), 'Schema already set as a federation subgraph'); schema.schemaDefinition.applyDirective( core.coreItself.nameInSchema, { - url: federationSpec.url.toString(), + // note that there is a mismatch between url and directives that are imported. This is because + // we want to maintain backward compatibility for those who have already upgraded and we had been upgrading the url to + // latest, but we never automatically import directives that exist past 2.4 + url: fedSpec.url.toString(), import: autoExpandedFederationSpec.directiveSpecs().map((spec) => `@${spec.name}`), } ); @@ -1226,21 +1243,25 @@ 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_WITH_FULL_IMPORTS = '@link(url: "https://specs.apollo.dev/federation/v2.7", import: ["@key", "@requires", "@provides", "@external", "@tag", "@extends", "@shareable", "@inaccessible", "@override", "@composeDirective", "@interfaceObject", "@authenticated", "@requiresScopes", "@policy", "@sourceAPI", "@sourceType", "@sourceField"])'; -// This is the full @link declaration that is added when upgrading fed v1 subgraphs to v2 version. It should only be used by tests. + +// This is the federation @link for tests that go through the asFed2SubgraphDocument function. export const FEDERATION2_LINK_WITH_AUTO_EXPANDED_IMPORTS = '@link(url: "https://specs.apollo.dev/federation/v2.7", import: ["@key", "@requires", "@provides", "@external", "@tag", "@extends", "@shareable", "@inaccessible", "@override", "@composeDirective", "@interfaceObject"])'; +// This is the federation @link for tests that go through the SchemaUpgrader. +export const FEDERATION2_LINK_WITH_AUTO_EXPANDED_IMPORTS_UPGRADED = '@link(url: "https://specs.apollo.dev/federation/v2.6", import: ["@key", "@requires", "@provides", "@external", "@tag", "@extends", "@shareable", "@inaccessible", "@override", "@composeDirective", "@interfaceObject"])'; + /** * Given a document that is assumed to _not_ be a fed2 schema (it does not have a `@link` to the federation spec), * returns an equivalent document that `@link` to the last known federation spec. * * @param document - the document to "augment". - * @param options.addAsSchemaExtension - defines whethere the added `@link` is added as a schema extension (`extend schema`) or + * @param options.addAsSchemaExtension - defines whether the added `@link` is added as a schema extension (`extend schema`) or * added to the schema definition. Defaults to `true` (added as an extension), as this mimics what we tends to write manually. * @param options.includeAllImports - defines whether we should auto import ALL latest federation v2 directive definitions or include * only limited set of directives (i.e. federation v2.4 definitions) */ export function asFed2SubgraphDocument(document: DocumentNode, options?: { addAsSchemaExtension?: boolean, includeAllImports?: boolean }): DocumentNode { - const importedDirectives = options?.includeAllImports ? federationSpec.directiveSpecs() : autoExpandedFederationSpec.directiveSpecs(); + const importedDirectives = options?.includeAllImports ? latestFederationSpec.directiveSpecs() : autoExpandedFederationSpec.directiveSpecs(); const directiveToAdd: ConstDirectiveNode = ({ kind: Kind.DIRECTIVE, name: { kind: Kind.NAME, value: linkDirectiveDefaultName }, @@ -1248,7 +1269,7 @@ export function asFed2SubgraphDocument(document: DocumentNode, options?: { addAs { kind: Kind.ARGUMENT, name: { kind: Kind.NAME, value: 'url' }, - value: { kind: Kind.STRING, value: federationSpec.url.toString() } + value: { kind: Kind.STRING, value: latestFederationSpec.url.toString() } }, { kind: Kind.ARGUMENT, @@ -1374,7 +1395,7 @@ export function buildSubgraph( export function newEmptyFederation2Schema(config?: SchemaConfig): Schema { const schema = new Schema(new FederationBlueprint(true), config); - setSchemaAsFed2Subgraph(schema); + setSchemaAsFed2Subgraph(schema, true); return schema; } diff --git a/query-planner-js/src/__tests__/testHelper.ts b/query-planner-js/src/__tests__/testHelper.ts index 390ada0aa..9974477a5 100644 --- a/query-planner-js/src/__tests__/testHelper.ts +++ b/query-planner-js/src/__tests__/testHelper.ts @@ -6,13 +6,17 @@ expect.addSnapshotSerializer(astSerializer); expect.addSnapshotSerializer(queryPlanSerializer); export function composeAndCreatePlanner(...services: ServiceDefinition[]): [Schema, QueryPlanner] { - return composeAndCreatePlannerWithOptions(services, {}); + return composeAndCreatePlannerWithOptions(services, {}, false); } -export function composeAndCreatePlannerWithOptions(services: ServiceDefinition[], config: QueryPlannerConfig): [Schema, QueryPlanner] { - const compositionResults = composeServices( - services.map((s) => ({ ...s, typeDefs: asFed2SubgraphDocument(s.typeDefs) })) - ); +export function composeFed2SubgraphsAndCreatePlanner(...services: ServiceDefinition[]): [Schema, QueryPlanner] { + return composeAndCreatePlannerWithOptions(services, {}, true); +} + +export function composeAndCreatePlannerWithOptions(services: ServiceDefinition[], config: QueryPlannerConfig, isFed2Subgraph: boolean = false): [Schema, QueryPlanner] { + const updatedServices = isFed2Subgraph ? services : services.map((s) => ({ ...s, typeDefs: asFed2SubgraphDocument(s.typeDefs) })); + + const compositionResults = composeServices(updatedServices); expect(compositionResults.errors).toBeUndefined(); return [ compositionResults.schema!.toAPISchema(), From 883c977b02413f6cbb5c888808602e74a28dd580 Mon Sep 17 00:00:00 2001 From: Chris Lenfest Date: Tue, 6 Feb 2024 17:14:33 -0600 Subject: [PATCH 2/3] spelling --- .changeset/lemon-yaks-think.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/lemon-yaks-think.md b/.changeset/lemon-yaks-think.md index e55755377..486430600 100644 --- a/.changeset/lemon-yaks-think.md +++ b/.changeset/lemon-yaks-think.md @@ -3,4 +3,4 @@ "@apollo/federation-internals": patch --- -When auto-upgrading a subgraph (i.e. one that does not explicitly @link the federation spec) do not go past v2.6. This is so that subgraphs will not inadvertenly require the latest join spec (which cause the router or gateway not to start if running an older version). +When auto-upgrading a subgraph (i.e. one that does not explicitly @link the federation spec) do not go past v2.6. This is so that subgraphs will not inadvertently require the latest join spec (which cause the router or gateway not to start if running an older version). From f673d330dd05e21afdc4256ef480cfaf8253e083 Mon Sep 17 00:00:00 2001 From: Chris Lenfest Date: Thu, 8 Feb 2024 10:01:28 -0600 Subject: [PATCH 3/3] Upgrade fed1 schema to 2.4 instead of 2.6 --- .changeset/lemon-yaks-think.md | 2 +- internals-js/src/__tests__/schemaUpgrader.test.ts | 2 +- internals-js/src/federation.ts | 7 ++----- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/.changeset/lemon-yaks-think.md b/.changeset/lemon-yaks-think.md index 486430600..f1d9b37ee 100644 --- a/.changeset/lemon-yaks-think.md +++ b/.changeset/lemon-yaks-think.md @@ -3,4 +3,4 @@ "@apollo/federation-internals": patch --- -When auto-upgrading a subgraph (i.e. one that does not explicitly @link the federation spec) do not go past v2.6. This is so that subgraphs will not inadvertently require the latest join spec (which cause the router or gateway not to start if running an older version). +When auto-upgrading a subgraph (i.e. one that does not explicitly @link the federation spec) do not go past v2.4. This is so that subgraphs will not inadvertently require the latest join spec (which cause the router or gateway not to start if running an older version). diff --git a/internals-js/src/__tests__/schemaUpgrader.test.ts b/internals-js/src/__tests__/schemaUpgrader.test.ts index de5dcd2bc..8c5c79065 100644 --- a/internals-js/src/__tests__/schemaUpgrader.test.ts +++ b/internals-js/src/__tests__/schemaUpgrader.test.ts @@ -320,7 +320,7 @@ test("fully upgrades a schema with no @link directive", () => { expect(printSchema(result.subgraphs!.get("subgraph")!.schema!)).toContain( `schema @link(url: "https://specs.apollo.dev/link/v1.0") - @link(url: "https://specs.apollo.dev/federation/v2.6", import: ["@key", "@requires", "@provides", "@external", "@tag", "@extends", "@shareable", "@inaccessible", "@override", "@composeDirective", "@interfaceObject"]) + @link(url: "https://specs.apollo.dev/federation/v2.4", import: ["@key", "@requires", "@provides", "@external", "@tag", "@extends", "@shareable", "@inaccessible", "@override", "@composeDirective", "@interfaceObject"]) { query: Query }` diff --git a/internals-js/src/federation.ts b/internals-js/src/federation.ts index e856fdb63..b71965618 100644 --- a/internals-js/src/federation.ts +++ b/internals-js/src/federation.ts @@ -105,9 +105,6 @@ const federationSpec = (version?: FeatureVersion): FederationSpecDefinition => { // directive from v2 definitions, we will continue expanding other directives (up to v2.4) to ensure backwards compatibility. const autoExpandedFederationSpec = federationSpec(new FeatureVersion(2, 4)); -// For subgraphs that don't explicitly specify a federation version, we need a version number to link in the upgraded schema. -const autoUpgradedFederationSpec = federationSpec(new FeatureVersion(2, 6)); - const latestFederationSpec = federationSpec(); // We don't let user use this as a subgraph name. That allows us to use it in `query graphs` to name the source of roots @@ -1221,7 +1218,7 @@ export function setSchemaAsFed2Subgraph(schema: Schema, useLatest: boolean = fal assert(core, 'Schema should now be a core schema'); } - const fedSpec = useLatest ? latestFederationSpec : autoUpgradedFederationSpec; + const fedSpec = useLatest ? latestFederationSpec : autoExpandedFederationSpec; assert(!core.getByIdentity(fedSpec.identity), 'Schema already set as a federation subgraph'); schema.schemaDefinition.applyDirective( @@ -1248,7 +1245,7 @@ export const FEDERATION2_LINK_WITH_FULL_IMPORTS = '@link(url: "https://specs.apo export const FEDERATION2_LINK_WITH_AUTO_EXPANDED_IMPORTS = '@link(url: "https://specs.apollo.dev/federation/v2.7", import: ["@key", "@requires", "@provides", "@external", "@tag", "@extends", "@shareable", "@inaccessible", "@override", "@composeDirective", "@interfaceObject"])'; // This is the federation @link for tests that go through the SchemaUpgrader. -export const FEDERATION2_LINK_WITH_AUTO_EXPANDED_IMPORTS_UPGRADED = '@link(url: "https://specs.apollo.dev/federation/v2.6", import: ["@key", "@requires", "@provides", "@external", "@tag", "@extends", "@shareable", "@inaccessible", "@override", "@composeDirective", "@interfaceObject"])'; +export const FEDERATION2_LINK_WITH_AUTO_EXPANDED_IMPORTS_UPGRADED = '@link(url: "https://specs.apollo.dev/federation/v2.4", import: ["@key", "@requires", "@provides", "@external", "@tag", "@extends", "@shareable", "@inaccessible", "@override", "@composeDirective", "@interfaceObject"])'; /** * Given a document that is assumed to _not_ be a fed2 schema (it does not have a `@link` to the federation spec),