Fix operator precedence skipping type=bom dependency imports

[kalayciburak]

Summary

importDependencyManagement skipped every dependency with type=bom because of operator precedence:

if (!(pom && import) || bom) {
    continue;
}

For type=bom the || bom term is always true, so BOM-typed import-scoped entries never load managed dependencies. Maven 4 defines Type.BOM (mapped to the pom extension), so these imports should be processed the same way as type=pom + scope=import.

This change:

• treats (pom || bom) && import as an import BOM/POM entry
• allows type=bom in the import-scope validator message (warning only for other types)
• adds a regression test that imports the existing test remote-repo BOM with type=bom

Fixes #12589

Checklist

• 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.
• Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied.
• Run mvn verify to make sure basic checks pass.
• You have run the Core IT successfully.
• I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004

Test plan

Executed:

mvn -pl impl/maven-impl test -Dtest=DefaultModelBuilderTest,DefaultModelValidatorTest

Result: 98 tests, 0 failures (includes new testImportScopeBomTypeIsProcessed).

[gnodet]

Clean, well-scoped bug fix for operator precedence in importDependencyManagement that silently skipped type=bom dependencies. The fix is logically correct, the regression test adequately covers the scenario, and the validator update is consistent.

Non-blocking observations:

1. Validator assertion drift (low): The existing testBadImportScopeType assertion checks for substring "must be 'pom'" which still passes against the new message "must be 'pom' or 'bom'..." but doesn't verify the updated text. Consider updating the assertion to match the new full message.

2. Pre-existing cycle message (low): The cycle detection message says "The dependencies of type=pom and with scope=import form a cycle" — now that type=bom is also processed, this could be misleading for bom-typed cycles. Not introduced by this PR.

3. Nice design choice: The PR's expression !((pom || bom) && import) is semantically cleaner than the alternative !(pom && import) && !bom suggested in the issue — the latter would have incorrectly allowed type=bom entries without scope=import to pass through the filter.

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]

Clean, well-scoped bug fix for operator precedence in importDependencyManagement that silently skipped type=bom import-scoped dependencies. The logic fix is verified correct, the regression test adequately covers the scenario, and the validator update is consistent.

Minor observations (non-blocking):

• The existing testBadImportScopeType assertion checks for substring "must be 'pom'" which still passes against the new message via assertContains (substring match), so it does not actually verify the updated validator text. Consider updating the assertion to "must be 'pom' or 'bom'" to confirm the new wording. Not a blocker since no code in this PR touches that test.
• The cycle detection message says "The dependencies of type=pom and with scope=import form a cycle" — now that type=bom is also processed, this message could be misleading for bom-typed cycles. Pre-existing issue, not introduced by this PR.
• The fix's expression !("pom" || "bom") && "import") is semantically cleaner and more correct than the alternative suggested in the issue.
• The compat layer (compat/maven-model-builder) has a similar import check that only handles type=pom, which is expected since type=bom is a Maven 4 concept.

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]

Correct operator-precedence fix for type=bom dependency imports

The original bug (introduced in commit d075fe7, [MNG-8084]) had !("pom" && "import") || "bom" which always evaluated true for type=bom entries, causing the continue to fire unconditionally and silently skip all BOM imports. The fix correctly rewrites this as !("pom" || "bom") && "import"), properly processing both pom and bom typed imports.

Key observations:

• The DefaultModelValidator is updated consistently to accept both pom and bom types
• The compat layer (maven-model-builder) correctly only tests for pom since type=bom is a Maven 4 concept
• The regression test with a dedicated POM fixture adequately covers the scenario
• Single squashed commit with clean history

No issues found.

---

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]

Pushed a fixup commit: type=bom should NOT require scope=import for BOM import processing.

The bom type inherently implies import semantics — unlike type=pom which needs scope=import to be treated as a BOM import. The filter is now (pom && import) || bom instead of (pom || bom) && import.

Changes:

• DefaultModelBuilder: !(("pom" && "import") || "bom") — processes type=bom regardless of scope
• Test: renamed to testBomTypeImpliesImportWithoutScope, test POM no longer has <scope>import</scope>
• Test POM comment: clarifies the intent

All 579 tests in maven-impl pass.

[gnodet]

Thanks for fixing the operator-precedence bug in importDependencyManagement — that fix is correct and the test is clear 👍

However, the validator change in DefaultModelValidator looks wrong to me.

The validator check at line 1195-1196 is guarded by if ("import".equals(dependency.getScope())), so it only fires when scope=import is explicitly set. Adding "bom" there:

if (!"pom".equals(dependency.getType()) && !"bom".equals(dependency.getType())) {
    // "must be 'pom' or 'bom' to import the managed dependencies."

implies that type=bom requires scope=import — but your own builder fix and test demonstrate the opposite: type=bom inherently implies import semantics and does not need scope=import (the test POM import-bom-type.xml has <type>bom</type> without any <scope>import</scope>).

Concretely:

Scenario	Builder processes it?	Validator behavior (with this PR)
type=bom (no scope)	✅ yes — correct	No warning (good — this block isn't entered)
type=bom + scope=import	✅ yes	Silently accepted — message says bom is a valid import type
type=pom + scope=import	✅ yes	Silently accepted

The second row is the problem: scope=import is redundant with type=bom, but the validator now frames bom as just another valid type that needs scope=import to work. The warning message "must be 'pom' or 'bom' to import" reinforces this misunderstanding.

Since type=bom doesn't require scope=import, I'd suggest dropping the validator change entirely. The original check (type must be 'pom' when scope=import) is still correct — scope=import is specific to type=pom. If someone writes the redundant type=bom, scope=import, the existing warning is actually helpful because it nudges them toward the correct pattern (just type=bom, no scope needed).