From 8fd61ad0f1ca5da171018fdc5aa05fdc4c450cb5 Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Thu, 4 Jun 2026 10:22:05 +0200 Subject: [PATCH] Filter project repos with uninterpolated property expressions (#12204) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * mvnup: comment out repositories with undefined property expressions Projects like uima-uimaj, opennlp-sandbox, and seatunnel-shade define repositories with ${eclipseP2RepoId} in the repo ID. This property is never defined — it was used as a literal string in Maven 3. Maven 4 rejects it with IllegalArgumentException at runtime. Extend the mvnup compatibility fix strategy to detect and comment out repositories and plugin repositories whose id or url contain undefined property expressions, following the same pattern used for dependencies (#12080). Handles both root-level and profile-level repositories. Co-Authored-By: Claude Opus 4.6 * Filter project repositories with uninterpolated property expressions When a project POM defines repositories with ${...} expressions that cannot be resolved (e.g. ${eclipseP2RepoId}), Maven 4 previously failed with InternalErrorException. This change downgrades the model validation from ERROR to WARNING and filters such repositories before they reach the resolver, logging a warning instead of failing the build. Co-Authored-By: Claude Opus 4.6 --------- Co-authored-by: Claude Opus 4.6 --- .../mvnup/goals/CompatibilityFixStrategy.java | 84 +++++++++ .../goals/CompatibilityFixStrategyTest.java | 175 ++++++++++++++++++ .../project/DefaultProjectBuildingHelper.java | 11 ++ .../impl/model/DefaultModelValidator.java | 8 +- .../impl/model/DefaultModelValidatorTest.java | 26 +-- .../MavenITgh11140RepoDmUnresolvedTest.java | 13 +- .../MavenITgh11140RepoInterpolationTest.java | 11 +- 7 files changed, 297 insertions(+), 31 deletions(-) diff --git a/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/CompatibilityFixStrategy.java b/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/CompatibilityFixStrategy.java index f18b5161d70e..2d3d4e42e046 100644 --- a/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/CompatibilityFixStrategy.java +++ b/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/CompatibilityFixStrategy.java @@ -144,6 +144,7 @@ public UpgradeResult doApply(UpgradeContext context, Map pomMap) hasIssues |= fixUnsupportedRepositoryExpressions(pomDocument, context); hasIssues |= fixIncorrectParentRelativePaths(pomDocument, pomPath, pomMap, context); hasIssues |= fixUndefinedPropertyExpressions(pomDocument, allDefinedProperties, context); + hasIssues |= fixUndefinedPropertyExpressionsInRepositories(pomDocument, allDefinedProperties, context); if (hasIssues) { context.success("Maven 4 compatibility issues fixed"); @@ -411,6 +412,89 @@ private boolean fixUndefinedPropertyExpressions( .reduce(false, Boolean::logicalOr); } + /** + * Fixes repositories with undefined property expressions by commenting them out. + */ + private boolean fixUndefinedPropertyExpressionsInRepositories( + Document pomDocument, Set allDefinedProperties, UpgradeContext context) { + Element root = pomDocument.root(); + + Stream repositoryContainers = Stream.concat( + Stream.of( + new RepositoryContainer( + root.childElement(REPOSITORIES).orElse(null), REPOSITORY, REPOSITORIES), + new RepositoryContainer( + root.childElement(PLUGIN_REPOSITORIES).orElse(null), + PLUGIN_REPOSITORY, + PLUGIN_REPOSITORIES)) + .filter(c -> c.element != null), + root.childElement(PROFILES).stream() + .flatMap(profiles -> profiles.childElements(PROFILE)) + .flatMap(profile -> Stream.of( + new RepositoryContainer( + profile.childElement(REPOSITORIES) + .orElse(null), + REPOSITORY, + "profile repositories"), + new RepositoryContainer( + profile.childElement(PLUGIN_REPOSITORIES) + .orElse(null), + PLUGIN_REPOSITORY, + "profile pluginRepositories")) + .filter(c -> c.element != null))); + + return repositoryContainers + .map(c -> fixUndefinedPropertyExpressionsInRepositorySection( + c.element, c.elementType, allDefinedProperties, pomDocument, context, c.sectionName)) + .reduce(false, Boolean::logicalOr); + } + + private record RepositoryContainer(Element element, String elementType, String sectionName) {} + + private boolean fixUndefinedPropertyExpressionsInRepositorySection( + Element repositoriesElement, + String elementType, + Set allDefinedProperties, + Document pomDocument, + UpgradeContext context, + String sectionName) { + boolean fixed = false; + List repositories = + repositoriesElement.childElements(elementType).toList(); + Editor editor = new Editor(pomDocument); + + for (Element repository : repositories) { + Set undefinedProps = findUndefinedPropertiesInRepository(repository, allDefinedProperties); + if (!undefinedProps.isEmpty()) { + String propLabel = undefinedProps.size() > 1 ? "properties" : "property"; + String propsStr = "'" + String.join("', '", undefinedProps) + "'"; + + Comment comment = editor.commentOutElement(repository); + String elementXml = comment.content().trim(); + comment.content( + " mvnup: commented out - undefined " + propLabel + " " + propsStr + "\n" + elementXml + " "); + + context.detail("Fixed: Commented out " + elementType + " with undefined " + propLabel + " " + propsStr + + " in " + sectionName); + fixed = true; + } + } + + return fixed; + } + + private Set findUndefinedPropertiesInRepository(Element repository, Set allDefinedProperties) { + Set undefinedProperties = new HashSet<>(); + + String id = repository.childText("id"); + String url = repository.childText("url"); + + collectUndefinedExpressions(id, allDefinedProperties, undefinedProperties); + collectUndefinedExpressions(url, allDefinedProperties, undefinedProperties); + + return undefinedProperties; + } + /** * Fixes undefined property expressions in a specific dependencies section. */ diff --git a/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/CompatibilityFixStrategyTest.java b/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/CompatibilityFixStrategyTest.java index 00dacbb1c11b..b00f62ad6478 100644 --- a/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/CompatibilityFixStrategyTest.java +++ b/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/CompatibilityFixStrategyTest.java @@ -921,6 +921,181 @@ void shouldRecognizePropertyFromGrandparent() throws Exception { } } + @Nested + @DisplayName("Undefined Property Expression in Repositories Fixes") + class UndefinedPropertyExpressionInRepositoriesTests { + + @Test + @DisplayName("should comment out repository with undefined property in id") + void shouldCommentOutRepositoryWithUndefinedPropertyInId() throws Exception { + String pomXml = """ + + + 4.0.0 + test + test + 1.0.0 + + + ${eclipseP2RepoId} + https://repo.example.com + + + + """; + + Document document = Document.of(pomXml); + Map pomMap = Map.of(Paths.get("pom.xml"), document); + + UpgradeContext context = createMockContext(); + UpgradeResult result = strategy.doApply(context, pomMap); + + assertTrue(result.success(), "Compatibility fix should succeed"); + assertTrue(result.modifiedCount() > 0, "Should have commented out repository"); + + String xml = DomUtils.toXml(document); + assertTrue(xml.contains("mvnup: commented out"), "Should contain comment-out marker"); + assertTrue(xml.contains("eclipseP2RepoId"), "Should mention the undefined property"); + + Element root = document.root(); + Element repos = DomUtils.findChildElement(root, "repositories"); + assertEquals(0, repos.childElements("repository").count(), "Should have no repository elements"); + } + + @Test + @DisplayName("should comment out repository with undefined property in url") + void shouldCommentOutRepositoryWithUndefinedPropertyInUrl() throws Exception { + String pomXml = """ + + + 4.0.0 + test + test + 1.0.0 + + + my-repo + ${undefinedBaseUrl}/releases + + + + """; + + Document document = Document.of(pomXml); + Map pomMap = Map.of(Paths.get("pom.xml"), document); + + UpgradeContext context = createMockContext(); + UpgradeResult result = strategy.doApply(context, pomMap); + + assertTrue(result.success(), "Compatibility fix should succeed"); + assertTrue(result.modifiedCount() > 0, "Should have commented out repository"); + + String xml = DomUtils.toXml(document); + assertTrue(xml.contains("mvnup: commented out"), "Should contain comment-out marker"); + assertTrue(xml.contains("undefinedBaseUrl"), "Should mention the undefined property"); + } + + @Test + @DisplayName("should not comment out repository with defined property") + void shouldNotCommentOutRepositoryWithDefinedProperty() throws Exception { + String pomXml = """ + + + 4.0.0 + test + test + 1.0.0 + + my-custom-repo + + + + ${repoId} + https://repo.example.com + + + + """; + + Document document = Document.of(pomXml); + Map pomMap = Map.of(Paths.get("pom.xml"), document); + + UpgradeContext context = createMockContext(); + strategy.doApply(context, pomMap); + + Element root = document.root(); + Element repos = DomUtils.findChildElement(root, "repositories"); + assertEquals(1, repos.childElements("repository").count(), "Repository should still be present"); + } + + @Test + @DisplayName("should comment out plugin repository with undefined property") + void shouldCommentOutPluginRepositoryWithUndefinedProperty() throws Exception { + String pomXml = """ + + + 4.0.0 + test + test + 1.0.0 + + + ${eclipseP2RepoId} + https://plugins.example.com + + + + """; + + Document document = Document.of(pomXml); + Map pomMap = Map.of(Paths.get("pom.xml"), document); + + UpgradeContext context = createMockContext(); + UpgradeResult result = strategy.doApply(context, pomMap); + + assertTrue(result.success(), "Compatibility fix should succeed"); + assertTrue(result.modifiedCount() > 0, "Should have commented out plugin repository"); + + String xml = DomUtils.toXml(document); + assertTrue(xml.contains("mvnup: commented out"), "Should contain comment-out marker"); + + Element root = document.root(); + Element repos = DomUtils.findChildElement(root, "pluginRepositories"); + assertEquals( + 0, repos.childElements("pluginRepository").count(), "Should have no pluginRepository elements"); + } + + @Test + @DisplayName("should not comment out repository with well-known property") + void shouldNotCommentOutRepositoryWithWellKnownProperty() throws Exception { + String pomXml = """ + + + 4.0.0 + test + test + 1.0.0 + + + local-repo + file://${project.basedir}/repo + + + + """; + + Document document = Document.of(pomXml); + Map pomMap = Map.of(Paths.get("pom.xml"), document); + + UpgradeContext context = createMockContext(); + strategy.doApply(context, pomMap); + + Element root = document.root(); + Element repos = DomUtils.findChildElement(root, "repositories"); + assertEquals(1, repos.childElements("repository").count(), "Repository should still be present"); + } + } + @Nested @DisplayName("Strategy Description") class StrategyDescriptionTests { diff --git a/impl/maven-core/src/main/java/org/apache/maven/project/DefaultProjectBuildingHelper.java b/impl/maven-core/src/main/java/org/apache/maven/project/DefaultProjectBuildingHelper.java index f8f77c7ceab3..6bde396a97e7 100644 --- a/impl/maven-core/src/main/java/org/apache/maven/project/DefaultProjectBuildingHelper.java +++ b/impl/maven-core/src/main/java/org/apache/maven/project/DefaultProjectBuildingHelper.java @@ -92,6 +92,13 @@ public List createArtifactRepositories( List internalRepositories = new ArrayList<>(); for (Repository repository : pomRepositories) { + if (containsExpression(repository.getId()) || containsExpression(repository.getUrl())) { + logger.warn( + "Skipping repository '{}' (url: '{}') containing an uninterpolated property expression", + repository.getId(), + repository.getUrl()); + continue; + } internalRepositories.add(MavenRepositorySystem.buildArtifactRepository(repository)); } @@ -267,6 +274,10 @@ public void selectProjectRealm(MavenProject project) { Thread.currentThread().setContextClassLoader(projectRealm); } + private static boolean containsExpression(String value) { + return value != null && value.contains("${"); + } + private List toAetherArtifacts(final List pluginArtifacts) { return new ArrayList<>(RepositoryUtils.toArtifacts(pluginArtifacts)); } diff --git a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java index 44b335333608..f17504b7d0fa 100644 --- a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java +++ b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java @@ -1535,11 +1535,11 @@ private void validateRawRepository( if (matcher.find()) { addViolation( problems, - Severity.ERROR, + Severity.WARNING, Version.V40, prefix + prefix2 + "[" + repository.getId() + "].id", null, - "contains an uninterpolated expression.", + "contains an uninterpolated expression; the repository will be skipped.", repository); } } @@ -1561,11 +1561,11 @@ && validateStringNotEmpty( if (matcher.find()) { addViolation( problems, - Severity.ERROR, + Severity.WARNING, Version.V40, prefix + prefix2 + "[" + repository.getId() + "].url", null, - "contains an uninterpolated expression.", + "contains an uninterpolated expression; the repository will be skipped.", repository); } } diff --git a/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelValidatorTest.java b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelValidatorTest.java index 36ba2745a08a..c65391872cf8 100644 --- a/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelValidatorTest.java +++ b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelValidatorTest.java @@ -907,31 +907,33 @@ void repositoryWithBasedirExpression() throws Exception { SimpleProblemCollector result = validateRaw("raw-model/repository-with-basedir-expression.xml"); // This test runs on raw model without interpolation, so all expressions appear uninterpolated // In the real flow, supported expressions would be interpolated before validation - assertViolations(result, 0, 3, 0); + assertViolations(result, 0, 0, 3); } @Test void repositoryWithUnsupportedExpression() throws Exception { SimpleProblemCollector result = validateRaw("raw-model/repository-with-unsupported-expression.xml"); - // Unsupported expressions should cause validation errors - assertViolations(result, 0, 1, 0); + // Unsupported expressions should cause validation warnings (repos will be skipped at build time) + assertViolations(result, 0, 0, 1); } @Test void repositoryWithUninterpolatedId() throws Exception { SimpleProblemCollector result = validateRaw("raw-model/repository-with-uninterpolated-id.xml"); - // Uninterpolated expressions in repository IDs should cause validation errors + // Uninterpolated expressions in repository IDs should cause validation warnings + // (repos will be skipped at build time) // distributionManagement repositories skip expression check since parent properties // may not be available at file model validation stage - assertViolations(result, 0, 2, 0); + assertViolations(result, 0, 0, 2); - // Check that repository ID validation errors are present for repositories and pluginRepositories - assertTrue(result.getErrors().stream() - .anyMatch(error -> error.contains("repositories.repository.[${repository.id}].id") - && error.contains("contains an uninterpolated expression"))); - assertTrue(result.getErrors().stream() - .anyMatch(error -> error.contains("pluginRepositories.pluginRepository.[${plugin.repository.id}].id") - && error.contains("contains an uninterpolated expression"))); + // Check that repository ID validation warnings are present for repositories and pluginRepositories + assertTrue(result.getWarnings().stream() + .anyMatch(warning -> warning.contains("repositories.repository.[${repository.id}].id") + && warning.contains("contains an uninterpolated expression"))); + assertTrue(result.getWarnings().stream() + .anyMatch( + warning -> warning.contains("pluginRepositories.pluginRepository.[${plugin.repository.id}].id") + && warning.contains("contains an uninterpolated expression"))); } @Test diff --git a/its/core-it-suite/src/test/java/org/apache/maven/it/MavenITgh11140RepoDmUnresolvedTest.java b/its/core-it-suite/src/test/java/org/apache/maven/it/MavenITgh11140RepoDmUnresolvedTest.java index b3a29292399a..671665828134 100644 --- a/its/core-it-suite/src/test/java/org/apache/maven/it/MavenITgh11140RepoDmUnresolvedTest.java +++ b/its/core-it-suite/src/test/java/org/apache/maven/it/MavenITgh11140RepoDmUnresolvedTest.java @@ -32,17 +32,14 @@ class MavenITgh11140RepoDmUnresolvedTest extends AbstractMavenIntegrationTestCas } @Test - void testFailsOnUnresolvedPlaceholders() throws Exception { + void testWarnsOnUnresolvedPlaceholders() throws Exception { File testDir = extractResources("/gh-11140-repo-dm-unresolved"); Verifier verifier = newVerifier(testDir.getAbsolutePath()); - try { - verifier.addCliArgument("validate"); - verifier.execute(); - } catch (VerificationException expected) { - // Expected to fail due to unresolved placeholders during model validation - } - // We expect error mentioning uninterpolated expression + verifier.addCliArgument("validate"); + verifier.execute(); + // Build should succeed, but warn about uninterpolated expressions (repos are skipped) + verifier.verifyErrorFreeLog(); verifier.verifyTextInLog("contains an uninterpolated expression"); } } diff --git a/its/core-it-suite/src/test/java/org/apache/maven/it/MavenITgh11140RepoInterpolationTest.java b/its/core-it-suite/src/test/java/org/apache/maven/it/MavenITgh11140RepoInterpolationTest.java index d354b33f2ec8..30b34510a311 100644 --- a/its/core-it-suite/src/test/java/org/apache/maven/it/MavenITgh11140RepoInterpolationTest.java +++ b/its/core-it-suite/src/test/java/org/apache/maven/it/MavenITgh11140RepoInterpolationTest.java @@ -73,18 +73,15 @@ private static String getBaseUri(Path base) { } @Test - void testUnresolvedPlaceholderFailsResolution() throws Exception { + void testUnresolvedPlaceholderWarnsAndSkipsRepository() throws Exception { File testDir = extractResources("/gh-11140-repo-interpolation"); Verifier verifier = newVerifier(testDir.getAbsolutePath()); // Do NOT set env vars, so placeholders stay verifier.addCliArgument("validate"); - try { - verifier.execute(); - } catch (VerificationException expected) { - // Expected to fail due to unresolved placeholders during model validation - } - // We expect error mentioning uninterpolated expression + verifier.execute(); + // Build should succeed, but warn about uninterpolated expressions + verifier.verifyErrorFreeLog(); verifier.verifyTextInLog("contains an uninterpolated expression"); } }