Fix MavenSettings value equality dropping LST-carried mirrors - #8189
Draft
pdelagrave wants to merge 4 commits into
Draft
Fix MavenSettings value equality dropping LST-carried mirrors#8189pdelagrave wants to merge 4 commits into
pdelagrave wants to merge 4 commits into
Conversation
`@EqualsAndHashCode(onlyExplicitlyIncluded = true)` with no field included made any two `MavenSettings` instances equal, so the settings-equality guard in `MavenExecutionContextView#getMirrors(MavenSettings)` could never detect that supplied settings differ from those on the execution context. A mirror carried only in an LST's `MavenResolutionResult#getMavenSettings()` was silently dropped whenever the execution context also carried settings, sending resolution to public Maven Central instead of the mirror. Compare all nested settings types by value, excluding the lazily computed `mavenLocal` cache.
Mirrors, profiles, and servers without an id collapsed onto a single null map key during `MavenSettings` merge, so all but one such entry were silently dropped. Merge all four entry types through one shared id-keyed helper that keeps id-less entries from both sides and cannot collide with real ids.
Recipes construct a `MavenPomDownloader` per visited tag, and merging plus value-comparing two settings graphs on every construction is measurable across large runs. Cache the merged settings and the mapped mirrors per parsed-settings instance, invalidated when settings or mirrors are replaced on the context.
…tion Organizations use `mirrorOf=central` (or `*`) to keep resolution on an internal repository manager, often with public Maven Central blocked at the network level. These tests pin the guarantee that a mirror carried in the LST's Maven settings redirects all metadata resolution to the mirror — including the implicitly added Maven Central and when run-time execution context settings carry no mirror — and that `repo.maven.apache.org` is never contacted.
1 task
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
1 participant
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.
What's changed?
MavenSettingsand its nested types now compare by value. The class was annotated@EqualsAndHashCode(onlyExplicitlyIncluded = true)with no field included, so any two instances were equal and every instance had the same hash code, while the nested list-holder types compared by identity. The lazily computedmavenLocalcache field is excluded from equality, andtoStringnow prints the non-credential fields.MavenSettings#mergeshort-circuits on equal inputs and merges profiles, mirrors, servers, and proxies through one shared id-keyed helper, so entries without an id are no longer collapsed onto a single map key.Since recipes construct a
MavenPomDownloaderper visited tag,MavenExecutionContextViewnow memoizes the effective settings and the mapped mirrors per parsed-settings instance (Caffeine, weak keys).A new
MavenCentralMirrorTestpins the guarantee that amirrorOf=central(or*) mirror carried in an LST'sMavenResolutionResult#getMavenSettings()redirects all metadata resolution to the mirror, including the implicitly added Maven Central repository and when the run-timeExecutionContextcarries settings without any mirror, and thatrepo.maven.apache.orgis never contacted. Equality and merge regression tests were added toMavenSettingsTest.What's your motivation?
<mirrorOf>central</mirrorOf>in their Maven settings to keep all dependency resolution on an internal repository manager, with public Maven Central often blocked at the network level.MavenExecutionContextView#getMirrors(MavenSettings)decides whether supplied settings override the mirrors on the execution context by comparing the two for equality. With the degenerateequals, that comparison always reported "same" whenever the context carried settings, so the mirrors of the effective settings computed per Merge provided and built MavenSettings #4956 were silently discarded and resolution fell through to public Maven Central. In environments where Central is blocked this manifests as long connection timeouts while the recipe run otherwise appears to succeed.Anything in particular you'd like reviewers to focus on?
getMirrorsguard, which value equality makes behave as its javadoc describes. I'd appreciate a second opinion on that.MavenSecuritySettingsandRawGradleModulecarry the same annotation pattern but have no equality consumer today; they were left untouched to keep the change focused.Have you considered any alternatives or workarounds?
A reference-equality check in the
getMirrorsguard would fix the known call sites without touchingequals, but it leaves the identity-blind equality in place for the next consumer to trip on.Checklist