Avoid >= comparison for FeatureVersion objects (#2883)

This won't become a problem in practice until we have `major` or `minor`
version numbers greater than 9, because `toString`-based comparison of
single digits works as expected.

Author: benjamn

.changeset/beige-pumas-lick.md

@@ -0,0 +1,5 @@
+---
+"@apollo/federation-internals": patch
+---
+
+Avoid `>=` comparison for `FeatureVersion` objects

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<TestFeatureDefinition>('test')

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.

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,20 +139,20 @@ 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],
       }));
       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))!);
     }
   }

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));