Repository navigation
Add scoped plugin service discovery API - #13625
Hiteshsai007 wants to merge 7 commits into
Conversation
Expose a Maven 4 PluginManager service that resolves declared project plugins into public runtime metadata and performs component lookups with the plugin realm as the context classloader. This removes the need for build extensions to combine internal plugin manager and container APIs for plugin-contributed services.
|
We've updated the classloader improvements tracking issue (#13223) to include a Layer 0b — Public Realm API that aligns with the direction of this PR but proposes a slightly different shape. The idea: instead of exposing public interface PluginRealm {
ClassLoader getClassLoader();
Optional<ModuleLayer> getModuleLayer(); // for future JPMS ModuleLayer support
Lookup getLookup(); // scoped to this realm's import chain
}
Benefits:
See #13223 for the full plan, specifically the "Layer 0b" and "Design Principle — Component Visibility Follows Classloading Rules" sections. Claude Code on behalf of Guillaume Nodet |
Replace direct plugin classloader access with a PluginRealm that exposes the classloader, an optional module layer, and a realm-scoped Lookup. Keep TCCL management inside the Lookup implementation so callers cannot accidentally perform an unscoped component lookup.
gnodet-bot
left a comment
There was a problem hiding this comment.
Overall the design is clean: PluginRealm as a rich API type over a bare ClassLoader is the right direction, RealmLookup.scoped() correctly restores the TCCL, and the test coverage for TCCL restore and exception propagation is solid.
A few things need to be addressed before merge.
Binary-breaking API change — Plugin.getClassLoader() was removed and replaced by Plugin.getRealm(). Any embedder or plugin already calling plugin.getClassLoader() gets a compile error. This is intentional (4.1.0 is still in development), but please confirm it is tracked, and add a note in the PR description or a BREAKING CHANGES entry.
Accessor naming convention — PluginRealm uses getClassLoader(), getModuleLayer(), getLookup(). Maven 4 API prefers noun-style accessors (classLoader(), moduleLayer(), lookup()). See e.g. Project, Session, MojoExecution. Worth aligning before stabilizing this API.
Nullable model field — DefaultPlugin.model can be null (when the model parameter is null). Plugin.getModel() is already @Nullable, so no contract violation, but a @Nullable annotation on the constructor parameter would make the intent explicit.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Use Maven 4 noun-style realm accessors, document the new API, and preserve undeclared-plugin errors. Add the requested exception constructors and serialization metadata, nullable annotation, and regression coverage.
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — all four previous findings have been addressed:
- ✅
Plugin.getRealm()now has@since 4.1.0 - ✅
PluginRealmmethods all have@returntags - ✅
PluginManagerExceptionhas@Serial serialVersionUIDand both constructors - ✅
findPlugin()is now called outside thetryblock —IllegalArgumentExceptionpropagates correctly
One new gap: PluginManager.loadPlugin() throws IllegalArgumentException (from findPlugin() when the plugin is not declared by the project) but this is not documented in the @throws section. Callers relying on the API contract have no signal to expect or catch IAE — they’ll see it as an undeclared runtime exception with no documentation on the condition that triggers it.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Declare the IllegalArgumentException thrown by PluginManager.loadPlugin when the requested plugin is not present in the project build.
gnodet-bot
left a comment
There was a problem hiding this comment.
All five findings from the prior reviews are now addressed. One new documentation-accuracy issue remains.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Generalize the interface for use at all levels (session, project, plugin). The same Realm shape — classLoader(), moduleLayer(), lookup() — applies uniformly; the difference is what the Lookup sees, not the type. Module layers are not plugin-specific (extensions and core can also use them, e.g. JLine per apache#13491). See apache#13223 (Layer 0b) for the design rationale. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — the @throws IllegalArgumentException is now documented on PluginManager.loadPlugin() (thread 1 addressed). Thread 2 (Realm.lookup() Javadoc accuracy) remains open.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Document that realm lookup delegates with the realm classloader as the TCCL and that component visibility depends on container filtering.
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier reviews — all previous findings are now resolved:
- ✅
Plugin.getClassLoader()→Plugin.getRealm()binary break: intentional for 4.1.0, documented - ✅ Accessor naming:
Realmuses noun-style (classLoader(),moduleLayer(),lookup()) - ✅
@throws IllegalArgumentExceptiondocumented onPluginManager.loadPlugin() - ✅
Realm.lookup()Javadoc: softened to accurately describe TCCL-switching semantics
The API is clean and the implementation is correct. LGTM.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Use the current context classloader, with the Maven implementation classloader as a final fallback, when legacy descriptors have no realm attached. Restore regression coverage for minimal mojo descriptors used by expression evaluation.
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier reviews — the new commit (734bfed) is clean.
All prior findings remain addressed:
- ✅
Plugin.getRealm()replacesgetClassLoader()— breaking change intentional for 4.1.0, documented with@since 4.1.0 - ✅
Realminterface uses noun-style accessors (classLoader(),moduleLayer(),lookup()) - ✅
PluginManagerExceptionhas@Serial serialVersionUIDand both constructors - ✅
findPlugin()is called outside thetryblock —IllegalArgumentExceptionpropagates unchecked as documented - ✅
Realm.lookup()Javadoc accurately describes TCCL-switching behavior (no longer overstates visibility isolation) - ✅
DefaultRealm.RealmLookup.scoped()restores TCCL infinally— correct
The null ClassRealm fallback chain in DefaultPlugin (TCCL → class-level classloader) is a safe defensive measure for the case where realm setup hasn't completed.
LGTM.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Resolves #13516
Build extensions that inspect project plugins and discover plugin-provided services currently need to use internal Maven APIs. This requires combining plugin resolution, realm creation, Plexus component lookup, and manual thread context classloader management.
This change introduces a public Maven 4 service for loading a project plugin and discovering components within that plugin's classloader scope.
How this solves the problem
The new
PluginManagerAPI provides two operations:Load a project plugin
loadPlugin(...)resolves missing plugin versions, loads the plugin descriptor, creates the plugin realm when necessary, and returns the publicPluginruntime representation.Expose plugin runtime metadata
The returned
Pluginprovides access to its model, descriptor, artifact, classloader, lifecycle mappings, and resolved dependencies without requiring consumers to access the internalMavenPluginManager.Perform classloader-scoped service discovery
lookupList(...)performs component discovery with the selected plugin's classloader as the thread context classloader.Restore the original classloader
The original thread context classloader is restored after every lookup, including when component provisioning fails.
The existing plugin snapshot construction from
DefaultMojoExecutionis also moved intoDefaultPlugin, ensuring that mojo executions and the new service expose consistent plugin metadata.Why this matters
Build extensions such as Tycho need to inspect project plugin dependencies and discover SPI implementations contributed by individual plugins.
Previously, this required using internal
LifecyclePluginResolver,MavenPluginManager,BuildPluginManager, andPlexusContainerAPIs while manually managing the thread context classloader. These internal dependencies make extensions more tightly coupled to Maven implementation details and harder to migrate to the Maven 4 API.By keeping plugin resolution, realm creation, and scoped component lookup inside Maven, extensions can implement this behavior using stable Maven 4 API types.
Testing
Added
DefaultPluginManagerTestcoverage verifying:PluginAPI.The focused
maven-corereactor tests forDefaultPluginManagerTestandDefaultMojoExecutionTestpass successfully with 6 tests and 0 failures.Checklist
mvn verifyto make sure basic checks pass.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 checkbox.