Repository navigation
Fix Maven 4 @Resolution injection in generated plugin descriptors - #1234
goutamadwant wants to merge 3 commits into
Conversation
Carry resolution annotations through the scanner and descriptor model, then write their field, path scope, and request type to Maven 4 plugin.xml. Extend the Maven 4 integration test to execute a mojo with direct and inherited resolution fields and a real compile dependency.
gnodet-bot
left a comment
There was a problem hiding this comment.
Two low-severity findings in the new @Resolution support:
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
@gnodet-bot addressedc both review comments: |
gnodet-bot
left a comment
There was a problem hiding this comment.
Review of PR #1234: Fix Maven 4 @resolution injection in generated plugin descriptors
This is a re-review of the updated commits (head SHA 85655a16).
Previous review findings are addressed. Three low-severity Javadoc gaps remain in the new public API on ExtendedMojoDescriptor:
Findings:
-
addResolution()missing Javadoc — The pre-existingaddAfterLink()method directly above has@paramand@since 4.0.0documentation.addResolution()should be consistent. -
getResolutions()incomplete Javadoc — The single-line/** Returns the Maven 4 resolution injections declared by this mojo. */is missing@returnand@since 4.0.0tags, which are present on the analogousgetAfterLinks(). -
ResolutionEntryinner class missing@since 4.0.0andtoString()— The class-level Javadoc lacks@since. The innerAfterLinkclass (pre-existing) hastoString()for debuggability;ResolutionEntryshould too.
Additionally: no unit test covers the collectResolutions() hierarchy traversal semantics — only an IT was added. And equals()/hashCode() on ExtendedMojoDescriptor don't include resolutions, mirroring the pre-existing gap for afterLinks (non-blocking).
Verdict: The feature logic looks correct. The Javadoc gaps are minor but worth fixing before merge for API consistency.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
@gnodet-bot addressed the three API documentation and |
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — all 5 findings have been addressed in the latest commits.
- ✅
toString()added toResolutionAnnotationContent - ✅ Empty
<requestType/>XML emission fixed (conditional guard inPluginDescriptorFilesGenerator) - ✅
addResolution()Javadoc added with@paramand@since 4.0.0 - ✅
getResolutions()Javadoc completed with@returnand@since 4.0.0 - ✅
ResolutionEntryinner class now carries@since 4.0.0andtoString()
LGTM.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Fixes #1230.
A Maven 4 mojo can declare an
@Resolutionfield, but Plugin Tools currently omits it fromplugin.xml. Maven then leaves the field null even when the project has matching dependencies.This change carries the annotated field,
pathScope, andrequestTypethrough scanning and descriptor generation, including fields inherited from a superclass. It writes<resolutions>only for Maven 4 descriptors. Thev4api-4xintegration test checks the generated XML and runs a mojo with direct and inherited resolution fields against a real compile dependency.Validation:
masterwith Maven 4.0.0-rc-7, the plugin builds, but the descriptor has no<resolutions>and goal execution fails because the compile dependency was not injected.v4api-4xintegration test passes with Maven 4.0.0-rc-7 after the fix.mvn -Prun-its verifypasses with Maven 3.9.16 and JDK 21: 32 plugin integration tests pass, one Maven 4 fixture is skipped under Maven 3, and 12 plugin-report integration tests pass.git diff --checkpasses.To make clear that you license your contribution under the Apache License Version 2.0, January 2004, check one of the following: