smallrye / smallrye-common

Common utilities for SmallRye
Apache License 2.0
20 stars 24 forks source link

VersionScheme.MAVEN version comparison fails #333

Open gastaldi opened 1 month ago

gastaldi commented 1 month ago

Given that 3.8.0.redhat-00001 is less than 3.8.0.SP1-redhat-00001, the following snippet returns 1 (expected is -1):

VersionScheme.MAVEN.compare("3.8.0.redhat-00001", "3.8.0.SP1-redhat-00001")

_Originally posted by @gastaldi in https://github.com/smallrye/smallrye-common/pull/330#discussion_r1683430729_

dmlloyd commented 1 month ago

I posted this in the other thread:

I believe that custom qualifiers come after predefined qualifiers (like SP) according to the Maven rules.

$ jbang --main=org.eclipse.aether.util.version.GenericVersionScheme org.apache.maven.resolver:maven-resolver-util:1.9.6 1.redhat 1.sp
Display parameters as parsed by Maven Resolver (in canonical form and as a list of tokens) and comparison result:
1. 1.redhat -> 1.redhat; tokens: [1, redhat]
   1.redhat > 1.sp
2. 1.sp -> 1.sp; tokens: [1, 1]
$ jbang --main=org.eclipse.aether.util.version.GenericVersionScheme org.apache.maven.resolver:maven-resolver-util:1.9.6 3.8.0.redhat-00001 3.8.0.SP1-redhat-00001
Display parameters as parsed by Maven Resolver (in canonical form and as a list of tokens) and comparison result:
1. 3.8.0.redhat-00001 -> 3.8.0.redhat-00001; tokens: [3, 8, redhat, 1]
   3.8.0.redhat-00001 > 3.8.0.SP1-redhat-00001
2. 3.8.0.SP1-redhat-00001 -> 3.8.0.SP1-redhat-00001; tokens: [3, 8, 1, 1, redhat, 1]

This was also verified using 1.9.7 and 1.9.8. In light of this fact, I think this is not a bug, WDYT?

gastaldi commented 1 month ago

I don't know, I believe that the absence of a predefined qualifier should assume Final or RELEASE for comparison purposes.

Any thoughts @aloubyansky?

dmlloyd commented 1 month ago

Well, that would be nice. :-)

However, Maven doesn't have any way to know whether you intend to put Final/0/etc. before a qualifier. It only has rules for leading zeros and trailing zeros. Additionally, it appears to be the intent of the Maven developers that . and - (and the empty separator) will become equivalent.

What we are doing, when we add our redhat qualifier, is we're basically saying "here is our special version that comes immediately after the version we qualify". But syntactically, this is not actually what happens. The only way to do such a thing is to add 0.0.0.0.0.... (to infinity) in between the original version number and our qualifier, which is not ideal. Maven general qualifiers were not really intended for this situation, hence our current troubles.

Addressing a syntactic weakness like this is actually very difficult, because if you do change the rules for this use case, then you will likely break some (or many) other use cases. Thus we have to look at bugs such as this one as "we made a wrong assumption about how this works" rather than "it doesn't work the way we want, so it should be fixed".

In practice, I think adding a 0 or Final when productizing versions of things which do not have (standard) qualifiers before redhat is probably the right solution.

aloubyansky commented 1 month ago

Here is an implementation from the Maven resolver, to compare.

    public static void main(String[] args) throws Exception {
        compare("3.8.5", "3.8.5.redhat-00005");
        compare("3.8.5.SP1", "3.8.5.redhat-00005");
        compare("3.8.5.SP1", "3.8.5.SP1-redhat-00005");
        compare("3.8.5.SP2", "3.8.5.SP1-redhat-00005");
    }

    private static void compare(String v1, String v2) throws InvalidVersionSpecificationException {
        System.out.print(v1);
        var versionScheme = new org.eclipse.aether.util.version.GenericVersionScheme();
        final Version version1 = versionScheme.parseVersion(v1);
        final Version version2 = versionScheme.parseVersion(v2);
        final int result = version1.compareTo(version2);
        if(result < 0) {
            System.out.print(" < ");
        } else if(result == 0) {
            System.out.print(" = ");
        } else {
            System.out.print(" > ");
        }
        System.out.println(v2);
    }

results in

3.8.5 < 3.8.5.redhat-00005
3.8.5.SP1 < 3.8.5.redhat-00005
3.8.5.SP1 < 3.8.5.SP1-redhat-00005
3.8.5.SP2 > 3.8.5.SP1-redhat-00005

Where 3.8.5.SP1 < 3.8.5.redhat-00005 is not correct.

aloubyansky commented 1 month ago

From the two implementations, I'd probably pick org.eclipse.aether.util.version.GenericVersionScheme. At least it seems to pick the latest downstream version.

dmlloyd commented 1 month ago

To be perfectly clear, my jbang examples above were also from the Maven resolver, same class. So we agree that Maven presently considers 3.8.5.SP1 < 3.8.5.redhat-00005, and the SmallRye version implementation does also? Thus I would consider this "not a bug", though we might also consider it "not very nice" as well.

gastaldi commented 1 month ago

I'd say that Maven is wrong and so is Smallrye for not considering an empty milestone as a release during comparison 😀

aloubyansky commented 1 month ago

Thank you both :)