slachiewicz commented on PR #1668: URL: https://github.com/apache/maven-dependency-plugin/pull/1668#issuecomment-5233380104
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) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
