Repository navigation
Conversation
…hancedConfigurationConverter The Plexus AbstractConfigurationConverter.instantiateObject() uses Class.newInstance() which requires a public no-arg constructor. This prevents using package-private classes (or inner classes) as @parameter configuration types in Maven 4 API plugins. EnhancedCompositeBeanHelper.setFieldValue() already handles non-public fields by calling field.setAccessible(true). This commit makes object instantiation symmetric: override the call site in EnhancedConfigurationConverter.fromConfiguration() with a local instantiate() method that uses getDeclaredConstructor() + setAccessible(true), consistent with the field injection strategy.
gnodet-bot
left a comment
There was a problem hiding this comment.
The fix is mechanically correct and symmetric with EnhancedCompositeBeanHelper.setFieldValue(). One gap before merge: no test was added to validate the fixed scenario.
Also, please set the milestone to 4.1.0 (issue #13459 targets it).
This review was generated by an AI agent, Hermès on behalf of @gnodet.
…or instantiation)
gnodet-bot
left a comment
There was a problem hiding this comment.
Previous finding (missing test) is fully addressed. The fix is correct: getDeclaredConstructor() + setAccessible(true) properly handles package-private constructors, and the test validly reproduces the original bug (Class.newInstance() fails because the access check uses AbstractConfigurationConverter as the caller, which is in a different package from PackagePrivateBean).
This review was generated by an AI agent, Hermès on behalf of @gnodet.
| } | ||
| } | ||
|
|
||
| private static Object instantiate(Class<?> type, PlexusConfiguration configuration) |
There was a problem hiding this comment.
I'm not familiar with this part of the code, so maybe my remark will be irrelevant. But the instantiateObject method is apparently inherited from the parent class. Maybe this new instantiate method should be added next to it. Then:
- Check if
instantiateObjectis still used and, if not, remove it. - This pull request said that this change is "symmetric with
EnhancedCompositeBeanHelper.setFieldValue". If symmetric, then can the latter method share the newinstantiatemethod introduced by this pull request (if that method can put moved to the parent class)?
There was a problem hiding this comment.
The parent class has a method instantiateObject, but it's protected final.
There was a problem hiding this comment.
Yes but protected final method can be used in subclasses. Maybe the same should be done for instantiate.
Problem
AbstractConfigurationConverter.instantiateObject()(fromorg.eclipse.sisu.plexus) usesClass.newInstance()which requires a public no-arg constructor. This prevents using package-private classes (or static inner classes with non-public constructors) as@Parameterconfiguration types in Maven 4 API plugins — anIllegalAccessExceptionis thrown at configuration time.This was discovered while working on removing the
maven-archiverdependency frommaven-jar-plugin(apache/maven-jar-plugin#618): the plugin'sArchiveConfigurationtype would benefit from being a package-private class but currently cannot be used as a@Parametertype.Fixes #13459.
Solution
EnhancedConfigurationConverteralready overridesfromConfiguration(), which is the call site ofinstantiateObject(). SinceinstantiateObjectisprotected finalinAbstractConfigurationConverter(from sisu, not overridable), we replace the call with a localinstantiate()method that uses:This is symmetric with how
EnhancedCompositeBeanHelper.setFieldValue()already handles non-public fields (viafield.setAccessible(true)). The fix makes constructor instantiation consistent with field injection.Alternative
A one-line fix in
eclipse-sisu/sisu.plexus'sAbstractConfigurationConverter.instantiateObject()would be the cleanest long-term solution, but that requires a sisu release cycle. This Maven-side fix is immediately actionable.