smallrye / smallrye-common

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

Introduce `VersionRange` #330

Closed gastaldi closed 2 months ago

gastaldi commented 3 months ago

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

gastaldi commented 3 months ago

@dmlloyd @aloubyansky I introduced an utility class to check the range introduced in https://github.com/quarkusio/quarkus/pull/41908

aloubyansky commented 3 months ago

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 commented 3 months ago

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

aloubyansky commented 3 months ago

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

aloubyansky commented 3 months ago

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 commented 3 months ago

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

dmlloyd commented 2 months ago

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 commented 2 months ago

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