Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add LexicographicSortedVersion #72

Merged
merged 2 commits into from
Jan 15, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
using NUnit.Framework;
using Octopus.Versioning.Unsortable;

namespace Octopus.Versioning.Tests.Unsortable;

public class UnsortableVersionCompareTests
{
static readonly UnsortableVersionParser UnsortableVersionParser = new();

[Test]
[TestCase("release", "release", 0)]
[TestCase("release", "qrelease", 1)]
[TestCase("release", "srelease", -1)]
[TestCase("123", "123", 0)]
[TestCase("123", "100", 1)]
[TestCase("123", "321", -1)]
[TestCase("123Release", "123Release", 0)]
[TestCase("123Release", "100Release", 1)]
[TestCase("123Release", "321Release", -1)]
[TestCase("release-1", "release-1", 0)]
[TestCase("release-1", "release-0", 1)]
[TestCase("release-1", "release-2", -1)]
[TestCase("release.1", "release.1", 0)]
[TestCase("release.1", "release.0", 1)]
[TestCase("release.1", "release.2", -1)]
[TestCase("release_1", "release_1", 0)]
[TestCase("release_1", "release_0", 1)]
[TestCase("release_1", "release_2", -1)]
[TestCase("release-1", "release_1", 0)]
[TestCase("release.1", "release_1", 0)]
[TestCase("release-1", "release_0", 1)]
[TestCase("release.1", "release_2", -1)]
[TestCase("release+123", "release+321", 0)]
[TestCase("release-1+123", "release-1+321", 0)]
[TestCase("release-1+123", "release-0+321", 1)]
[TestCase("release-1+123", "release-2+321", -1)]
public void TestComparisons(string version1, string version2, int result)
{
var parsedVersion1 = UnsortableVersionParser.Parse(version1);
var parsedVersion2 = UnsortableVersionParser.Parse(version2);
Assert.AreEqual(result, parsedVersion1.CompareTo(parsedVersion2));
}

[Test]
[TestCase("release-1", "release-1", true)]
[TestCase("release-1", "release-2", false)]
public void TestEquality(string version1, string version2, bool result)
{
var parsedVersion1 = UnsortableVersionParser.Parse(version1);
var parsedVersion2 = UnsortableVersionParser.Parse(version2);
Assert.AreEqual(result, Equals(parsedVersion1, parsedVersion2));
}

[Test]
public void TestGetHashCode()
{
var versionString = "release-1";
var parsedVersion = UnsortableVersionParser.Parse(versionString);

Assert.AreEqual(versionString.GetHashCode(), parsedVersion.GetHashCode());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
using System;
using NUnit.Framework;
using Octopus.Versioning.Unsortable;

namespace Octopus.Versioning.Tests.Unsortable;

[TestFixture]
public class UnsortableVersionParserTests
{
[Test]
// Release
[TestCase("foobar", "foobar", "")]
[TestCase("2db4a87840113c", "2db4a87840113c", "")]
[TestCase("123456", "123456", "")]
[TestCase("foobar-qwerty", "foobar-qwerty", "")]
[TestCase("foobar.qwerty", "foobar.qwerty", "")]
[TestCase("foobar_qwerty", "foobar_qwerty", "")]
[TestCase("foobar-12345", "foobar-12345", "")]
// Metadata
[TestCase("foobar+12345", "foobar", "12345")]
[TestCase("foobar+123.456", "foobar", "123.456")]
[TestCase("foobar+123_456", "foobar", "123_456")]
[TestCase("foobar+123-456", "foobar", "123-456")]
[TestCase("foobar+123+456", "foobar", "123+456")]
[TestCase("foobar+1.2_3-4+5", "foobar", "1.2_3-4+5")]
[TestCase("foobar+qwerty", "foobar", "qwerty")]
[TestCase("foobar-qwerty+12345", "foobar-qwerty", "12345")]
// Fail Cases
[TestCase("!@#$%^", "", "")]
[TestCase("foobar-!@#$%", "", "")]
[TestCase("foobar-qwerty+!@#$%", "", "")]
[TestCase("foo bar", "", "")]
[TestCase("foobar-qwe ty", "", "")]
[TestCase("foobar+123 456", "", "")]
[TestCase("foo bar-qwe ty+123 456", "", "")]
[TestCase("!foobar", "", "")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whats going on with these versions?
Rather than returning empty versions shouldn't we throw with an exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is using the TryParse method which catches the thrown exception and will return false + an empty version if it fails to parse, this follows the pattern from the other parsers. There is a few other tests in this suite that test the Parse method to see if it throws an exception

[TestCase("foo!bar", "", "")]
[TestCase("foobar!", "", "")]
public void ShouldParseSuccessfully(string input, string expectedRelease, string expectedMetadata)
{
_ = new UnsortableVersionParser().TryParse(input, out var parsedVersion);
AssertVersionNumbersAreZero(parsedVersion);
Assert.AreEqual(expectedRelease, parsedVersion.Release);
Assert.AreEqual(expectedMetadata, parsedVersion.Metadata);
}

[Test]
public void ShouldThrowExceptionOnEmptyInput()
{
var input = "";
Assert.Catch<ArgumentException>(() => new UnsortableVersionParser().Parse(input));
}

[Test]
public void ShouldThrowExceptionOnWhiteSpaceInput()
{
var input = " ";
Assert.Catch<ArgumentException>(() => new UnsortableVersionParser().Parse(input));
}

[Test]
public void ShouldThrowExceptionOnNullInput()
{
Assert.Catch<ArgumentException>(() => new UnsortableVersionParser().Parse(null));
}

[Test]
public void ShouldThrowExceptionOnFailureToParse()
{
var input = "bad versions string";
Assert.Catch<ArgumentException>(() => new UnsortableVersionParser().Parse(input));
}

void AssertVersionNumbersAreZero(UnsortableVersion version)
{
Assert.AreEqual(0, version.Major);
Assert.AreEqual(0, version.Minor);
Assert.AreEqual(0, version.Patch);
Assert.AreEqual(0, version.Revision);
}
}
136 changes: 136 additions & 0 deletions source/Octopus.Versioning/Unsortable/UnsortableVersion.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
using System;
using System.Collections.Generic;
using System.Linq;

namespace Octopus.Versioning.Unsortable
{
public class UnsortableVersion: IVersion
{
public UnsortableVersion(string release, string? metadata, string? originalString)
{
Metadata = metadata;
Release = release;
OriginalString = originalString ?? string.Empty;
}

public int Major => 0;
public int Minor => 0;
public int Patch => 0;
public int Revision => 0;
public bool IsPrerelease => false;
public IEnumerable<string> ReleaseLabels => Enumerable.Empty<string>();
public string? Metadata { get; }
public bool HasMetadata => !string.IsNullOrWhiteSpace(Metadata);
public string Release { get; }
public string OriginalString { get; }

public VersionFormat Format => VersionFormat.Unsortable;

public int CompareTo(object obj)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Borrowed the lexicographical sorting bits from OctopusVersion

{
if (!(obj is IVersion objVersion))
return -1;

if (string.Compare(Release.AlphaNumericOnly(), (objVersion.Release ?? string.Empty).AlphaNumericOnly(), StringComparison.Ordinal) != 0)
return CompareReleaseLabels(Release.AlphaNumericOnly().Split('.', '-', '_'), (objVersion.Release ?? string.Empty).AlphaNumericOnly().Split('.', '-', '_'));

return 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 Do we get any benefit from this? I thinking we could just return 0 every time

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, it sucks that IVersion is also IComparable 🤔

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to separate the two interfaces? So that IVersion does not directly extend IComparable.
This means that you'd have to go to all existing implementations of IVersion and add IComparable to it explicitly. This should give the benefit that you won't have to implement IComparable methods on Unsortable, which itself seems kind of misleading.

Alternatively, have another interface like ISortableVersion that inherits from IVersion and IComparable. That way a consumer of ISortableVersion knows for sure that whatever it is getting is sortable and can be ordered.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is possible but the effort required might be too great, there is quite a few places around Octopus and Calamari that expect IVersion to sortable e.g. https://github.com/OctopusDeploy/Calamari/blob/b63a7e5ed31644b85a3ade67534eae97b806b055/source/Calamari.Shared/Integration/Certificates/FileSystem/PackageStore.cs#L77 so we would end having to update all of these places well

I think UnsortableVersion is probably a bad name for this since it is technically sortable just only lexicographically and not by the version numbers so maybe we just need to make it obvious how it sorts, compared to the other version types.

@zentron Any thoughts on this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I don't like that we call this UnsortableVersion but then sort it.

The way I see it we have 2 options.

  1. Actually, separate out the interfaces as suggested by @kevjt
  2. Rename this version type to something like LexicographicSortedVersion and continue with this approach.

Off the top of my head, I can't think what downsides we would have with using the lexicographic versioning strategy for our commit hashes. We could get some slight performance improvement from not comparing something that can't be compared, but it's probably of minimal impact in the real world, all things considered.

Before moving forward with it, I'd be interested to see a quick spike in how many places would be impacted from a separate sortable interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To close this off, I did a quick spike to see how much effort would be required to split the interface and it's a decent amount, there is about 130 different usages of IVersion within Octopus Server and there is a few spots where the usages of IVersion it is always expected to be compared, an example of this would be the PackageCache where a package could be both IVersion and ISortableVersion so we would have to make some changes to functionality and it's hard to get insight to how that change would affect other places in code.

So we are continuing with option 2

}

public override string ToString()
{
return OriginalString;
}

public override bool Equals(object obj)
{
if (obj is IVersion objVersion)
return CompareTo(objVersion) == 0;

return false;
}

public override int GetHashCode()
{
return Release.GetHashCode();
}

/// <summary>
/// Compares sets of release labels.
/// </summary>
static int CompareReleaseLabels(IEnumerable<string> version1, IEnumerable<string> version2)
{
var result = 0;

using var a = version1.GetEnumerator();
using var b = version2.GetEnumerator();

var aExists = a.MoveNext();
var bExists = b.MoveNext();

while (aExists || bExists)
{
if (!aExists && bExists)
return -1;

if (aExists && !bExists)
return 1;

// compare the labels
result = CompareRelease(a.Current, b.Current);

if (result != 0)
return result;

aExists = a.MoveNext();
bExists = b.MoveNext();
}

return result;
}

/// <summary>
/// Release labels are compared as numbers if they are numeric, otherwise they will be compared
/// as strings.
/// </summary>
static int CompareRelease(string version1, string version2)
{
var version1Num = 0;
var version2Num = 0;
var result = 0;

// check if the identifiers are numeric
var v1IsNumeric = int.TryParse(version1, out version1Num);
var v2IsNumeric = int.TryParse(version2, out version2Num);

// if both are numeric compare them as numbers
if (v1IsNumeric && v2IsNumeric)
{
result = version1Num.CompareTo(version2Num);
}
else if (v1IsNumeric || v2IsNumeric)
{
// numeric labels come before alpha labels
if (v1IsNumeric)
result = -1;
else
result = 1;
}
else
{
// Ignoring 2.0.0 case sensitive compare. Everything will be compared case insensitively as 2.0.1 specifies.
var stringCompareResult = StringComparer.OrdinalIgnoreCase.Compare(version1, version2);
if (stringCompareResult < 0)
{
result = -1;
}
else if (stringCompareResult > 0)
{
result = 1;
}
}

return result;
}
}
}
56 changes: 56 additions & 0 deletions source/Octopus.Versioning/Unsortable/UnsortableVersionParser.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
using System;
using System.Text.RegularExpressions;

namespace Octopus.Versioning.Unsortable
{
public class UnsortableVersionParser
{
const string Release = "release";
const string Meta = "buildmetadata";

static readonly Regex VersionRegex = new Regex($@"^(?<{Release}>([A-Za-z0-9]*?)([.\-_\\]([A-Za-z0-9.\-_\\]*?)?)?)?" +
$@"(?:\+(?<{Meta}>[A-Za-z0-9_\-.\\+]*?))?\s*$"
);

public UnsortableVersion Parse(string? version)
{
if (string.IsNullOrWhiteSpace(version))
throw new ArgumentException("The version can not be an empty string");

var sanitisedVersion = version ?? string.Empty;
// SemVerFactory treated the original string as if it had no spaces at all
var noSpaces = sanitisedVersion.Replace(" ", "");

// We parse on the original string. This *does not* tolerate spaces in prerelease fields or metadata
// just like SemVerFactory.
var result = VersionRegex.Match(sanitisedVersion);

if (!result.Success)
throw new ArgumentException("The supplied version was not valid");

return new UnsortableVersion(
result.Groups[Release].Success ? result.Groups[Release].Value : string.Empty,
result.Groups[Meta].Success ? result.Groups[Meta].Value : string.Empty,
noSpaces
);
}

public bool TryParse(string version, out UnsortableVersion parsedVersion)
{
try
{
parsedVersion = Parse(version);
return true;
}
catch
{
parsedVersion = new UnsortableVersion(
string.Empty,
string.Empty,
null
);
return false;
}
}
}
}
22 changes: 22 additions & 0 deletions source/Octopus.Versioning/VersionFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using Octopus.Versioning.Maven;
using Octopus.Versioning.Octopus;
using Octopus.Versioning.Semver;
using Octopus.Versioning.Unsortable;

namespace Octopus.Versioning
{
Expand All @@ -19,6 +20,8 @@ public static IVersion CreateVersion(string input, VersionFormat format)
return CreateDockerTag(input);
case VersionFormat.Octopus:
return CreateOctopusVersion(input);
case VersionFormat.Unsortable:
return CreateUnsortableVersion(input);
default:
return CreateSemanticVersion(input);
}
Expand All @@ -34,6 +37,8 @@ public static IVersion CreateVersion(string input, VersionFormat format)
return TryCreateDockerTag(input);
case VersionFormat.Octopus:
return TryCreateOctopusVersion(input);
case VersionFormat.Unsortable:
return TryCreateUnsortableVersion(input);
default:
return TryCreateSemanticVersion(input);
}
Expand Down Expand Up @@ -150,5 +155,22 @@ public static IVersion CreateOctopusVersion(string input)
return null;
}
}

public static IVersion CreateUnsortableVersion(string input)
{
return new UnsortableVersionParser().Parse(input);
}

public static IVersion? TryCreateUnsortableVersion(string input)
{
try
{
return CreateUnsortableVersion(input);
}
catch
{
return null;
}
}
}
}
Loading
Loading