Skip to content

[MDEP-858] Replace Maven Artifact Transfer with Maven Resolver - #1668

Closed
wilx wants to merge 6 commits into
apache:masterfrom
wilx:issue-1355-get-resolver
Closed

[MDEP-858] Replace Maven Artifact Transfer with Maven Resolver#1668
wilx wants to merge 6 commits into
apache:masterfrom
wilx:issue-1355-get-resolver

Conversation

@wilx

@wilx wilx commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Summary

Replace the remaining Maven Artifact Transfer usage with Maven Resolver APIs:

  • migrate dependency:get artifact and dependency resolution;
  • migrate repository-layout installation in dependency:copy-dependencies;
  • migrate dependency:purge-local-repository resolution while preserving filtering, fallback resolution, and aggregated failures;
  • extend ResolverUtil with the shared Resolver operations and remove the now-unused maven-artifact-transfer dependency and DependableCoordinate implementation.

The migration preserves custom artifact types, alternate repositories, copied POMs, snapshot base-version handling, Maven local-repository metadata, and purge fallback behavior.

Review follow-up additionally:

  • preserves separate packaging and classifier parameters for short dependency:get coordinates and validates that a version is present;
  • delegates alternate repository parsing, mirrors, proxies, and authentication to the active Resolver session, with server-backed authentication/proxy/non-proxy tests;
  • preserves the local repository manager content type and installs companion project POM metadata;
  • derives purge scope and optional selectors from a root dependency, preserves classifiers in the fallback, and logs the original resolution failures at debug level;
  • keeps the non-filtered resolver helper's non-resolved root-artifact contract used by go-offline;
  • preserves dependency:get's unconditional refresh policy for explicitly supplied repositories without changing the session-controlled policy used by other ResolverUtil callers;
  • trims comma-separated repository entries and each alternate repository ID, layout, and URL field before constructing Resolver repositories.

Fixes #1355

Verification

  • Maven 3: nice mvn clean verify -- 432 tests, 0 failures/errors, 1 skipped; dependency analysis clean
  • Maven 3: nice mvn clean -Prun-its verify -- 94 passed, 0 failures/errors, 5 JRE-conditioned skips
  • Maven 4 with JDK 21: focused clean TestGetMojo, ResolverUtilTest, and TestCopyDependenciesMojo2 run -- 41 tests, 0 failures/errors/skips
  • Maven 3: focused go-offline, intermodule go-offline, and the two changed purge integration tests -- 4 passed
  • spotless:check passed on every commit revision; git diff --check passed on the final head

Following this checklist to help us incorporate your
contribution quickly and easily:

  • Your pull request should address just one issue, without pulling in other changes.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Each commit in the pull request should have a meaningful subject line and body.
    Note that commits might be squashed by a maintainer on merge.
  • Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied.
    This may not always be possible but is a best-practice.
  • Run mvn verify to make sure basic checks pass.
    A more thorough check will be performed on your pull request automatically.
  • You have run the integration tests successfully (mvn -Prun-its verify).

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

@wilx
wilx marked this pull request as ready for review August 2, 2026 09:43
@slachiewicz

Copy link
Copy Markdown
Member

Thanks for taking this on — the coverage is impressive (413 unit tests plus the ITs on both Maven 3 and 4), and most of the migration maps cleanly onto Resolver.

I ran a review focused on the behaviours that a green test suite of that shape tends not to reach: scope selection, coordinate merging, local-repository typing, and the purge fallback path. Nine points below, ordered by severity. The first two look like real regressions, and neither is visible in the current tests.


1. purge-local-repository now purges test/provided/optional dependencies

ResolverUtil.resolveDependenciesForArtifact does collectRequest.setRootArtifact(rootArtifact) (ResolverUtil.java:277) and leaves getRoot() null. The maven-artifact-transfer code it replaces built new CollectRequest(new Dependency(rootArtifact, null), repos) — i.e. setRoot(...).

That difference matters to Resolver's selectors. In 1.9.25, ScopeDependencySelector.deriveChildSelector returns this — non-transitive, selectDependency always true — when context.getDependency() == null, but a transitive copy that filters test and provided when a root Dependency is present. OptionalDependencySelector behaves the same way.

Concretely, on a project whose only extra dependency is junit:junit:4.13.2:test:

  • before: junit is excluded from the collect result and left alone in ~/.m2/repository
  • after: junit and its transitive hamcrest are deleted and re-resolved

Offline, or if the artifact is no longer in the configured remotes, reResolveArtifacts then adds it to missingArtifacts and the build fails with Failed to refresh project dependencies / required artifacts missing: junit:junit:jar:4.13.2 — a failure that could not happen before.

2. dependency:get silently ignores -Dpackaging and -Dclassifier

The removed block set only the tokens actually present, on the same coordinate setPackaging/setClassifier had already populated, so unspecified positions kept the -D values. The replacement stores the raw string and ResolverUtil.createArtifactFromString re-parses it in isolation (items.length > 3 ? items[3] : null, items.length > 4 ? items[4] : null); ParamArtifact.getPackaging() and getClassifier() are never consulted on that branch.

mvn dependency:get -Dartifact=org.apache.maven:maven-model:2.0.9 -Dclassifier=sources

used to download maven-model-2.0.9-sources.jar and now downloads maven-model-2.0.9.jar, with no warning — the wrong file, silently. Similarly -Dartifact=g:a:1.0 -Dpackaging=zip used to fetch a-1.0.zip and now fails with ... a:jar:1.0 was not found.

3. Local repository loses its content type

ResolverUtil.java:106 builds new LocalRepository(localRepositoryDirectory) with no content type. Maven31RepositoryManager.setLocalRepositoryBasedir passed new LocalRepository(basedir, resolveRepositoryType(session.getLocalRepository())), so a simple local repo stayed simple; with an empty type DefaultLocalRepositoryProvider always picks the higher-priority EnhancedLocalRepositoryManagerFactory.

Effect: copy-dependencies -DuseRepositoryLayout=true now writes _remote.repositories tracking files into target/dependency alongside each jar. Anything treating that directory as a simple local repository, or comparing its contents for reproducibility, sees files that were not there before.

4. version is never validated in GetMojo

execute() guards on artifactId only, and ParamArtifact.isDataSet() — which exists for exactly this — is unused. mvn dependency:get -DgroupId=org.apache.maven -DartifactId=maven-model passes the guard, Aether emptifies the null version, and the user gets Could not find artifact org.apache.maven:maven-model:jar: rather than being told the version parameter is missing.

5. Purge fallback drops the classifier

The non-transitive fallback builds new DefaultArtifact(groupId, artifactId, null, extension, version), hard-coding a null classifier — so for <type>test-jar</type> or <classifier>tests</classifier> it resolves and purges the main jar and leaves the classified artifact behind. resolverDependencies, built ~30 lines above with RepositoryUtils.toDependency, already holds the correct Artifact including classifier and type-derived extension. Reusing it also lets the artifactHandlerManager constructor parameter go — it has no other use in this mojo.

6. installArtifact no longer installs the companion POM

Maven31ArtifactInstaller.install walked getMetadataList() and, for each ProjectArtifactMetadata, added new SubArtifact(aetherArtifact, "", "pom") to the same InstallRequest. ResolverUtil.installArtifact (:121) issues new InstallRequest().addArtifact(artifact) and nothing else. For an artifact carrying that metadata, useRepositoryLayout=true produces a jar with no .pom beside it, and a build pointed at that directory cannot read the descriptor.

I marked this one plausible rather than confirmed — it depends on a dependency artifact actually carrying ProjectArtifactMetadata, which I did not reproduce end to end.

7. The resolution exception is swallowed

The rewritten catch never logs e, not even at debug. When transitive purge resolution fails for a non-obvious reason (a 401 from a repository, a malformed POM), the user sees only the "falling back to non-transitive mode" line, the fallback silently purges a different set, and -X reveals nothing about the real cause.

8. GetMojo still carries the legacy repository stack

The goal keeps org.apache.maven.repository.RepositorySystem, the ArtifactRepositoryLayout map, MavenArtifactRepository and manual mirror/proxy/auth injection, with RepositoryUtils.toRepos bolted on the end, rather than reusing ResolverUtil.remoteRepositories(List<String>).

That leaves two repository-spec parsers with different grammars: ALT_REPO_SYNTAX_PATTERN = "(.+)::(.*)::(.+)" needs three segments, while ResolverUtil.prepareRemoteRepository accepts id::url. So -DremoteRepositories=myrepo::https://repo.acme.com fails here with "Invalid syntax for repository" while working in every goal routed through ResolverUtil. It also means the stated goal of dropping the deprecated library is only half met for this mojo, since it still needs maven-compat.

9. Dead null branch in repositorySystemSession(File)

The if (localRepositoryDirectory != null) branch is unreachable — the single caller always passes the required outputDirectory. As written the method doubles as "return the ambient session", so a future caller passing null would install into the user's ~/.m2/repository with no error. Objects.requireNonNull and a name like localRepositorySession(File) would remove both.


Points 1–5 and 8 I was able to confirm against the Resolver 1.9.25 sources and the decompiled maven-artifact-transfer implementations; 6, 7 and 9 are reasoned from the code and worth your judgement rather than treated as settled.

Happy to open a PR against your branch for any of these if that is easier than folding them in.

Generated-by: Claude Opus 5 (1M context)

@wilx
wilx force-pushed the issue-1355-get-resolver branch from e597448 to 740f138 Compare August 9, 2026 20:50
@wilx

wilx commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review. I reproduced and addressed all nine points on the rebased branch:

  1. Purge collection now uses a root Dependency when applying the filter, so Resolver derives its scope and optional selectors. The IT covers direct test/provided dependencies and an optional transitive dependency.
  2. Short dependency:get coordinates now retain separately supplied packaging and classifier; explicit coordinate tokens still take precedence. Both directions are unit-tested.
  3. Repository-layout installation preserves the active local repository manager's content type (simple, or Maven's enhanced mapped to its public default type). Both manager types are tested.
  4. GetMojo validates the complete coordinate after parsing, including the required version.
  5. Purge fallback reuses the Resolver dependency artifact, retaining classifier and type-derived extension. The bad-descriptor IT now exercises a sources artifact.
  6. Installation includes each companion POM represented by ProjectArtifactMetadata; this is covered at the install request level and by repository-layout output assertions.
  7. Both the transitive collection failure and per-artifact fallback failure retain their original exception in debug logging.
  8. GetMojo now uses the Resolver session's repositories and ResolverUtil parser, including id::url; the legacy repository-system/layout/auth stack and its now-unused dependencies are gone.
  9. The helper is now explicitly localRepositorySession(File) and rejects null.

One shared-helper interaction also surfaced during the full IT run: go-offline needs a non-resolved root artifact, whereas filtered purge resolution needs a root dependency. Those contracts are now separate and covered by a unit regression plus both go-offline ITs.

Verification on the final head:

  • nice mvn clean verify: 425 tests, 0 failures/errors, 1 existing skip
  • nice mvn clean -Prun-its verify: 94 passed, 0 failures/errors, 5 JRE-conditioned skips
  • focused Maven 4/JDK 21 tests: 34 passed
  • spotless:check on every commit revision

wilx added 4 commits August 9, 2026 22:58
Replace the Maven Artifact Transfer resolvers used by dependency:get while preserving repository and artifact-type behavior. Keep the component dependency for goals that still use it.

Fixes apache#1355.
… Resolver

Replace Maven Artifact Transfer installation used by dependency:copy-dependencies in the repository-layout path while preserving Maven local-repository metadata, copied POMs, and snapshot behavior.

Fixes apache#1355.
Replace the Maven Artifact Transfer resolvers while preserving dependency filtering, fallback resolution, and aggregated failure reporting. Re-resolve each artifact once because Resolver descriptor resolution also restores its POM.

Fixes apache#1355.
Drop the unused DependableCoordinate interface from ArtifactItem and remove the now-unused Maven Artifact Transfer dependency. Keep the artifact configuration getters unchanged.

Fixes apache#1355.
@wilx
wilx force-pushed the issue-1355-get-resolver branch from 740f138 to fd2706e Compare August 9, 2026 21:01
wilx added 2 commits August 9, 2026 23:29
Keep dependency:get compatible with its historical behavior by applying the always-update policy to user-supplied repositories. Leave the existing session-controlled policy available to other ResolverUtil callers.

Fixes apache#1355.
Normalize comma-separated repository entries in dependency:get and trim each alternate-repository ID, layout, and URL field before constructing Resolver repositories.

Fixes apache#1355.
@slachiewicz

Copy link
Copy Markdown
Member

Please check my work here #1677
after all 3 PR woudl land - Your's part can be simpler

@wilx

wilx commented Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

Closing in favour of #1680.

@wilx wilx closed this Aug 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request maintenance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[MDEP-858] Drop maven-artifact-transfer

3 participants