Hiteshsai007 commented on PR #12417: URL: https://github.com/apache/maven/pull/12417#issuecomment-4932785968
## Re-review — New Commits The double-interpolation issue from the previous review is **correctly fixed** — BOM properties are now imported *before* interpolation in `readEffectiveModel()`, eliminating the second `interpolateModel()` call. The opt-in mechanism via `maven.bom.import.properties` user property with `@Config` annotation is a sound design choice that preserves backward compatibility. The method rename from `doLoadDependencyManagement` → `doLoadBomModel` is a sensible improvement. ### Addressed Findings **🟢 Fixed — BOM filter condition skips `type=bom` dependencies** The operator precedence issue in `DefaultModelBuilder.java` that was skipping `type=bom` dependencies has been resolved. The logic `!(A || B)` has been corrected to properly include both `type=pom` (with `scope=import`) and `type=bom` imports. **🟢 Fixed — Unnecessary blank line removed** The cosmetic blank line accidentally removed in `importDependencyManagement` has been restored to maintain code style compliance. **🟢 Fixed — Unit tests for opt-in behavior and precedence** Robust unit tests (`testBomPropertyImportDisabledByDefault` and `testBomPropertyImportEnabled`) have been added to `DefaultModelBuilderTest.java`. These tests thoroughly verify: - **Opt-in mechanism:** Properties are completely ignored by default unless `-Dmaven.bom.import.properties=true` is set. - **Precedence:** Project properties correctly override BOM properties. - **Conflict resolution:** The first declared BOM wins in the event of a conflict. These tests handle Maven API model resolution properly by programmatic installation of the BOM artifacts into the test session's local repository (`target/.m2/repository`). This avoids test infrastructure issues with the default mock session and ensures reliable execution. ### Conclusion The changes successfully fulfill the feature request for opt-in BOM property inheritance while remaining robust against edge cases. The newly introduced integration and unit tests are comprehensive and pass consistently. Great work! The PR is ready for merging. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
