feat: correctly sort bazel module versions (#145)

Author: kormide

package.json

@@ -28,7 +28,6 @@
     "extract-zip": "^2.0.1",
     "gcp-metadata": "^6.0.0",
     "nodemailer": "^6.7.8",
-    "semver": "^7.5.4",
     "simple-git": "^3.16.0",
     "source-map-support": "^0.5.21",
     "tar": "^6.2.0",
@@ -43,7 +42,6 @@
     "@types/mailparser": "^3.4.4",
     "@types/node": "^18.6.2",
     "@types/nodemailer": "^6.4.5",
-    "@types/semver": "^7.5.6",
     "@types/source-map-support": "^0.5.4",
     "@types/tar": "^6.1.10",
     "@types/uuid": "^9.0.0",

src/domain/metadata-file.spec.ts

@@ -284,7 +284,7 @@ describe("constructor", () => {
     expect(metadata.maintainers[0].github).toBeUndefined();
   });
 
-  test("sorts versions by semver", () => {
+  test("sorts semver versions", () => {
     mockMetadataFile(`\
 {
     "homepage": "https://foo.bar",
@@ -333,8 +333,8 @@ describe("constructor", () => {
     ]);
   });
 
-  test("sorts non-semver versions above semver versions", () => {
-    // See: https://docs.bazel.build/versions/5.0.0/bzlmod.html#version-format
+  test("sorts versions with a different number of identifiers", () => {
+    // See: https://bazel.build/external/module#version_format
     mockMetadataFile(`\
 {
     "homepage": "https://foo.bar",
@@ -352,11 +352,11 @@ describe("constructor", () => {
 `);
     const metadata = new MetadataFile("metadata.json");
 
-    expect(metadata.versions).toEqual(["20210324.2", "1.0.0", "2.0.0"]);
+    expect(metadata.versions).toEqual(["1.0.0", "2.0.0", "20210324.2"]);
   });
 
-  test("sorts non-semver versions lexicographically", () => {
-    // See: https://docs.bazel.build/versions/5.0.0/bzlmod.html#version-format
+  test("sorts non-numeric versions lexicographically", () => {
+    // See: https://bazel.build/external/module#version_format
     mockMetadataFile(`\
 {
     "homepage": "https://foo.bar",
@@ -365,38 +365,16 @@ describe("constructor", () => {
         "github:bar/rules_foo"
     ],
     "versions": [
-      "55",
-      "12.4.2.1.1",
-      "20210324.2"
+      "xyz",
+      "abc.e",
+      "abc.d"
     ],
     "yanked_versions": {}
 }
 `);
     const metadata = new MetadataFile("metadata.json");
 
-    expect(metadata.versions).toEqual(["12.4.2.1.1", "20210324.2", "55"]);
-  });
-
-  test("sorts non-semver versions that look like semver as non-semver", () => {
-    // https://github.com/bazel-contrib/publish-to-bcr/issues/97
-    mockMetadataFile(`\
-{
-    "homepage": "https://foo.bar",
-    "maintainers": [],
-    "repository": [
-        "github:bar/rules_foo"
-    ],
-    "versions": [
-      "1.0.0-rc0",
-      "1.0.0-rc1",
-      "1.0.0rc1"
-    ],
-    "yanked_versions": {}
-}
-`);
-    const metadata = new MetadataFile("metadata.json");
-
-    expect(metadata.versions).toEqual(["1.0.0rc1", "1.0.0-rc0", "1.0.0-rc1"]);
+    expect(metadata.versions).toEqual(["abc.d", "abc.e", "xyz"]);
   });
 });
 

src/domain/metadata-file.ts

@@ -1,5 +1,5 @@
 import fs from "node:fs";
-import { compare as semverCompare, valid as validSemver } from "semver";
+import { compareVersions } from "./version.js";
 
 export class MetadataFileError extends Error {
   constructor(path: string, message: string) {
@@ -62,7 +62,7 @@ export class MetadataFile {
     }
 
     this.metadata = json;
-    this.sortVersions();
+    this.metadata.versions.sort(compareVersions);
   }
 
   public get maintainers(): ReadonlyArray<Maintainer> {
@@ -87,7 +87,7 @@ export class MetadataFile {
 
   public addVersions(...versions: ReadonlyArray<string>): void {
     this.metadata.versions.push(...versions);
-    this.sortVersions();
+    this.metadata.versions.sort(compareVersions);
   }
 
   public addYankedVersions(yankedVersions: {
@@ -135,20 +135,4 @@ export class MetadataFile {
 
     return [];
   }
-
-  private sortVersions(): void {
-    const semver = this.metadata.versions.filter(
-      (v: string) => !!validSemver(v, { loose: false })
-    );
-    const nonSemver = this.metadata.versions.filter(
-      (v: string) => !validSemver(v)
-    );
-
-    this.metadata.versions = [
-      ...nonSemver.sort(),
-      ...semver.sort((a: string, b: string) =>
-        semverCompare(a, b, { loose: false })
-      ),
-    ];
-  }
 }

src/domain/version.spec.ts

@@ -0,0 +1,72 @@
+import { compareVersions } from "./version";
+
+describe("compareVersions", () => {
+  it("should sort semvers", () => {
+    expect(
+      [
+        "2.0.0",
+        "1.0.0",
+        "0.32.1",
+        "0.32.11",
+        "2.11.0",
+        "1.0.0-rc1",
+        "1.0.0-rc0",
+        "1.0.0-rc23",
+        "1.0.1-rc1",
+        "2.10.1",
+      ].sort(compareVersions)
+    ).toEqual([
+      "0.32.1",
+      "0.32.11",
+      "1.0.0-rc0",
+      "1.0.0-rc1",
+      "1.0.0-rc23",
+      "1.0.0",
+      "1.0.1-rc1",
+      "2.0.0",
+      "2.10.1",
+      "2.11.0",
+    ]);
+  });
+
+  it("should sort versions with more than 3 components", () => {
+    expect(["6.4.0.2", "6.4.0", "6.4.0.2-rc0"].sort(compareVersions)).toEqual([
+      "6.4.0",
+      "6.4.0.2-rc0",
+      "6.4.0.2",
+    ]);
+  });
+
+  it("should sort duplciates", () => {
+    expect(["1.0.0", "2.0.0", "1.0.0"].sort(compareVersions)).toEqual([
+      "1.0.0",
+      "1.0.0",
+      "2.0.0",
+    ]);
+  });
+
+  it("should sort versions with non-numeric identifiers", () => {
+    expect(
+      ["z", "b.aa.b", "a.ab.b-rcfoo", "a.ab.b", "a.ab.a", "a.aa.b", "x.y"].sort(
+        compareVersions
+      )
+    ).toEqual([
+      "a.aa.b",
+      "a.ab.a",
+      "a.ab.b-rcfoo",
+      "a.ab.b",
+      "b.aa.b",
+      "x.y",
+      "z",
+    ]);
+  });
+
+  it("should sort numeric and non-numeric identifiers", () => {
+    expect(["x.7.z", "1.2.3", "x.6.y", "a.b.c"].sort(compareVersions)).toEqual([
+      "1.2.3",
+      "a.b.c",
+      "x.6.y",
+      "x.7.z",
+    ]);
+  });
+});

src/domain/version.ts

@@ -0,0 +1,93 @@
+/**
+ * Compare bazel module versions
+ *
+ * Adapted from https://github.com/bazelbuild/bazel-central-registry/blob/127d91703baf4e39eb66fc907d255b37d6162792/tools/registry.py#L85
+ */
+export function compareVersions(a: string, b: string) {
+  return Version.compare(new Version(a), new Version(b));
+}
+
+class Version {
+  public static compare(a: Version, b: Version): number {
+    const result = Version.compareIdentifiers(a.release, b.release);
+    if (result) {
+      return result;
+    }
+
+    if (a.prerelease.length === 0) {
+      return 1;
+    }
+    if (b.prerelease.length === 0) {
+      return -1;
+    }
+
+    return Version.compareIdentifiers(a.prerelease, b.prerelease);
+  }
+
+  private static compareIdentifiers(a: Identifier[], b: Identifier[]) {
+    const l = Math.min(a.length, b.length);
+    for (let i = 0; i < l; i++) {
+      const result = Identifier.compare(a[i], b[i]);
+      if (result) {
+        return result;
+      }
+    }
+
+    if (a.length > b.length) {
+      return 1;
+    } else if (b.length > a.length) {
+      return -1;
+    }
+
+    return 0;
+  }
+
+  private readonly prerelease: Identifier[];
+  private readonly release: Identifier[];
+
+  public constructor(version: string) {
+    const pattern =
+      /^([a-zA-Z0-9.]+)(?:-([a-zA-Z0-9.-]+))?(?:\+[a-zA-Z0-9.-]+)?$/;
+    const match = version.match(pattern);
+    if (!match) {
+      throw new Error(`Invalid module version '${version}'`);
+    }
+
+    this.release = this.convertToIdentifiers(match[1]);
+    this.prerelease = this.convertToIdentifiers(match[2]);
+  }
+
+  private convertToIdentifiers(version: string): Identifier[] {
+    return (version && version.split(".").map((i) => new Identifier(i))) || [];
+  }
+}
+
+class Identifier {
+  public static compare(a: Identifier, b: Identifier): number {
+    if (typeof a.value !== typeof b.value) {
+      if (typeof a.value === "number") {
+        return -1;
+      } else {
+        return 1;
+      }
+    }
+
+    if (typeof a.value === "string") {
+      if (a.value < b.value) {
+        return -1;
+      } else if (a.value === b.value) {
+        return 0;
+      }
+      return 1;
+    } else {
+      return a.value - (b.value as number);
+    }
+  }
+
+  private readonly value: string | number;
+
+  public constructor(value: string) {
+    const numeric = parseInt(value);
+    this.value = isNaN(numeric) ? value : numeric;
+  }
+}

yarn.lock

@@ -1348,11 +1348,6 @@
     "@types/tough-cookie" "*"
     form-data "^2.5.0"
 
-"@types/semver@^7.5.6":
-  version "7.5.6"
-  resolved "https://registry.yarnpkg.com/@types/semver/-/semver-7.5.6.tgz#c65b2bfce1bec346582c07724e3f8c1017a20339"
-  integrity sha512-dn1l8LaMea/IjDoHNd9J52uBbInB796CDffS6VdIxvqYCPSG0V0DzHp76GpaWnlhg88uYyPbXCDIowa86ybd5A==
-
 "@types/send@*":
   version "0.17.4"
   resolved "https://registry.yarnpkg.com/@types/send/-/send-0.17.4.tgz#6619cd24e7270793702e4e6a4b958a9010cfc57a"