Add API dependency scope support - #12723
Conversation
gnodet
left a comment
There was a problem hiding this comment.
The design intent — splitting compile into transitive api and non-transitive compile (similar to Gradle's api/implementation) — is a good direction for Maven 4.1. However, the implementation has significant gaps that would cause regressions:
Critical: Consumer POM builder regression
DefaultConsumerPomBuilder.hasDependencyScope() (line 292–301) returns true (remove dependency) when !scope.isTransitive(). With this PR changing COMPILE to non-transitive, all compile-scoped dependencies (and dependencies with no explicit scope, which default to COMPILE) would be stripped from consumer POMs.
The existing integration test MavenITgh11162ConsumerPomScopesTest explicitly asserts that compile-scoped dependencies are kept in the consumer POM (assertEquals(2, numDeps, "Consumer POM should keep only compile and runtime dependencies")) — this test would fail with the current change.
The consumer POM builder needs to be updated to handle the new api scope and adjust its filtering logic accordingly.
Missing test coverage
The PR description says it extends validator test coverage, but the only test change adds scopes.add(new DepScope("api")) to the mock scope list — no actual test methods are added. Needed:
- Unit test:
apiscope rejected undermodelVersion4.0.0 - Unit test:
apiscope accepted undermodelVersion4.1.0 - Integration test:
apiscope transitivity behavior vs non-transitivecompile - Update
MavenITmng8750NewScopesTestto cover theapiscope
Stale javadoc
The COMPILE javadoc still says "Compile, runtime and test." but doesn't mention it is no longer transitive. Since the new API javadoc explicitly says "exported transitively," the COMPILE javadoc should clarify the distinction — e.g., "Compile, runtime and test. Non-transitive; use API to export transitively."
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of gnodet
gnodet
left a comment
There was a problem hiding this comment.
This PR introduces an api dependency scope (MNG-8099) but has two critical regressions and several lesser issues that need to be addressed before it can be merged.
Critical issues:
-
Consumer POM regression — Changing
COMPILEfromtransitive=truetotransitive=falsewithout updatingDefaultConsumerPomBuilder.hasDependencyScope()(which uses!scope.isTransitive()as its removal predicate at line 241) causes all compile-scoped dependencies — and dependencies with no explicit scope (the most common case, which defaults to COMPILE) — to be silently stripped from consumer POMs. The existing integration testMavenITgh11162ConsumerPomScopesTestexplicitly asserts that compile dependencies are present and would fail with this change. -
Maven 4 transitive resolution broken — Both
Maven4ScopeManagerConfigurationfiles (impl and compat) passDependencyScope.COMPILE.isTransitive()tocreateDependencyScope(). After this change, that evaluates tofalse, causing the resolver to treatcompileas non-transitive in Maven 4 mode. This means transitive dependencies of compile-scoped libraries will not be resolved — a massive behavioral change with no migration path. Maven 3 mode is unaffected (Maven3ScopeManagerConfigurationhardcodestrue), but all Maven 4 users would be affected.
Other issues:
-
Insufficient test coverage — The only test change adds
scopes.add(new DepScope("api"))to the mock scope list inDefaultModelValidatorTest.setUp(), but no actual test methods are added. This PR needs: (a) unit test thatapiscope is rejected undermodelVersion4.0.0, (b) unit test thatapiscope is accepted undermodelVersion4.1.0, (c) integration test for api/compile transitivity behavior, and (d) update toMavenITmng8750NewScopesTestto cover theapiscope. -
Incomplete javadoc — The
COMPILEjavadoc still says "Compile, runtime and test." without mentioning it is no longer transitive, while the newAPIjavadoc says "exported transitively." The distinction between the two scopes is entirely about transitivity, so theCOMPILEjavadoc should clarify the semantic change. -
Enum ordering — The
APIenum constant is placed beforeNONEandUNDEFINED, breaking the existing ordering convention. Consider placing it afterCOMPILEfor logical consistency.
Additional note: This PR largely duplicates the already-open PR #12745 (by Hiteshsai007, opened Aug 13) which makes the same changes to the same files. The efforts should be consolidated rather than duplicated.
This review was generated by an AI agent (Claude Code) and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
|
Hi @charangowdamd-cmd! I noticed we are both working on MNG-8099. I have an open PR as well (#12745) where I've been working with the maintainers to fix some of the tricky regressions with Consumer POM generation and Maven 3 backward compatibility. Would you like to collaborate? I'd be happy to incorporate any missing pieces from your PR into mine and add you as a Co-author on the commits! |
gnodet
left a comment
There was a problem hiding this comment.
This PR introduces a new api dependency scope (MNG-8099) and makes compile non-transitive, mirroring Gradle's api/implementation split. However, the implementation has critical gaps that would break existing Maven 4 projects:
Critical Issues
1. Consumer POM builder not updated (high severity)
DefaultConsumerPomBuilder.hasDependencyScope() uses !scope.isTransitive() to strip non-transitive dependencies from published consumer POMs. After this PR, ALL compile-scoped dependencies (and dependencies with no explicit scope, which default to compile) would be silently stripped from every consumer POM. The existing integration test MavenITgh11162ConsumerPomScopesTest would fail — it explicitly asserts that compile-scoped dependencies must be present in the consumer POM.
2. No version-aware handling in resolution (high severity)
Maven4ScopeManagerConfiguration is used for ALL Maven 4 projects regardless of model version. After this PR, compile-scoped transitive dependencies would be eliminated during resolution for both 4.0.0 and 4.1.0 projects via the nonTransitiveDependencyScopes filter in buildResolutionScopes(). Since 4.0.0 POMs cannot use the api scope (the validator correctly rejects it), they have no transitive alternative. The MNG-8099 design explicitly states the compile non-transitivity change "should only be done with the new modelVersion to opt into."
Additional Issues
3. Insufficient test coverage (medium severity)
The only test change adds new DepScope("api") to a mock list. No tests validate: (a) 4.0.0 POM with <scope>api</scope> is rejected, (b) 4.1.0 POM with <scope>api</scope> is accepted, (c) the compile transitivity change, or (d) the resolution behavior of the new api scope.
4. Undocumented scope semantics (medium severity)
The COMPILE Javadoc still says "Compile, runtime and test" without mentioning it is now non-transitive. The semantic relationship between api (transitive) and compile (now non-transitive) is not documented.
This review was generated by an AI agent (Claude Code) and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
| @@ -64,7 +69,7 @@ public enum DependencyScope { | |||
| /** | |||
There was a problem hiding this comment.
Changing COMPILE from isTransitive=true to isTransitive=false has cascading effects:
-
DefaultConsumerPomBuilder.hasDependencyScope()(line ~300) returnsscope == null || !scope.isTransitive()— after this change, it returnstruefor compile scope, causingremoveIf(lines 267, 285) to strip ALL compile-scoped dependencies from consumer POMs. Dependencies with no explicit scope default to COMPILE (line 296), so they would also be stripped. -
Maven4ScopeManagerConfiguration.buildResolutionScopes()filters!s.isTransitive()to buildnonTransitiveDependencyScopes(lines 140-141). After this change, compile joins that set, and its transitive dependencies are eliminated during resolution.
This would need version-aware handling — per MNG-8099, compile should only become non-transitive for 4.1.0 modelVersion POMs.
… (APPROVE), apache#12723 (REQUEST_CHANGES)
Summary
Adds support for the new
apidependency scope and includes the corresponding validation updates.Changes
DependencyScope.APIand markCOMPILEas non-transitiveVerification