Copilot commented on code in PR #13669:
URL: https://github.com/apache/maven/pull/13669#discussion_r4427808241
##########
impl/maven-core/src/test/java/org/apache/maven/project/ProjectBuilderTest.java:
##########
@@ -283,6 +283,63 @@ void testFailedParentStillInitializesChild(@TempDir Path
tempDir) throws Excepti
}
}
+ @Test
+ void testLocalParentResolutionWithValidationLevels(@TempDir Path tempDir)
throws Exception {
+ Path parentPom = tempDir.resolve("pom.xml");
+ Files.writeString(parentPom, """
+ <project>
+ <modelVersion>4.0.0</modelVersion>
+ <groupId>test</groupId>
+ <artifactId>parent</artifactId>
+ <version>1</version>
+ <packaging>pom</packaging>
+ </project>
+ """);
+
+ Path childPom =
Files.createDirectory(tempDir.resolve("child")).resolve("pom.xml");
+ Files.writeString(childPom, """
+ <project xmlns="http://maven.apache.org/POM/4.1.0";>
+ <modelVersion>4.1.0</modelVersion>
+ <parent>
+ <groupId>test</groupId>
+ <artifactId>parent</artifactId>
+ <version>1</version>
+ <relativePath>../pom.xml</relativePath>
+ </parent>
+ <artifactId>child</artifactId>
+ </project>
+ """);
Review Comment:
The test POM uses `xmlns` and `modelVersion` `4.1.0`, which may not be
recognized consistently across Maven/model versions and can make the test flaky
for reasons unrelated to this PR’s intent. Consider using the canonical POM
namespace/version (`http://maven.apache.org/POM/4.0.0` and `4.0.0`) while
keeping the parent section structure that triggers the intended validation
difference.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java:
##########
@@ -2989,7 +2993,17 @@ private <T> T cache(Source source, String tag,
Supplier<T> supplier) throws Mode
boolean isBuildRequest() {
return request.getRequestType() ==
ModelBuilderRequest.RequestType.BUILD_PROJECT
|| request.getRequestType() ==
ModelBuilderRequest.RequestType.BUILD_EFFECTIVE
- || request.getRequestType() ==
ModelBuilderRequest.RequestType.BUILD_CONSUMER;
+ || request.getRequestType() ==
ModelBuilderRequest.RequestType.BUILD_CONSUMER
+ || request.getRequestType() ==
ModelBuilderRequest.RequestType.LOAD_PROJECT;
+ }
+
+ /**
+ * Whether the model has to be parsed and validated strictly. This is
the case for the build requests, but not
+ * for {@link ModelBuilderRequest.RequestType#LOAD_PROJECT}, which is
a project only in terms of semantics
+ * (local parents, profile activation) and keeps the lenient defaults
of the consumer requests.
+ */
+ boolean isStrictRequest() {
+ return isBuildRequest() && request.getRequestType() !=
ModelBuilderRequest.RequestType.LOAD_PROJECT;
Review Comment:
`isBuildRequest()` now returns `true` for `RequestType.LOAD_PROJECT`, which
is explicitly *not* a build/strict request. Even with `isStrictRequest()`
added, this can be misleading for future readers/maintainers. Consider renaming
`isBuildRequest()` to something reflecting its new meaning (e.g., “project
semantics request”), or replace `isStrictRequest()` with an explicit positive
check (e.g., strict if requestType is one of the `BUILD_*` types) to avoid the
somewhat surprising `isBuildRequest() && != LOAD_PROJECT` logic.
##########
impl/maven-core/src/test/java/org/apache/maven/project/ProjectBuilderTest.java:
##########
@@ -283,6 +283,63 @@ void testFailedParentStillInitializesChild(@TempDir Path
tempDir) throws Excepti
}
}
+ @Test
+ void testLocalParentResolutionWithValidationLevels(@TempDir Path tempDir)
throws Exception {
+ Path parentPom = tempDir.resolve("pom.xml");
+ Files.writeString(parentPom, """
+ <project>
+ <modelVersion>4.0.0</modelVersion>
+ <groupId>test</groupId>
+ <artifactId>parent</artifactId>
+ <version>1</version>
+ <packaging>pom</packaging>
+ </project>
+ """);
+
+ Path childPom =
Files.createDirectory(tempDir.resolve("child")).resolve("pom.xml");
+ Files.writeString(childPom, """
+ <project xmlns="http://maven.apache.org/POM/4.1.0";>
+ <modelVersion>4.1.0</modelVersion>
+ <parent>
+ <groupId>test</groupId>
+ <artifactId>parent</artifactId>
+ <version>1</version>
+ <relativePath>../pom.xml</relativePath>
+ </parent>
+ <artifactId>child</artifactId>
+ </project>
+ """);
+
+ MavenSession session = createMavenSession(null);
+ session.getRequest().setRootDirectory(tempDir);
+ org.apache.maven.project.ProjectBuilder builder =
+
getContainer().lookup(org.apache.maven.project.ProjectBuilder.class);
+
+ for (int validationLevel :
+ List.of(ModelBuildingRequest.VALIDATION_LEVEL_MINIMAL,
ModelBuildingRequest.VALIDATION_LEVEL_STRICT)) {
+ ProjectBuildingRequest request = new
DefaultProjectBuildingRequest();
+ request.setRepositorySession(session.getRepositorySession());
+ request.setValidationLevel(validationLevel);
+ // processPlugins=false would be a consumer request, which never
looks up a local parent
+ request.setProcessPlugins(true);
+ ProjectBuildingResult result = builder.build(childPom.toFile(),
request);
+ MavenProject child = result.getProject();
+
+ assertNotNull(child.getParentArtifact());
+ assertEquals("parent", child.getParentArtifact().getArtifactId());
+ assertEquals(parentPom.toFile(), child.getParentFile());
+
+ boolean hasRelativePathWarning = result.getProblems().stream()
+ .anyMatch(problem ->
+ problem.getMessage().contains("only specify
relativePath or groupId/artifactId"));
+
+ assertEquals(
+ validationLevel ==
ModelBuildingRequest.VALIDATION_LEVEL_STRICT,
+ hasRelativePathWarning,
+ "Unexpected validation result for level " +
validationLevel);
Review Comment:
Matching on a hard-coded substring of the problem message makes the test
brittle (message text can change, get reworded, or vary by locale). If
`ModelProblem` exposes a stable identifier/severity/type, prefer asserting on
that (e.g., problem severity + a dedicated problem ID/code), or at least
constrain the match more structurally (e.g., by validating
`problem.getException()`/`getLocation()`/`getSource()` if available) to reduce
false positives.
--
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]
