[MNG-8766] Add WorkspaceReader SPI to maven-api-spi for IDE integration

[gnodet]

Problem

Three major Java IDEs independently override the same internal Maven component — PluginDependenciesResolver — for identical reasons:

IDE	Class	Approach
IntelliJ IDEA	Maven40PluginDependenciesResolver	implements interface directly, @Priority(10)
Eclipse m2e	EclipsePluginDependenciesResolver	extends DefaultPluginDependenciesResolver
NetBeans	NbPluginDependenciesResolver	extends DefaultPluginDependenciesResolver

The root cause: m2e's source still carries the original comment from 2009:

Plugin realms are cached and there is currently no way to purge cached realms due to MNG-4194. Workspace plugins cannot be cached, so we disable this until MNG-4194 is fixed.

DefaultPluginRealmCache is @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:

package org.apache.maven.api.spi;

public interface WorkspaceReader extends SpiService {
    Optional<Path> findArtifact(Artifact artifact);
    List<String> findVersions(Artifact artifact);

    default boolean isApplicableForPluginResolution() {
        return true;
    }
}

• Uses Maven 4 API types exclusively — no maven-resolver-api dependency required
• isApplicableForPluginResolution() lets IDE integrators opt their reader out of plugin resolution without touching any internal component
• SpiWorkspaceReaderAdapter bridges SPI implementations into the resolver workspace reader chain
• DefaultPluginDependenciesResolver filters out non-applicable readers when building plugin sessions

Migration for IDE integrators

Instead of extending DefaultPluginDependenciesResolver:

// BEFORE (internal, fragile)
@Named @Singleton
class MyIdePluginDependenciesResolver extends DefaultPluginDependenciesResolver {
    @Override public Artifact resolve(Plugin plugin, ...) {
        try (var d = myWorkspaceReader.disable()) {
            return super.resolve(plugin, ...);
        }
    }
}

// AFTER (SPI, stable)
@Named
class MyIdeWorkspaceReader implements org.apache.maven.api.spi.WorkspaceReader {
    @Override public Optional<Path> findArtifact(Artifact artifact) { ... }
    @Override public List<String> findVersions(Artifact artifact) { ... }
    @Override public boolean isApplicableForPluginResolution() { return false; }
}

Changes

• maven-api-spi: new WorkspaceReader SPI interface
• maven-core: SpiWorkspaceReaderAdapter bridges SPI → resolver; DefaultMaven injects SPI readers; DefaultPluginDependenciesResolver filters them per plugin session
• IT mng-8766: verifies SPI reader with isApplicableForPluginResolution()=false is not called during plugin resolution

[gnodet-bot]

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.

[gnodet-bot]

💡 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.

Suggested change

	// 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");

[gnodet]

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.

[laeubi]

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?

[laeubi]

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.

[gnodet]

The isApplicableForPluginResolution() method was removed entirely in 44e96719ff based on your feedback. The current SPI has no such method — the WorkspaceReader interface now only exposes findArtifact() and findVersions(). The design discussion is captured in the PR thread comment above.

[gnodet-bot]

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> to lookup.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 — DefaultMavenSessionScopeTest updated 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.

[gnodet]

Thanks @laeubi, you're right on both counts.

The approach has been reworked in 44e96719ff:

• Removed isApplicableForPluginResolution() from the WorkspaceReader SPI — opting out of plugin resolution was the wrong lever.
• Added PluginRealmCache.invalidate(Artifact) (with a default no-op for backward compat) so IDE integrators can purge stale plugin realms when a workspace artifact is rebuilt. DefaultPluginRealmCache implements it by evicting all entries whose resolved artifacts match the given groupId:artifactId:version and disposing the associated ClassRealm.

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 isApplicableFor(ResolutionContext) with an enum — agreed that would be overkill given the direction change.

[gnodet-bot]

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.

[gnodet-bot]

💡 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().

Suggested change

	<description>SPI WorkspaceReader extension that opts out of plugin resolution</description>
	<description>SPI WorkspaceReader extension for IDE workspace artifact resolution</description>

[gnodet-bot]

💡 Same stale description — the IT no longer verifies plugin resolution exclusion.

Suggested change

	<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>

[gnodet-bot]

Re-review after 7b734aae0a. Both stale <description> elements fixed exactly as suggested — extension pom now says "IDE workspace artifact resolution" and project pom says "discovered and consulted for artifact resolution."

All prior findings addressed. SPI design, adapter, cache invalidation, and IT remain solid from the previous review.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

Re-review after 8850b59ed8. Good fix — moving SPI reader discovery from doExecute() to setupWorkspaceReader() is correct: core extensions aren't in the container until after buildGraph(), so the earlier lookup.lookupList() call always returned empty. Using getProjectScopedExtensionComponents() is consistent with the legacy WorkspaceReader discovery on the line above.

One stale comment from the previous approach — see inline.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

💡 Stale comment — this code no longer uses lookupList() directly; it uses getProjectScopedExtensionComponents() which scans project realms.

Suggested change

	// n+1) SPI workspace readers (org.apache.maven.api.spi) — discovered here, after buildGraph()
	// has loaded core extensions into the container so they are visible via lookupList.
	// n+1) SPI workspace readers (org.apache.maven.api.spi) — discovered here, after buildGraph()
	// has loaded core extensions into the container so they are visible via getProjectScopedExtensionComponents.

[gnodet]

Fixed in 8bbf426.

[gnodet-bot]

Re-review after 438208ff5e (spotless formatting).

The formatting fix is fine, but the stale comment from the previous review is still not addressed — see inline.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

💡 Not addressed from previous review — this comment still says "visible via lookupList" but the code uses getProjectScopedExtensionComponents(). Raised in the previous review at 8850b59ed8.

Suggested change

	// n+1) SPI workspace readers (org.apache.maven.api.spi) — discovered here, after buildGraph()
	// has loaded core extensions into the container so they are visible via lookupList.
	// n+1) SPI workspace readers (org.apache.maven.api.spi) — discovered here, after buildGraph()
	// has loaded core extensions into the container so they are visible via getProjectScopedExtensionComponents.

[gnodet]

Fixed in 8bbf426.

[gnodet-bot]

Re-review after 8bbf426e4c. Stale comment fixed — now correctly references getProjectScopedExtensionComponents instead of lookupList.

All prior findings from previous reviews are addressed. SPI design, adapter, cache invalidation, and IT remain solid.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

Re-review after 3f224de701 — spotless formatting only (wrapped long comment line in DefaultMaven). No logic change. All prior findings remain addressed.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

Re-review after b36eba696c. The switch from getProjectScopedExtensionComponents() to the maven-di Injector via SpiWorkspaceReadersHolder is the correct fix — SPI components annotated with @org.apache.maven.api.di.Named live in the maven-di layer, not Plexus/SISU, so Plexus lookups always returned empty. The new @Singleton holder receives all named WorkspaceReader bindings via Map<String, WorkspaceReader> injection and is instantiated lazily after buildGraph(), when extension bindings are visible. The META-INF/maven/org.apache.maven.api.di.Inject registration is correct.

No new issues. All prior findings remain addressed.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

Re-review after 23fb055bbc. Two clean fixes:

• DefaultMaven — lookup simplified from lookup.lookup(Injector.class).getInstance(SpiWorkspaceReadersHolder.class) to lookup.lookup(SpiWorkspaceReadersHolder.class). Correct: the holder is @Named @Singleton under maven-di, bridged to Sisu via SisuDiBridgeModule, so a plain Plexus lookup() finds it. The comment now accurately describes this mechanism.
• SpiWorkspaceReadersHolder — @Nullable added on the Map<String, WorkspaceReader> parameter with a null-safe initialization guard. Correct fix for the zero-readers case: maven-di injects null for an unsatisfied Map<K,V> binding rather than throwing.

All prior findings remain addressed. No new issues introduced.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

Re-review after 23fb055bbc. The SpiWorkspaceReadersHolder approach is cleaner than the direct Injector access from b36eba696c — using Lookup.lookup() keeps the code consistent with how other components are looked up in DefaultMaven, and the META-INF/maven/org.apache.maven.api.di.Inject registration is the right registration mechanism.

Two issues found:

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

⚠️ StringVersion.compareTo() uses String.compareTo() — lexicographic ordering. This breaks for multi-digit version components: "10".compareTo("9") returns a negative value ('1' < '9'), so version 10 would sort before 9.

The contract of org.apache.maven.api.Version requires semantic ordering. Any SPI implementation that sorts or compares versions (e.g. to pick the highest available workspace version) will silently get wrong results.

Since this is a lightweight wrapper whose toString() is used for equality and lookup, and Maven has a proper VersionParser available via the session, the safest fix is to delegate compareTo to a real parser — or at minimum document the limitation explicitly in the Javadoc so implementors know not to rely on it.

Suggested change

	@Override
	public int compareTo(Version o) {
	return version.compareTo(o.toString());
	private record StringVersion(String version) implements Version {
	@Override
	public int compareTo(Version o) {
	// NOTE: lexicographic comparison only — does not honour semantic version ordering.
	// This wrapper is intended for artifact coordinate lookup only (via toString());
	// do not use compareTo() for version range evaluation or sorting.
	return version.compareTo(o.toString());
	}

[gnodet]

Fixed in 4a192fe. Added Javadoc to StringVersion explicitly documenting that compareTo() uses lexicographic string ordering and is not correct for semantic version comparison — directing callers to use VersionParser when ordering matters. Since this wrapper is solely used to pass artifact coordinates to SPI workspace readers (not for version sorting), using a full parser here would add unnecessary session coupling; the documentation makes the limitation explicit for any future reader implementations that might call getVersion().compareTo().

[gnodet-bot]

💡 getReaders() returns the internal ArrayList directly. While SpiWorkspaceReadersHolder is a @Singleton and the list is never modified after construction, exposing a mutable List through a public method is a correctness risk — any caller can call .add() / .remove() / .clear() and silently corrupt the shared state for subsequent builds in the same session.

Return an unmodifiable view:

Suggested change

	return readers;
	}
	public List<WorkspaceReader> getReaders() {
	return Collections.unmodifiableList(readers);

[gnodet]

Fixed in 4a192fe. getReaders() now returns Collections.unmodifiableList(readers).

[gnodet-bot]

Re-review after 4a192fef92. Both findings from the previous review are addressed:

• getReaders() mutable list — Collections.unmodifiableList(readers) is now returned. Correct; the @Nullable path that initialises readers to Collections.emptyList() already returns an unmodifiable list, so the double-wrap is harmless.
• StringVersion.compareTo() Javadoc — The new class-level Javadoc is accurate and complete: it calls out the lexicographic limitation, states that the wrapper is used solely for coordinate lookup, and directs implementors to VersionParser when ordering matters.

All prior findings from previous reviews remain addressed. The SPI interface, adapter, holder, DI discovery path, and IT are solid.

This review was generated by an AI agent, Hermès on behalf of @gnodet.