Skip to content

Commit

Permalink
Implicitly upgrade federation version to the max required by any used…
Browse files Browse the repository at this point in the history
… 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
#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 <[email protected]>
  • Loading branch information
tninesling and clenfest authored Feb 5, 2024
1 parent f9662c7 commit 33b937b
Show file tree
Hide file tree
Showing 8 changed files with 212 additions and 35 deletions.
6 changes: 6 additions & 0 deletions .changeset/forty-dolls-type.md
Original file line number Diff line number Diff line change
@@ -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
4 changes: 0 additions & 4 deletions composition-js/src/__tests__/compose.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'
);
Expand Down
121 changes: 120 additions & 1 deletion composition-js/src/__tests__/hints.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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();
});
})
8 changes: 8 additions & 0 deletions composition-js/src/hints.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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 {
Expand Down
57 changes: 48 additions & 9 deletions composition-js/src/merging/merge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ import {
LinkDirectiveArgs,
sourceIdentity,
FeatureUrl,
CoreFeature,
Subgraph,
} from "@apollo/federation-internals";
import { ASTNode, GraphQLError, DirectiveLocation } from "graphql";
import {
Expand Down Expand Up @@ -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<string, string> {
// 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
Expand Down
6 changes: 6 additions & 0 deletions internals-js/src/definitions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
CoreSpecDefinition,
extractCoreFeatureImports,
FeatureUrl,
FeatureVersion,
findCoreSpecVersion,
isCoreSpecDirectiveApplication,
removeAllCoreFeatures,
Expand All @@ -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.';
Expand Down Expand Up @@ -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 {
Expand Down
22 changes: 22 additions & 0 deletions internals-js/src/specs/coreSpec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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>): 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
*
Expand Down
23 changes: 2 additions & 21 deletions internals-js/src/specs/sourceSpec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -147,15 +147,13 @@ export class SourceSpecDefinition extends FeatureDefinition {
return this.directive<SourceFieldDirectiveArgs>(schema, 'sourceField')!;
}

private getSourceDirectives(schema: Schema, errors: GraphQLError[]) {
private getSourceDirectives(schema: Schema) {
const result: {
sourceAPI?: DirectiveDefinition<SourceAPIDirectiveArgs>;
sourceType?: DirectiveDefinition<SourceTypeDirectiveArgs>;
sourceField?: DirectiveDefinition<SourceFieldDirectiveArgs>;
} = {};

let federationVersion: FeatureVersion | undefined;

schema.schemaDefinition.appliedDirectivesOf<LinkDirectiveArgs>('link')
.forEach(linkDirective => {
const { url, import: imports } = linkDirective.arguments();
Expand All @@ -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;
}

Expand All @@ -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
Expand Down

0 comments on commit 33b937b

Please sign in to comment.