Fix #13068: make PluginDependenciesResolver methods default

[ascheman]

Summary

Forward port of 349395c, merged into maven-4.0.x via #13069.

Closes #13068.

resolveCoreExtensionAndFlatten and resolvePluginAndFlatten were added to the internal PluginDependenciesResolver as abstract methods. That happened on both lines — #12329 here, #12335 on maven-4.0.x — but the fix so far landed only on maven-4.0.x, so 4.1.0 would reintroduce the identical regression.

Implementations that live out of tree are compiled against one Maven and run on another, so the first plugin resolution dies with AbstractMethodError. IntelliJ IDEA, Eclipse m2e and NetBeans all override this component; IntelliJ implements the interface directly and therefore breaks. Christofer Dutz confirmed on Apache PLC4X that a build carrying this change imports cleanly where stock rc-6 fails.

What differs from the maven-4.0.x commit

The production change and the unit test are byte-identical. Only the IT needed adjusting, because master has moved on in three ways:

• No TestSuiteOrdering entry. That class now orders by name pattern (MavenITgh<number>) instead of keeping an explicit list, so MavenITgh13068… is picked up automatically. The file is untouched here.
• IT harness. AbstractMavenIntegrationTestCase no longer takes a version range, extractResources returns Path, and newVerifier(String) is deprecated in favour of newVerifier(Path). The IT was rewritten to the current idiom.
• ITs run under failsafe here, not surefire.

Verification

Run locally against this branch:

mvn verify -Papache-release -Dgpg.skip=true     BUILD SUCCESS   (0 javadoc findings)
unit  PluginDependenciesResolverDefaultMethodsTest   Tests run: 3, Failures: 0, Errors: 0, Skipped: 0
IT    MavenITgh13068LegacyPluginDependenciesResolverTest
      failsafe:integration-test @ core-it-suite
      Tests run: 1, Failures: 0, Errors: 0, Skipped: 0
      [gh-13068] legacy PluginDependenciesResolver installed
      [gh-13068] resolvePlugin reached for maven-clean-plugin

The two marker lines are asserted by the test, so it cannot pass without the extension actually displacing the default component. On maven-4.0.x the same IT was verified red without the change, failing with AbstractMethodError at DefaultMavenPluginManager.createPluginRealm.

Note

The resolveCoreExtensionAndFlatten default is best effort — a dedicated implementation additionally reads the extension's artifact descriptor to apply relocations and run MavenPluginDependenciesValidator. Same caveat as on the 4.0.x PR, where @gnodet approved it as is.

[gnodet]

The fix is correct. Two abstract methods made default — the minimal change needed to restore backward compatibility with every out-of-tree implementation of PluginDependenciesResolver. CI is green across all platforms (JDK 17/21/25, Linux/macOS/Windows). The implementation is byte-identical to the maven-4.0.x commit.

One low-severity gap in the unit test: both delegation tests pass null as the session argument, then either assert assertNull(resolver.session) (which is trivially true) or don't check session at all. The test therefore doesn't actually exercise that the session is forwarded through the default method chain — it would pass even if the default method dropped the session on the floor. Flagging it as a nit; it doesn't affect correctness of the production fix, only test coverage depth.

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

[gnodet]

🔍 Nit — trivial session assertion: null is passed as the session argument and this assertion checks that null comes out the other side — which is guaranteed regardless of whether the default method actually forwards the argument. To make this test meaningful, pass a non-null mock or stub session and assert assertSame:

Suggested change

	assertNull(resolver.session);
	assertNull(resolver.session);

Suggested improvement (cannot be a one-click suggestion due to the required change in the call site at line 104):

// line 103-110
RepositorySystemSession session = mock(RepositorySystemSession.class);
assertSame(RESULT, resolver.resolvePluginAndFlatten(plugin, artifact, filter, repositories, session));

assertSame(plugin, resolver.plugin);
assertSame(artifact, resolver.pluginArtifact);
assertSame(filter, resolver.dependencyFilter);
assertSame(repositories, resolver.repositories);
assertSame(session, resolver.session);

(Add import static org.mockito.Mockito.mock; or use Mockito.mock(RepositorySystemSession.class) — the project already has Mockito on the test classpath.)

[gnodet]

🔍 Nit — session forwarding not tested: resolveCoreExtensionAndFlatten is called with null for the session, but the test has no assertion that the session is passed through to resolvePlugin. A non-null session should be passed and verified with assertSame(session, resolver.session) — same pattern as the sibling test.

[gnodet]

Re-review after second commit ("Address review: assert the session is forwarded", 62967e1).

Both nits from the first pass are resolved:

• resolvePluginAndFlattenDelegatesToResolvePlugin: now uses mock(RepositorySystemSession.class) and asserts assertSame(session, resolver.session) — the delegation is actually exercised.
• resolveCoreExtensionAndFlattenDelegatesToResolvePlugin: same pattern, line 135 now asserts session forwarding.

The fix itself is solid: two methods converted from abstract to default, Javadoc correctly documents the delegation chain and the circular-recursion trap for any implementation that tries to delegate resolvePlugin back to resolvePluginAndFlatten without overriding the latter. The newerMethodsAreDefaultMethods reflection test is a clean regression guard against future accidental reintroduction of abstract. The IT is faithful to current master conventions (newVerifier(Path), no version range constructor, failsafe) and the extension POM correctly compiles against 4.0.0-rc-5 to reproduce the binary-compat scenario.

No outstanding findings. Ready to merge.

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