From 33b937b18d3c7ca6af14b904696b536399e597d1 Mon Sep 17 00:00:00 2001 From: Taylor Ninesling Date: Mon, 5 Feb 2024 12:08:28 -0600 Subject: [PATCH] Implicitly upgrade federation version to the max required by any used directive (#2929) # Overview The recent addition of `SourceSpecDefinition` surfaced a case where a subgraph with a `@link` to federation < 2.7 and a `@link` to the source spec (which requires federation >= 2.7) results in an unexpected error during composition. Validation for this case was introduced in https://github.com/apollographql/federation/pull/2914, raising a composition error when the versions are mismatched. Since the existing behavior will implicitly upgrade to a later minor version of federation when two subgraphs are mismatched, it makes sense to do the same with feature requirements. # Example Subgraph A has a `@link` to fed version 2.5 Subgraph B has a `@link` to fed version 2.7 Subgraph C has a `@link` to fed version 2.5 and a separate `@link` to the source spec Previous behavior: 1. Composing Subgraph A and Subgraph B silently upgrades to fed version 2.7 2. Composing Subgraph A and Subgraph C results in a composition error New behavior: 1. Composing Subgraph A and Subgraph B silently upgrades to fed version 2.7 2. Composing Subgraph A and Subgraph C upgrades to fed version 2.7 and raises a hint explaining why --------- Co-authored-by: Chris Lenfest --- .changeset/forty-dolls-type.md | 6 + composition-js/src/__tests__/compose.test.ts | 4 - composition-js/src/__tests__/hints.test.ts | 121 ++++++++++++++++++- composition-js/src/hints.ts | 8 ++ composition-js/src/merging/merge.ts | 57 +++++++-- internals-js/src/definitions.ts | 6 + internals-js/src/specs/coreSpec.ts | 22 ++++ internals-js/src/specs/sourceSpec.ts | 23 +--- 8 files changed, 212 insertions(+), 35 deletions(-) create mode 100644 .changeset/forty-dolls-type.md diff --git a/.changeset/forty-dolls-type.md b/.changeset/forty-dolls-type.md new file mode 100644 index 000000000..9e496da7e --- /dev/null +++ b/.changeset/forty-dolls-type.md @@ -0,0 +1,6 @@ +--- +"@apollo/composition": minor +"@apollo/federation-internals": minor +--- + +When a linked directive requires a federation version higher than the linked federation spec, upgrade to the implied version and issue a hint diff --git a/composition-js/src/__tests__/compose.test.ts b/composition-js/src/__tests__/compose.test.ts index c3770a657..5b314b46f 100644 --- a/composition-js/src/__tests__/compose.test.ts +++ b/composition-js/src/__tests__/compose.test.ts @@ -5101,10 +5101,6 @@ describe('@source* directives', () => { const messages = result.errors!.map(e => e.message); - expect(messages).toContain( - '[bad] Schemas that @link to https://specs.apollo.dev/source must also @link to federation version v2.7 or later (found v2.5)' - ); - expect(messages).toContain( '[bad] @sourceAPI(name: "A?!") must specify name using only [a-zA-Z0-9-_] characters' ); diff --git a/composition-js/src/__tests__/hints.test.ts b/composition-js/src/__tests__/hints.test.ts index b6b62aa23..9969132ca 100644 --- a/composition-js/src/__tests__/hints.test.ts +++ b/composition-js/src/__tests__/hints.test.ts @@ -6,8 +6,9 @@ import { HINTS, } from '../hints'; import { MergeResult, mergeSubgraphs } from '../merging'; -import { composeAsFed2Subgraphs } from './testHelper'; +import { assertCompositionSuccess, composeAsFed2Subgraphs } from './testHelper'; import { formatExpectedToMatchReceived } from './matchers/toMatchString'; +import { composeServices } from '../compose'; function mergeDocuments(...documents: DocumentNode[]): MergeResult { const subgraphs = new Subgraphs(); @@ -1177,3 +1178,121 @@ describe('when shared field has intersecting but non equal runtime types in diff ); }); }); + +describe('when a directive causes an implicit federation version upgrade', () => { + const olderFederationSchema = gql` + extend schema + @link(url: "https://specs.apollo.dev/federation/v2.5", import: ["@key"]) + + type Query { + a: String! + } + `; + + const newerFederationSchema = gql` + extend schema + @link(url: "https://specs.apollo.dev/federation/v2.7", import: ["@key"]) + + type Query { + b: String! + } + `; + + const autoUpgradedSchema = gql` + extend schema + @link(url: "https://specs.apollo.dev/federation/v2.5", import: ["@key", "@shareable"]) + @link(url: "https://specs.apollo.dev/source/v0.1", import: [ + "@sourceAPI" + "@sourceType" + "@sourceField" + ]) + @sourceAPI( + name: "A" + http: { baseURL: "https://api.a.com/v1" } + ) + { + query: Query + } + + type Query @shareable { + resources: [Resource!]! @sourceField( + api: "A" + http: { GET: "/resources" } + ) + } + + type Resource @shareable @key(fields: "id") @sourceType( + api: "A" + http: { GET: "/resources/{id}" } + selection: "id description" + ) { + id: ID! + description: String! + } + `; + + it('should hint that the version was upgraded to satisfy directive requirements', () => { + const result = composeServices([ + { + name: 'already-newest', + typeDefs: newerFederationSchema, + }, + { + name: 'old-but-not-upgraded', + typeDefs: olderFederationSchema, + }, + { + name: 'upgraded', + typeDefs: autoUpgradedSchema, + } + ]); + + assertCompositionSuccess(result); + expect(result).toRaiseHint( + HINTS.IMPLICITLY_UPGRADED_FEDERATION_VERSION, + 'Subgraph upgraded has been implicitly upgraded from federation v2.5 to v2.7', + '@link' + ); + }); + + it('should show separate hints for each upgraded subgraph', () => { + const result = composeServices([ + { + name: 'upgraded-1', + typeDefs: autoUpgradedSchema, + }, + { + name: 'upgraded-2', + typeDefs: autoUpgradedSchema + }, + ]); + + assertCompositionSuccess(result); + expect(result).toRaiseHint( + HINTS.IMPLICITLY_UPGRADED_FEDERATION_VERSION, + 'Subgraph upgraded-1 has been implicitly upgraded from federation v2.5 to v2.7', + '@link' + ); + expect(result).toRaiseHint( + HINTS.IMPLICITLY_UPGRADED_FEDERATION_VERSION, + 'Subgraph upgraded-2 has been implicitly upgraded from federation v2.5 to v2.7', + '@link' + ); + }); + + it('should not raise hints if the only upgrade is caused by a link directly to the federation spec', () => { + const result = composeServices([ + { + name: 'already-newest', + typeDefs: newerFederationSchema, + }, + { + name: 'old-but-not-upgraded', + typeDefs: olderFederationSchema, + }, + ]); + + assertCompositionSuccess(result); + expect(result).toNotRaiseHints(); + }); +}) diff --git a/composition-js/src/hints.ts b/composition-js/src/hints.ts index de7a3d7ec..d6dbf8b8e 100644 --- a/composition-js/src/hints.ts +++ b/composition-js/src/hints.ts @@ -199,6 +199,13 @@ const INCONSISTENT_RUNTIME_TYPES_FOR_SHAREABLE_RETURN = makeCodeDefinition({ description: 'Indicates that a @shareable field returns different sets of runtime types in the different subgraphs in which it is defined.', }); +const IMPLICITLY_UPGRADED_FEDERATION_VERSION = makeCodeDefinition({ + code: 'IMPLICITLY_UPGRADED_FEDERATION_VERSION', + level: HintLevel.INFO, + description: 'Indicates that a directive requires a higher federation version than is explicitly linked.' + + ' In this case, the supergraph uses the federation version required by the directive.' +}); + export const HINTS = { INCONSISTENT_BUT_COMPATIBLE_FIELD_TYPE, INCONSISTENT_BUT_COMPATIBLE_ARGUMENT_TYPE, @@ -227,6 +234,7 @@ export const HINTS = { DIRECTIVE_COMPOSITION_INFO, DIRECTIVE_COMPOSITION_WARN, INCONSISTENT_RUNTIME_TYPES_FOR_SHAREABLE_RETURN, + IMPLICITLY_UPGRADED_FEDERATION_VERSION, } export class CompositionHint { diff --git a/composition-js/src/merging/merge.ts b/composition-js/src/merging/merge.ts index 2a6682360..82da8fa90 100644 --- a/composition-js/src/merging/merge.ts +++ b/composition-js/src/merging/merge.ts @@ -74,6 +74,8 @@ import { LinkDirectiveArgs, sourceIdentity, FeatureUrl, + CoreFeature, + Subgraph, } from "@apollo/federation-internals"; import { ASTNode, GraphQLError, DirectiveLocation } from "graphql"; import { @@ -343,19 +345,56 @@ class Merger { } private getLatestFederationVersionUsed(): FeatureVersion { - const latestVersion = this.subgraphs.values().reduce((latest: FeatureVersion | undefined, subgraph) => { - const version = subgraph.metadata()?.federationFeature()?.url?.version; - if (!latest) { - return version; + const versions = this.subgraphs.values() + .map((s) => this.getLatestFederationVersionUsedInSubgraph(s)) + .filter(isDefined); + + return FeatureVersion.max(versions) ?? FEDERATION_VERSIONS.latest().version; + } + + private getLatestFederationVersionUsedInSubgraph(subgraph: Subgraph): FeatureVersion | undefined { + const linkedFederationVersion = subgraph.metadata()?.federationFeature()?.url.version; + if (!linkedFederationVersion) { + return undefined; + } + + // Check if any of the directives imply a newer version of federation than is explicitly linked + const versionsFromFeatures: FeatureVersion[] = []; + for (const feature of subgraph.schema.coreFeatures?.allFeatures() ?? []) { + const version = feature.minimumFederationVersion(); + if (version) { + versionsFromFeatures.push(version); } - if (!version) { - return latest; + } + const impliedFederationVersion = FeatureVersion.max(versionsFromFeatures); + if (!impliedFederationVersion?.satisfies(linkedFederationVersion) || linkedFederationVersion >= impliedFederationVersion) { + return linkedFederationVersion; + } + + // If some of the directives are causing an implicit upgrade, put one in the hint + let featureCausingUpgrade: CoreFeature | undefined; + for (const feature of subgraph.schema.coreFeatures?.allFeatures() ?? []) { + if (feature.minimumFederationVersion() == impliedFederationVersion) { + featureCausingUpgrade = feature; + break; } - return latest >= version ? latest : version; - }, undefined); - return latestVersion ?? FEDERATION_VERSIONS.latest().version; + } + + if (featureCausingUpgrade) { + this.hints.push(new CompositionHint( + HINTS.IMPLICITLY_UPGRADED_FEDERATION_VERSION, + `Subgraph ${subgraph.name} has been implicitly upgraded from federation ${linkedFederationVersion} to ${impliedFederationVersion}`, + featureCausingUpgrade.directive.definition, + featureCausingUpgrade.directive.sourceAST ? + addSubgraphToASTNode(featureCausingUpgrade.directive.sourceAST, subgraph.name) : + undefined + )); + } + + return impliedFederationVersion; } + private prepareSupergraph(): Map { // TODO: we will soon need to look for name conflicts for @core and @join with potentially user-defined directives and // pass a `as` to the methods below if necessary. However, as we currently don't propagate any subgraph directives to diff --git a/internals-js/src/definitions.ts b/internals-js/src/definitions.ts index f2ac566ae..4a8df1c12 100644 --- a/internals-js/src/definitions.ts +++ b/internals-js/src/definitions.ts @@ -27,6 +27,7 @@ import { CoreSpecDefinition, extractCoreFeatureImports, FeatureUrl, + FeatureVersion, findCoreSpecVersion, isCoreSpecDirectiveApplication, removeAllCoreFeatures, @@ -53,6 +54,7 @@ import { validateSchema } from "./validate"; import { createDirectiveSpecification, createScalarTypeSpecification, DirectiveSpecification, TypeSpecification } from "./directiveAndTypeSpecification"; import { didYouMean, suggestionList } from "./suggestions"; import { aggregateError, ERRORS, withModifiedErrorMessage } from "./error"; +import { coreFeatureDefinitionIfKnown } from "./knownCoreFeatures"; const validationErrorCode = 'GraphQLValidationFailed'; const DEFAULT_VALIDATION_ERROR_MESSAGE = 'The schema is not a valid GraphQL schema.'; @@ -1014,6 +1016,10 @@ export class CoreFeature { const elementImport = this.imports.find((i) => i.name === name); return elementImport ? (elementImport.as ?? name) : this.nameInSchema + '__' + name; } + + minimumFederationVersion(): FeatureVersion | undefined { + return coreFeatureDefinitionIfKnown(this.url)?.minimumFederationVersion; + } } export class CoreFeatures { diff --git a/internals-js/src/specs/coreSpec.ts b/internals-js/src/specs/coreSpec.ts index 3c2efef96..8c4b95200 100644 --- a/internals-js/src/specs/coreSpec.ts +++ b/internals-js/src/specs/coreSpec.ts @@ -640,6 +640,28 @@ export class FeatureVersion { return new this(+match[1], +match[2]) } + /** + * Find the maximum version in a collection of versions, returning undefined in the case + * that the collection is empty. + * + * # Example + * ``` + * expect(FeatureVersion.max([new FeatureVersion(1, 0), new FeatureVersion(2, 0)])).toBe(new FeatureVersion(2, 0)) + * expect(FeatureVersion.max([])).toBe(undefined) + * ``` + */ + public static max(versions: Iterable): FeatureVersion | undefined { + let max: FeatureVersion | undefined; + + for (const version of versions) { + if (!max || version > max) { + max = version; + } + } + + return max; + } + /** * Return true if and only if this FeatureVersion satisfies the `required` version * diff --git a/internals-js/src/specs/sourceSpec.ts b/internals-js/src/specs/sourceSpec.ts index ad59abbf2..ade88c144 100644 --- a/internals-js/src/specs/sourceSpec.ts +++ b/internals-js/src/specs/sourceSpec.ts @@ -147,15 +147,13 @@ export class SourceSpecDefinition extends FeatureDefinition { return this.directive(schema, 'sourceField')!; } - private getSourceDirectives(schema: Schema, errors: GraphQLError[]) { + private getSourceDirectives(schema: Schema) { const result: { sourceAPI?: DirectiveDefinition; sourceType?: DirectiveDefinition; sourceField?: DirectiveDefinition; } = {}; - let federationVersion: FeatureVersion | undefined; - schema.schemaDefinition.appliedDirectivesOf('link') .forEach(linkDirective => { const { url, import: imports } = linkDirective.arguments(); @@ -175,25 +173,8 @@ export class SourceSpecDefinition extends FeatureDefinition { } }); } - if (featureUrl && featureUrl.name === 'federation') { - federationVersion = featureUrl.version; - } }); - if (result.sourceAPI || result.sourceType || result.sourceField) { - // Since this subgraph uses at least one of the @source{API,Type,Field} - // directives, it must also use v2.7 or later of federation. - if (!federationVersion || federationVersion.lt(this.minimumFederationVersion)) { - errors.push(ERRORS.SOURCE_FEDERATION_VERSION_REQUIRED.err( - `Schemas that @link to ${ - sourceIdentity - } must also @link to federation version ${ - this.minimumFederationVersion - } or later (found ${federationVersion})`, - )); - } - } - return result; } @@ -203,7 +184,7 @@ export class SourceSpecDefinition extends FeatureDefinition { sourceAPI, sourceType, sourceField, - } = this.getSourceDirectives(schema, errors); + } = this.getSourceDirectives(schema); if (!(sourceAPI || sourceType || sourceField)) { // If none of the @source* directives are present, nothing needs