[MNG-8099] Add explicit 'api' scope for dependencies and make 'compile' non-transitive for Maven 4 - #12745
[MNG-8099] Add explicit 'api' scope for dependencies and make 'compile' non-transitive for Maven 4#12745Hiteshsai007 wants to merge 6 commits into
Conversation
…' non-transitive for Maven 4
gnodet
left a comment
There was a problem hiding this comment.
Thanks for working on MNG-8099, @Hiteshsai007! The api/compile scope split concept is well-motivated (mirroring Gradle's api/implementation distinction). A few issues need to be addressed:
🔴 Critical: Consumer POM regression — compile-scoped dependencies silently stripped
DefaultConsumerPomBuilder.hasDependencyScope() uses !scope.isTransitive() to decide which dependencies to remove from consumer POMs. With COMPILE changing from transitive=true to transitive=false, all compile-scoped dependencies — and dependencies with no explicit scope (the most common case, which defaults to COMPILE) — will be stripped from consumer POMs.
This breaks downstream dependency resolution for essentially every Maven 4 project. The method is not gated on model version, so even modelVersion=4.0.0 projects are affected. The PR's backward compatibility claim ("Maven 3 / modelVersion 4.0.0: No change") is incorrect for this code path.
This is the same regression identified in our review of PR #12723. The fix requires hasDependencyScope() to use a different criterion than isTransitive() — e.g., checking whether the scope should appear in consumer POMs (compile, api, runtime) directly.
🔴 Critical: Resolver treats compile as non-transitive
Both Maven4ScopeManagerConfiguration files pass DependencyScope.COMPILE.isTransitive() to createDependencyScope(). After this change, the resolver will treat compile as non-transitive in Maven 4, meaning transitive dependencies of compile-scoped libraries won't be resolved — a massive behavioral change with no migration path.
🔴 Accidental files committed
Two files are included in the diff that shouldn't be:
issue_comment.md— a binary (UTF-16) file containing a GitHub issue comment about PR #12744plexus-sec-dispatcher— a git submodule reference (160000mode) pointing to commita3b5741
Both must be removed before merging.
🔴 No tests provided
The PR checklist marks "Write unit tests" as complete, but zero test files are modified or added. A change of this magnitude to Maven's dependency scope system needs comprehensive test coverage for:
- Consumer POM generation with compile vs api-scoped dependencies
- Transitive resolution behavior for both scopes
- Model validation of api scope in 4.0.0 vs 4.1.0 POMs
- Backward compatibility with Maven 3
🟡 MavenModelVersion does not detect api scope
The auto-generated MavenModelVersion class does not check for api-scoped dependencies. Since API.isTransitive()=true, api-scoped deps survive hasDependencyScope() filtering, but the consumer POM could be written with modelVersion=4.0.0 — creating an inconsistency where a 4.0.0 POM contains a scope only valid in 4.1.0+.
Recommendations:
- Update
hasDependencyScope()to not rely onisTransitive()for determining consumer POM inclusion - Gate the compile→non-transitive behavior on model version (as MNG-8099 description states: "only with the new modelVersion to opt into")
- Remove the accidental files
- Add comprehensive tests
- Address MavenModelVersion detection of the api scope
The direction is right — the implementation just needs more work to handle the cross-cutting impacts. Happy to re-review once updated!
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of gnodet
|
I question the idea that only API dependencies should be transitive and not the dependencies with the Or maybe you mean In other words, keep a separation of tasks: Maven controls what to put on the module-path, and |
gnodet
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — The concept of splitting compile/api scopes (mirroring Gradle's api/implementation distinction) is well-motivated, but the implementation introduces critical regressions that would break consumer POM generation and dependency resolution for essentially every Maven 4 project. Additionally, accidental files are committed and no tests are provided.
Critical Issues
-
Consumer POM regression (high): Changing
COMPILE.isTransitive()tofalsecausesDefaultConsumerPomBuilder.hasDependencyScope()(line 241:return scope == null || !scope.isTransitive()) to strip all compile-scoped dependencies from consumer POMs. Since compile is Maven's default scope, essentially all unscoped dependencies would be silently removed from published artifacts. This is not gated by model version — it applies unconditionally. -
Resolver regression (high):
Maven4ScopeManagerConfigurationpassesDependencyScope.COMPILE.isTransitive()(nowfalse) tocreateDependencyScope(). InbuildResolutionScopes(), COMPILE now falls intononTransitiveDependencyScopes, causing the resolver to eliminate transitive dependencies of compile-scoped libraries. This breaks virtually all Maven 4 builds. (Maven 3 is unaffected sinceMaven3ScopeManagerConfigurationhardcodestrue.) -
Accidental files (high):
issue_comment.md(a UTF-16 binary file containing a GitHub comment) andplexus-sec-dispatcher(a git submodule reference at mode 160000) were accidentally committed. Both must be removed. -
No tests (high): Zero test files are modified or added despite the PR checklist marking "Write unit tests" as complete. A change of this magnitude requires comprehensive tests for consumer POM generation, dependency resolution, scope inheritance, and backward compatibility.
-
MavenModelVersion gap (medium): The auto-generated
MavenModelVersion.is_4_1_0()does not inspect dependency scopes. A 4.1.0 source POM withapiscope dependencies would produce a consumer POM detected as 4.0.0, yet containingapiscope entries that Maven 3 cannot parse. -
Stale Javadoc (low): The
COMPILEJavadoc still says "Compile, runtime and test." without clarifying the non-transitive semantics.
Prior Feedback
A prior review by @gnodet (Aug 13) raised the same critical issues. No new commits have been pushed since that review, so none of those issues have been addressed. @desruisseaux also raised design concerns about whether making compile non-transitive is the right approach.
There is also a duplicate PR #12723 by a different author targeting the same MNG issue — coordination may be needed.
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
| * Compile, runtime and test. | ||
| */ | ||
| COMPILE("compile", true), | ||
| COMPILE("compile", false), |
There was a problem hiding this comment.
Critical regression: Changing COMPILE from transitive=true to transitive=false has cascading effects:
-
DefaultConsumerPomBuilder.hasDependencyScope()uses!scope.isTransitive()to decide which dependencies to strip from consumer POMs. With this change, all compile-scoped dependencies (the most common case, since compile is the default scope) will be silently removed from consumer POMs. -
Maven4ScopeManagerConfigurationpassesDependencyScope.COMPILE.isTransitive()tocreateDependencyScope(). COMPILE now falls intononTransitiveDependencyScopesinbuildResolutionScopes(), causing the resolver to eliminate transitive dependencies of compile-scoped libraries.
At minimum, DefaultConsumerPomBuilder.hasDependencyScope() needs to be updated to not rely on isTransitive() for consumer POM inclusion, and the compile-to-non-transitive behavior should be gated on model version so 4.0.0 projects retain current behavior.
- Fix Consumer POM regression: Update hasDependencyScope to use an inclusion list rather than !isTransitive, ensuring non-transitive compile dependencies are kept. - Fix Resolver regression: Map compile scope to api scope internally for models < 4.1.0 to preserve transitive resolution for older artifacts. - Fix MavenModelVersion gap: Inject hasApiDependency check into Modello generation so 4.1.0 is required when using api scope. - Fix Stale Javadoc: Update DependencyScope.COMPILE to indicate it is non-transitive. - Remove accidentally committed issue_comment.md and plexus-sec-dispatcher. Signed-off-by: Hitesh <hiteshsaibv.24cs@saividya.ac.in>
|
Thanks for the thorough review and for catching these critical edge cases, @gnodet! I've pushed a new commit addressing all 6 points raised: 1. Consumer POM regression (high)Fixed: Replaced 2. Resolver regression (high)Fixed: To maintain full backward compatibility for older projects while enforcing non-transitive 3. Accidental files (high)Fixed: Cleaned up the working tree. 4. No tests (high)I am currently working on adding the comprehensive unit tests to cover consumer POM generation and the transitive resolution behavior. I'll push these up in a follow-up commit shortly to fulfill this requirement! 5. MavenModelVersion gap (medium)Fixed: Updated the Modello template ( 6. Stale Javadoc (low)Fixed: Updated the Javadoc for |
gnodet
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — Re-review after new commits. Thank you for addressing several issues from the previous review — the accidental files are removed, the consumer POM hasDependencyScope() fix is correct, MavenModelVersion now detects api-scoped dependencies as requiring 4.1.0, and the Javadoc is updated.
However, critical issues remain:
Still blocking
-
No tests (high): Zero test files are modified or added. A change that alters the transitivity semantics of Maven's most fundamental scope requires comprehensive test coverage:
- Consumer POM generation retains compile, api, and runtime deps while stripping provided/test/system
- Resolver treats compile as non-transitive and api as transitive for 4.1.0 models
- Resolver remaps compile→api for 4.0.0 models (backward compat)
- Model validation rejects api scope in 4.0.0 POMs
- MavenModelVersion detects api-scoped dependencies as requiring 4.1.0
-
Per-dependency MavenModelVersion instantiation (high): The scope remapping in
DefaultArtifactDescriptorReader.convert()instantiatesnew MavenModelVersion()and callsgetModelVersion(model)on every individual dependency. For N dependencies, this creates N objects and scans the entire model N times. The model version is invariant per model — compute it once inpopulateResult()before the dependency loops. -
Feature detection vs declared version (medium): The scope remapping uses
MavenModelVersion().getModelVersion(model)(feature detection) instead ofmodel.getModelVersion()(declared version). If a developer writes a 4.1.0 POM using only compile scope (intending non-transitive) without other 4.1.0 syntactic features, feature detection returns "4.0.0" and the code remaps compile→api (transitive), violating the developer's explicit intent. Usingmodel.getModelVersion()would be simpler and correct. -
Compile-non-transitive is itself a 4.1.0 feature (medium): The template adds
hasApiDependency()as a 4.1.0 check, but there's no mechanism to detect that "compile with non-transitive semantics" is itself a 4.1.0 feature. A 4.1.0 POM using only compile (no api) produces a consumer POM with modelVersion=4.0.0, causing downstream resolution to remap compile→api. This design limitation should at minimum be documented. -
Formatting (low): Missing blank line between the new
hasApiDependency(ModelBase)method and the existinghas(String)method inmodel-version.vm.
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
| String scope = dependency.getScope() != null ? dependency.getScope() : ""; | ||
| if ("compile".equals(scope) || "".equals(scope)) { | ||
| String modelVersion = new org.apache.maven.model.v4.MavenModelVersion().getModelVersion(model); | ||
| if (modelVersion == null || modelVersion.startsWith("4.0.")) { |
There was a problem hiding this comment.
Performance + correctness: new MavenModelVersion().getModelVersion(model) is called per dependency inside convert(), which runs in two loops (direct + managed deps). This creates N objects and scans the entire model N times.
Additionally, MavenModelVersion.getModelVersion() performs feature detection (scanning model fields), not declared-version reading. If a 4.1.0 POM uses only compile scope without other 4.1.0 features, this returns "4.0.0" and incorrectly remaps compile→api.
Suggested fix: compute the version once in populateResult() using model.getModelVersion() (declared version) and pass it to convert():
// In populateResult(), before the loops:
String modelVersion = model.getModelVersion();
// In convert():
if ("compile".equals(scope) || "".equals(scope)) {
if (modelVersion == null || modelVersion.startsWith("4.0.")) {
scope = "api";
}
}|
I still do not understand what is the goal here. A |
Signed-off-by: Hitesh <hiteshsaibv.24cs@saividya.ac.in>
|
Hi @desruisseaux, thank you for the feedback! The primary goal here is to establish strict compile-time isolation at the build-tool level, mirroring the distinction Gradle makes between While it's absolutely true that a library needs its internal dependencies on the classpath to function (which is why both If project While the Java Module System ( By making |
|
Thanks again for the re-review, @gnodet! I've pushed a new commit addressing all the remaining feedback points: 1. No tests (Fixed)Added comprehensive unit tests covering the new behavior:
2. Per-dependency MavenModelVersion instantiation (Fixed)Refactored 3. Feature detection vs declared version (Fixed)As part of the fix above, 4. Compile-non-transitive is itself a 4.1.0 feature (Documented)Good catch on the design limitation! Since we cannot force 5. Formatting (Fixed)Added the missing blank line in |
|
Thanks @Hiteshsai007 for the reply. I suspected that it was for providing different classpaths at compile-time versus runtime. But it is a complication for Java modules. We would need to specify that for dependencies placed on the module-path, the distinction between |
|
Hi @desruisseaux, those are great points, and the intersection with JPMS is definitely an important consideration. I don't believe this introduces technical debt, but rather bridges a critical feature gap that the community has been requesting for a long time. There are a few reasons why adding this to Maven is beneficial even alongside Java modules:
Ultimately, this gives developers the tools to enforce encapsulation today, without forcing them to undertake a full migration to JPMS before they are ready. |
gnodet
left a comment
There was a problem hiding this comment.
Re-review (3rd pass) — All prior blocking issues resolved 👍
Great progress @Hiteshsai007! The four issues from the previous review are all addressed in commit 589b172:
✅ Tests added — 3 in MavenModelVersionTest (api scope in direct deps, managed deps, compile-only staying at 4.0.0) + 2 in ConsumerPomBuilderTest (scope retention/stripping)
✅ Per-dependency MavenModelVersion perf — Fixed: remapCompileToApi boolean computed once in populateResult() using model.getModelVersion()
✅ Feature-detection vs declared-version — Resolved: code now uses model.getModelVersion() directly with clear comment about respecting developer intent
✅ Compile-non-transitive documentation — Documented in Javadoc on DependencyScope.COMPILE
Non-blocking suggestions
-
[medium] Missing test for the remap logic itself — The backward-compatibility remap in
DefaultArtifactDescriptorReader.populateResult()(compile→api for 4.0.0 POMs) is critical logic, but the new tests cover model-version detection and consumer POM scope retention — not the remap path itself. A test asserting that a 4.0.0 model triggers compile→api remap and a 4.1.0 model does not would guard this invariant against regression. -
[low] Stale comments in
DefaultConsumerPomBuilder— Lines 266 and 284 still say "Only keep transitive scopes", butCOMPILEis now non-transitive yet explicitly retained byhasDependencyScope(). Consider updating to "Only keep consumer-visible scopes (compile, api, runtime)" to match the new semantics. -
[low] Profile path untested —
hasApiDependencyinmodel-version.vmcorrectly traverses profiles, but none of the new tests place an api-scoped dependency inside a<profile>block. AMavenModelVersionTestcase for this would cover the profile traversal path.
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
|
|
||
| for (Repository repository : model.getRepositories()) { | ||
| result.addRepository(session.toRepository( | ||
| session.getService(RepositoryFactory.class).createRemote(repository))); |
There was a problem hiding this comment.
Non-blocking — test coverage gap: The remapCompileToApi logic computed here and applied in convert() is the cornerstone of backward compatibility, but it has no direct unit test. Consider adding a test in DefaultArtifactDescriptorReaderTest that verifies:
- 4.0.0 modelVersion →
compilescope is remapped toapi - 4.1.0 modelVersion →
compilescope is left as-is
| scope = DependencyScope.forId(scopeId); | ||
| } | ||
| return scope == null || !scope.isTransitive(); | ||
| return scope != DependencyScope.COMPILE && scope != DependencyScope.RUNTIME && scope != DependencyScope.API; |
There was a problem hiding this comment.
Non-blocking — stale comments: The hasDependencyScope() method now explicitly checks for COMPILE, RUNTIME, and API instead of using isTransitive(). The comments at lines 266 and 284 still say "Only keep transitive scopes" but COMPILE is no longer transitive. Consider updating them to "Only keep consumer-visible scopes (compile, api, runtime)" to match the new logic.
…g issues resolved)
I believe that the low adoption is because using Java Module in Maven 3 and Gradle is very hard, especially when we need to deal with the We could enforce the requirement that |
Signed-off-by: Hitesh <hiteshsaibv.24cs@saividya.ac.in>
Signed-off-by: Hitesh <hiteshsaibv.24cs@saividya.ac.in>
|
Thanks for the thorough review process, @gnodet! I'm glad we've ironed out the edge cases. I've just pushed two commits to address the final non-blocking suggestions:
Thanks again for the guidance! |
|
Hi @desruisseaux, I completely agree that Maven 4's improvements to Java Modules are going to be a game changer, and lowering that barrier to entry is incredibly important! While using JPMS as an incentive for better encapsulation is an interesting idea, there are a few reasons why providing
In short, bringing this feature to Maven provides immediate correctness and performance benefits to the entire ecosystem today, while laying a cleaner foundation for those who will migrate to Java Modules tomorrow. |
gnodet
left a comment
There was a problem hiding this comment.
Re-review (4th pass) — Great progress on test coverage!
All three suggestions from the 3rd review are addressed:
✅ Stale comments fixed — Both comments in DefaultConsumerPomBuilder.java now correctly read "Only keep consumer-visible scopes (compile, api, runtime)"
✅ Profile path test added — testApiScopeInProfileRequires410() covers api-scoped dependencies inside profile blocks
✅ Remap test added — DefaultArtifactDescriptorReaderTest covers all four key remap scenarios (compile→api, empty→api, compile stays, empty stays)
However, the remap test has a constructor issue that needs fixing:
Blocking finding
🔴 [high] Test will NPE before executing — new DefaultArtifactDescriptorReader(null, null, null, null, null) at line 37 passes all nulls to a constructor that calls Objects.requireNonNull on every parameter (see lines 101-114 of the production class). The test will throw NullPointerException: "versionResolver cannot be null" before convert() is ever reached.
Fix: Use Mockito mocks for the constructor arguments — Mockito is already available as a test dependency in this module. Example:
DefaultArtifactDescriptorReader reader = new DefaultArtifactDescriptorReader(
Mockito.mock(VersionResolver.class),
Mockito.mock(ArtifactResolver.class),
Mockito.mock(ModelBuilder.class),
Mockito.mock(RepositoryEventDispatcher.class),
Collections.emptyMap());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
| void testRemapCompileToApi() throws Exception { | ||
| // Create an instance of DefaultArtifactDescriptorReader | ||
| DefaultArtifactDescriptorReader reader = new DefaultArtifactDescriptorReader(null, null, null, null, null); | ||
|
|
There was a problem hiding this comment.
🔴 Will NPE before the test executes. The constructor calls Objects.requireNonNull on every parameter (lines 101-114 of DefaultArtifactDescriptorReader.java), so this throws NullPointerException: "versionResolver cannot be null" immediately.
Use Mockito mocks instead:
| DefaultArtifactDescriptorReader reader = new DefaultArtifactDescriptorReader( | |
| Mockito.mock(VersionResolver.class), | |
| Mockito.mock(ArtifactResolver.class), | |
| Mockito.mock(ModelBuilder.class), | |
| Mockito.mock(RepositoryEventDispatcher.class), | |
| Collections.emptyMap()); |
(Also add the corresponding imports for Mockito, VersionResolver, ArtifactResolver, ModelBuilder, RepositoryEventDispatcher, and Collections.)
Signed-off-by: Hitesh <hiteshsaibv.24cs@saividya.ac.in>
|
Thanks for catching that, @gnodet! You're absolutely right—that I have pushed a new commit to fix this finding: Everything should be fully up-to-date now. Thank you for being so thorough throughout this review process! |
gnodet
left a comment
There was a problem hiding this comment.
Re-review (5th pass) — LGTM! 🎉
The constructor NPE from the 4th review is fixed correctly — Mockito mocks for all constructor dependencies, Collections.emptyMap() for the relocation sources map.
All issues across 5 review cycles are now fully resolved:
✅ Tests added (MavenModelVersionTest + ConsumerPomBuilderTest + DefaultArtifactDescriptorReaderTest)
✅ Per-dependency MavenModelVersion performance fixed
✅ Feature-detection vs declared-version gap resolved
✅ Compile-non-transitive documented in Javadoc
✅ Stale comments updated to "consumer-visible scopes"
✅ Profile traversal path tested
✅ Remap logic tested with all 4 scenarios
✅ Constructor NPE fixed with Mockito mocks
Great persistence working through the feedback, @Hiteshsai007!
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
Resolves #10786 (MNG-8099)
Summary
This PR introduces a new transitive
apidependency scope for Maven 4 and makes the existingcompilescope non-transitive. This aligns Maven's dependency model with modern build tools like Gradle, which distinguish between API (publicly exposed) and implementation (internal) dependencies.Problem
Currently in Maven, the
compilescope is transitive — meaning if library A depends on library B withcompilescope, any project depending on A will also see B on its compile classpath. This leads to "leaky" dependency graphs where implementation details are exposed to consumers, causing:Solution
1. New
apiScope (DependencyScope.java)API("api", true)— a transitive scope for dependencies that form part of the public APICOMPILE("compile", false)from transitive to non-transitive — for implementation-only dependencies2. Scope Manager Configuration (
Maven4ScopeManagerConfiguration.java)apidependency scope in both:impl/maven-impl/.../Maven4ScopeManagerConfiguration.javacompat/maven-resolver-provider/.../Maven4ScopeManagerConfiguration.javaapiscope is configured withall()build paths (compile + runtime), matchingcompile's path visibility3. Path Scope Updates (
PathScope.java)DependencyScope.APIto all four standard path scopes:MAIN_COMPILE— api dependencies appear on the compile classpathMAIN_RUNTIME— api dependencies appear on the runtime classpathTEST_COMPILE— api dependencies appear on the test compile classpathTEST_RUNTIME— api dependencies appear on the test runtime classpath4. Model Validation (
DefaultModelValidator.java)DependencyScope.APIto the list of Maven 4-only scopes that are rejected when used withmodelVersion4.0.0 (legacy POMs)apiscope is only valid for Maven 4.1.0+ model versionsUsage (Maven 4)
Backward Compatibility
compilescope continues to behave as before (transitive) throughMaven3ScopeManagerConfiguration, and theapiscope is rejected by validation.apifor dependencies they want to expose transitively.Following this checklist to help us incorporate your contribution quickly and easily:
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.