gnodet commented on code in PR #12745:
URL: https://github.com/apache/maven/pull/12745#discussion_r3973924682
##########
api/maven-api-core/src/main/java/org/apache/maven/api/DependencyScope.java:
##########
@@ -66,6 +66,26 @@ public enum DependencyScope {
*/
COMPILE("compile", true),
Review Comment:
🟢 **Nit: consider positioning after RUNTIME**
The new `API` and `IMPLEMENTATION` enum constants are placed between
`COMPILE` and `RUNTIME`. This is defensible (grouping compile-related scopes
together), but it changes the ordinal values of `RUNTIME`, `PROVIDED`, `TEST`,
`TEST_ONLY`, `TEST_RUNTIME`, and `SYSTEM`. Since `DependencyScope` is an
`@Experimental` API this is technically fine — but worth noting that any code
using `ordinal()` or `values()` ordering will see a change.
Not blocking — just flagging for awareness.
##########
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:
🟡 **Missing blank line before `private boolean has(String str)`**
The `hasNewScopes(ModelBase)` method ends at line 190 and the `has(String)`
method starts immediately at line 191 with no blank line separator. Every other
method pair in this file has a blank line between them.
```suggestion
}
private boolean has(String str) {
```
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java:
##########
@@ -481,6 +481,7 @@ && equals(parent.getArtifactId(), model.getArtifactId())) {
Severity errOn30 = getSeverity(validationLevel,
ModelValidator.VALIDATION_LEVEL_MAVEN_3_0);
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` uses `Objects.equals()` which only matches
exactly `4.2.0`**
The existing `isModelVersion41OrMore` is defined as
`!Objects.equals(MODEL_VERSION_4_0_0, ...)` — a negation-based check that
correctly includes 4.1.0, 4.2.0, and any future version.
But `isModelVersion42OrMore` uses `Objects.equals(MODEL_VERSION_4_2_0, ...)`
which means it only matches `"4.2.0"` exactly. If a future `4.3.0` model
version is added, this check would be `false`, and the validator would
incorrectly reject `api`/`implementation` scopes on 4.3.0 POMs.
This should use a comparison-based check consistent with how the mixins
validation works a few lines above (line 364):
```suggestion
boolean isModelVersion42OrMore = compareModelVersions("4.2.0",
model.getModelVersion()) >= 0;
```
Note: `compareModelVersions` returns negative when the first arg is newer,
zero when equal, positive when the second is newer. So
`compareModelVersions("4.2.0", actual) >= 0` means `actual >= 4.2.0`.
##########
impl/maven-core/src/main/java/org/apache/maven/internal/transformation/impl/DefaultConsumerPomBuilder.java:
##########
@@ -275,23 +277,45 @@ private Model buildEffectiveModel(RepositorySystemSession
session, MavenProject
Function.identity(),
this::merge,
LinkedHashMap::new));
- // Only keep transitive scopes
+ // Only keep consumer-visible scopes (compile, api, runtime,
implementation)
directDependencies.values().removeIf(DefaultConsumerPomBuilder::hasDependencyScope);
+ // Map 4.2.0 scopes to their 4.0.0 consumer POM equivalents
(api→compile, implementation→runtime)
+ directDependencies.replaceAll((k, v) -> mapScopeForConsumerPom(v));
model = model.withDependencies(directDependencies.isEmpty() ? null
: directDependencies.values());
}
return model;
}
- private static boolean hasDependencyScope(Dependency dependency) {
+ static boolean hasDependencyScope(Dependency dependency) {
String scopeId = dependency.getScope();
DependencyScope scope;
if (scopeId == null || scopeId.isEmpty()) {
scope = DependencyScope.COMPILE;
} else {
scope = DependencyScope.forId(scopeId);
}
- return scope == null || !scope.isTransitive();
+ return scope != DependencyScope.COMPILE
Review Comment:
🟡 **The `hasDependencyScope` filter logic is correct but the name is
confusing**
The method returns `true` for scopes that should be *removed* (it's used
with `removeIf`). The new explicit allowlist approach (`scope != COMPILE &&
scope != RUNTIME && scope != API && scope != IMPLEMENTATION`) is clearer than
the old `!scope.isTransitive()` — good change.
However, note that `scope == null` (unknown scope string) now returns `true`
(= remove), where previously an unknown scope with `isTransitive() == false`
would also have been removed. So behavior is consistent. đź‘Ť
--
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]
