David Smiley created SOLR-18432:
-----------------------------------
Summary: SolrVersion problems with prerelease (SNAPSHOT)
Key: SOLR-18432
URL: https://issues.apache.org/jira/browse/SOLR-18432
Project: Solr
Issue Type: Bug
Reporter: David Smiley
{{SolrVersion.LATEST_STRING}} is a hardcoded constant (rewritten by
{{{}dev-tools/scripts/addVersion.py{}}}) that never carries a qualifier. The
Gradle build version, by contrast, defaults to {{-SNAPSHOT}}
({{{}build.gradle{}}}: {{baseVersion}} + {{{}version.suffix{}}}), and only an
actual release build overrides it via {{{}-Dversion.release=x.y.z{}}}.
So on every branch, between releases, the two disagree: the build produces
{{10.1.0-SNAPSHOT}} while {{SolrVersion.LATEST}} claims to be {{{}10.1.0{}}}.
Note this is not a main-vs-release-branch distinction – {{branch_10_0}} is
equally {{10.0.1-SNAPSHOT}} between bugfix releases. Nothing enforces
consistency between {{{}build.gradle{}}}'s {{baseVersion}} and
{{{}LATEST_STRING{}}}.
Arguably {{LATEST_STRING}} should carry the same {{-SNAPSHOT}} suffix the build
does, so that a dev build does not misrepresent itself as a release. Before
that can even be considered, however, {{SolrVersion}} and its callers have to
tolerate a prerelease qualifier – and today they do not.
h2. Problems
Verified against semver4j 6.0.0 (the backing library). Parsing itself is fine:
{{new Semver("10.1.0-SNAPSHOT")}} works and major/minor/patch are still 10/1/0.
Build metadata ({{{}+b123{}}}) is harmless. Prerelease qualifiers are not.
h3. 1. satisfies() rejects everything
semver4j follows node-semver semantics, where a prerelease version satisfies no
range unless the range itself names a prerelease:
{code:java}
new Semver("10.1.0-SNAPSHOT").satisfies(">=9.0.0") -> false
.satisfies("10.x") -> false
.satisfies("~10.1") -> false
{code}
{color:#ffab00}_(If you read that and aren't shocked; nothing can shock
you!)_{color}
{{PackageManager}} (~line 941) gates package installs on
{{{}SolrVersion.LATEST.satisfies(manifest.versionConstraint){}}}. With a
suffixed {{{}LATEST{}}}, *every* package would be rejected on a dev build. This
is the hard blocker.
That rule exists for npm dependency resolution, where you must not silently
pull a prerelease. It is the wrong rule for "which Solr line is this node".
h3. 2. Ordering makes a dev build look older than the release
Per semver, {{{}10.1.0-SNAPSHOT < 10.1.0{}}}.
{{ZkController.checkClusterVersionCompatibility}} compares
{{SolrVersion.LATEST}} against the lowest version among live nodes and warns
"our version is older than cluster version". A dev node joining a cluster of
released nodes of the same x.y would trip that warning, even though it is in
fact the newer code.
Not fatal – the refuse-to-start guards compare major and minor as ints, which
prerelease does not affect – but the warning is wrong and the reported "lowest
version in cluster" from {{ZkStateReader.fetchLowestSolrVersion}} becomes
misleading in a mixed dev/release cluster.
h3. 3. getPrereleaseVersion() misused as an implementation version
{{{}SystemInfoProvider.getLuceneInfo(){}}}:
{code:java}
info.solrImplVersion = implVersion == null ?
SolrVersion.LATEST.getPrereleaseVersion() : implVersion;
{code}
This is an independent pre-existing bug – a wrong method pick introduced in
SOLR-16458 (PR #4078), not something the suffix causes. Today it happens to
return {{""}} because {{LATEST}} has no qualifier, so nobody noticed. With a
suffix it would report {{"SNAPSHOT"}} as the Solr implementation version over
the {{/api/node/system}} API. {{NodeSystemInfoProviderTest}} currently asserts
this behaviour.
h2. Suggested direction
Fix the three breakages first; they are worth fixing on their own merits
regardless of whether {{LATEST_STRING}} ever grows a suffix.
* Add {{{}SolrVersion.withoutPrerelease(){}}}, backed by semver4j's
{{{}withClearedPreRelease(){}}}.
* Have {{satisfies()}} evaluate on the cleared version, documented as ignoring
the qualifier. This fixes {{PackageManager}} and keeps future callers off the
same landmine. (Alternative: leave {{satisfies()}} literal and strip at the
{{PackageManager}} call site – more explicit, less protective.)
* Leave {{{}compareTo{}}}/{{{}equals{}}} as strict semver; they are public API
and {{-SNAPSHOT < release}} is correct there. Instead normalise at the one
ordering site that cares, {{{}ZkController{}}}'s {{lessThan}} check, while
still printing full versions in the log and exception messages.
* Fix the {{solrImplVersion}} fallback to use {{{}LATEST_STRING{}}}, and
update the test.
Coverage note: there is no clean way to integration-test the {{ZkController}}
fix, since tripping it requires *our own* version to be a prerelease and
{{SolrVersion.LATEST}} is a compile-time constant with no injection seam.
{{ZkControllerTest}} can only fabricate the cluster's version, which is the
non-tripping direction. Coverage would land in {{TestSolrVersion}} unless we
think a seam is worth adding. (DWS: not in favor of a seam)
Whether {{LATEST_STRING}} should then actually carry {{{}-SNAPSHOT{}}}, and how
the release process would strip it, is deliberately left to a follow-up. The
likely cleanest answer is to stop hardcoding it and generate it from the Gradle
version, since {{-Dversion.release}} already produces exactly the right value
for a release build and would need no release-wizard changes at all.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
