gnodet-bot commented on code in PR #13320:
URL: https://github.com/apache/maven/pull/13320#discussion_r4166046821
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelInterpolator.java:
##########
@@ -236,6 +236,12 @@ String doCallback(
if (value == null && !restricted) {
value = request.getSystemProperties().get("env." + expression);
}
+ // session.modelVersion – if the caller did not supply this property
explicitly via
+ // user/system properties, derive it from the model's own
<modelVersion> element.
+ // This makes ${session.modelVersion} usable in POM interpolation
(MNG-7984).
+ if (value == null &&
Constants.MAVEN_SESSION_MODEL_VERSION.equals(expression)) {
+ value = model.getModelVersion();
+ }
Review Comment:
💡 **Not guarded by `restricted`** — `session.modelVersion` starts with
`session.`, not `maven.`, so it doesn't pass `isSafeExternalExpression()`. For
repository-resolved external models (where `restricted == true`), user/system
property lookup for this expression would return `null`, but this fallback
still resolves it from `model.getModelVersion()`.
Since `modelVersion` is already present in the model XML itself (not a
secret), this is likely safe — but it's worth noting this intentionally
bypasses the restricted-interpolation guardrails. A brief comment explaining
why this is safe here would help future readers.
##########
api/maven-api-core/src/main/java/org/apache/maven/api/Constants.java:
##########
@@ -868,5 +868,30 @@ public final class Constants {
@Config(type = "java.lang.Boolean", defaultValue = "false")
public static final String MAVEN_MODEL_DEPENDENCY_INTERPOLATION_FULL =
"maven.model.dependencyInterpolation.full";
+ /**
+ * User property key to declare or override the effective POM model
version for the session.
+ * When explicitly set (e.g. via {@code -Dsession.modelVersion=4.0.0}),
this value controls
+ * which set of defaults Maven applies (resolver behaviour,
dependency-manager transitivity,
+ * etc.) and is also exposed as an interpolation expression ({@code
${session.modelVersion}})
+ * inside POM files.
+ * <p>
+ * If the property is <em>not</em> set explicitly, Maven derives the value
automatically:
+ * during POM interpolation the model's own {@code <modelVersion>} element
is used, while
+ * at session-setup time the property is absent and Maven falls back to
its built-in
+ * defaults (Maven 4 behaviour).
+ * </p>
+ * <ul>
+ * <li>{@code 4.0.0} – apply Maven 3-compatible defaults (e.g. disable
resolver
+ * dependency-manager transitivity)</li>
+ * <li>{@code 4.1.0} or higher – apply Maven 4 defaults</li>
+ * </ul>
+ *
+ * @since 4.2.0
+ * @see org.apache.maven.api.services.ModelBuilder#MODEL_VERSION_4_0_0
+ * @see org.apache.maven.api.services.ModelBuilder#MODEL_VERSION_4_1_0
+ */
+ @Config(readOnly = true)
+ public static final String MAVEN_SESSION_MODEL_VERSION =
"session.modelVersion";
+
Review Comment:
💡 **Missing `source` on `@Config`** — Other `readOnly` constants in this
file (lines 33–81) use `@Config(readOnly = true, source =
Config.Source.SYSTEM_PROPERTIES)`. This one omits `source`. Is this intentional
because the property can come from user properties (not just system)? If so, a
comment would help clarify. If not, should be aligned with the existing pattern.
##########
impl/maven-core/src/main/java/org/apache/maven/internal/aether/DefaultRepositorySystemSessionFactory.java:
##########
@@ -405,8 +405,12 @@ public SessionBuilder
newRepositorySessionBuilder(MavenExecutionRequest request)
sessionBuilder.setRepositoryListener(repositoryListener);
// may be overridden
+ // Use Maven 3-compatible defaults if either maven3Personality is set
OR if session.modelVersion
+ // is explicitly declared as 4.0.0 (MNG-7984: switch defaults based on
model version).
+ boolean usingMaven3CompatModel =
Features.maven3CompatModelVersion(mergedProps);
Review Comment:
⚠️ **Scope manager inconsistency** — This line correctly switches the
dependency-manager transitivity default based on `maven3CompatModelVersion()`,
but the `MavenSessionBuilderSupplier` constructed earlier (line 186) still
receives only `mavenMaven3Personality`.
When a user sets `-Dsession.modelVersion=4.0.0` *without*
`maven3Personality=true`:
- **Scope manager** → `Maven4ScopeManagerConfiguration` (Maven 4 scopes)
- **Dependency manager** → non-transitive / `ClassicDependencyManager`
(Maven 3 behavior)
These two are tightly coupled — `ClassicDependencyManager` expects the Maven
3 scope set, while `Maven4ScopeManagerConfiguration` defines a different scope
set. This mismatch could lead to unexpected resolution behavior.
The supplier construction at line 186 should also use
`maven3CompatModelVersion(mergedProps)` instead of just
`mavenMaven3Personality`:
```suggestion
boolean usingMaven3CompatModel =
Features.maven3CompatModelVersion(mergedProps);
MavenSessionBuilderSupplier supplier = new
MavenSessionBuilderSupplier(repoSystem, usingMaven3CompatModel);
```
Then the variable here at line 410 can reuse it instead of recomputing.
##########
api/maven-api-core/src/main/java/org/apache/maven/api/feature/Features.java:
##########
@@ -84,6 +85,30 @@ public static boolean deployBuildPom(@Nullable Map<String,
?> userProperties) {
return doGet(userProperties, Constants.MAVEN_DEPLOY_BUILD_POM, true);
}
+ /**
+ * Check if the session's declared model version ({@code
session.modelVersion}) implies
+ * Maven 3-compatible defaults.
+ * <p>
+ * Returns {@code true} when <em>either</em>:
+ * <ul>
+ * <li>{@link #mavenMaven3Personality(Map)} is {@code true}, or</li>
+ * <li>{@code session.modelVersion} is explicitly set to {@code
"4.0.0"}.</li>
+ * </ul>
+ * This is the single place to check whether resolver and model-builder
defaults should
+ * revert to the Maven 3 / POM 4.0.0 behaviour (MNG-7984).
+ *
+ * @param userProperties the merged user/system/profile properties map
+ * @return {@code true} if Maven 3-compatible model defaults should be used
+ * @since 4.2.0
+ */
+ public static boolean maven3CompatModelVersion(@Nullable Map<String, ?>
userProperties) {
+ if (mavenMaven3Personality(userProperties)) {
+ return true;
+ }
Review Comment:
💡 **Javadoc claims "single place to check"** — The doc says this is "the
single place to check whether resolver and model-builder defaults should revert
to the Maven 3 / POM 4.0.0 behaviour." But `DefaultModelBuilder` (lines 1327
and 2008) still uses `Features.mavenMaven3Personality()` directly for its own
Maven3-mode validation (e.g. rejecting `4.1.0` models). If
`maven3CompatModelVersion` is truly the canonical check, those call sites
should be updated too — or the Javadoc should be narrowed to say "resolver
defaults" rather than "resolver and model-builder defaults."
--
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]
