gnodet-bot commented on code in PR #12745:
URL: https://github.com/apache/maven/pull/12745#discussion_r3976129021
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java:
##########
@@ -500,6 +500,7 @@ && equals(parent.getArtifactId(), model.getArtifactId())) {
}
boolean isModelVersion41OrMore =
!Objects.equals(ModelBuilder.MODEL_VERSION_4_0_0, model.getModelVersion());
+ boolean isModelVersion42OrMore =
Objects.equals(ModelBuilder.MODEL_VERSION_4_2_0, model.getModelVersion());
Review Comment:
🔴 **Bug: `isModelVersion42OrMore` only matches `"4.2.0"` exactly.**
`Objects.equals(MODEL_VERSION_4_2_0, model.getModelVersion())` returns
`false` for any future model version (`4.3.0`, etc.), meaning
`api`/`implementation` scopes would be incorrectly **rejected** on 4.3.0+ POMs.
Contrast with `isModelVersion41OrMore` on the line above, which uses
negation (`!Objects.equals(MODEL_VERSION_4_0_0, ...)`) — that's future-proof
because any version that isn't 4.0.0 passes.
The mixins validation at line 364 of this same file already demonstrates the
correct pattern: `compareModelVersions("4.2.0", model.getModelVersion()) < 0`.
Consistent fix:
```suggestion
boolean isModelVersion42OrMore = isModelVersion41OrMore
&& !Objects.equals(ModelBuilder.MODEL_VERSION_4_1_0,
model.getModelVersion());
```
Alternatively, use `compareModelVersions` like the mixins validation does —
but the negation pattern above is simpler and consistent with how
`isModelVersion41OrMore` is computed.
##########
src/mdo/model-version.vm:
##########
@@ -161,11 +161,33 @@ public class ${className} {
#end
#set ( $pfx = "||" )
#end
+ #if ( $v == "4_2_0" && $class.name == "Model" )
+ $pfx hasNewScopes(${var}) // Dependency scopes api / implementation
+ #end
);
}
#end
#end
+ private boolean hasNewScopes(Model model) {
+ return hasNewScopes((ModelBase) model)
+ || model.getProfiles().stream().anyMatch(this::hasNewScopes);
+ }
+
+ private boolean hasNewScopes(Profile profile) {
+ return hasNewScopes((ModelBase) profile);
+ }
+
+ private boolean hasNewScopes(ModelBase model) {
+ return model != null
+ && (model.getDependencies().stream().anyMatch(dependency ->
+ "api".equals(dependency.getScope()) ||
"implementation".equals(dependency.getScope()))
+ || (model.getDependencyManagement() != null
+ &&
model.getDependencyManagement().getDependencies().stream()
+ .anyMatch(dependency ->
+
"api".equals(dependency.getScope())
+ ||
"implementation".equals(dependency.getScope()))));
+ }
private boolean has(String str) {
Review Comment:
🟡 **Still missing blank line** (re-raised from 3rd review). Every other
method pair in this file has a blank separator.
```suggestion
}
private boolean has(String str) {
```
--
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]
