You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "Tarun4201 (via GitHub)" <gi...@apache.org> on 2026/10/02 13:10:36 UTC
[PR] [MNG-7984] Switch defaults based on model version used (Fix #8857) [maven]
Tarun4201 opened a new pull request, #13320:
URL: https://github.com/apache/maven/pull/13320
- Add MAVEN_SESSION_MODEL_VERSION ('session.modelVersion') constant to Constants
- Add Features.maven3CompatModelVersion(userProperties) helper to detect Maven 3-compatible mode from personality or session.modelVersion=4.0.0
- Update DefaultRepositorySystemSessionFactory to apply Maven 3 resolver defaults when session.modelVersion is 4.0.0
- Support ${session.modelVersion} in DefaultModelInterpolator deriving from model.getModelVersion() if not set in properties
- Add unit tests in FeaturesTest and DefaultModelInterpolatorTest
<!--
Please open pull requests against one of the following branches:
- master — default target for all changes (new features + bug fixes).
If the change should also go to maven-4.0.x, request a backport in the PR.
- maven-3.10.x — target for 3.x changes (new features + bug fixes).
If the change should also go to maven-3.9.x, request a backport in the PR.
maven-3.9.x should receive only serious bug fixes, as it is approaching EOL.
Backports are typically handled by maintainers;
however, after agreement you may open a separate backport PR to the target maintenance branch.
-->
Following this checklist to help us incorporate your
contribution quickly and easily:
- [ ] Your pull request should address just one issue, without pulling in other changes.
- [ ] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [ ] Each commit in the pull request should have a meaningful subject line and body.
Note that commits might be squashed by a maintainer on merge.
- [ ] Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied.
This may not always be possible but is a best-practice.
- [ ] Run `mvn verify` to make sure basic checks pass.
A more thorough check will be performed on your pull request automatically.
- [ ] You have run the [Core IT][core-its] successfully.
If your pull request is about ~20 lines of code you don't need to sign an
[Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf) if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [ ] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [ ] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
[core-its]: https://maven.apache.org/core-its/core-it-suite/
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-7984] Switch defaults based on model version used (Fix #8857) [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #13320:
URL: https://github.com/apache/maven/pull/13320#issuecomment-5983354956
I think this is wrong. We decided to use a user property instead of relying on the modelVersion, as this is slightly different.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-7984] Switch defaults based on model version used (Fix #8857) [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet closed pull request #13320: [MNG-7984] Switch defaults based on model version used (Fix #8857)
URL: https://github.com/apache/maven/pull/13320
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-7984] Switch defaults based on model version used (Fix #8857) [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-7984] Switch defaults based on model version used (Fix #8857) [maven]
Posted by "Tarun4201 (via GitHub)" <gi...@apache.org>.
Tarun4201 commented on PR #13320:
URL: https://github.com/apache/maven/pull/13320#issuecomment-5953466007
Thanks for the thorough review! All points have been addressed in commit `4f685c2`:
1. **Scope Manager & Dependency Manager Alignment**:
- `MavenSessionBuilderSupplier` now receives `usingMaven3CompatModel = Features.maven3CompatModelVersion(mergedProps)` so that `ScopeManager` and `DependencyManager` remain synchronized under `-Dsession.modelVersion=4.0.0`.
- Reused `usingMaven3CompatModel` at line 410 without redundant recalculation.
2. **`@Config` Annotation on `MAVEN_SESSION_MODEL_VERSION`**:
- Updated to `@Config(source = Config.Source.USER_PROPERTIES)` and removed `readOnly = true`, aligning with the property's intent as a user-configurable property.
3. **Clarified Javadoc**:
- Narrowed `Features.maven3CompatModelVersion()` Javadoc to specifically describe "resolver defaults" (scope manager configuration and dependency manager transitivity).
4. **Restricted Mode Documentation**:
- Added an explanatory comment in `DefaultModelInterpolator` clarifying why resolving `${session.modelVersion}` from `model.getModelVersion()` is safe even when `restricted == true`, as it reads public metadata directly from the model's XML.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org