gnodet-bot commented on code in PR #11405:
URL: https://github.com/apache/maven/pull/11405#discussion_r4256304613
##########
api/maven-api-core/src/main/java/org/apache/maven/api/Constants.java:
##########
@@ -868,5 +868,17 @@ public final class Constants {
@Config(type = "java.lang.Boolean", defaultValue = "false")
public static final String MAVEN_MODEL_DEPENDENCY_INTERPOLATION_FULL =
"maven.model.dependencyInterpolation.full";
+ /**
+ * System property to keep temp material for diagnostics.
+ * When set to {@code true} via {@code -Dmaven.tempfile.keep=true},
temporary files and directories
+ * created by {@link org.apache.maven.api.services.TempFileService} are
not deleted at session end.
+ * This is a JVM system property only (read via {@link
Boolean#getBoolean(String)});
+ * it is not resolved through Maven's property mechanism (pom.xml,
profiles, settings.xml).
+ *
+ * @since 4.1.0
+ */
+ @Config(type = "java.lang.Boolean", defaultValue = "false")
Review Comment:
⚠️ **`@Config` without `source = Config.Source.SYSTEM_PROPERTIES`
contradicts the Javadoc**
The Javadoc states this property is "not resolved through Maven's property
mechanism (pom.xml, profiles, settings.xml)" — but `@Config` with the default
`source = USER_PROPERTIES` signals to Maven's config tooling that users CAN set
it through those mechanisms. In practice, anything set via
`pom.xml`/`settings.xml` goes through Maven's property merger (not
`System.setProperty()`), so `Boolean.getBoolean()` silently ignores it.
Choose one:
1. **Make it truly Maven-property-configurable** by replacing
`Boolean.getBoolean(MAVEN_TEMPFILE_KEEP)` with session property resolution
(e.g.
`Boolean.parseBoolean(session.getUserProperties().get(MAVEN_TEMPFILE_KEEP))`) —
and update the Javadoc accordingly.
2. **Keep it a JVM-only `-D` flag** by removing `@Config` (since it's not
participting in Maven's config resolution) or adding `source =
Config.Source.SYSTEM_PROPERTIES` + `readOnly = false` if a SYSTEM_PROPERTIES
source without readOnly is appropriate here.
```suggestion
@Config(type = "java.lang.Boolean", defaultValue = "false", source =
Config.Source.USER_PROPERTIES)
public static final String MAVEN_TEMPFILE_KEEP = "maven.tempfile.keep";
```
Note: the suggestion above keeps the annotation but makes the source
explicit — the real fix is aligning the implementation with the declared source.
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/TempFileService.java:
##########
@@ -0,0 +1,75 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.api.services;
+
+import java.io.IOException;
+import java.nio.file.Path;
+
+import org.apache.maven.api.Service;
+import org.apache.maven.api.Session;
+import org.apache.maven.api.annotations.Experimental;
+import org.apache.maven.api.annotations.Nonnull;
+import org.apache.maven.api.annotations.Nullable;
+
+/**
+ * Service to create and track temporary files/directories for a Maven build.
+ * All created paths are deleted automatically when the session ends.
+ *
+ * @since 4.1.0
+ */
+@Experimental
+public interface TempFileService extends Service {
+
+ /**
+ * Creates a temp file in the default temp directory.
+ */
Review Comment:
📝 **Missing `@throws` on all four factory methods and `cleanup()`**
Public API methods that declare `throws IOException` must document it.
Without `@throws`, callers have no Javadoc guidance on when the exception is
thrown. Same applies to the other three `create*` methods and `cleanup()`.
Example fix for all five:
```suggestion
/**
* Creates a temp file in the default temp directory.
*
* @throws IOException if the file cannot be created
*/
```
##########
impl/maven-core/src/main/java/org/apache/maven/internal/impl/TempFileCleanupParticipant.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.internal.impl;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import org.apache.maven.AbstractMavenLifecycleParticipant;
+import org.apache.maven.api.Session;
+import org.apache.maven.api.services.TempFileService;
+import org.apache.maven.execution.MavenSession;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Hooks into the Maven lifecycle and removes all temp material after the
session.
+ */
+@Named
+@Singleton
+public final class TempFileCleanupParticipant extends
AbstractMavenLifecycleParticipant {
+
+ private static final Logger LOGGER =
LoggerFactory.getLogger(TempFileCleanupParticipant.class);
+
+ private final TempFileService tempFileService;
+
+ @Inject
+ public TempFileCleanupParticipant(final TempFileService tempFileService) {
+ this.tempFileService = tempFileService;
+ }
+
+ @Override
+ public void afterSessionEnd(final MavenSession mavenSession) {
+ // Bridge to the API Session (available in Maven 4).
+ final Session apiSession = mavenSession.getSession();
+ try {
+ tempFileService.cleanup(apiSession);
+ } catch (final Exception e) {
+ // We're at session end; just log. Maven already reported build
result.
+ LOGGER.warn("Temp cleanup failed: {}", e.getMessage());
Review Comment:
🐛 **Stack trace lost; catch is too broad**
Two problems:
1. `e.getMessage()` logs only the exception message, discarding the full
stack trace. In production, when temp cleanup fails silently at session end,
the message alone ("Temp cleanup failed: null" for NPE, or a terse IOException
message) makes root-cause diagnosis impossible. SLF4J supports logging the full
exception: `LOGGER.warn("Temp cleanup failed", e)` (no `{}` placeholder — SLF4J
appends the throwable automatically).
2. `cleanup()` only declares `throws IOException`. Catching `Exception`
broadly hides any unexpected `RuntimeException` (NPE, `IllegalStateException`)
under the same generic log message.
```suggestion
} catch (final IOException e) {
// We're at session end; just log. Maven already reported build
result.
LOGGER.warn("Temp cleanup failed", e);
}
```
##########
impl/maven-core/src/test/java/org/apache/maven/internal/impl/DefaultTempFileServiceTest.java:
##########
@@ -0,0 +1,237 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.internal.impl;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.Objects;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Supplier;
+
+import org.apache.maven.api.Constants;
+import org.apache.maven.api.Session;
+import org.apache.maven.api.SessionData;
+import org.apache.maven.api.services.TempFileService;
+import org.apache.maven.execution.MavenSession;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+/**
+ * Tests for DefaultTempFileService and TempFileCleanupParticipant.
+ */
+class DefaultTempFileServiceTest {
+
+ @AfterEach
+ void clearKeepProp() {
+ System.clearProperty(Constants.MAVEN_TEMPFILE_KEEP);
+ }
+
+ @Test
+ void createsFilesAndDirectoriesAndCleansThemUp() throws IOException {
+ final TempFileService svc = new DefaultTempFileService();
+ final SessionData data = new MapBackedSessionData();
+ final Session session = mock(Session.class);
+ when(session.getData()).thenReturn(data);
+
+ final Path f1 = svc.createTempFile(session, "tfs-", ".bin");
+ final Path d1 = svc.createTempDirectory(session, "tfs-");
+ final Path nested = Files.createTempFile(d1, "inner-", ".tmp");
+
+ assertTrue(Files.exists(f1), "temp file must exist");
+ assertTrue(Files.exists(d1), "temp dir must exist");
+ assertTrue(Files.exists(nested), "nested file must exist");
+
+ svc.cleanup(session);
+
+ assertFalse(Files.exists(f1), "temp file must be deleted");
+ assertFalse(Files.exists(d1), "temp dir tree must be deleted");
+ assertFalse(Files.exists(nested), "nested file must be deleted");
+ }
+
+ @Test
+ void registerExternalPathIsAlsoDeleted() throws IOException {
+ final TempFileService svc = new DefaultTempFileService();
+ final SessionData data = new MapBackedSessionData();
+ final Session session = mock(Session.class);
+ when(session.getData()).thenReturn(data);
+
+ final Path externalDir = Files.createTempDirectory("ext-");
+ final Path nested = Files.createTempFile(externalDir, "ext-inner-",
".tmp");
+ assertTrue(Files.exists(externalDir));
+ assertTrue(Files.exists(nested));
+
+ svc.register(session, externalDir);
+ svc.cleanup(session);
+
+ assertFalse(Files.exists(externalDir), "registered external dir must
be deleted recursively");
+ assertFalse(Files.exists(nested));
+ }
+
+ @Test
+ void createsFileInSpecifiedDirectory() throws IOException {
+ final TempFileService svc = new DefaultTempFileService();
+ final SessionData data = new MapBackedSessionData();
+ final Session session = mock(Session.class);
+ when(session.getData()).thenReturn(data);
+
+ final Path customDir = Files.createTempDirectory("custom-dir-");
+ try {
+ final Path f = svc.createTempFile(session, "dir-", ".tmp",
customDir);
+ assertTrue(Files.exists(f), "temp file must exist");
+ assertTrue(f.startsWith(customDir), "temp file must be inside the
specified directory");
+
+ svc.cleanup(session);
+
+ assertFalse(Files.exists(f), "temp file must be deleted");
+ } finally {
+ Files.deleteIfExists(customDir);
+ }
+ }
+
+ @Test
+ void createsDirectoryInSpecifiedParent() throws IOException {
+ final TempFileService svc = new DefaultTempFileService();
+ final SessionData data = new MapBackedSessionData();
+ final Session session = mock(Session.class);
+ when(session.getData()).thenReturn(data);
+
+ final Path parentDir = Files.createTempDirectory("parent-dir-");
+ try {
+ final Path d = svc.createTempDirectory(session, "nested-",
parentDir);
+ assertTrue(Files.exists(d), "temp directory must exist");
+ assertTrue(d.startsWith(parentDir), "temp directory must be inside
the specified parent");
+
+ svc.cleanup(session);
+
+ assertFalse(Files.exists(d), "temp directory must be deleted");
+ } finally {
+ Files.deleteIfExists(parentDir);
+ }
+ }
+
+ @Test
+ void keepPropertySkipsCleanup() throws IOException {
+ final TempFileService svc = new DefaultTempFileService();
+ final SessionData data = new MapBackedSessionData();
+ final Session session = mock(Session.class);
+ when(session.getData()).thenReturn(data);
+
+ final Path f = svc.createTempFile(session, "keep-", ".tmp");
+ assertTrue(Files.exists(f));
+
+ System.setProperty(Constants.MAVEN_TEMPFILE_KEEP, "true");
+ svc.cleanup(session);
+
+ assertTrue(Files.exists(f), "cleanup must be skipped when
-Dmaven.tempfile.keep=true");
+
+ // turn cleanup back on and verify it deletes
+ System.clearProperty(Constants.MAVEN_TEMPFILE_KEEP);
+ svc.cleanup(session);
+ assertFalse(Files.exists(f));
+ }
+
+ @Test
+ void cleanupParticipantDelegatesCleanupToService() throws IOException {
+ final TempFileService svc = new DefaultTempFileService();
+ final SessionData data = new MapBackedSessionData();
+ final Session apiSession = mock(Session.class);
+ when(apiSession.getData()).thenReturn(data);
+
+ final Path f = svc.createTempFile(apiSession, "participant-", ".tmp");
Review Comment:
⚠️ **`System.setProperty` without local try-finally — parallel execution
risk**
`@AfterEach` mitigates sequential pollution but does not protect against
parallel test execution (JUnit 5 allows it). If `svc.cleanup(session)` on line
164 throws, `@AfterEach` still runs — but if another test starts between the
`setProperty` and the `@AfterEach` cleanup, it will see the mutated property.
Guard with a `try-finally` inside the test method:
```suggestion
System.setProperty(Constants.MAVEN_TEMPFILE_KEEP, "true");
try {
svc.cleanup(session);
assertTrue(Files.exists(f), "cleanup must be skipped when
-Dmaven.tempfile.keep=true");
} finally {
System.clearProperty(Constants.MAVEN_TEMPFILE_KEEP);
}
```
--
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]
