gnodet-bot commented on code in PR #13603:
URL: https://github.com/apache/maven/pull/13603#discussion_r4312185384
##########
impl/maven-core/src/main/java/org/apache/maven/execution/MavenExecutionRequest.java:
##########
@@ -258,6 +258,44 @@ public interface MavenExecutionRequest {
boolean isNoSnapshotUpdates();
+ // Resolver 2.x artifact and metadata update policies
+
+ /**
+ * Sets the update policy to apply to artifact resolution.
+ * Valid values: {@code always}, {@code daily}, {@code interval:N}
(minutes), {@code never}.
+ *
+ * @param policy the update policy string, or {@code null} to use the
legacy snapshot flags
+ * @return this request
+ * @since 4.0.0
+ */
+ MavenExecutionRequest setArtifactsUpdatePolicy(String policy);
+
Review Comment:
⚠️ **Misleading Javadoc** — `@param policy` says "or `null` to use the
legacy snapshot flags" but that is only true when **both** artifact and
metadata policies are null (line 270: `if (getArtifactsUpdatePolicy() == null
&& getMetadataUpdatePolicy() == null)`). When one dimension is set and the
other is null, the null one uses the resolver default — not the legacy flag.
PR #13604 (master port) correctly captures this:
```suggestion
* @param policy the update policy string, or {@code null} to defer to
the legacy snapshot flags
* (only when the metadata update policy is also {@code
null})
* @return this request
* @since 4.0.0
*/
MavenExecutionRequest setArtifactsUpdatePolicy(String policy);
```
##########
impl/maven-core/src/main/java/org/apache/maven/internal/aether/DefaultRepositorySystemSessionFactory.java:
##########
@@ -195,10 +195,22 @@ public SessionBuilder
newRepositorySessionBuilder(MavenExecutionRequest request)
sessionBuilder.setOffline(request.isOffline());
sessionBuilder.setChecksumPolicy(request.getGlobalChecksumPolicy());
- sessionBuilder.setUpdatePolicy(
- request.isNoSnapshotUpdates()
- ? RepositoryPolicy.UPDATE_POLICY_NEVER
- : request.isUpdateSnapshots() ?
RepositoryPolicy.UPDATE_POLICY_ALWAYS : null);
+
+ if (request.getArtifactsUpdatePolicy() == null &&
request.getMetadataUpdatePolicy() == null) {
+ // we go "old" way
+ sessionBuilder.setUpdatePolicy(
+ request.isNoSnapshotUpdates()
+ ? RepositoryPolicy.UPDATE_POLICY_NEVER
+ : request.isUpdateSnapshots() ?
RepositoryPolicy.UPDATE_POLICY_ALWAYS : null);
+ } else {
+ // we go "new" way: only set explicitly-specified dimensions
+ if (request.getArtifactsUpdatePolicy() != null) {
+
sessionBuilder.setArtifactUpdatePolicy(request.getArtifactsUpdatePolicy());
+ }
+ if (request.getMetadataUpdatePolicy() != null) {
Review Comment:
🐛 **Silent flag suppression without warning** — when new-style options are
mixed with legacy flags (e.g. `mvn -U --metadata-update-policy daily`), the
`-U` flag is silently discarded. The user gets artifact policy = resolver
default (not `always`), with no indication that their `-U` flag was ignored.
PR #13604 (the master port of this same feature) adds a `logger.warn()` for
exactly this case:
```suggestion
} else {
// New-style per-type policies take precedence; warn if
old-style flags were also set
if (request.isNoSnapshotUpdates() ||
request.isUpdateSnapshots()) {
logger.warn("Both new-style update policy options and legacy
snapshot flags (-U/-nsu) were specified. "
+ "The new-style policies take precedence; the
legacy flags are ignored.");
}
if (request.getArtifactsUpdatePolicy() != null) {
sessionBuilder.setArtifactUpdatePolicy(request.getArtifactsUpdatePolicy());
}
if (request.getMetadataUpdatePolicy() != null) {
sessionBuilder.setMetadataUpdatePolicy(request.getMetadataUpdatePolicy());
}
}
```
The warning is actionable (the user can remove the conflicting flag), which
satisfies the project's warning policy. The same fix applies to the compat
`MavenCli.java`.
##########
impl/maven-core/src/main/java/org/apache/maven/execution/MavenExecutionRequest.java:
##########
@@ -258,6 +258,44 @@ public interface MavenExecutionRequest {
boolean isNoSnapshotUpdates();
+ // Resolver 2.x artifact and metadata update policies
+
+ /**
+ * Sets the update policy to apply to artifact resolution.
+ * Valid values: {@code always}, {@code daily}, {@code interval:N}
(minutes), {@code never}.
+ *
+ * @param policy the update policy string, or {@code null} to use the
legacy snapshot flags
+ * @return this request
+ * @since 4.0.0
+ */
+ MavenExecutionRequest setArtifactsUpdatePolicy(String policy);
+
+ /**
+ * Returns the update policy to apply to artifact resolution.
+ *
+ * @return the policy string, or {@code null} if the legacy snapshot flags
are used
+ * @since 4.0.0
+ */
+ String getArtifactsUpdatePolicy();
+
+ /**
+ * Sets the update policy to apply to metadata resolution.
+ * Valid values: {@code always}, {@code daily}, {@code interval:N}
(minutes), {@code never}.
+ *
+ * @param policy the update policy string, or {@code null} to use the
legacy snapshot flags
+ * @return this request
+ * @since 4.0.0
+ */
+ MavenExecutionRequest setMetadataUpdatePolicy(String policy);
+
Review Comment:
⚠️ **Same Javadoc issue** — the `@param` for `setMetadataUpdatePolicy` has
the same misleading null description.
```suggestion
* @param policy the update policy string, or {@code null} to defer to
the legacy snapshot flags
* (only when the artifacts update policy is also {@code
null})
* @return this request
* @since 4.0.0
*/
MavenExecutionRequest setMetadataUpdatePolicy(String policy);
```
##########
impl/maven-core/src/test/java/org/apache/maven/internal/aether/DefaultRepositorySystemSessionFactoryTest.java:
##########
@@ -498,6 +498,50 @@ private static RemoteRepository repository(String id,
String url) {
return new RemoteRepository.Builder(id, "default", url).build();
}
+ @Test
+ void artifactsUpdatePolicyTest() throws InvalidRepositoryException {
+ DefaultRepositorySystemSessionFactory systemSessionFactory = new
DefaultRepositorySystemSessionFactory(
+ aetherRepositorySystem,
+ eventSpyDispatcher,
+ information,
+ defaultTypeRegistry,
+ versionScheme,
+ Collections.emptyMap(),
+ versionFilterBuilder);
+
+ // No policy set: legacy path, no per-type policy
+ MavenExecutionRequest request = new DefaultMavenExecutionRequest();
+ request.setLocalRepository(getLocalRepository());
+ org.eclipse.aether.RepositorySystemSession session =
systemSessionFactory.newRepositorySession(request);
+ assertNull(session.getArtifactUpdatePolicy());
+ assertNull(session.getMetadataUpdatePolicy());
+
+ // Artifacts policy only
+ request = new DefaultMavenExecutionRequest();
+ request.setLocalRepository(getLocalRepository());
+
request.setArtifactsUpdatePolicy(RepositoryPolicy.UPDATE_POLICY_ALWAYS);
+ session = systemSessionFactory.newRepositorySession(request);
+ assertEquals(RepositoryPolicy.UPDATE_POLICY_ALWAYS,
session.getArtifactUpdatePolicy());
+ assertNull(session.getMetadataUpdatePolicy());
+
+ // Metadata policy only
+ request = new DefaultMavenExecutionRequest();
+ request.setLocalRepository(getLocalRepository());
+ request.setMetadataUpdatePolicy(RepositoryPolicy.UPDATE_POLICY_NEVER);
+ session = systemSessionFactory.newRepositorySession(request);
+ assertNull(session.getArtifactUpdatePolicy());
+ assertEquals(RepositoryPolicy.UPDATE_POLICY_NEVER,
session.getMetadataUpdatePolicy());
+
+ // Both per-type policies set (interval:N)
+ request = new DefaultMavenExecutionRequest();
+ request.setLocalRepository(getLocalRepository());
+ request.setArtifactsUpdatePolicy("interval:30");
+ request.setMetadataUpdatePolicy(RepositoryPolicy.UPDATE_POLICY_DAILY);
+ session = systemSessionFactory.newRepositorySession(request);
+ assertEquals("interval:30", session.getArtifactUpdatePolicy());
+ assertEquals(RepositoryPolicy.UPDATE_POLICY_DAILY,
session.getMetadataUpdatePolicy());
+ }
+
protected ArtifactRepository getLocalRepository() throws
InvalidRepositoryException {
File repoDir = new File(getBasedir(),
"target/local-repo").getAbsoluteFile();
Review Comment:
⚠️ **Missing mixed-flag test** — `artifactsUpdatePolicyTest()` covers 4
cases but not the conflict case: `mvn -U --metadata-update-policy daily`. With
the current dispatch logic, this produces `artifactPolicy=null` (not `always`).
That behavior should be explicit in a test — either asserting `null` (if
explicit-only is the documented semantic) or asserting `always` (if bridging is
restored). The revert removed the test that documented this case.
Add a 5th case:
```suggestion
// Mixed: legacy -U + explicit metadata policy → artifact policy
from legacy (-U = always)
request = new DefaultMavenExecutionRequest();
request.setLocalRepository(getLocalRepository());
request.setUpdateSnapshots(true); // -U
request.setMetadataUpdatePolicy(RepositoryPolicy.UPDATE_POLICY_DAILY);
session = systemSessionFactory.newRepositorySession(request);
// Document the actual behavior: explicit-only means -U is ignored
for artifacts
assertNull(session.getArtifactUpdatePolicy());
assertEquals(RepositoryPolicy.UPDATE_POLICY_DAILY,
session.getMetadataUpdatePolicy());
}
```
(Adjust the assertion to match whichever semantic you intend — the point is
to make it explicit and prevent silent regression.)
--
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]
