From 4fd34a4fdf5bacf5dd9ffdbca177a61957a65d3b Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Fri, 21 Mar 2025 14:13:53 +0100 Subject: [PATCH 1/3] Fix UT by making sure repositories are interpolated before being used --- .../maven/impl/model/DefaultModelBuilder.java | 23 ++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java index 298dc260727d..e918edf2a49f 100644 --- a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java +++ b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java @@ -69,7 +69,6 @@ import org.apache.maven.api.model.Model; import org.apache.maven.api.model.Parent; import org.apache.maven.api.model.Profile; -import org.apache.maven.api.model.Repository; import org.apache.maven.api.services.BuilderProblem; import org.apache.maven.api.services.BuilderProblem.Severity; import org.apache.maven.api.services.Interpolator; @@ -498,9 +497,21 @@ public ModelBuilderException newModelBuilderException() { return new ModelBuilderException(result); } - public void mergeRepositories(List toAdd, boolean replace) { - List repos = - toAdd.stream().map(session::createRemoteRepository).toList(); + public void mergeRepositories(Model model, boolean replace) { + if (model.getRepositories().isEmpty()) { + return; + } + // We need to interpolate the repositories before we can use them + Model interpolatedModel = interpolateModel( + Model.newBuilder() + .pomFile(model.getPomFile()) + .repositories(model.getRepositories()) + .build(), + request, + this); + List repos = interpolatedModel.getRepositories().stream() + .map(session::createRemoteRepository) + .toList(); if (replace) { Set ids = repos.stream().map(RemoteRepository::getId).collect(Collectors.toSet()); repositories = repositories.stream() @@ -1016,7 +1027,7 @@ Model resolveAndReadParentExternally(Model childModel, DefaultProfileActivationC // add repositories specified by the current model so that we can resolve the parent if (!childModel.getRepositories().isEmpty()) { var previousRepositories = repositories; - mergeRepositories(childModel.getRepositories(), false); + mergeRepositories(childModel, false); if (!Objects.equals(previousRepositories, repositories)) { if (logger.isDebugEnabled()) { logger.debug("Merging repositories from " + childModel.getId() + "\n" @@ -1196,7 +1207,7 @@ private Model readEffectiveModel() throws ModelBuilderException { if (!resultModel.getRepositories().isEmpty()) { List oldRepos = repositories.stream().map(Object::toString).toList(); - mergeRepositories(resultModel.getRepositories(), true); + mergeRepositories(resultModel, true); List newRepos = repositories.stream().map(Object::toString).toList(); if (!Objects.equals(oldRepos, newRepos)) { From 1b6848e2dfea004bc754fcab057c2508892979c3 Mon Sep 17 00:00:00 2001 From: Tamas Cservenak Date: Thu, 27 Mar 2025 11:45:29 +0100 Subject: [PATCH 2/3] Add UT --- .../impl/model/DefaultModelBuilderTest.java | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelBuilderTest.java b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelBuilderTest.java index c0a04a92dfc3..b3b60be5aef6 100644 --- a/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelBuilderTest.java +++ b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelBuilderTest.java @@ -18,10 +18,17 @@ */ package org.apache.maven.impl.model; +import java.lang.reflect.Field; import java.nio.file.Path; import java.nio.file.Paths; +import java.util.Arrays; +import java.util.List; +import java.util.Map; +import org.apache.maven.api.RemoteRepository; import org.apache.maven.api.Session; +import org.apache.maven.api.model.Model; +import org.apache.maven.api.model.Repository; import org.apache.maven.api.services.ModelBuilder; import org.apache.maven.api.services.ModelBuilderRequest; import org.apache.maven.api.services.ModelBuilderResult; @@ -60,6 +67,53 @@ public void testPropertiesAndProfiles() { assertEquals("21", result.getEffectiveModel().getProperties().get("maven.compiler.release")); } + @Test + public void testMergeRepositories() throws Exception { + // this is here only to trigger mainSession creation; unrelated + ModelBuilderRequest request = ModelBuilderRequest.builder() + .session(session) + .userProperties(Map.of("firstParentRepo", "https://some.repo")) + .requestType(ModelBuilderRequest.RequestType.BUILD_PROJECT) + .source(Sources.buildSource(getPom("props-and-profiles"))) + .build(); + ModelBuilder.ModelBuilderSession session = builder.newSession(); + session.build(request); // ignored result value; just to trigger mainSession creation + + Field mainSessionField = DefaultModelBuilder.ModelBuilderSessionImpl.class.getDeclaredField("mainSession"); + mainSessionField.setAccessible(true); + DefaultModelBuilder.ModelBuilderSessionState state = + (DefaultModelBuilder.ModelBuilderSessionState) mainSessionField.get(session); + Field repositoriesField = DefaultModelBuilder.ModelBuilderSessionState.class.getDeclaredField("repositories"); + repositoriesField.setAccessible(true); + + List repositories; + // before merge + repositories = (List) repositoriesField.get(state); + assertEquals(1, repositories.size()); // central + + Model model = Model.newBuilder() + .repositories(Arrays.asList( + Repository.newBuilder() + .id("first") + .url("${firstParentRepo}") + .build(), + Repository.newBuilder() + .id("second") + .url("${secondParentRepo}") + .build())) + .build(); + state.mergeRepositories(model, false); + + // after merge + repositories = (List) repositoriesField.get(state); + assertEquals(3, repositories.size()); + assertEquals(repositories.get(0).getId(), "first"); + assertEquals(repositories.get(0).getUrl(), "https://some.repo"); // interpolated + assertEquals(repositories.get(1).getId(), "second"); + assertEquals(repositories.get(1).getUrl(), "${secondParentRepo}"); // un-interpolated (no source) + assertEquals(repositories.get(2).getId(), "central"); // default + } + private Path getPom(String name) { return Paths.get("src/test/resources/poms/factory/" + name + ".xml").toAbsolutePath(); } From 65308394f5a719653ef6432e840c4799937053b6 Mon Sep 17 00:00:00 2001 From: Tamas Cservenak Date: Thu, 27 Mar 2025 11:57:54 +0100 Subject: [PATCH 3/3] Flip assert order to right one --- .../maven/impl/model/DefaultModelBuilderTest.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelBuilderTest.java b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelBuilderTest.java index b3b60be5aef6..2b012185c4bf 100644 --- a/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelBuilderTest.java +++ b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelBuilderTest.java @@ -107,11 +107,11 @@ public void testMergeRepositories() throws Exception { // after merge repositories = (List) repositoriesField.get(state); assertEquals(3, repositories.size()); - assertEquals(repositories.get(0).getId(), "first"); - assertEquals(repositories.get(0).getUrl(), "https://some.repo"); // interpolated - assertEquals(repositories.get(1).getId(), "second"); - assertEquals(repositories.get(1).getUrl(), "${secondParentRepo}"); // un-interpolated (no source) - assertEquals(repositories.get(2).getId(), "central"); // default + assertEquals("first", repositories.get(0).getId()); + assertEquals("https://some.repo", repositories.get(0).getUrl()); // interpolated + assertEquals("second", repositories.get(1).getId()); + assertEquals("${secondParentRepo}", repositories.get(1).getUrl()); // un-interpolated (no source) + assertEquals("central", repositories.get(2).getId()); // default } private Path getPom(String name) {