Fix BOM version resolution for sibling modules in dependencyManagement

[Hiteshsai007]

Description

This PR addresses, fixing a regression in Maven 4 where version and groupId inference were skipped for dependencies declared within the <dependencyManagement> section of a BOM-packaged project.

Issue: #11147

How it works:
I updated the transformFileToRaw method within DefaultModelBuilder to ensure that model.getDependencyManagement().getDependencies() is processed with the same inferDependencyVersion and inferDependencyGroupId logic that is already applied to direct dependencies (model.getDependencies()).

Why this is necessary:
Previously, if a BOM subproject declared sibling reactor modules within its <dependencyManagement> block but omitted the <version> tags (expecting them to be resolved from the reactor), the transformation to the raw model would skip them entirely. This resulted in the installed consumer POM missing the required version tags for those managed dependencies. This change ensures that the reactor versions are properly inherited and written to the consumer POM.

Testing:

• Successfully verified the fix locally using the bom-example reproducer provided in the issue, confirming that the resulting consumer POM now contains the correctly inferred versions.
• Formatted with spotless:apply and successfully passed all maven-impl unit tests. Note: Due to the complexity of simulating a full multi-module reactor state directly within DefaultModelBuilderTest, a standalone unit test for this specific code path was omitted in favor of the reproducer validation.

---

Following this checklist to help us incorporate your
contribution quickly and easily:

• Your pull request should address just one issue, without pulling in other changes.
• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• Each commit in the pull request should have a meaningful subject line and body.
  Note that commits might be squashed by a maintainer on merge.
• Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied.
  This may not always be possible but is a best-practice.
• Run mvn verify to make sure basic checks pass.
  A more thorough check will be performed on your pull request automatically.
• You have run the Core IT successfully.

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

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 check-box.

• I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
• In any other case, please file an Apache Individual Contributor License Agreement.

[elharo]

1. Needs tests
2. Needs a link to github issue

[Hiteshsai007]

@elharo Thanks for the review! I've addressed both items:

1. Tests: Added testBomDependencyManagementVersionInference() in DefaultModelBuilderTest that verifies version inference works for dependencies in <dependencyManagement>, not just <dependencies>. The test registers a sibling module in the reactor, then calls transformFileToRaw on a BOM model with a managed dependency missing its version, and asserts the version is correctly inferred.

2. Issue link: Updated the PR description to include Fixes #11147.

All 13 tests in DefaultModelBuilderTest pass with zero failures.

[gnodet]

Review: Fix BOM version resolution for sibling modules in dependencyManagement

The core fix is correct and well-targeted — transformFileToRaw was only processing model.getDependencies() and ignoring model.getDependencyManagement().getDependencies(), which is the root cause of issue #11147. The fix correctly extends the same inferDependencyVersion and inferDependencyGroupId logic to managed dependencies. The test demonstrates the fix works, and the test POM files are well-structured.

Minor observations

1. Unnecessary list copy when only dependencyManagement changes (low)
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java

When model.getDependencies() is non-empty (so newDeps is allocated) but only mgmtChanged flips changed to true, the code enters if (changed) and then if (newDeps != null) passes, calling builder.dependencies(newDeps) with an identical list. This is harmless but creates an unnecessary copy. Consider tracking a separate boolean depsChanged flag.

2. Duplicated loop logic (low)

The for-loop bodies for regular dependencies and dependencyManagement dependencies are near-identical, differing only in the source list, target list, and changed-flag variable. Consider extracting a helper method to reduce duplication — though this is minor with only two occurrences.

Overall this is a clean, well-scoped fix. CI should be triggered (rebase to master) to validate the full test suite.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of Guillaume Nodet

[Hiteshsai007]

Thanks for the review @gnodet! Both suggestions have been addressed in the latest commit:

1. Unnecessary list copy fix:
Replaced the single boolean changed flag with separate boolean depsChanged and boolean mgmtChanged flags. Now builder.dependencies(newDeps) is only called when regular dependencies actually changed, avoiding the unnecessary copy when only dependencyManagement was modified.

2. Duplicated loop logic:
Extracted a new inferDependencies(Model, List<Dependency>, List<Dependency>) helper method that encapsulates the shared loop logic for inferring missing version/groupId. Both the regular dependencies and dependencyManagement dependencies processing now delegate to this single method, eliminating the duplication.

The branch has also been rebased onto the latest master to trigger CI.

[gnodet]

Re-review — New Commit

The latest commit (acbea96) cleanly addresses both concerns from the previous review:

1. Unnecessary list copy — Fixed by tracking separate depsChanged and mgmtChanged flags, so builder.dependencies(newDeps) is only called when regular dependencies actually changed.
2. Duplicated loop logic — Extracted into a private inferDependencies helper method with proper Javadoc. Clean and well-structured.

The core fix remains correct and well-scoped. The test follows existing conventions in DefaultModelBuilderTest and exercises the specific scenario from issue #11147.

Minor Observations

• The test asserts assertNotNull(managedDep.getVersion()) but does not check the specific expected version value — asserting "1.0-SNAPSHOT" would make failures more diagnosable.
• elharo's earlier CHANGES_REQUESTED reviews (needs tests, needs issue URL) have both been addressed.

No CI checks are reported for this branch — a rebase may be needed to trigger CI. ✅

---

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of Guillaume Nodet

[Hiteshsai007]

Thanks for the review and the approval, @gnodet!

I've just pushed a small follow-up commit to address your minor observations:

• Updated the test assertion from assertNotNull(managedDep.getVersion()) to assertEquals("1.0-SNAPSHOT", managedDep.getVersion()) to make any potential failures more diagnosable.
• Pushed the latest changes to the branch, which should re-trigger the CI checks.

Waiting on the CI results now!

[gnodet]

Re-review — Feedback Addressed

The new commit (d932300) addresses the observation from the previous review: the test now uses assertEquals("1.0-SNAPSHOT", managedDep.getVersion(), "Version should be inferred from the reactor sibling module") instead of assertNotNull(managedDep.getVersion()), making test failures more diagnosable. ✅

No other changes — PR remains clean.

---

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of Guillaume Nodet

[gnodet]

Code Review

The core fix is correct — extending transformFileToRaw to process dependencyManagement dependencies is the right approach for resolving sibling module BOM version inference, and the refactoring into a shared inferDependencies helper is clean.

However, CI is failing due to Spotless formatting violations in multiple locations:

1. DefaultModelBuilderTest.java:491: The assertEquals call exceeds the line length limit and must be wrapped.
2. DefaultModelBuilder.java:625: The mgmtChanged = inferDependencies(...) call needs different line wrapping.
3. DefaultModelBuilder.java:654: The inferDependencies method signature needs reformatting.

Running mvn spotless:apply in the impl/maven-impl module should fix all three violations.

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]

Code Review (Re-review)

The core fix — extending transformFileToRaw to process dependencyManagement dependencies and the new inferDependencies helper — is correct and well-scoped. However, the latest commit introduced several issues that need to be addressed before merge:

1. Incorrect indentation in test file (high)

File: impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelBuilderTest.java

The entire test file was reformatted from 4-space indentation (project standard, enforced by Palantir Java Format via Spotless) to 8-space indentation. Other test files in the same module (e.g., DefaultModelXmlFactoryTest.java) use 4-space indentation. This will fail Spotless checks. The indentation needs to be reverted to 4-space.

2. Out-of-scope formatting changes (high)

File: impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java

Approximately 47 formatting-only changes outside the functional fix scope inflate the diff from ~85 functional lines to ~327 total changed lines. These include Javadoc/comment line rewrapping, variable declaration reformatting, record body splitting, and @param tag alignment. While each might match a formatter's output, the master code already passes Spotless, making these changes unnecessary noise.

3. Missing test method — rebase needed (high)

File: impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelBuilderTest.java

The testDuplicateProfileIdsRetainActivations test method (merged to master in commit 67a94a1 via PR #12419) is completely missing from the PR branch. The PR's diff rewrites the entire class body, and this test was dropped in the process. A rebase onto current master is needed to restore it.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

[Hiteshsai007]

Thanks for the careful re-review @gnodet — all three points are addressed. Rather than patching on top, I rebuilt the branch from a clean upstream base and re-applied only the functional change, since two of the three findings were artifacts of my editor reformatting files it touched.

1. Test indentation reverted to 4 spaces

You were right that the whole class had been re-indented to 8 spaces. DefaultModelBuilderTest.java is now byte-identical to upstream except for the added test method and one new import (DependencyManagement).

2. Out-of-scope formatting changes removed

The ~47 unrelated hunks in DefaultModelBuilder.java (javadoc/comment rewrapping, record body splitting, @param alignment) are gone. That file is now +42/-11, all of it inside transformFileToRaw and the new helper. Total diff is 174 insertions across 5 files, of which 84 are the three new test POMs.

3. testDuplicateProfileIdsRetainActivations restored

Dropped because my diff rewrote the entire class body. The rebase brings it back — it now sits alongside the new test rather than being replaced by it.

The fix itself (unchanged from the version you approved in acbea96)

transformFileToRaw only iterated model.getDependencies(), so a BOM subproject listing sibling reactor modules under <dependencyManagement> without a <version> never had those versions inferred, and they were missing from the raw and consumer POM. The same inferDependencyVersion / inferDependencyGroupId logic now runs over model.getDependencyManagement().getDependencies():

• The shared loop is extracted into a private inferDependencies(Model, List<Dependency>, List<Dependency>) helper returning whether anything changed.
• depsChanged and managedDepsChanged are tracked separately, so builder.dependencies(newDeps) is only called when the regular dependency list actually changed — no identical-list copy when only dependencyManagement was modified.
• When nothing is inferred, the original Model instance is returned unchanged, as before.
  testBomDependencyManagementVersionInference registers a sibling module in the reactor, runs transformFileToRaw on a BOM model whose managed dependency omits its version, and asserts assertEquals("1.0-SNAPSHOT", managedDep.getVersion(), ...).

Note on the rebase base

The branch is rebased onto af74c4e5 rather than current master. It merges into today's master with zero conflicts and I confirmed both test methods are present in the merge result. Happy to rebase onto the exact tip if CI needs it.

[gnodet]

Re-review after addressing feedback ✅

All three prior findings are resolved:

1. 8-space indentation → Fixed — all added lines use correct 4-space indentation.
2. Out-of-scope formatting → Fixed — diff is now purely functional, no whitespace-only changes.
3. Missing test from rebase → Resolved — new commit only adds the test method and import, so no overlap with master-side additions. Simulated merge confirms both test methods survive cleanly.

The extracted inferDependencies helper is a clean refactoring, and the new testBomDependencyManagementVersionInference test properly exercises the BOM use case.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

[github-actions]

@gnodet Please assign appropriate label to PR according to the type of change.