[MNG-8766] Add WorkspaceReader SPI to maven-api-spi for IDE integration - #13094
[MNG-8766] Add WorkspaceReader SPI to maven-api-spi for IDE integration#13094gnodet wants to merge 4 commits into
Conversation
Three major Java IDEs (IntelliJ IDEA, Eclipse m2e, Apache NetBeans) all override the internal PluginDependenciesResolver component for the same reason: they need their workspace reader to not participate in plugin resolution. Plugin realms are cached by DefaultPluginRealmCache (@singleton) and cannot be purged within a session, so resolving plugins from the IDE workspace causes stale classloaders when workspace sources change. This commit introduces a proper SPI in maven-api-spi: org.apache.maven.api.spi.WorkspaceReader The interface uses Maven 4 API types exclusively (no maven-resolver-api dependency required), with three methods: - findArtifact(Artifact): Optional<Path> - findVersions(Artifact): List<String> - isApplicableForPluginResolution(): boolean (default true) IDE integrators can implement this SPI and return false from isApplicableForPluginResolution() to opt their workspace reader out of plugin resolution, without touching any internal Maven component. Implementation: - SpiWorkspaceReaderAdapter bridges SPI implementations into the resolver workspace reader chain (added in DefaultMaven) - DefaultPluginDependenciesResolver filters out non-applicable readers when building plugin sessions - IT mng-8766 verifies that SPI readers with isApplicableForPluginResolution returning false are not called during plugin resolution
gnodet-bot
left a comment
There was a problem hiding this comment.
Clean SPI design that solves a real pain point for all three major IDE integrators. The adapter pattern is sound, the filtering logic is correct, and the IT covers the critical negative case.
One observation on the IT — see inline.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
| // The SPI workspace reader should be called for artifact resolution in the main session | ||
| // (e.g., during project dependency resolution, model building, etc.) | ||
| List<String> logLines = verifier.loadLogLines(); |
There was a problem hiding this comment.
💡 The comment says the SPI reader "should be called for artifact resolution" but there's no assertTrue verifying it was called for regular dependency resolution (e.g. findArtifact for junit:junit). The test only asserts the negative case (not called for plugin resolution).
Adding a positive assertion would make the test truly verify both directions of the contract and catch regressions where SPI readers are silently dropped from the main chain entirely.
| // The SPI workspace reader should be called for artifact resolution in the main session | |
| // (e.g., during project dependency resolution, model building, etc.) | |
| List<String> logLines = verifier.loadLogLines(); | |
| // The SPI workspace reader should be called for artifact resolution in the main session | |
| // (e.g., during project dependency resolution, model building, etc.) | |
| List<String> logLines = verifier.loadLogLines(); | |
| boolean hasFindArtifactCalls = | |
| logLines.stream().anyMatch(line -> line.contains("[SPI-WR] findArtifact(")); | |
| assertTrue( | |
| hasFindArtifactCalls, | |
| "SPI workspace reader should be consulted during regular artifact resolution"); |
There was a problem hiding this comment.
Fixed in d1d0447. Added the positive assertion for regular artifact resolution. Also fixed the root cause of the CI failure: SPI workspace readers were injected via constructor (javax.inject), but extensions register their beans in the Maven 4 DI system. Switched to lookup.lookupList() which finds them through the Sisu-DI bridge.
|
I'm not sure this really solve the issue or is wanted. Instead one more want to be able to purge the cache (so no disabling is actually needed) isn't it? |
| default boolean isApplicableForPluginResolution() { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
I would say such a method should be much more generic, e.g. to cover other cases of isApplicableFor(...) so maybe it can get an enum to tell if its a dependency, an extension, a plugin, ... or whatever are the cases. Also it seems odd to have this default implemented and saing IDE should return false. this sounds really odd from an OO/SPI point of view at least.
SPI WorkspaceReader implementations loaded from core extensions are registered in the Maven 4 DI system (via @org.apache.maven.api.di.Named). Constructor injection via javax.inject cannot see these beans because they live in a different DI world. Switch to lookup.lookupList() which goes through PlexusContainer; the SisuDiBridgeModule bridges Maven 4 DI beans back into Guice/Sisu, making them visible to Plexus lookups. Also add positive assertion in the IT to verify the SPI workspace reader is actually consulted during regular artifact resolution.
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after d1d0447c7a. All prior findings addressed:
- Positive assertion added — the IT now verifies
[SPI-WR] findArtifact(appears in logs during regular resolution, confirming the SPI reader participates in the main session. - Discovery fix — switched from constructor-injected
List<WorkspaceReader>tolookup.lookupList(), which correctly picks up beans registered by core extensions (loaded after container bootstrap). This was the root cause of the CI failure. - Test updated —
DefaultMavenSessionScopeTestupdated to match the new constructor signature.
The adapter (SpiWorkspaceReaderAdapter) correctly bridges resolver ↔ API types. LightweightApiArtifact.key() matches the default Artifact.key() contract. The filtering in DefaultPluginDependenciesResolver creates a new session/chain without mutating the original — thread-safe.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
…Cache.invalidate(Artifact) Instead of filtering workspace readers out of plugin resolution, expose a proper invalidate(Artifact) SPI on PluginRealmCache so IDEs can purge stale plugin realms when a workspace artifact is rebuilt. This approach is more accurate: IDEs trust their own build, so if a plugin has been rebuilt in the workspace, it should be used. The invalidate() method lets the IDE evict the cached realm on demand, rather than blanket-excluding workspace readers from plugin resolution. Changes: - PluginRealmCache: add default invalidate(org.apache.maven.api.Artifact) - DefaultPluginRealmCache: implement invalidate() by evicting matching entries (matched on groupId:artifactId:version) and disposing their ClassRealms - WorkspaceReader SPI: remove isApplicableForPluginResolution() - SpiWorkspaceReaderAdapter: remove isApplicableForPluginResolution() - DefaultPluginDependenciesResolver: remove filterWorkspaceReadersForPluginResolution() - IT: simplify test to verify SPI discovery and artifact resolution consultation
|
Thanks @laeubi, you're right on both counts. The approach has been reworked in 44e96719ff:
The reasoning: IDEs trust their own build — if they've rebuilt a plugin from the workspace, they want it used. The right API is cache invalidation on demand, not blanket exclusion from resolution. As for the generics of |
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after 44e96719ff. The rework based on @laeubi's feedback is a significant improvement — PluginRealmCache.invalidate(Artifact) is the right abstraction (cache invalidation on demand vs. blanket exclusion from resolution).
The SPI interface, adapter, and DI discovery are solid. Two cosmetic issues from the rework — stale <description> elements that still reference the old approach.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
| <packaging>jar</packaging> | ||
|
|
||
| <name>Maven Integration Test :: spi-workspace-reader</name> | ||
| <description>SPI WorkspaceReader extension that opts out of plugin resolution</description> |
There was a problem hiding this comment.
💡 Stale description from the previous approach — the extension no longer "opts out of plugin resolution." The SPI now provides workspace artifact resolution, and cache invalidation is handled separately via PluginRealmCache.invalidate().
| <description>SPI WorkspaceReader extension that opts out of plugin resolution</description> | |
| <description>SPI WorkspaceReader extension for IDE workspace artifact resolution</description> |
| <packaging>jar</packaging> | ||
|
|
||
| <name>Maven Integration Test :: mng-8766</name> | ||
| <description>Verify that SPI WorkspaceReader is used for dependency resolution but not for plugin resolution.</description> |
There was a problem hiding this comment.
💡 Same stale description — the IT no longer verifies plugin resolution exclusion.
| <description>Verify that SPI WorkspaceReader is used for dependency resolution but not for plugin resolution.</description> | |
| <description>Verify that SPI WorkspaceReader is discovered and consulted for artifact resolution.</description> |
Problem
Three major Java IDEs independently override the same internal Maven component —
PluginDependenciesResolver— for identical reasons:Maven40PluginDependenciesResolver@Priority(10)EclipsePluginDependenciesResolverDefaultPluginDependenciesResolverNbPluginDependenciesResolverDefaultPluginDependenciesResolverThe root cause: m2e's source still carries the original comment from 2009:
DefaultPluginRealmCacheis@Singleton. Plugin realms cannot be purged within a session. If the IDE workspace reader resolves a plugin, the cached realm becomes stale when workspace sources change. The only available workaround is to disable the IDE workspace reader during plugin resolution — which requires overriding an internal component.This was raised in the Maven dev list: [DISCUSS] No supported extension point for plugin/extension resolution (in IDEs)?
Solution
Introduce a proper SPI in
maven-api-spi:maven-resolver-apidependency requiredisApplicableForPluginResolution()lets IDE integrators opt their reader out of plugin resolution without touching any internal componentSpiWorkspaceReaderAdapterbridges SPI implementations into the resolver workspace reader chainDefaultPluginDependenciesResolverfilters out non-applicable readers when building plugin sessionsMigration for IDE integrators
Instead of extending
DefaultPluginDependenciesResolver:Changes
maven-api-spi: newWorkspaceReaderSPI interfacemaven-core:SpiWorkspaceReaderAdapterbridges SPI → resolver;DefaultMaveninjects SPI readers;DefaultPluginDependenciesResolverfilters them per plugin sessionmng-8766: verifies SPI reader withisApplicableForPluginResolution()=falseis not called during plugin resolution