Don't raise a shared version property to a version its siblings never published - #8727
Open
MBoegers wants to merge 8 commits into
Open
Don't raise a shared version property to a version its siblings never published#8727MBoegers wants to merge 8 commits into
MBoegers wants to merge 8 commits into
Conversation
Pins both directions of the shared-property gate per POM shape: the decouple that #8656 asks for, and the property bump that must survive it. The Gradle equivalent (#7491) shipped only the first direction and had to be relaxed ten days later in #7694; nesting the pair per shape makes that omission visible.
`UpgradeDependencyVersion` wrote the new version into a `<properties>` entry without checking the other artifacts resolving through that property, leaving those siblings unresolvable. Now the dependencies consuming each property are collected during the scan, and when the new version is not published for all of them the targeted dependency is decoupled from the property with an explicit `<version>` instead.
Route the plugin dependency and annotation processor path upgrades through the same seam as regular dependencies, so those too decouple from a shared property rather than raising it to a version their siblings lack; the paths are now recorded as property consumers as well. Identify a property by the POM that declares it, so same-named properties in unrelated modules no longer block one another, and answer the "is this version published" probe once per coordinate.
A glob can target every consumer of one property. Before the shared-property check, the competing property writes raced and the last one won, breaking both dependencies including the one the recipe was asked to upgrade; checking the new version against every consumer subsumes the separate agreement phase the Gradle recipe spells out. The divergent case is asserted by shape, since '2.x' tracks whatever Jackson last published.
Every source file writes to the one accumulator, and a runtime that visits source files in parallel writes to it from several threads at once. The version-existence verdicts are worse than the rest: they are written during the edit phase and their value comes from a download, so the window is as wide as the network is slow. Backed by HashMap and HashSet those writes silently drop entries — 880 of 4000 in the accompanying test. A dropped entry in the consumer index is not a crash but a missing sibling, which is exactly how a shared property gets raised to a version that sibling never published, so the corruption reinstates the bug the check exists to prevent. The probe is also lifted out of computeIfAbsent. A mapping function that blocks on a download holds its bin against every other writer, and the contract asks that it be short and touch nothing else; losing the race merely repeats a probe that the pom cache already dedupes, and the winning verdict is returned so all callers still agree.
A property can be consumed by a module of the build as well as by an external artifact, and a module is the one consumer no repository can answer for: probing it returns 404 whether or not the bump is safe. Exempting it keeps that 404 from blocking every property a module touches, but raises the property out from under the module, which breaks it exactly as it breaks an external sibling — the reported failure with an in-repo sibling in place of a published one. A module is now checked against the version it already carries, which needs no repository. The gate has no branch that lets a module consumer through, since a module's version does not coincide with an arbitrary new version of an external artifact; what keeps the rule from reaching builds it has no business touching is the multi-module property bump, which is asserted separately.
Partitioning the input showed three classes nothing exercised: a BOM import sharing the property with an artifact versioned on a different scheme, a managed entry reached from a dependency that declares no version of its own, and a version that merely contains a placeholder rather than being one. The first two behave correctly and are now held there; the third does nothing, and the boundary is pinned because the obvious repair would write through the gate.
A metadata download that fails is not evidence that a version is missing, so the check answers as though it were present. That is the safe default — the opposite would decouple every shared property on a degraded run — but it silently returns the check to the behaviour it exists to replace, and nothing in the output says which of the two happened. The probe was the one download in this recipe bypassing the failures table it already declares, so a run can now be asked afterwards whether the check actually had anything to go on.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
UpgradeDependencyVersionwrites the new version into a<properties>entry without checking that the version exists for the other artifacts resolving through that property. Every sibling reading the property is left pointing at a version that was never published, and the POM stops resolving.The failure surfaces as
Unable to download POM: com.fasterxml.jackson.core:jackson-core:2.21, and the same for every other artifact sharing that${jackson.version}.UpgradeJackson_2_3_Dependenciespinsjackson-annotationsto2.21, which is a real release ofjackson-annotationsand of nothing else — it moved to bare minor versions at 2.20 while the rest of Jackson 2.x kept three parts. The recipe isn't at fault; any exact version that only one consumer of a shared property publishes does this.Before this change the property becomes
2.21andjackson-coreis unresolvable.Behaviour
When the new version is not published for every consumer of the property, the targeted dependency is decoupled instead: an explicit
<version>on the dependency or on itsdependencyManagemententry, with the property untouched. Properties with a single consumer behave exactly as before. This is already whatrewrite-gradledoes for a shared version variable, so the two stay aligned on identical input.Three things beyond the report
A glob breaks the artifact it was asked to upgrade. With
com.fasterxml.jackson.core:*, the competing property writes race and the last one wins, sojackson-annotationsends up broken alongsidejackson-core. Checking the new version against every consumer rather than only the untargeted ones subsumes the separate agreement phase the Gradle recipe spells out, and both artifacts now decouple to versions that exist.A module of the build is a sibling too. Modules were exempt from the existence check, which is tempting because a module publishes nothing to probe and would 404 every time. But exempting it raises the property out from under the module and breaks it exactly as it breaks an external artifact — the same failure with an in-repo sibling. A module is now checked against the version it already carries, which needs no repository.
The accumulator could not survive the runtime it runs in. Every source file writes to it, and a runtime that visits source files in parallel writes from several threads at once. The version-existence verdicts are the worst of them: written during the edit phase with a value that comes from a download, so the window is as wide as the network is slow. Backed by
HashMapandHashSetthose writes drop entries — 880 of 4000 in the accompanying test — and a dropped entry in the consumer index is a missing sibling, which is how a property gets raised to a version that sibling never published. The corruption reinstates the bug the check exists to prevent, only under the parallel runner where no test was looking.The probe is also lifted out of
computeIfAbsent, whose contract asks for a short mapping function; one that blocks on a download holds its bin against every other writer.Tests
UpgradeDependencyVersionSharedPropertyTestpins both directions per POM shape — the decouple, and the property bump that has to survive it. The pairing is the point. The Gradle equivalent (UpgradeDependencyVersion (Gradle): respect shared version variables #7491) shipped with three tests, all asserting that a shared variable is not bumped when that would break a sibling and none asserting that it still is when it wouldn't, so the gate it introduced was free to be far stricter than intended and had to be relaxed ten days later in UpgradeDependencyVersion for Gradle preserves shared version variable references when upgrading dependencies #7694. Nesting the two directions together makes shipping half a gate visible at a glance.Partitioning the input turned up three more classes nothing reached: a BOM import sharing a property with an artifact on a different version scheme, a managed entry reached from a dependency that declares no version of its own, and a version that merely contains a placeholder rather than being one. The first two were already correct and are now held there; the third does nothing, and the boundary is pinned because the obvious repair reaches for a property that does not exist and would write straight through the gate.
Full
:rewrite-maven:testis green — 1541 tests.Two deliberate choices worth a reviewer's attention
A metadata download that fails is answered as though the version were present. Failing closed would decouple every shared property on a degraded run, which is the far more expensive mistake. The cost is that a network-degraded run quietly reverts to the old behaviour, so the probe now goes through the
MavenMetadataFailurestable the recipe already declares — it was the one download bypassing it — and a run can be asked afterwards whether the check had anything to go on. Narrowing this further, by treating a clean 404 from every repository as evidence while a transport error stays open, is the obvious next step and is deliberately not here: it is the piece with real over-correction risk and nothing to test it against yet.Not in here
propertyKeyfalls back to a null path when no POM in the sources declares the property, so every property inherited from a remote parent lands on one key. Two module trees inheriting different remote parents that declare the same property name would merge their consumer sets, which over-decouples. Narrow and cosmetic, but worth keying on the declaring POM's GAV eventually.Credit
Built on
tim/taipei-v2from Don't bump a shared version property to a version its siblings lack #8460. The first two commits are Tim's, unchanged.Fixes UpgradeDependencyVersion bumps a shared version property to a version its siblings never published #8656