GH-1835: Expose qualifiers for generic version scheme - #1837
Conversation
This allows nice version selection. Also, provide a version filter that leaves out preview (sans snapshot) versions. Finally, made generic classes public, they were package protected. Fixes apache#1835
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Nice work extracting the qualifier logic into its own public class — the separation of concerns is clean, and the refactoring in GenericVersion.Tokenizer preserves semantic equivalence. A few observations below.
Claude Code on behalf of Guillaume Nodet
| // simple case: full qualifier label is present (assuming once) | ||
| for (Map.Entry<String, Integer> entry : QUALIFIERS.entrySet()) { | ||
| // TODO on map we have empty string "" as key | ||
| if (!entry.getKey().isEmpty() && v.contains(entry.getKey())) { |
There was a problem hiding this comment.
The v.contains(entry.getKey()) substring check can produce false positives for short qualifier labels. For example:
"1.0-arced"→ matches"rc"→ incorrectly detected as RC"1.0-legacy"→ matches"ga"→ incorrectly detected as GA"1.0-sacred"→ matches"cr"→ incorrectly detected as RC"1.0-dispatcher"→ matches"sp"→ incorrectly detected as SP
In practice Maven versions are fairly structured so the false positive rate may be low, but since this is a new public API (@since 2.0.17), it might be worth considering a tokenization-based or word-boundary approach (e.g., splitting on ., -, _ first and then doing exact matches on each token — similar to what tokenQualifier already does internally for the parsed path).
What do you think?
| QUALIFIERS.put("final", QUALIFIER_ZERO); | ||
| QUALIFIERS.put("release", QUALIFIER_ZERO); | ||
| QUALIFIERS.put("", QUALIFIER_ZERO); // TODO: is this entry valid? | ||
| QUALIFIERS.put("sp", QUALIFIER_SP); |
There was a problem hiding this comment.
nit: This TODO was carried over from the original internal code. Now that this class is part of the public API, it might be worth either resolving it (documenting why the empty-string entry exists) or removing it if it's no longer relevant.
| GenericQualifiersVersionFilter filter = GenericQualifiersVersionFilter.previewVersionFilter(); | ||
| assertSame(filter, derive(filter, "g:a:1")); | ||
| } | ||
| } |
There was a problem hiding this comment.
The filter-level tests are good and cover the main scenarios. It might be valuable to add direct unit tests for GenericQualifiers.qualifier(String) as well — particularly for edge cases like single-character versions, the short-form detection ("a1", "b2", "m3"), and version strings that could trigger the substring false positives mentioned in the other comment.
Tokenizer never could get to it. It was most probably a legacy.
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Looks good — clean extraction of qualifier logic into a public API with solid test coverage.
A few non-blocking observations:
- Qualifier map type change:
TreeMap(CASE_INSENSITIVE_ORDER)→LinkedHashMap+ explicittoLowerCase()— behaviorally equivalent for ASCII labels, just noting the mechanism changed. CharsetEncoderin loop (GenericVersionScheme.main()): could be hoisted above the loop, but negligible since it's CLI code.releasePreviewVersionFilter()naming: might surprise callers who expect snapshots to be filtered — a brief Javadoc note clarifying snapshots are intentionally retained could help.
Nice improvement to assertVersions() diagnostics too.
Claude Code on behalf of Guillaume Nodet
Do not mix snapshots into qualifier filtering. Yes, snapshot is also a qualifier but is much "bigger" than other qualifiers (and is much more known). By dropping it, thew new filter will not deal with them anymore, and will stop the possible confusion (and there are s and ns filters too). All in all, this simplifies things: "preview" versions are already somewhat documented, and they other set is now named "pre-release" versions (previews including rc/cr).
Agreed, and dropped 3rd filter that handled snapshots too, in hope to lessen confusion (multiple filters dealing with snapshots), as while snapshots are qualifier, they are also well known by users and well documented in Maven. Basically kept these two:
|
Rename existing PredicateVersionFilter to ArtifactPredicateVersionFilter to align it with existing VersionPredicateVersionFilter. Also, UTs were in wrong package (typo). Finally, fix snapshot and release filters, as they were wrong, they totally neglected timestamped snapshots, and also dash in "-SNAPSHOT" is not mandatory.
2 participants
This allows nice version selection. Also, provide a version filter that leaves out preview (sans snapshot) versions.
Finally, made generic classes public, they were package protected.
Fixes #1835