Remove maven-compat from GetMojo - #1677
Open
slachiewicz wants to merge 1 commit into
Open
Conversation
GetMojo used the legacy org.apache.maven.repository.RepositorySystem, whose only implementation, LegacyRepositorySystem, lives in maven-compat. It used it for nothing but injectMirror/injectProxy/injectAuthentication on the repositories named by the remoteRepositories parameter. org.apache.maven.bridge.MavenRepositorySystem is the usual replacement but a plugin cannot use it: maven-core does not export org.apache.maven.bridge to plugin class realms, so referencing it compiles, passes every unit test, and then fails at runtime with NoClassDefFoundError. Do the same selection in the plugin instead, in RepositorySessionInjector, against the repository session's mirror, proxy and authentication selectors -- Maven builds those from the same settings, already decrypted, and Resolver consults them itself when it transfers. Everything it touches is in a package maven-core does export. Also copy SnapshotArtifactRepositoryMetadata from maven-compat into the test tree, the only other thing the plugin needed from it, and cover the proxy and credential paths, which had no test that failed when the injection was removed.
slachiewicz
requested review from
elharo and
slawekjaranowski
and
a lite review from Copilot
August 8, 2026 17:32
There was a problem hiding this comment.
Pull request overview
Removes the maven-compat test dependency by eliminating GetMojo’s reliance on legacy RepositorySystem injection and instead applying mirrors/proxies/authentication via the active RepositorySystemSession selectors (which are available to plugins at runtime).
Changes:
- Replaces
GetMojo’s legacy mirror/proxy/auth injection with a new in-pluginRepositorySessionInjectorthat reads from the repository session selectors. - Strengthens
GetMojo’s unit tests to ensure proxy/auth behavior is actually exercised (including isolating the local repo via@TempDir) and adds focused unit coverage for the new injector. - Vendors
SnapshotArtifactRepositoryMetadatainto the test tree to keep tests compiling/running withoutmaven-compat, and removes themaven-compatdependency frompom.xml.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/main/java/org/apache/maven/plugins/dependency/GetMojo.java |
Switches repository injection to session-based selectors via RepositorySessionInjector. |
src/main/java/org/apache/maven/plugins/dependency/utils/RepositorySessionInjector.java |
New implementation of mirror/proxy/auth application based on RepositorySystemSession selectors. |
src/test/java/org/apache/maven/plugins/dependency/utils/RepositorySessionInjectorTest.java |
Adds direct unit tests for mirror/proxy/auth behavior of the new injector. |
src/test/java/org/apache/maven/plugins/dependency/TestGetMojo.java |
Updates tests to populate repository session selectors from settings and isolates local repo to ensure transfers occur. |
src/test/java/org/apache/maven/artifact/repository/metadata/SnapshotArtifactRepositoryMetadata.java |
Vendors compat-only metadata class into tests to allow dropping maven-compat. |
pom.xml |
Removes the maven-compat test dependency. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+186
to
+190
| Authentication result = new Authentication( | ||
| authCtx.get(AuthenticationContext.USERNAME), authCtx.get(AuthenticationContext.PASSWORD)); | ||
| result.setPrivateKey(authCtx.get(AuthenticationContext.PRIVATE_KEY_PATH)); | ||
| result.setPassphrase(authCtx.get(AuthenticationContext.PRIVATE_KEY_PASSPHRASE)); | ||
| return result; |
elharo
reviewed
Aug 8, 2026
| * <code>settings.xml</code>, with the servers and proxies already decrypted, and they are the same selectors | ||
| * Resolver consults when it performs the transfer. | ||
| */ | ||
| public class RepositorySessionInjector { |
Contributor
There was a problem hiding this comment.
There seems to be only one use of this class. Unless you intend this to be a general utility for external use, consider moving it into the package where it's used and making it non-public
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.
Removes
maven-compat. No baseline change — the pom is already maven 3.9.16 / resolver 1.9.25, so<prerequisites>is untouched.GetMojo's constructor loses a parameter: the legacyorg.apache.maven.repository.RepositorySystemwas only ever used forinjectMirror,injectProxyandinjectAuthentication.Not via
MavenRepositorySystem— it is not visible to pluginsThat swap was made first and it does not work. maven-core does not export
org.apache.maven.bridgeto plugin class realms. In plexus-classworlds, a plain package name matches recursively while the.*form matches only classes directly in that package — so maven-core exportsorg.apache.maven.artifact.**,org.apache.maven.repository.**,org.apache.maven.settings.**,org.apache.maven.RepositoryUtilsandorg.eclipse.aether.**, and nothing underorg.apache.maven.bridge. Same in 3.9.16 and 4.0.0-rc-5.It compiled and 412 unit tests passed. It failed only under
-Prun-its:So for changes touching maven-core internals,
mvn verifyis not a sufficient bar — the plugin-testing harness uses one flat classpath and has no realm isolation.Selection now lives in the plugin, in a new
RepositorySessionInjector. That is viable because the session-based inject methods need no privileged API: they only readsession.getMirrorSelector()/getProxySelector()/getAuthenticationSelector()and write onto theArtifactRepository.Two details replicated from maven-core rather than assumed:
createArtifactRepositorysubstitutesnew ArtifactRepositoryPolicy()for null policies — a naivenew MavenArtifactRepository(...)would reintroduce the null-policy trap — and it honoursArtifactRepositoryLayout2.newMavenArtifactRepository.Proxy and authentication: preserved, with three narrow deltas
Mirrors are unchanged. maven-core's
getMirror(ArtifactRepository, List<Mirror>)and resolver'sDefaultMirrorSelectorwere compared directly: same two-pass lookup, samematchPatternhandling of*,external:*,external:http:*,!idand comma lists, same layout matching.Proxies — three differences, all moving toward what the rest of Maven already does:
httpproxy now also applies tohttpsrepositories when nohttpsproxy is defined. The legacyequalsIgnoreCasedid not fall back, so a repository that previously bypassed the proxy will now be proxied. This is the release-note item.dav:/davs:are normalised to http/https before matching; the legacy code matched the literal string and never matched.nonProxyHostsmatching is now case-insensitive. Same|splitting and./*escaping.Authentication — two differences, both inert: a
<server>with no credentials yieldsnullrather thanAuthentication(null, null), whichRepositoryUtils.toAuthenticationcollapses to null anyway; and duplicate<server>ids go first-wins to last-wins.Suggested release note: "
dependency:getnow selects proxies through the repository session, so anhttpproxy also applies tohttpsrepositories when nohttpsproxy is configured, matching how Maven proxies every other download."The existing auth test was not testing anything
test:test:1.0persists in the checked-intarget/test-classes/unit/get-test/target/local-repo, so withoutcleanthe test passed with authentication injection deleted outright. Fixed with a@TempDirlocal repository. After that, deleting the inject call fails all three oftestRemoteRepositoriesAuthentication,testRemoteRepositoriesProxyandtestRemoteRepositoriesNonProxyHosts— and the proxy test asserts the failure names the proxy host, so it cannot pass for the wrong reason.The new tests were also run against unmodified master's settings-list implementation: 6/6 pass. Same tests, both implementations, same outcomes — so they characterise behaviour rather than merely agreeing with the new code.
Numbers
origin/masterFull 94-project IT suite both sides, per-IT diff empty across all 86 reported projects,
dependency:analyze-onlyclean.One judgement call
SnapshotArtifactRepositoryMetadatais vendored into the test tree rather than deleting its two usages.TestCopyDependenciesMojo2.assertArtifactExistsiteratesartifact.getMetadataList()and asserts a file exists — dropping the attachment makes that loop empty and silently removes the check. Vendoring is safe here: the whole hierarchy above it is in maven-core and only the leaf is compat-only. Same approach as MPLUGIN-384.