Skip to content

[4.0.x] Filter project repos with uninterpolated property expressions #12213

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
gnodet wants to merge 1 commit into from
Closed

[4.0.x] Filter project repos with uninterpolated property expressions #12213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 84 additions & 0 deletions ...li/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/CompatibilityFixStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ public UpgradeResult doApply(UpgradeContext context, Map<Path, Document> 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");
Expand Down Expand Up @@ -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<String> allDefinedProperties, UpgradeContext context) {
Element root = pomDocument.root();

Stream<RepositoryContainer> 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<String> allDefinedProperties,
Document pomDocument,
UpgradeContext context,
String sectionName) {
boolean fixed = false;
List<Element> repositories =
repositoriesElement.childElements(elementType).toList();
Editor editor = new Editor(pomDocument);

for (Element repository : repositories) {
Set<String> 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<String> findUndefinedPropertiesInRepository(Element repository, Set<String> allDefinedProperties) {
Set<String> 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.
*/
Expand Down
175 changes: 175 additions & 0 deletions ...rc/test/java/org/apache/maven/cling/invoker/mvnup/goals/CompatibilityFixStrategyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = """
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0">
<modelVersion>4.0.0</modelVersion>
<groupId>test</groupId>
<artifactId>test</artifactId>
<version>1.0.0</version>
<repositories>
<repository>
<id>${eclipseP2RepoId}</id>
<url>https://repo.example.com</url>
</repository>
</repositories>
</project>
""";

Document document = Document.of(pomXml);
Map<Path, Document> 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 = """
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0">
<modelVersion>4.0.0</modelVersion>
<groupId>test</groupId>
<artifactId>test</artifactId>
<version>1.0.0</version>
<repositories>
<repository>
<id>my-repo</id>
<url>${undefinedBaseUrl}/releases</url>
</repository>
</repositories>
</project>
""";

Document document = Document.of(pomXml);
Map<Path, Document> 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 = """
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0">
<modelVersion>4.0.0</modelVersion>
<groupId>test</groupId>
<artifactId>test</artifactId>
<version>1.0.0</version>
<properties>
<repoId>my-custom-repo</repoId>
</properties>
<repositories>
<repository>
<id>${repoId}</id>
<url>https://repo.example.com</url>
</repository>
</repositories>
</project>
""";

Document document = Document.of(pomXml);
Map<Path, Document> 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 = """
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0">
<modelVersion>4.0.0</modelVersion>
<groupId>test</groupId>
<artifactId>test</artifactId>
<version>1.0.0</version>
<pluginRepositories>
<pluginRepository>
<id>${eclipseP2RepoId}</id>
<url>https://plugins.example.com</url>
</pluginRepository>
</pluginRepositories>
</project>
""";

Document document = Document.of(pomXml);
Map<Path, Document> 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 = """
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0">
<modelVersion>4.0.0</modelVersion>
<groupId>test</groupId>
<artifactId>test</artifactId>
<version>1.0.0</version>
<repositories>
<repository>
<id>local-repo</id>
<url>file://${project.basedir}/repo</url>
</repository>
</repositories>
</project>
""";

Document document = Document.of(pomXml);
Map<Path, Document> 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 {
Expand Down
11 changes: 11 additions & 0 deletions impl/maven-core/src/main/java/org/apache/maven/project/DefaultProjectBuildingHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,13 @@ public List<ArtifactRepository> createArtifactRepositories(
List<ArtifactRepository> 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));
}

Expand Down Expand Up @@ -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<org.eclipse.aether.artifact.Artifact> toAetherArtifacts(final List<Artifact> pluginArtifacts) {
return new ArrayList<>(RepositoryUtils.toArtifacts(pluginArtifacts));
}
Expand Down
8 changes: 4 additions & 4 deletions impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand All @@ -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);
}
}
Expand Down
26 changes: 14 additions & 12 deletions impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelValidatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading
Loading