Skip to content

Add API dependency scope support - #12723

Open
charangowdamd-cmd wants to merge 1 commit into
apache:masterfrom
charangowdamd-cmd:fix/api-scope-validation
Open

Add API dependency scope support#12723
charangowdamd-cmd wants to merge 1 commit into
apache:masterfrom
charangowdamd-cmd:fix/api-scope-validation

Conversation

@charangowdamd-cmd

Copy link
Copy Markdown

Summary

Adds support for the new api dependency scope and includes the corresponding validation updates.

Changes

  • Add DependencyScope.API and mark COMPILE as non-transitive
  • Include the new scope in the resolver scope manager configuration
  • Include it in the supported path scopes
  • Update model validation for 4.0.0 compatibility checks
  • Extend the validator test coverage for the new scope

Verification

  • Relevant Maven validation test was run for the modified area
  • The branch was pushed to the fork and is ready for review

@gnodet gnodet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: api scope rejected under modelVersion 4.0.0
  • Unit test: api scope accepted under modelVersion 4.1.0
  • Integration test: api scope transitivity behavior vs non-transitive compile
  • Update MavenITmng8750NewScopesTest to cover the api scope

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 gnodet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Consumer POM regression — Changing COMPILE from transitive=true to transitive=false without updating DefaultConsumerPomBuilder.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 test MavenITgh11162ConsumerPomScopesTest explicitly asserts that compile dependencies are present and would fail with this change.

  2. Maven 4 transitive resolution broken — Both Maven4ScopeManagerConfiguration files (impl and compat) pass DependencyScope.COMPILE.isTransitive() to createDependencyScope(). After this change, that evaluates to false, causing the resolver to treat compile as 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 (Maven3ScopeManagerConfiguration hardcodes true), but all Maven 4 users would be affected.

Other issues:

  1. Insufficient test coverage — The only test change adds scopes.add(new DepScope("api")) to the mock scope list in DefaultModelValidatorTest.setUp(), but no actual test methods are added. This PR needs: (a) unit test that api scope is rejected under modelVersion 4.0.0, (b) unit test that api scope is accepted under modelVersion 4.1.0, (c) integration test for api/compile transitivity behavior, and (d) update to MavenITmng8750NewScopesTest to cover the api scope.

  2. Incomplete javadoc — The COMPILE javadoc still says "Compile, runtime and test." without mentioning it is no longer transitive, while the new API javadoc says "exported transitively." The distinction between the two scopes is entirely about transitivity, so the COMPILE javadoc should clarify the semantic change.

  3. Enum ordering — The API enum constant is placed before NONE and UNDEFINED, breaking the existing ordering convention. Consider placing it after COMPILE for 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

gnodet added a commit to gnodet/maven that referenced this pull request Aug 17, 2026
@Hiteshsai007

Copy link
Copy Markdown
Contributor

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 gnodet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
/**

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing COMPILE from isTransitive=true to isTransitive=false has cascading effects:

  1. DefaultConsumerPomBuilder.hasDependencyScope() (line ~300) returns scope == null || !scope.isTransitive() — after this change, it returns true for compile scope, causing removeIf (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.

  2. Maven4ScopeManagerConfiguration.buildResolutionScopes() filters !s.isTransitive() to build nonTransitiveDependencyScopes (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.

gnodet added a commit to gnodet/maven that referenced this pull request Aug 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants