[MNG-5527] Honor BOM relocation during dependency management import

[goutamadwant]

Fixes #6804.

Follow BOM relocation chains in both model builders, including partial relocations. Preserve dependency-management precedence, import exclusions, and the modern resolver's selected version when resolving a range.

Report invalid relocation coordinates, missing relocation targets, and relocation/import cycles without changing ordinary imported-model validation behavior. Preserve sharing of ordinary in-flight imports while avoiding cyclic waits introduced by relocation traversal.

Validation:

• The reported failure reproduces on untouched master and passes with the fix through both the CLI and the new Core IT.

• All 39 focused relocation, compatibility, and concurrency tests pass.

• mvn verify passes: 3,152 unit tests, zero failures/errors, 16 skips, plus one reactor integration test.

• Full forked Core IT: 1,074 tests, three failing tests, 43 skips. MNG3955 and MNG4411 fail identically on untouched master. MavenITMvnupToolchainPluginStrategyTest passes after refreshing its previously modified generated fixture. MNG4590 and the new MNG5527 test pass in the full run.

• This pull request addresses one issue without unrelated changes.

• The description explains what changes, how, and why.

• Commits have meaningful subjects and bodies.

• Regression tests cover the behavioral changes; the reported case fails without the fix.

• mvn verify passes.

• The Core IT suite passes.

• 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]

no abbrevations please

[goutamadwant]

renamed the variables to use full names.

[elharo]

maybe you should break this method up

[goutamadwant]

Done.

[elharo]

move into the try block, don't split declaration and initialization

[goutamadwant]

Sure. Moved the declaration and initialization of importResult inside the try block, together with the result handling. Let me know if this is good now

[gnodet]

I reviewed the three comments from @elharo. Here's my assessment:

---

Comment 1 — "no abbreviations please" (line 1185, importMgmt)

importMgmt is not new to this PR — it already exists on master (along with depMgmt) in the same method. The PR merely moved this code into an extracted loadDependencyManagement method. While renaming abbreviations is a fine cleanup, it shouldn't be a blocker for this PR since it's pre-existing code. If there's a desire to rename these, it could be done as a separate cleanup across the file.

Comment 2 — "maybe you should break this method up" (line 1199, @SuppressWarnings("checkstyle:methodlength"))

The @SuppressWarnings("checkstyle:methodlength") was on master's importDependencyManagement before this PR. This PR already did break the method up: it extracted loadDependencyManagement out of importDependencyManagement, which went from ~160 lines down to ~15. The new method inherited the suppress annotation because the relocation-chain logic added more code. Further decomposition is possible (e.g., extracting the relocation handling) but this is already a significant improvement over master.

Comment 3 — "move into the try block, don't split declaration and initialization" (line 1303)

The final ModelBuildingResult importResult; pattern is carried verbatim from master (previously at the same indentation inside the loop). It's a pre-existing pattern, not introduced by this PR. That said, it's a trivial fix — the variable can be moved inside the try block:

try {
    final ModelBuildingResult importResult = build(importRequest, importIds);
    problems.addAll(importResult.getProblems());
    importModel = importResult.getEffectiveModel();
} catch (ModelBuildingException e) {
    problems.addAll(e.getProblems());
    return null;
}

This is a reasonable cleanup the contributor could apply.

---

Summary: All three comments point at pre-existing patterns that this PR carried over unchanged. Comment 3 is the only one with a concrete, actionable fix that's trivial to apply. The other two are either already addressed by the PR (method extraction) or out of scope (renaming existing abbreviations).

[renechoi]

Maven 4 already has a relocation SPI: MavenArtifactRelocationSource (distributionManagement, userProperties), walked with cycle detection in DefaultArtifactDescriptorReader.loadPom. This adds a second walker for distributionManagement only, and imports resolve via DefaultModelResolver, which never consults those sources, so maven.relocations.entries cannot reach an imported BOM. Intended?

Also a relocation-only loop reports "dependencies of type=pom and with scope=import form a cycle" though one import is declared; loadPom words it "Artifact relocations form a cycle".

[gnodet-bot]

Good observations @renechoi. Two distinct points here:

1. Relationship with MavenArtifactRelocationSource / DefaultArtifactDescriptorReader.loadPom

These operate at different layers:

• DefaultArtifactDescriptorReader.loadPom (resolver layer) — handles relocations during dependency resolution, when the resolver reads an artifact descriptor. It walks relocations via MavenArtifactRelocationSource (which includes DistributionManagementArtifactRelocationSource and UserPropertiesArtifactRelocationSource). This path is used when resolving regular dependencies.

• DefaultModelBuilder.importDependencyManagement (model builder layer) — handles BOM imports (<scope>import</scope>) during model building. This path resolves the imported POM directly via ModelResolver, builds its effective model, and extracts <dependencyManagement>. It never goes through DefaultArtifactDescriptorReader, so the relocation SPI is never consulted.

Before this PR, if a BOM declared <distributionManagement><relocation>, the model builder would silently ignore it and import nothing (or the wrong thing). This PR adds relocation-following specifically to the model builder's import path.

Regarding maven.relocations.entries (user-properties-based relocations): correct, those cannot currently reach an imported BOM through this path. That's by design for this fix — the scope is limited to distributionManagement relocations, which is the mechanism actually used in published POMs. Extending user-properties relocations to BOM imports would be a separate enhancement.

2. Error message wording for relocation-only cycles

Valid point. When a cycle involves only relocation hops (no actual scope=import dependencies form the cycle), the message "The dependencies of type=pom and with scope=import form a cycle" is misleading. The PR does distinguish between relocation cycles and import cycles internally (see cycleIncludesRelocation in the Maven 4 model builder), but the error message template is reused. This could be improved with a more specific message like "The import POM relocation chain forms a cycle" when the cycle includes a relocation hop.

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

[goutamadwant]

Maven 4 already has a relocation SPI: MavenArtifactRelocationSource (distributionManagement, userProperties), walked with cycle detection in DefaultArtifactDescriptorReader.loadPom. This adds a second walker for distributionManagement only, and imports resolve via DefaultModelResolver, which never consults those sources, so maven.relocations.entries cannot reach an imported BOM. Intended?

Also a relocation-only loop reports "dependencies of type=pom and with scope=import form a cycle" though one import is declared; loadPom words it "Artifact relocations form a cycle".

Yes, this fix is limited to distributionManagement relocations in imported BOMs. Imports use ModelResolver rather than the artifact-descriptor relocation SPI; extending maven.relocations.entries to BOM imports would be a separate change.

Changed the diagnostic to “The import POMs form a cycle”, covering ordinary imports, relocations, and mixed cycles. Both model builders now assert the message and cycle path.

@gnodet, could you approve the new CI run?

[gnodet]

Second commit (a7b8f49) addresses all outstanding review feedback:

1. Variable renames — depMgmt → dependencyManagement, importMgmt → importedManagement, importMgmts → importedManagements. Pre-existing abbreviations cleaned up per @elharo's request.

2. Method extraction — resolveImportModel() extracted from loadDependencyManagement(), removing the @SuppressWarnings("checkstyle:methodlength"). The extraction is a pure refactoring: workspace-first resolution with early return, then fallback to ModelResolver, same semantics as before with cleaner flow.

3. Declaration inside try — importResult is now declared and initialized inside the try block as requested.

4. Cycle diagnostic — Message changed to "The import POMs form a cycle: ..." in both model builders (compat and impl), covering relocations, imports, and mixed cycles. Concise and accurate regardless of whether the cycle involves relocation hops.

5. New tests — detectsImportCycle() in the compat builder test and detectsDirectImportCycle() in the impl builder test, both asserting the updated cycle message.

CI status: all completed checks are green. The one failure (integration-tests (macos-latest, 17)) is an infrastructure issue — the "Set up JDK 17" step failed, not related to this PR. Remaining checks still in progress.

Changes look solid. @gnodet — this is ready for your review when CI completes.

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

[gnodet]

CI has been approved and is running. Status so far:

• All 9 full-build jobs: ✅ passed (all platforms, JDK 17/21/25)
• integration-tests (ubuntu 21, 25): ✅ passed
• integration-tests (macos 17): ❌ infra failure ("Set up JDK 17" step failed — not related to this PR)
• Remaining integration-tests: still in progress

The second commit addresses all review feedback cleanly:

1. Variable renames (depMgmt → dependencyManagement, etc.)
2. resolveImportModel() extracted, removing the @SuppressWarnings
3. importResult declaration moved inside the try block
4. Cycle diagnostic updated to "The import POMs form a cycle"
5. New cycle-detection tests in both builders

Looks good. I'll re-check once the remaining integration tests complete.

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