[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.

[gnodet-bot]

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

Verdict: 💬 COMMENT — All prior review feedback (elharo + gnodet) addressed. One gap in the compat builder worth fixing before merge.

Prior findings — all addressed

• ✅ depMgmt → dependencyManagement, importMgmts → importedManagements — renamed.
• ✅ Method broken up — importDependencyManagement now delegates to loadDependencyManagement and resolveImportModel.
• ✅ Variable declarations moved inside try blocks.

Finding: potential NPE in compat resolveImportModel when workspace returns null and modelResolver is absent

In compat/maven-model-builder, the guard at the top of resolveImportModel only catches the case where both resolvers are null:

if (workspaceResolver == null && modelResolver == null) { throw new NullPointerException(...); }

If workspaceResolver is non-null but returns null (model not in workspace — a legitimate outcome), execution falls through to:

importSource = modelResolver.resolveModel(groupId, artifactId, version);

modelResolver could be null here — for example, IDE embedders that supply a WorkspaceModelResolver but no legacy ModelResolver. This produces an NPE with no actionable diagnostic.

Fix: add a null guard before the fallthrough:

if (modelResolver == null) {
    problems.add(new ModelProblemCollectorRequest(Severity.ERROR, Version.BASE)
            .setMessage("Cannot resolve import POM "
                    + ModelProblemUtils.toId(groupId, artifactId, version)
                    + ": not found in workspace and no ModelResolver provided")
            .setLocation(dependency.getLocation("")));
    return null;
}

Impl builder: concurrency analysis

The ImportModelCacheEntry + ReentrantLock + volatile ImportedModel pattern is correct:

• Normal import paths block (via lock.lock()) until the in-flight build completes, then re-read cached.model — avoids duplicate work.
• Relocation paths skip waiting via tryLock to prevent deadlock when a relocation chain leads back to a currently-building import — produces at most duplicate work, not a correctness problem.
• volatile on cached.model guarantees cross-thread visibility without needing a full synchronized block.
• BomRelocationConcurrencyTest covers concurrent cycles, concurrent aliases, and the shared-build path (concurrentOrdinaryImportsBuildSharedBomOnce).

No concurrency bugs in the impl builder.

Test coverage

BomRelocationTest (compat) covers simple relocation, partial chains, cycles (relocation→import, import→relocation, import→import), workspace path, invalid coordinates, and deduplication (resolutions.get("test:new:1") == 1).

BomRelocationTest + BomRelocationConcurrencyTest (impl) mirror the compat coverage and add multi-thread correctness.

---

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

[goutamadwant]

@gnodet-bot @gnodet Fixed. A workspace miss without a fallback ModelResolver now reports a normal model-building error with the missing BOM coordinates and import location. thanks!

[gnodet-bot]

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

Verdict: ✅ APPROVE

Prior finding — addressed

✅ The modelResolver == null guard in compat resolveImportModel is now in place. When a workspace resolver is configured but returns null (model absent from workspace) and no fallback ModelResolver is provided, an ERROR-severity problem with coordinates and source location is reported instead of throwing an NPE.

The severity choice is correct: FATAL is reserved for workspace resolver exceptions (the catch (UnresolvableModelException e) path above). A misconfigured embedder that provides only a workspace resolver without a fallback is a build-configuration error, not a fatal runtime failure — ERROR is appropriate and consistent with the UnresolvableModelException path.

New tests

Five new test methods in BomRelocationTest (compat) cover the full matrix:

• reportsWorkspaceMissWithoutModelResolver — plain miss, empty workspace, no fallback
• reportsWorkspaceRelocationTargetMissWithoutModelResolver — relocation chain where the target is missing
• fallsBackToModelResolverAfterWorkspaceMiss — workspace returns null, fallback resolver succeeds
• fallsBackToModelResolverForWorkspaceRelocationTarget — relocation chain where only the target needs fallback
• preservesWorkspaceResolutionFailure — workspace exception (FATAL) takes precedence over fallback

assertMissingWorkspaceModel asserts ERROR severity, message contains the missing coordinates, modelId == "test:consumer:1", and both lineNumber > 0 / columnNumber > 0 (location tracking enabled). Thorough.

No new issues.

---

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