Auto merge of #3304 - plaets:issue-3294, r=Turbo87

Improve invalid semver string handling

* Add a tooltip message and a new release track indicator (`?`) to components/version-list/row
* Handle null returned from `semverParse` in models/version
* Add `loose` option to `semverParse` (https://github.com/npm/node-semver#functions)

I placed the checks so that "invalid version" has higher priority than "first version" but lower than "yanked".
I also modified css of the version list so that long versions do not overflow from tooltips.
Before
![image](https://user-images.githubusercontent.com/49844988/108100395-272b2680-7086-11eb-838b-7341c16ee4b1.png)
After
![image](https://user-images.githubusercontent.com/49844988/108100455-37db9c80-7086-11eb-9159-2565a90440df.png)

Fixes #3294

Author: bors

app/components/download-graph.js

@@ -108,7 +108,7 @@ export default class DownloadGraph extends Component {
 
     let versionsList = [...versions];
     try {
-      semverSort(versionsList);
+      semverSort(versionsList, { loose: true });
     } catch {
       // Catches exceptions thrown when a version number is invalid
       // see issue #3295

app/components/version-list/row.hbs

@@ -9,16 +9,18 @@
   ...attributes
 >
   <div local-class="version">
-    <div local-class="release-track">
+    <div local-class="release-track" data-test-release-track>
       {{#if @version.yanked}}
         {{svg-jar "trash"}}
+      {{else if @version.invalidSemver}}
+        ?
       {{else if @version.isFirst}}
         {{svg-jar "star"}}
       {{else}}
         {{@version.releaseTrack}}
       {{/if}}
 
-      <EmberTooltip @text={{this.releaseTrackTitle}} @side="right" />
+      <EmberTooltip @text={{this.releaseTrackTitle}} @side="right" data-test-release-track-title/>
     </div>
 
     <LinkTo
@@ -27,6 +29,7 @@
       local-class="num-link"
       {{on "focusin" (fn this.setFocused true)}}
       {{on "focusout" (fn this.setFocused false)}}
+      data-test-release-track-link
     >
       {{@version.num}}
     </LinkTo>

app/components/version-list/row.js

@@ -13,6 +13,9 @@ export default class VersionRow extends Component {
     if (version.yanked) {
       return 'This version was yanked';
     }
+    if (version.invalidSemver) {
+      return `Failed to parse version ${version.num}`;
+    }
     if (version.isFirst) {
       return 'This is the first version that was released';
     }

app/components/version-list/row.module.css

@@ -46,6 +46,10 @@
         z-index: 1;
         cursor: help;
     }
+
+    :global(.ember-tooltip) {
+        word-break: break-all;
+    }
 }
 
 .version {

app/models/version.js

@@ -44,29 +44,41 @@ export default class Version extends Model {
     return oldestVersion === this;
   }
 
-  @cached get semver() {
-    return semverParse(this.num);
+  get semver() {
+    return semverParse(this.num, { loose: true });
+  }
+
+  get invalidSemver() {
+    return this.semver === null;
   }
 
   get isPrerelease() {
+    if (this.invalidSemver) {
+      return false;
+    }
+
     return this.semver.prerelease.length !== 0;
   }
 
   get releaseTrack() {
+    if (this.invalidSemver) {
+      return null;
+    }
+
     let { semver } = this;
     return `${semver.major}.${semver.major === 0 ? semver.minor : 'x'}`;
   }
 
   @cached get isHighestOfReleaseTrack() {
-    if (this.isPrerelease) {
+    if (this.isPrerelease || this.invalidSemver) {
       return false;
     }
 
     let { crate, semver, releaseTrack } = this;
     let { versions } = crate;
     // find all other non-prerelease versions on the same release track
     let sameTrackVersions = versions.filter(
-      it => it !== this && !it.yanked && !it.isPrerelease && it.releaseTrack === releaseTrack,
+      it => it !== this && !it.yanked && !it.isPrerelease && !it.invalidSemver && it.releaseTrack === releaseTrack,
     );
     // check if we're the "highest"
     return sameTrackVersions.every(it => it.semver.compare(semver) === -1);

mirage/serializers/crate.js

@@ -56,9 +56,9 @@ export default BaseSerializer.extend({
     versions = versions.filter(it => !it.yanked);
 
     let versionNums = versions.models.map(it => it.num);
-    semverSort(versionNums);
+    semverSort(versionNums, { loose: true });
     hash.max_version = versionNums[0] ?? '0.0.0';
-    hash.max_stable_version = versionNums.find(it => !prerelease(it)) ?? null;
+    hash.max_stable_version = versionNums.find(it => !prerelease(it, { loose: true })) ?? null;
 
     let newestVersions = versions.models.sort((a, b) => compareIsoDates(b.updated_at, a.updated_at));
     hash.newest_version = newestVersions[0]?.num ?? '0.0.0';

tests/components/version-list-row-test.js

@@ -0,0 +1,47 @@
+import { render } from '@ember/test-helpers';
+import { module, test } from 'qunit';
+
+import { hbs } from 'ember-cli-htmlbars';
+
+import { setupRenderingTest } from 'cargo/tests/helpers';
+
+import setupMirage from '../helpers/setup-mirage';
+
+module('Component | VersionList::Row', function (hooks) {
+  setupRenderingTest(hooks);
+  setupMirage(hooks);
+
+  test('handle non-standard semver strings', async function (assert) {
+    let crate = this.server.create('crate', { name: 'foo' });
+    this.server.create('version', { crate, num: '0.4.0-alpha.01', created_at: Date.now(), updated_at: Date.now() });
+    this.server.create('version', { crate, num: '0.3.0-alpha.01', created_at: Date.now(), updated_at: Date.now() });
+
+    let store = this.owner.lookup('service:store');
+    let crateRecord = await store.findRecord('crate', crate.name);
+    let versions = (await crateRecord.versions).toArray();
+    this.firstVersion = versions[0];
+    this.secondVersion = versions[1];
+
+    await render(hbs`<VersionList::Row @version={{this.firstVersion}} />`);
+    assert.dom('[data-test-release-track] svg').exists();
+    assert.dom('[data-test-release-track-link]').hasText('0.4.0-alpha.01');
+
+    await render(hbs`<VersionList::Row @version={{this.secondVersion}} />`);
+    assert.dom('[data-test-release-track]').hasText('0.3');
+    assert.dom('[data-test-release-track-link]').hasText('0.3.0-alpha.01');
+  });
+
+  test('handle node-semver parsing errors', async function (assert) {
+    let crate = this.server.create('crate', { name: 'foo' });
+    let version = '18446744073709551615.18446744073709551615.18446744073709551615';
+    this.server.create('version', { crate, num: version });
+
+    let store = this.owner.lookup('service:store');
+    let crateRecord = await store.findRecord('crate', crate.name);
+    this.version = (await crateRecord.versions).toArray()[0];
+
+    await render(hbs`<VersionList::Row @version={{this.version}} />`);
+    assert.dom('[data-test-release-track]').hasText('?');
+    assert.dom('[data-test-release-track-link]').hasText(version);
+  });
+});

tests/models/version-test.js

@@ -78,19 +78,38 @@ module('Model | Version', function (hooks) {
       assert.true(isPrerelease);
       assert.strictEqual(releaseTrack, '0.0');
     });
+
+    test('parses 0.3.0-alpha.01 (non-standard) correctly', async function (assert) {
+      let { semver, releaseTrack, isPrerelease } = await prepare(this, { num: '0.3.0-alpha.01' });
+      assert.strictEqual(semver.major, 0);
+      assert.strictEqual(semver.minor, 3);
+      assert.strictEqual(semver.patch, 0);
+      assert.deepEqual(semver.prerelease, ['alpha', 1]);
+      assert.true(isPrerelease);
+      assert.strictEqual(releaseTrack, '0.3');
+    });
+
+    test('invalidSemver is true for unparseable versions', async function (assert) {
+      let { invalidSemver } = await prepare(this, {
+        num: '18446744073709551615.18446744073709551615.1844674407370955161',
+      });
+      assert.true(invalidSemver);
+    });
   });
 
   module('isHighestOfReleaseTrack', function () {
     test('happy path', async function (assert) {
       let nums = [
         '0.4.0-rc.1',
+        '0.3.24-alpha.02',
         '0.3.23',
         '0.3.22',
         '0.3.21-pre.0',
         '0.3.20',
         '0.3.3',
         '0.3.2',
         '0.3.1',
+        '0.3.0-alpha.01',
         '0.3.0',
         '0.2.1',
         '0.2.0',
@@ -110,13 +129,15 @@ module('Model | Version', function (hooks) {
         versions.map(it => ({ num: it.num, isHighestOfReleaseTrack: it.isHighestOfReleaseTrack })),
         [
           { num: '0.4.0-rc.1', isHighestOfReleaseTrack: false },
+          { num: '0.3.24-alpha.02', isHighestOfReleaseTrack: false },
           { num: '0.3.23', isHighestOfReleaseTrack: true },
           { num: '0.3.22', isHighestOfReleaseTrack: false },
           { num: '0.3.21-pre.0', isHighestOfReleaseTrack: false },
           { num: '0.3.20', isHighestOfReleaseTrack: false },
           { num: '0.3.3', isHighestOfReleaseTrack: false },
           { num: '0.3.2', isHighestOfReleaseTrack: false },
           { num: '0.3.1', isHighestOfReleaseTrack: false },
+          { num: '0.3.0-alpha.01', isHighestOfReleaseTrack: false },
           { num: '0.3.0', isHighestOfReleaseTrack: false },
           { num: '0.2.1', isHighestOfReleaseTrack: true },
           { num: '0.2.0', isHighestOfReleaseTrack: false },