Preserve dependency scope in plugin artifacts - fix #12497#12503
Preserve dependency scope in plugin artifacts - fix #12497#12503slawekjaranowski wants to merge 1 commit into
Conversation
What changed: - Added `RepositoryUtils.toArtifact(ArtifactResult)` to convert resolved artifacts and copy dependency metadata from the resolution request. - Set Maven `Artifact` scope and optional flag from the resolved dependency node instead of converting only the raw Aether artifact. - Updated `DefaultMavenPluginManager` to use the new conversion path for plugin realm and extension artifact resolution. Why: - `PluginDescriptor.getArtifacts()` returned artifacts with `null` scope in Maven 3.10.0-rc-1. - Restoring scope/optional metadata brings behavior back in line with 3.9.x and avoids regressions in plugins that depend on artifact scope. fix apache#12497
d51a7ab to
4ae9f94
Compare
gnodet
left a comment
There was a problem hiding this comment.
AI Review — PR #12503
Verdict: ✅ APPROVE
Summary: This PR correctly fixes a regression in 3.10.0-rc-1 where PluginDescriptor.getArtifacts() returned artifacts with null scope. The new RepositoryUtils.toArtifact(ArtifactResult) overload navigates the ArtifactResult → ArtifactRequest → DependencyNode → Dependency chain to recover scope and optional metadata. The approach is minimal, null-safe via Optional, and follows existing codebase patterns.
Findings
Important — CI Spotless failure
RepositoryUtils.java line 177: Spotless rejects the space before the closing ) — } ); should be });. All three build jobs fail at spotless:check. Running mvn spotless:apply -B in maven-core will fix it.
// Current (fails Spotless):
} );
// Fix:
});Suggestion — Javadoc
The new method only has @since 3.10.0. A brief description would help future readers:
/**
* Converts a resolved {@link ArtifactResult} to a Maven Artifact,
* preserving dependency scope and optional flag from the resolution
* request's dependency node.
*
* @since 3.10.0
*/Suggestion — Test coverage
No test for the new toArtifact(ArtifactResult) overload. A test verifying scope/optional preservation through the ArtifactResult → DependencyNode → Dependency chain would help prevent this regression from resurfacing.
What I Appreciate
- Follows existing patterns precisely — the private
toArtifact(Dependency)already doessetScope()/setOptional(), and this recovers the same info from theArtifactResultpath. - Null-safety via
Optional.ofNullable(...).map(...).map(...).ifPresent(...)— if any link is absent, the artifact retains its prior state gracefully. - The method reference change in
DefaultMavenPluginManageris clean and unambiguous. - Backward compatible — the root plugin artifact with
nullscope gets a no-opsetScope(null).
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
Review posted for apache#12503 (preserve dependency scope in plugin artifacts). APPROVED with Spotless formatting note. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
What changed:
RepositoryUtils.toArtifact(ArtifactResult)to convert resolved artifacts and copy dependency metadata from the resolution request.Artifactscope and optional flag from the resolved dependency node instead of converting only the raw Aether artifact.DefaultMavenPluginManagerto use the new conversion path for plugin realm and extension artifact resolution.Why:
PluginDescriptor.getArtifacts()returned artifacts withnullscope in Maven 3.10.0-rc-1.fix #12497
Following this checklist to help us incorporate your
contribution quickly and easily:
Note that commits might be squashed by a maintainer on merge.
This may not always be possible but is a best-practice.
mvn verifyto make sure basic checks pass.A more thorough check will be performed on your pull request automatically.
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.