Use required version of federation coreSpec rather than latest (#2528)

For core specs used by federation, rather than using the latest version of a core spec, we will use the latest version that is implied by the version of federation requested to be composed. This will be true going forward only, will not downgrade any existing supergraph schemas.

---------

Co-authored-by: Trevor Scheer <trevor.scheer@gmail.com>

Author: clenfest

.changeset/gentle-lies-give.md

@@ -0,0 +1,7 @@
+---
+"@apollo/composition": minor
+"@apollo/federation-internals": minor
+---
+
+For CoreSpecDefintions that opt in, we've added the ability to tie the core spec version to a particular federation version. That means that if there's a new version of, say, the join spec, you won't necessarily get the new version in the supergraph schema if no subgraph requires it.
+  

composition-js/src/merging/merge.ts

@@ -66,6 +66,11 @@ import {
   addSubgraphToError,
   printHumanReadableList,
   ArgumentMerger,
+  JoinSpecDefinition,
+  CoreSpecDefinition,
+  FeatureVersion,
+  FEDERATION_VERSIONS,
+  InaccessibleSpecDefinition,
 } from "@apollo/federation-internals";
 import { ASTNode, GraphQLError, DirectiveLocation } from "graphql";
 import {
@@ -79,13 +84,8 @@ import { inspect } from "util";
 import { collectCoreDirectivesToCompose, CoreDirectiveInSubgraphs } from "./coreDirectiveCollector";
 import { CompositionOptions } from "../compose";
 
-
-const linkSpec = LINK_VERSIONS.latest();
 type FieldOrUndefinedArray = (FieldDefinition<any> | undefined)[];
 
-const joinSpec = JOIN_VERSIONS.latest();
-const inaccessibleSpec = INACCESSIBLE_VERSIONS.latest();
-
 export type MergeResult = MergeSuccess | MergeFailure;
 
 type FieldMergeContextProperties = {
@@ -275,8 +275,17 @@ class Merger {
     sources: (SchemaElement<any, any> | undefined)[],
     dest: SchemaElement<any, any>,
   }[];
+  private joinSpec: JoinSpecDefinition;
+  private linkSpec: CoreSpecDefinition;
+  private inaccessibleSpec: InaccessibleSpecDefinition;
+  private latestFedVersionUsed: FeatureVersion;
 
   constructor(readonly subgraphs: Subgraphs, readonly options: CompositionOptions) {
+    this.latestFedVersionUsed = this.getLatestFederationVersionUsed();
+    this.joinSpec = JOIN_VERSIONS.getMinimumRequiredVersion(this.latestFedVersionUsed);
+    this.linkSpec = LINK_VERSIONS.getMinimumRequiredVersion(this.latestFedVersionUsed);
+    this.inaccessibleSpec = INACCESSIBLE_VERSIONS.getMinimumRequiredVersion(this.latestFedVersionUsed);
+
     this.names = subgraphs.names();
     this.composeDirectiveManager = new ComposeDirectiveManager(
       this.subgraphs,
@@ -293,20 +302,34 @@ class Merger {
     this.appliedDirectivesToMerge = [];
   }
 
+  private getLatestFederationVersionUsed(): FeatureVersion {
+    const latestVersion =  this.subgraphs.values().reduce((latest: FeatureVersion | undefined, subgraph) => {
+      const version = subgraph.metadata()?.federationFeature()?.url?.version;
+      if (!latest) {
+        return version;
+      }
+      if (!version) {
+        return latest;
+      }
+      return latest >= version ? latest : version;
+    }, undefined);
+    return latestVersion ?? FEDERATION_VERSIONS.latest().version;
+  }
+
   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
     // the supergraph outside of a few well-known ones, we don't bother yet.
-    linkSpec.addToSchema(this.merged);
-    const errors = linkSpec.applyFeatureToSchema(this.merged, joinSpec, undefined, joinSpec.defaultCorePurpose);
+    this.linkSpec.addToSchema(this.merged);
+    const errors = this.linkSpec.applyFeatureToSchema(this.merged, this.joinSpec, undefined, this.joinSpec.defaultCorePurpose);
     assert(errors.length === 0, "We shouldn't have errors adding the join spec to the (still empty) supergraph schema");
 
     const directivesMergeInfo = collectCoreDirectivesToCompose(this.subgraphs);
     for (const mergeInfo of directivesMergeInfo) {
       this.validateAndMaybeAddSpec(mergeInfo);
     }
 
-    return joinSpec.populateGraphEnum(this.merged, this.subgraphs);
+    return this.joinSpec.populateGraphEnum(this.merged, this.subgraphs);
   }
 
   private validateAndMaybeAddSpec({feature, name, definitionsPerSubgraph, compositionSpec}: CoreDirectiveInSubgraphs) {
@@ -339,8 +362,8 @@ class Merger {
     // If we get here with `nameInSupergraph` unset, it means there is no usage for the directive at all and we
     // don't bother adding the spec to the supergraph.
     if (nameInSupergraph) {
-      const specInSupergraph = compositionSpec.supergraphSpecification();
-      const errors = linkSpec.applyFeatureToSchema(this.merged, specInSupergraph, nameInSupergraph === specInSupergraph.url.name ? undefined : nameInSupergraph, specInSupergraph.defaultCorePurpose);
+      const specInSupergraph = compositionSpec.supergraphSpecification(this.latestFedVersionUsed);
+      const errors = this.linkSpec.applyFeatureToSchema(this.merged, specInSupergraph, nameInSupergraph === specInSupergraph.url.name ? undefined : nameInSupergraph, specInSupergraph.defaultCorePurpose);
       assert(errors.length === 0, "We shouldn't have errors adding the join spec to the (still empty) supergraph schema");
       const argumentsMerger = compositionSpec.argumentsMerger?.call(null, this.merged);
       if (argumentsMerger instanceof GraphQLError) {
@@ -395,7 +418,7 @@ class Merger {
     this.addDirectivesShallow();
 
     const typesToMerge = this.merged.types()
-      .filter((type) => !linkSpec.isSpecType(type) && !joinSpec.isSpecType(type));
+      .filter((type) => !this.linkSpec.isSpecType(type) && !this.joinSpec.isSpecType(type));
 
     // Then, for object and interface types, we merge the 'implements' relationship, and we merge the unions.
     // We do this first because being able to know if a type is a subtype of another one (which relies on those
@@ -426,7 +449,7 @@ class Merger {
 
     for (const definition of this.merged.directives()) {
       // we should skip the supergraph specific directives, that is the @core and @join directives.
-      if (linkSpec.isSpecDirective(definition) || joinSpec.isSpecDirective(definition)) {
+      if (this.linkSpec.isSpecDirective(definition) || this.joinSpec.isSpecDirective(definition)) {
         continue;
       }
       this.mergeDirectiveDefinition(this.subgraphsSchema.map(s => s.directive(definition.name)), definition);
@@ -622,7 +645,7 @@ class Merger {
 
   private mergeImplements<T extends ObjectType | InterfaceType>(sources: (T | undefined)[], dest: T) {
     const implemented = new Set<string>();
-    const joinImplementsDirective = joinSpec.implementsDirective(this.merged)!;
+    const joinImplementsDirective = this.joinSpec.implementsDirective(this.merged)!;
     for (const [idx, source] of sources.entries()) {
       if (source) {
         const name = this.joinSpecName(idx);
@@ -753,7 +776,7 @@ class Merger {
   }
 
   private addJoinType(sources: (NamedType | undefined)[], dest: NamedType) {
-    const joinTypeDirective = joinSpec.typeDirective(this.merged);
+    const joinTypeDirective = this.joinSpec.typeDirective(this.merged);
     for (const [idx, source] of sources.entries()) {
       if (!source) {
         continue;
@@ -914,7 +937,7 @@ class Merger {
             // clarify to the later extraction process that this particular field doesn't come
             // from any particular subgraph (it comes indirectly from an @interfaceObject type,
             // but it's very much indirect so ...).
-            implemField.applyDirective(joinSpec.fieldDirective(this.merged), { graph: undefined });
+            implemField.applyDirective(this.joinSpec.fieldDirective(this.merged), { graph: undefined });
 
 
             // If we had to add a field here, it means that, for this particular implementation, the
@@ -937,7 +960,7 @@ class Merger {
     // implementation type when @interfaceObject is used. But we shouldn't copy the `join` spec directive
     // as those are for the interface field but are invalid for the implementation field.
     source.appliedDirectives.forEach((d) => {
-      if (!joinSpec.isSpecDirective(d.definition!)) {
+      if (!this.joinSpec.isSpecDirective(d.definition!)) {
         dest.applyDirective(d.name, {...d.arguments()})
       }
     });
@@ -1537,7 +1560,7 @@ class Merger {
     })) {
       return;
     }
-    const joinFieldDirective = joinSpec.fieldDirective(this.merged);
+    const joinFieldDirective = this.joinSpec.fieldDirective(this.merged);
     for (const [idx, source] of sources.entries()) {
       const usedOverridden = mergeContext.isUsedOverridden(idx);
       const unusedOverridden = mergeContext.isUnusedOverridden(idx);
@@ -1892,7 +1915,7 @@ class Merger {
   }
 
   private addJoinUnionMember(sources: (UnionType | undefined)[], dest: UnionType, member: ObjectType) {
-    const joinUnionMemberDirective = joinSpec.unionMemberDirective(this.merged);
+    const joinUnionMemberDirective = this.joinSpec.unionMemberDirective(this.merged);
     // We should always be merging with the latest join spec, so this should exists, but well, in prior versions where
     // the directive didn't existed, we simply did had any replacement so ...
     if (!joinUnionMemberDirective) {
@@ -1989,7 +2012,7 @@ class Merger {
     this.recordAppliedDirectivesToMerge(valueSources, value);
     this.addJoinEnumValue(valueSources, value);
 
-    const inaccessibleInSupergraph = this.mergedFederationDirectiveInSupergraph.get(inaccessibleSpec.inaccessibleDirectiveSpec.name);
+    const inaccessibleInSupergraph = this.mergedFederationDirectiveInSupergraph.get(this.inaccessibleSpec.inaccessibleDirectiveSpec.name);
     const isInaccessible = inaccessibleInSupergraph && value.hasAppliedDirective(inaccessibleInSupergraph.definition);
     // The merging strategy depends on the enum type usage:
     //  - if it is _only_ used in position of Input type, we merge it with an "intersection" strategy (like other input types/things).
@@ -2038,7 +2061,7 @@ class Merger {
   }
 
   private addJoinEnumValue(sources: (EnumValue | undefined)[], dest: EnumValue) {
-    const joinEnumValueDirective = joinSpec.enumValueDirective(this.merged);
+    const joinEnumValueDirective = this.joinSpec.enumValueDirective(this.merged);
     // We should always be merging with the latest join spec, so this should exists, but well, in prior versions where
     // the directive didn't existed, we simply did had any replacement so ...
     if (!joinEnumValueDirective) {
@@ -2080,7 +2103,7 @@ class Merger {
   }
 
   private mergeInput(sources: (InputObjectType | undefined)[], dest: InputObjectType) {
-    const inaccessibleInSupergraph = this.mergedFederationDirectiveInSupergraph.get(inaccessibleSpec.inaccessibleDirectiveSpec.name);
+    const inaccessibleInSupergraph = this.mergedFederationDirectiveInSupergraph.get(this.inaccessibleSpec.inaccessibleDirectiveSpec.name);
 
     // Like for other inputs, we add all the fields found in any subgraphs initially as a simple mean to have a complete list of
     // field to iterate over, but we will remove those that are not in all subgraphs.
@@ -2355,7 +2378,7 @@ class Merger {
   // is @inaccessible, which is necessary to exist in the supergraph for EnumValues to properly
   // determine whether the fact that a value is both input / output will matter
   private recordAppliedDirectivesToMerge(sources: (SchemaElement<any, any> | undefined)[], dest: SchemaElement<any, any>) {
-    const inaccessibleInSupergraph = this.mergedFederationDirectiveInSupergraph.get(inaccessibleSpec.inaccessibleDirectiveSpec.name);
+    const inaccessibleInSupergraph = this.mergedFederationDirectiveInSupergraph.get(this.inaccessibleSpec.inaccessibleDirectiveSpec.name);
     const inaccessibleName = inaccessibleInSupergraph?.definition.name;
     const names = this.gatherAppliedDirectiveNames(sources);
 

internals-js/src/__tests__/coreSpec.test.ts

@@ -3,7 +3,7 @@ import gql from "graphql-tag";
 import { buildSubgraph } from "../federation";
 import { assert } from "../utils";
 import { buildSchemaFromAST } from "../buildSchema";
-import { removeAllCoreFeatures } from "../coreSpec";
+import { removeAllCoreFeatures, FeatureDefinitions, FeatureVersion, FeatureDefinition, FeatureUrl } from "../coreSpec";
 import { errorCauses } from "../error";
 
 function expectErrors(
@@ -210,3 +210,36 @@ describe('removeAllCoreFeatures', () => {
     expect(schema.elementByCoordinate("@foo__quz")).toBeUndefined();
   });
 });
+
+class TestFeatureDefinition extends FeatureDefinition {
+  constructor(version: FeatureVersion, fedVersion?: FeatureVersion) {
+    super(new FeatureUrl('test', 'test', version), fedVersion);
+  }
+}
+
+describe('getMinimumRequiredVersion tests', () => {
+  it('various combinations', () => {
+    const versions = new FeatureDefinitions<TestFeatureDefinition>('test')
+      .add(new TestFeatureDefinition(new FeatureVersion(0, 1)))
+      .add(new TestFeatureDefinition(new FeatureVersion(0, 2), new FeatureVersion(1, 0)))
+      .add(new TestFeatureDefinition(new FeatureVersion(0, 3), new FeatureVersion(2,0)))
+      .add(new TestFeatureDefinition(new FeatureVersion(0, 4), new FeatureVersion(2,1)))
+      .add(new TestFeatureDefinition(new FeatureVersion(0, 5), new FeatureVersion(2,2)));
+
+    expect(versions.getMinimumRequiredVersion(new FeatureVersion(0, 1)).version).toEqual(new FeatureVersion(0, 1));
+    expect(versions.getMinimumRequiredVersion(new FeatureVersion(1, 0)).version).toEqual(new FeatureVersion(0, 2));
+    expect(versions.getMinimumRequiredVersion(new FeatureVersion(1, 1)).version).toEqual(new FeatureVersion(0, 2));
+    expect(versions.getMinimumRequiredVersion(new FeatureVersion(2, 0)).version).toEqual(new FeatureVersion(0, 3));
+    expect(versions.getMinimumRequiredVersion(new FeatureVersion(2, 1)).version).toEqual(new FeatureVersion(0, 4));
+    expect(versions.getMinimumRequiredVersion(new FeatureVersion(2, 2)).version).toEqual(new FeatureVersion(0, 5));
+    expect(versions.getMinimumRequiredVersion(new FeatureVersion(2, 3)).version).toEqual(new FeatureVersion(0, 5));
+
+    // now add a new major version and test again. All previous version should be forced to the new major
+    versions.add(new TestFeatureDefinition(new FeatureVersion(1, 0), new FeatureVersion(2, 4)));
+    versions.add(new TestFeatureDefinition(new FeatureVersion(1, 1), new FeatureVersion(2, 5)));
+
+    expect(versions.getMinimumRequiredVersion(new FeatureVersion(2, 3)).version).toEqual(new FeatureVersion(1, 0));
+    expect(versions.getMinimumRequiredVersion(new FeatureVersion(2, 4)).version).toEqual(new FeatureVersion(1, 0));
+    expect(versions.getMinimumRequiredVersion(new FeatureVersion(2, 5)).version).toEqual(new FeatureVersion(1, 1));
+  })
+})

internals-js/src/coreSpec.ts

@@ -2,7 +2,7 @@ import { ASTNode, DirectiveLocation, GraphQLError, StringValueNode } from "graph
 import { URL } from "url";
 import { CoreFeature, Directive, DirectiveDefinition, EnumType, ErrGraphQLAPISchemaValidationFailed, ErrGraphQLValidationFailed, InputType, ListType, NamedType, NonNullType, ScalarType, Schema, SchemaDefinition, SchemaElement, sourceASTs } from "./definitions";
 import { sameType } from "./types";
-import { assert, firstOf, MapWithCachedArrays } from './utils';
+import { assert, findLast, firstOf, MapWithCachedArrays } from './utils';
 import { aggregateError, ERRORS } from "./error";
 import { valueToString } from "./values";
 import { coreFeatureDefinitionIfKnown, registerKnownFeature } from "./knownCoreFeatures";
@@ -41,7 +41,8 @@ export abstract class FeatureDefinition {
   private readonly _directiveSpecs = new MapWithCachedArrays<string, DirectiveSpecification>();
   private readonly _typeSpecs = new MapWithCachedArrays<string, TypeSpecification>();
 
-  constructor(url: FeatureUrl | string) {
+  // A minimumFederationVersion that's undefined would mean that we won't produce that version in the supergraph SDL.
+  constructor(url: FeatureUrl | string, readonly minimumFederationVersion?: FeatureVersion) {
     this.url = typeof url === 'string' ? FeatureUrl.parse(url) : url;
   }
 
@@ -364,8 +365,8 @@ const linkImportTypeSpec = createScalarTypeSpecification({ name: 'Import' });
 export class CoreSpecDefinition extends FeatureDefinition {
   private readonly directiveDefinitionSpec: DirectiveSpecification;
 
-  constructor(version: FeatureVersion, identity: string = linkIdentity, name: string = linkDirectiveDefaultName) {
-    super(new FeatureUrl(identity, name, version));
+  constructor(version: FeatureVersion, minimumFederationVersion?: FeatureVersion, identity: string = linkIdentity, name: string = linkDirectiveDefaultName) {
+    super(new FeatureUrl(identity, name, version), minimumFederationVersion);
     this.directiveDefinitionSpec = createDirectiveSpecification({
       name,
       locations: [DirectiveLocation.SCHEMA],
@@ -587,6 +588,23 @@ export class FeatureDefinitions<T extends FeatureDefinition = FeatureDefinition>
     assert(this._definitions.length > 0, 'Trying to get latest when no definitions exist');
     return this._definitions[0];
   }
+
+  getMinimumRequiredVersion(fedVersion: FeatureVersion): T {
+    // this._definitions is already sorted with the most recent first
+    // get the first definition that is compatible with the federation version
+    // if the minimum version is not present, assume that we won't look for an older version
+    const def = this._definitions.find(def => def.minimumFederationVersion ? fedVersion >= def.minimumFederationVersion : true);
+    assert(def, `No compatible definition exists for federation version ${fedVersion}`);
+
+    // note that it's necessary that we can only get versions that have the same major version as the latest,
+    // because otherwise we can not guarantee compatibility. In this case, we want to return the oldest version with
+    // the same major version as the latest.
+    const latestMajor = this.latest().version.major;
+    if (def.version.major !== latestMajor) {
+      return findLast(this._definitions, def => def.version.major === latestMajor) ?? this.latest();
+    }
+    return def;
+  }
 }
 
 /**
@@ -789,11 +807,11 @@ export function findCoreSpecVersion(featureUrl: FeatureUrl): CoreSpecDefinition
 }
 
 export const CORE_VERSIONS = new FeatureDefinitions<CoreSpecDefinition>(coreIdentity)
-  .add(new CoreSpecDefinition(new FeatureVersion(0, 1), coreIdentity, 'core'))
-  .add(new CoreSpecDefinition(new FeatureVersion(0, 2), coreIdentity, 'core'));
+  .add(new CoreSpecDefinition(new FeatureVersion(0, 1), undefined, coreIdentity, 'core'))
+  .add(new CoreSpecDefinition(new FeatureVersion(0, 2), new FeatureVersion(2, 0), coreIdentity, 'core'));
 
 export const LINK_VERSIONS = new FeatureDefinitions<CoreSpecDefinition>(linkIdentity)
-  .add(new CoreSpecDefinition(new FeatureVersion(1, 0)));
+  .add(new CoreSpecDefinition(new FeatureVersion(1, 0), new FeatureVersion(2, 0)));
 
 registerKnownFeature(CORE_VERSIONS);
 registerKnownFeature(LINK_VERSIONS);

internals-js/src/directiveAndTypeSpecification.ts

@@ -22,7 +22,7 @@ import { valueEquals, valueToString } from "./values";
 import { sameType } from "./types";
 import { arrayEquals, assert } from "./utils";
 import { ArgumentCompositionStrategy } from "./argumentCompositionStrategies";
-import { FeatureDefinition } from "./coreSpec";
+import { FeatureDefinition, FeatureVersion } from "./coreSpec";
 
 export type DirectiveSpecification = {
   name: string,
@@ -31,7 +31,7 @@ export type DirectiveSpecification = {
 }
 
 export type DirectiveCompositionSpecification = {
-  supergraphSpecification: () => FeatureDefinition,
+  supergraphSpecification: (federationVersion: FeatureVersion) => FeatureDefinition,
   argumentsMerger?: (schema: Schema) => ArgumentMerger | GraphQLError,
 }
 
@@ -80,7 +80,7 @@ export function createDirectiveSpecification({
   repeatable?: boolean,
   args?: DirectiveArgumentSpecification[],
   composes?: boolean,
-  supergraphSpecification?: () => FeatureDefinition,
+  supergraphSpecification?: (fedVersion: FeatureVersion) => FeatureDefinition,
 }): DirectiveSpecification {
   let composition: DirectiveCompositionSpecification | undefined = undefined;
   if (composes) {

internals-js/src/federationSpec.ts

@@ -116,7 +116,7 @@ export class FederationSpecDefinition extends FeatureDefinition {
       repeatable: version >= (new FeatureVersion(2, 2)),
     }));
 
-    this.registerDirective(INACCESSIBLE_VERSIONS.latest().inaccessibleDirectiveSpec);
+    this.registerDirective(INACCESSIBLE_VERSIONS.getMinimumRequiredVersion(version).inaccessibleDirectiveSpec);
 
     this.registerDirective(createDirectiveSpecification({
       name: FederationDirectiveName.OVERRIDE,