gnodet-bot commented on code in PR #13601:
URL: https://github.com/apache/maven/pull/13601#discussion_r4304535009
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java:
##########
@@ -1270,13 +1270,8 @@ void buildEffectiveModel(Collection<String> importIds)
throws ModelBuilderExcept
.getParent()
.withRelativePath(resultModel.getParent().getRelativePath())));
}
-
// effective model validation
- modelValidator.validateEffectiveModel(
- session,
- resultModel,
- isBuildRequest() ? ModelValidator.VALIDATION_LEVEL_STRICT
: ModelValidator.VALIDATION_LEVEL_MINIMAL,
- this);
+ modelValidator.validateEffectiveModel(session, resultModel,
getValidationLevel(), this);
Review Comment:
⚠️ **Incomplete — `validateFileModel` not updated:** `validateRawModel` and
`validateEffectiveModel` now use `getValidationLevel()`, but
`validateFileModel` (at line 2050 of the file) still uses the hardcoded
`isBuildRequest() ? ModelValidator.VALIDATION_LEVEL_STRICT :
ModelValidator.VALIDATION_LEVEL_MINIMAL` pattern. All three validation phases
must be updated consistently or the fix is incomplete.
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/ModelBuilderRequest.java:
##########
@@ -220,6 +224,11 @@ public ModelBuilderRequestBuilder requestType(RequestType
requestType) {
return this;
}
+ public ModelBuilderRequestBuilder validationLevel(int validationLevel)
{
+ this.validationLevel = validationLevel;
Review Comment:
💡 **Builder method should accept `Integer`, not `int`:** The field is
`Integer validationLevel` (nullable — `null` means "use default"), but the
builder method takes primitive `int`. Once a caller writes
`.validationLevel(0)`, the value is non-null `Integer(0)` and the
`getValidationLevel()` fallback in `DefaultModelBuilder` can never trigger —
there is no way to express "use the default" after calling this builder method.
```suggestion
public ModelBuilderRequestBuilder validationLevel(Integer
validationLevel) {
```
##########
impl/maven-core/src/test/java/org/apache/maven/project/ProjectBuilderTest.java:
##########
@@ -283,6 +283,55 @@ 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>
+ <modelVersion>4.0.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)) {
+ for (boolean processPlugins : List.of(false, true)) {
+ ProjectBuildingRequest request = new
DefaultProjectBuildingRequest();
+ request.setRepositorySession(session.getRepositorySession());
+ request.setValidationLevel(validationLevel);
+ request.setProcessPlugins(processPlugins);
+
+ MavenProject child = builder.build(childPom.toFile(),
request).getProject();
+
+ assertNotNull(child.getParentArtifact());
+ assertEquals("parent",
child.getParentArtifact().getArtifactId());
Review Comment:
💡 **Test doesn't verify the fix:** The assertions only check that parent
linkage succeeded — they would pass even if `validationLevel` was never
forwarded. Consider adding an assertion that actually validates the level was
applied (e.g. intentionally inject a Maven 4-only validation error that should
only appear at `VALIDATION_LEVEL_STRICT`, then assert it is/isn't reported
based on the validation level used).
##########
impl/maven-core/src/main/java/org/apache/maven/project/DefaultProjectBuilder.java:
##########
@@ -466,19 +466,20 @@ void prepare() throws ProjectBuildingException {
&&
source.getLocation().endsWith("/org/apache/maven/project/standalone.xml");
ModelBuilderRequest.ModelBuilderRequestBuilder builder =
getModelBuildingRequest();
- ModelBuilderRequest.RequestType type = reactorMember
- || isStandalone
- || (pomFile != null
- && request.isProcessPlugins()
- && request.getValidationLevel()
- ==
ModelBuildingRequest.VALIDATION_LEVEL_STRICT)
- ? ModelBuilderRequest.RequestType.BUILD_EFFECTIVE
- : (parent
- ?
ModelBuilderRequest.RequestType.CONSUMER_PARENT
- :
ModelBuilderRequest.RequestType.CONSUMER_DEPENDENCY);
+ ModelBuilderRequest.RequestType type = reactorMember
+ || isStandalone
+ || (pomFile != null
+ && request.isProcessPlugins()
+ && request.getValidationLevel() ==
ModelBuildingRequest.VALIDATION_LEVEL_STRICT)
+ ? ModelBuilderRequest.RequestType.BUILD_EFFECTIVE
+ : (parent
+ ? ModelBuilderRequest.RequestType.CONSUMER_PARENT
+ : ModelBuilderRequest.RequestType.CONSUMER_DEPENDENCY);
+
MavenProject theProject = project;
ModelBuilderRequest modelRequest = builder.source(source)
.requestType(type)
+ .validationLevel(request.getValidationLevel())
.locationTracking(true)
Review Comment:
⛔ **Critical — validation domain mismatch:** `request.getValidationLevel()`
returns a value from the compat `ModelBuildingRequest` scale where
`VALIDATION_LEVEL_STRICT = 30` (Maven 3.0). But the SPI `ModelValidator` uses a
different scale where `VALIDATION_LEVEL_STRICT = 42` (Maven 4.2). Passing `30`
here will be interpreted as `VALIDATION_LEVEL_MAVEN_3_0` by the validator — not
STRICT — silently skipping 3.1, 4.0, 4.1, and 4.2 validation rules for every
default build request.
The translation should happen here (or in
`DefaultModelBuilder.getValidationLevel()`) before the compat value is
forwarded into the SPI domain. One option: map compat STRICT →
`ModelValidator.VALIDATION_LEVEL_STRICT`, compat MINIMAL →
`ModelValidator.VALIDATION_LEVEL_MINIMAL`, and treat any other value as
unknown/default.
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/ModelBuilderRequest.java:
##########
@@ -94,6 +94,8 @@ enum RepositoryMerging {
@Nonnull
RequestType getRequestType();
+ Integer getValidationLevel();
+
Review Comment:
⚠️ **Missing `@Nullable` and Javadoc:** The new method should match the
conventions of the `@Immutable` interface. Other nullable methods carry
`@Nullable` and a Javadoc comment explaining semantics. Suggested:
```suggestion
/**
* Returns the explicit validation level to apply, or {@code null} to
use the default
* level based on the {@linkplain #getRequestType() request type}.
*
* @return the validation level, or {@code null}
* @see org.apache.maven.api.services.model.ModelValidator
*/
@Nullable
Integer getValidationLevel();
```
##########
impl/maven-core/src/main/java/org/apache/maven/project/DefaultProjectBuilder.java:
##########
@@ -90,9 +90,9 @@
import org.apache.maven.impl.InternalSession;
import org.apache.maven.impl.resolver.ArtifactDescriptorUtils;
import org.apache.maven.internal.impl.InternalMavenSession;
+import org.apache.maven.model.building.ModelBuildingRequest;
import org.apache.maven.model.building.DefaultModelProblem;
Review Comment:
🔧 **Import ordering regression:** `ModelBuildingRequest` was moved before
`DefaultModelProblem`, breaking alphabetical order within the
`org.apache.maven.model.building.*` group. Please restore the original order.
##########
impl/maven-core/src/main/java/org/apache/maven/project/DefaultProjectBuilder.java:
##########
@@ -466,19 +466,20 @@ void prepare() throws ProjectBuildingException {
&&
source.getLocation().endsWith("/org/apache/maven/project/standalone.xml");
ModelBuilderRequest.ModelBuilderRequestBuilder builder =
getModelBuildingRequest();
- ModelBuilderRequest.RequestType type = reactorMember
- || isStandalone
- || (pomFile != null
- && request.isProcessPlugins()
- && request.getValidationLevel()
- ==
ModelBuildingRequest.VALIDATION_LEVEL_STRICT)
- ? ModelBuilderRequest.RequestType.BUILD_EFFECTIVE
- : (parent
- ?
ModelBuilderRequest.RequestType.CONSUMER_PARENT
- :
ModelBuilderRequest.RequestType.CONSUMER_DEPENDENCY);
+ ModelBuilderRequest.RequestType type = reactorMember
+ || isStandalone
Review Comment:
🔧 **Indentation regression:** The ternary expression lost its alignment. The
original consistently indented the continuation lines to align with the
surrounding block; the replacement uses two different indent depths (`11
spaces` for the assignment, `8 spaces` for the operator continuations). Please
restore the original alignment or run the formatter.
--
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]
