gnodet-bot commented on code in PR #13663:
URL: https://github.com/apache/maven/pull/13663#discussion_r4334189889
##########
compat/maven-model-builder/src/test/java/org/apache/maven/model/building/DefaultModelBuilderTest.java:
##########
@@ -291,4 +292,84 @@ public ModelResolver newCopy() {
return this;
}
}
+
+ @Test
+ public void testMng6402ImportedPomRetainedInDependencyManagement() throws
Exception {
+ String bomPom = "<project>\n"
+ + " <modelVersion>4.0.0</modelVersion>\n"
+ + " <groupId>thegroup</groupId>\n"
+ + " <artifactId>bom</artifactId>\n"
+ + " <version>1.0.0</version>\n"
+ + " <packaging>pom</packaging>\n"
+ + " <dependencyManagement>\n"
+ + " <dependencies>\n"
+ + " <dependency>\n"
+ + " <groupId>thegroup</groupId>\n"
+ + " <artifactId>managed-dep</artifactId>\n"
+ + " <version>2.0.0</version>\n"
+ + " </dependency>\n"
+ + " </dependencies>\n"
+ + " </dependencyManagement>\n"
+ + "</project>\n";
+
+ String projectPom = "<project>\n"
+ + " <modelVersion>4.0.0</modelVersion>\n"
+ + " <groupId>thegroup</groupId>\n"
+ + " <artifactId>project</artifactId>\n"
+ + " <version>1.0.0</version>\n"
+ + " <dependencyManagement>\n"
+ + " <dependencies>\n"
+ + " <dependency>\n"
+ + " <groupId>thegroup</groupId>\n"
+ + " <artifactId>bom</artifactId>\n"
+ + " <version>1.0.0</version>\n"
+ + " <type>pom</type>\n"
+ + " <scope>import</scope>\n"
+ + " </dependency>\n"
+ + " </dependencies>\n"
+ + " </dependencyManagement>\n"
+ + "</project>\n";
+
+ ModelBuilder builder = new DefaultModelBuilderFactory().newInstance();
+ DefaultModelBuildingRequest request = new
DefaultModelBuildingRequest();
+ request.setModelSource(new StringModelSource(projectPom));
+ request.setModelResolver(new BaseModelResolver() {
+ @Override
+ public ModelSource resolveModel(String groupId, String artifactId,
String version)
+ throws UnresolvableModelException {
+ if ("thegroup".equals(groupId) && "bom".equals(artifactId) &&
"1.0.0".equals(version)) {
+ return new StringModelSource(bomPom);
+ }
+ return super.resolveModel(groupId, artifactId, version);
+ }
+
+ @Override
+ public ModelSource resolveModel(Dependency dependency) throws
UnresolvableModelException {
+ return resolveModel(dependency.getGroupId(),
dependency.getArtifactId(), dependency.getVersion());
+ }
+ });
+
+ ModelBuildingResult result = builder.build(request);
+ Model effectiveModel = result.getEffectiveModel();
+ assertNotNull(effectiveModel.getDependencyManagement());
+ List<Dependency> dependencies =
effectiveModel.getDependencyManagement().getDependencies();
+
+ // Verify the managed dependency declared inside the BOM is present
+ Dependency managedDep = dependencies.stream()
+ .filter(d -> "managed-dep".equals(d.getArtifactId()))
+ .findFirst()
+ .orElse(null);
+ assertNotNull(managedDep, "Managed dependency from imported BOM must
be present");
+ assertEquals("2.0.0", managedDep.getVersion());
+
+ // Verify the imported BOM dependency itself is retained in
dependencyManagement (MNG-6402)
+ Dependency bomDep = dependencies.stream()
+ .filter(d -> "bom".equals(d.getArtifactId()))
+ .findFirst()
+ .orElse(null);
+ assertNotNull(bomDep, "The imported BOM dependency itself must be
retained in dependencyManagement");
+ assertEquals("1.0.0", bomDep.getVersion());
+ assertEquals("pom", bomDep.getType());
+ assertEquals("import", bomDep.getScope());
+ }
}
Review Comment:
💡 This test verifies the BOM entry is *retained* — which is the new compat
behaviour. If the compat change is reverted (as suggested above), this test
will fail. Please remove it together with the compat revert.
##########
compat/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java:
##########
@@ -1235,15 +1234,11 @@ private void importDependencyManagement(
List<DependencyManagement> importMgmts = null;
- for (Iterator<Dependency> it = depMgmt.getDependencies().iterator();
it.hasNext(); ) {
- Dependency dependency = it.next();
-
+ for (Dependency dependency : depMgmt.getDependencies()) {
if (!"pom".equals(dependency.getType()) ||
!"import".equals(dependency.getScope())) {
continue;
}
Review Comment:
⚠️ **Compat rule violation:** The `compat/` module must mirror Maven 3.9.x
exactly. Maven 3.9.x still calls `it.remove()` here — removing the import-scope
entry from the live list after processing. Dropping `it.remove()` in compat
without a corresponding 3.9.x change introduces a behaviour divergence. Please
revert this hunk; the fix belongs only in `impl/`.
##########
impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelBuilderTest.java:
##########
@@ -1883,6 +1891,40 @@ private static DefaultProfileActivationContext
newProfileActivationContext(
null, null, activeIds, inactiveIds, Map.of(), Map.of(),
Model.newInstance());
}
+ @Test
Review Comment:
🔍 Minor: the test method is named `testMng6402…` but loads fixtures from
`poms/factory/mng-8450/`. Consider adding dedicated `mng-6402` fixtures (or
renaming the reference) so that the test name and resource path align — makes
future refactoring easier.
--
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]
