Introduce VersionRange

[gastaldi]

This introduces a predicate that can be used for comparing if given a version is contained inside a version range pattern

[gastaldi]

@dmlloyd @aloubyansky I introduced an utility class to check the range introduced in quarkusio/quarkus#41908

[aloubyansky]

Looks nice @gastaldi, thanks! Could we also add tests involving version qualifiers? Right now the tests are limited to "Final" versions. So, for example, if a range starts with [1.0,...] then 1.0.0.Alpha3 shouldn't be accepted.

[aloubyansky]

But [1.0.a,..,] should include 1.0.0.Alpha3

[aloubyansky]

We may want to test SP and redhat in the mix as well, it's a known troublesome combination for dependabot, for example.

[aloubyansky]

The SP and redhat case would be more than what we currently need from this though. So shouldn't be a blocker if it appears to be problematic.

[gastaldi]

@aloubyansky I added more tests, see if they make sense

[aloubyansky]

Thanks, this looks good. What if you test 3.8.0.SP1-redhat-00001 for [3.8.0.redhat-00001,)?

[gastaldi]

It fails. Not sure why.

This returns 1:

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

[gastaldi]

I wonder if we shouldn't have a specific Red Hat VersionScheme for cases like these

[aloubyansky]

It's ok, it's not a blocker, I was just wondering. Dependabot has the same issue. We can fix it later. It'd be good to fix it though :)

[aloubyansky]

Maven resolver actually does handle it properly.

[gastaldi]

Hm, that sounds like a bug in MavenVersionScheme. WDYT @dmlloyd ?

[dmlloyd]

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]

[dmlloyd]

Make sure you aren't using the utilities in maven-artifact to test, because that is generally considered to be deprecated at this point (and IIUC is not used by Maven anymore).

[dmlloyd]

There are two main problems remaining that I can see.

The first problem is that of the parser. It operates by searching the same string ranges many times (using indexOf), and comparing indices to see which things come first (if they are found). It is hard to prove that such a parser is in fact correct. Normally a parser should proceed from left to right (or, rarely, from right to left) and visit each character once, making a decision at that point. In this way it is easier to verify that in some given parsing state, the successor states are all valid and make sense.

The second problem is the restriction classes, which implement a parallel variety of predicate compared to the whenXxx methods that I had you add. The point of adding these methods would be to have only one kind of predicate. For example, if I give a version range of (,1.2) then I'd want that to be exactly identical to calling whenLt("1.2"). As a side effect of this duplication, we have a couple extra classes, one of which is visible to the user.

I'd made an example commit here which you can feel free to use as you wish. This commit institutes a left-to-right parser and ensures there is only one way for the API to represent a range predicate.

One thing that this commit does not do, which we can look into if you think this would be a valid use case, is validate that a given version range is indeed valid (other than a couple very opportunistic checks). The reason for this again is that the predicate methods should behave identically to the range-parsed predicates. That is, if the parser rejects (2.0,1.0) then the predicate methods should also reject whenLt("2.0").and(whenGt("1.0")). In order for this to happen, the predicate methods would have to produce instances with a more comprehensive API (somewhat similar to your existing VersionRestriction class). I've left that off as something we could look into later, possibly. In my view, I think it would be totally valid for (2.0,1.0) to just yield () -> false (or something that behaves in an identical way), which is already what would happen if you combined the predicate methods (as they are currently defined) in an invalid way.

I hope this makes sense. Feel free to use some, all, or none of my example commit to get this over the line. Thanks!

[gastaldi]

@dmlloyd loved it! I've pushed your commit to my branch for your appreciation