diff --git a/.changeset/beige-pumas-lick.md b/.changeset/beige-pumas-lick.md new file mode 100644 index 000000000..e31423447 --- /dev/null +++ b/.changeset/beige-pumas-lick.md @@ -0,0 +1,5 @@ +--- +"@apollo/federation-internals": patch +--- + +Avoid `>=` comparison for `FeatureVersion` objects diff --git a/internals-js/src/specs/__tests__/coreSpec.test.ts b/internals-js/src/specs/__tests__/coreSpec.test.ts index 14bab63da..1a987d58f 100644 --- a/internals-js/src/specs/__tests__/coreSpec.test.ts +++ b/internals-js/src/specs/__tests__/coreSpec.test.ts @@ -217,6 +217,51 @@ class TestFeatureDefinition extends FeatureDefinition { } } +describe('FeatureVersion', () => { + it('toString-based comparisons', () => { + const v2_3 = new FeatureVersion(2, 3); + const v10_0 = new FeatureVersion(10, 0); + + expect(v2_3.toString()).toBe('v2.3'); + expect(v10_0.toString()).toBe('v10.0'); + + // Operators like <, <=, >, and >= use lexicographic comparison on + // version.toString() strings, but do not perform numeric lexicographic + // comparison of the major and minor numbers, so 'v10...' < 'v2...' and the + // following comparisons produce unintuitive results. + expect([ + v2_3 < v10_0, + v2_3 <= v10_0, + v2_3 > v10_0, + v2_3 >= v10_0, + ]).toEqual( + // This should really be [true, true, false, false], if JavaScript + // supported more flexible/general operator overloading. + [false, false, true, true], + ); + + expect(v2_3.compareTo(v10_0)).toBe(-1); + expect(v10_0.compareTo(v2_3)).toBe(1); + + expect(v2_3.strictlyGreaterThan(v10_0)).toBe(false); + expect(v10_0.strictlyGreaterThan(v2_3)).toBe(true); + + expect(v2_3.lt(v10_0)).toBe(true); + expect(v2_3.lte(v10_0)).toBe(true); + expect(v2_3.gt(v10_0)).toBe(false); + expect(v2_3.gte(v10_0)).toBe(false); + expect(v10_0.lt(v2_3)).toBe(false); + expect(v10_0.lte(v2_3)).toBe(false); + expect(v10_0.gt(v2_3)).toBe(true); + expect(v10_0.gte(v2_3)).toBe(true); + + expect(v2_3.equals(v10_0)).toBe(false); + expect(v10_0.equals(v2_3)).toBe(false); + expect(v2_3.equals(v2_3)).toBe(true); + expect(v10_0.equals(v10_0)).toBe(true); + }); +}); + describe('getMinimumRequiredVersion tests', () => { it('various combinations', () => { const versions = new FeatureDefinitions('test') diff --git a/internals-js/src/specs/coreSpec.ts b/internals-js/src/specs/coreSpec.ts index 1975e02e7..cc809b2de 100644 --- a/internals-js/src/specs/coreSpec.ts +++ b/internals-js/src/specs/coreSpec.ts @@ -690,6 +690,22 @@ export class FeatureVersion { return 0; } + public lt(other: FeatureVersion): boolean { + return this.compareTo(other) < 0; + } + + public lte(other: FeatureVersion): boolean { + return this.compareTo(other) <= 0; + } + + public gt(other: FeatureVersion): boolean { + return this.compareTo(other) > 0; + } + + public gte(other: FeatureVersion): boolean { + return this.compareTo(other) >= 0; + } + /** * Return true if this FeatureVersion is strictly greater than the provided one, * where ordering is meant by major and then minor number. diff --git a/internals-js/src/specs/federationSpec.ts b/internals-js/src/specs/federationSpec.ts index 70bb4be9c..2bb1909cf 100644 --- a/internals-js/src/specs/federationSpec.ts +++ b/internals-js/src/specs/federationSpec.ts @@ -119,7 +119,7 @@ export class FederationSpecDefinition extends FeatureDefinition { this.registerDirective(createDirectiveSpecification({ name: FederationDirectiveName.SHAREABLE, locations: [DirectiveLocation.OBJECT, DirectiveLocation.FIELD_DEFINITION], - repeatable: version >= (new FeatureVersion(2, 2)), + repeatable: version.gte(new FeatureVersion(2, 2)), })); this.registerSubFeature(INACCESSIBLE_VERSIONS.getMinimumRequiredVersion(version)); @@ -130,7 +130,7 @@ export class FederationSpecDefinition extends FeatureDefinition { args: [{ name: 'from', type: (schema) => new NonNullType(schema.stringType()) }], })); - if (version >= (new FeatureVersion(2, 1))) { + if (version.gte(new FeatureVersion(2, 1))) { this.registerDirective(createDirectiveSpecification({ name: FederationDirectiveName.COMPOSE_DIRECTIVE, locations: [DirectiveLocation.SCHEMA], @@ -139,7 +139,7 @@ export class FederationSpecDefinition extends FeatureDefinition { })); } - if (version >= (new FeatureVersion(2, 3))) { + if (version.gte(new FeatureVersion(2, 3))) { this.registerDirective(createDirectiveSpecification({ name: FederationDirectiveName.INTERFACE_OBJECT, locations: [DirectiveLocation.OBJECT], @@ -147,12 +147,12 @@ export class FederationSpecDefinition extends FeatureDefinition { this.registerSubFeature(TAG_VERSIONS.find(new FeatureVersion(0, 3))!); } - if (version >= (new FeatureVersion(2, 5))) { + if (version.gte(new FeatureVersion(2, 5))) { this.registerSubFeature(AUTHENTICATED_VERSIONS.find(new FeatureVersion(0, 1))!); this.registerSubFeature(REQUIRES_SCOPES_VERSIONS.find(new FeatureVersion(0, 1))!); } - if (version >= (new FeatureVersion(2, 6))) { + if (version.gte(new FeatureVersion(2, 6))) { this.registerSubFeature(POLICY_VERSIONS.find(new FeatureVersion(0, 1))!); } } diff --git a/internals-js/src/specs/joinSpec.ts b/internals-js/src/specs/joinSpec.ts index 2ee027db4..a7afa6c7a 100644 --- a/internals-js/src/specs/joinSpec.ts +++ b/internals-js/src/specs/joinSpec.ts @@ -83,7 +83,7 @@ export class JoinSpecDefinition extends FeatureDefinition { joinType.addArgument('extension', new NonNullType(schema.booleanType()), false); joinType.addArgument('resolvable', new NonNullType(schema.booleanType()), true); - if (this.version >= (new FeatureVersion(0, 3))) { + if (this.version.gte(new FeatureVersion(0, 3))) { joinType.addArgument('isInterfaceObject', new NonNullType(schema.booleanType()), false); } } @@ -93,7 +93,7 @@ export class JoinSpecDefinition extends FeatureDefinition { // The `graph` argument used to be non-nullable, but @interfaceObject makes us add some field in // the supergraph that don't "directly" come from any subgraph (they indirectly are inherited from // an `@interfaceObject` type), and to indicate that, we use a `@join__field(graph: null)` annotation. - const graphArgType = this.version >= (new FeatureVersion(0, 3)) + const graphArgType = this.version.gte(new FeatureVersion(0, 3)) ? graphEnum : new NonNullType(graphEnum); joinField.addArgument('graph', graphArgType); @@ -115,7 +115,7 @@ export class JoinSpecDefinition extends FeatureDefinition { joinImplements.addArgument('interface', new NonNullType(schema.stringType())); } - if (this.version >= (new FeatureVersion(0, 3))) { + if (this.version.gte(new FeatureVersion(0, 3))) { const joinUnionMember = this.addDirective(schema, 'unionMember').addLocations(DirectiveLocation.UNION); joinUnionMember.repeatable = true; joinUnionMember.addArgument('graph', new NonNullType(graphEnum));