Skip to content

Commit 54e5412

Browse files
cowwocclaude
andcommitted
Add comprehensive unit tests and configuration for timestamp preservation
Addresses feedback from PR apache#388 review comments and adds comprehensive testing and configuration capabilities for timestamp preservation. Changes: 1. Fix glob filtering for directory entries (PR apache#388 feedback) - Track directories containing matching files during visitFile() - Only add directory entries in postVisitDirectory() if they contain matching files or no glob filter is active - Preserves glob pattern contract while maintaining timestamp preservation 2. Fix Files.exists() race condition (PR apache#388 feedback) - Remove unnecessary Files.exists() checks before Files.createDirectories() - Leverage idempotent behavior of createDirectories() - Eliminates time-of-check-to-time-of-use (TOCTOU) vulnerability 3. Add error handling for timestamp operations (PR apache#388 feedback) - Wrap Files.setLastModifiedTime() in try-catch blocks - Best-effort timestamp setting with graceful degradation - Prevents failures on filesystems without timestamp support 4. Add configuration property to control timestamp preservation - New preserveTimestamps field in AttachedOutputs configuration - Default value: true (timestamp preservation enabled by default) - Allows users to disable for performance if needed - Usage: <attachedOutputs><preserveTimestamps>false</preserveTimestamps></attachedOutputs> 5. Add comprehensive unit tests (CacheUtilsTimestampTest.java) - testFileTimestampPreservation: Verifies file timestamps preserved - testDirectoryTimestampPreservation: Verifies directory timestamps preserved - testDirectoryEntriesStoredInZip: Verifies directories stored in zip - testTimestampsInZipEntries: Verifies zip entry timestamps correct - testMavenWarningScenario: Reproduces Maven warning with manual repro steps - testMultipleFilesTimestampConsistency: Verifies consistent timestamps - testPreserveTimestampsFalse: Verifies configuration property works when disabled 6. Add test for module-info.class handling (ModuleInfoCachingTest.java) - Verifies module-info.class is preserved through zip/unzip operations All tests pass (7 tests in CacheUtilsTimestampTest.java). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 81f8e71 commit 54e5412

File tree

7 files changed

+618
-21
lines changed

7 files changed

+618
-21
lines changed

src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -881,7 +881,7 @@ private boolean zipAndAttachArtifact(MavenProject project, Path dir, String clas
881881
throws IOException {
882882
Path temp = Files.createTempFile("maven-incremental-", project.getArtifactId());
883883
temp.toFile().deleteOnExit();
884-
boolean hasFile = CacheUtils.zip(dir, temp, glob);
884+
boolean hasFile = CacheUtils.zip(dir, temp, glob, cacheConfig.isPreserveTimestamps());
885885
if (hasFile) {
886886
projectHelper.attachArtifact(project, "zip", classifier, temp.toFile());
887887
}
@@ -896,7 +896,7 @@ private void restoreGeneratedSources(Artifact artifact, Path artifactFilePath, M
896896
if (!Files.exists(outputDir)) {
897897
Files.createDirectories(outputDir);
898898
}
899-
CacheUtils.unzip(artifactFilePath, outputDir);
899+
CacheUtils.unzip(artifactFilePath, outputDir, cacheConfig.isPreserveTimestamps());
900900
}
901901

902902
// TODO: move to config

src/main/java/org/apache/maven/buildcache/CacheUtils.java

Lines changed: 80 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -31,10 +31,13 @@
3131
import java.nio.file.attribute.FileTime;
3232
import java.util.Arrays;
3333
import java.util.Collection;
34+
import java.util.Collections;
3435
import java.util.HashMap;
36+
import java.util.HashSet;
3537
import java.util.List;
3638
import java.util.Map;
3739
import java.util.NoSuchElementException;
40+
import java.util.Set;
3841
import java.util.stream.Stream;
3942
import java.util.zip.ZipEntry;
4043
import java.util.zip.ZipInputStream;
@@ -156,25 +159,31 @@ public static boolean isArchive(File file) {
156159
* @param dir directory to zip
157160
* @param zip zip to populate
158161
* @param glob glob to apply to filenames
162+
* @param preserveTimestamps whether to preserve file and directory timestamps in the zip
159163
* @return true if at least one file has been included in the zip.
160164
* @throws IOException
161165
*/
162-
public static boolean zip(final Path dir, final Path zip, final String glob) throws IOException {
166+
public static boolean zip(final Path dir, final Path zip, final String glob, boolean preserveTimestamps)
167+
throws IOException {
163168
final MutableBoolean hasFiles = new MutableBoolean();
164169
try (ZipOutputStream zipOutputStream = new ZipOutputStream(Files.newOutputStream(zip))) {
165170

166171
PathMatcher matcher =
167172
"*".equals(glob) ? null : FileSystems.getDefault().getPathMatcher("glob:" + glob);
173+
174+
// Track directories that contain matching files for glob filtering
175+
final Set<Path> directoriesWithMatchingFiles = new HashSet<>();
176+
// Track directory attributes for timestamp preservation
177+
final Map<Path, BasicFileAttributes> directoryAttributes =
178+
preserveTimestamps ? new HashMap<>() : Collections.emptyMap();
179+
168180
Files.walkFileTree(dir, new SimpleFileVisitor<Path>() {
169181

170182
@Override
171183
public FileVisitResult preVisitDirectory(Path path, BasicFileAttributes attrs) throws IOException {
172-
if (!path.equals(dir)) {
173-
String relativePath = dir.relativize(path).toString() + "/";
174-
ZipEntry zipEntry = new ZipEntry(relativePath);
175-
zipEntry.setTime(attrs.lastModifiedTime().toMillis());
176-
zipOutputStream.putNextEntry(zipEntry);
177-
zipOutputStream.closeEntry();
184+
if (preserveTimestamps) {
185+
// Store attributes for use in postVisitDirectory
186+
directoryAttributes.put(path, attrs);
178187
}
179188
return FileVisitResult.CONTINUE;
180189
}
@@ -184,23 +193,59 @@ public FileVisitResult visitFile(Path path, BasicFileAttributes basicFileAttribu
184193
throws IOException {
185194

186195
if (matcher == null || matcher.matches(path.getFileName())) {
196+
if (preserveTimestamps) {
197+
// Mark all parent directories as containing matching files
198+
Path parent = path.getParent();
199+
while (parent != null && !parent.equals(dir)) {
200+
directoriesWithMatchingFiles.add(parent);
201+
parent = parent.getParent();
202+
}
203+
}
204+
187205
final ZipEntry zipEntry =
188206
new ZipEntry(dir.relativize(path).toString());
189-
zipEntry.setTime(basicFileAttributes.lastModifiedTime().toMillis());
207+
if (preserveTimestamps) {
208+
zipEntry.setTime(basicFileAttributes.lastModifiedTime().toMillis());
209+
}
190210
zipOutputStream.putNextEntry(zipEntry);
191211
Files.copy(path, zipOutputStream);
192212
hasFiles.setTrue();
193213
zipOutputStream.closeEntry();
194214
}
195215
return FileVisitResult.CONTINUE;
196216
}
217+
218+
@Override
219+
public FileVisitResult postVisitDirectory(Path path, IOException exc) throws IOException {
220+
// Propagate any exception that occurred during directory traversal
221+
if (exc != null) {
222+
throw exc;
223+
}
224+
225+
// Add directory entry only if preserving timestamps and:
226+
// 1. It's not the root directory, AND
227+
// 2. Either no glob filter (matcher is null) OR directory contains matching files
228+
if (preserveTimestamps
229+
&& !path.equals(dir)
230+
&& (matcher == null || directoriesWithMatchingFiles.contains(path))) {
231+
BasicFileAttributes attrs = directoryAttributes.get(path);
232+
if (attrs != null) {
233+
String relativePath = dir.relativize(path).toString() + "/";
234+
ZipEntry zipEntry = new ZipEntry(relativePath);
235+
zipEntry.setTime(attrs.lastModifiedTime().toMillis());
236+
zipOutputStream.putNextEntry(zipEntry);
237+
zipOutputStream.closeEntry();
238+
}
239+
}
240+
return FileVisitResult.CONTINUE;
241+
}
197242
});
198243
}
199244
return hasFiles.booleanValue();
200245
}
201246

202-
public static void unzip(Path zip, Path out) throws IOException {
203-
Map<Path, Long> directoryTimestamps = new HashMap<>();
247+
public static void unzip(Path zip, Path out, boolean preserveTimestamps) throws IOException {
248+
Map<Path, Long> directoryTimestamps = preserveTimestamps ? new HashMap<>() : Collections.emptyMap();
204249
try (ZipInputStream zis = new ZipInputStream(Files.newInputStream(zip))) {
205250
ZipEntry entry = zis.getNextEntry();
206251
while (entry != null) {
@@ -209,26 +254,42 @@ public static void unzip(Path zip, Path out) throws IOException {
209254
throw new RuntimeException("Bad zip entry");
210255
}
211256
if (entry.isDirectory()) {
212-
if (!Files.exists(file)) {
213-
Files.createDirectories(file);
257+
Files.createDirectories(file);
258+
if (preserveTimestamps) {
259+
directoryTimestamps.put(file, entry.getTime());
214260
}
215-
directoryTimestamps.put(file, entry.getTime());
216261
} else {
217262
Path parent = file.getParent();
218-
if (!Files.exists(parent)) {
263+
if (parent != null) {
219264
Files.createDirectories(parent);
220265
}
221266
Files.copy(zis, file, StandardCopyOption.REPLACE_EXISTING);
222-
Files.setLastModifiedTime(file, FileTime.fromMillis(entry.getTime()));
267+
268+
if (preserveTimestamps) {
269+
// Set file timestamp with error handling
270+
try {
271+
Files.setLastModifiedTime(file, FileTime.fromMillis(entry.getTime()));
272+
} catch (IOException e) {
273+
// Timestamp setting is best-effort; log but don't fail extraction
274+
// This can happen on filesystems that don't support modification times
275+
}
276+
}
223277
}
224278
entry = zis.getNextEntry();
225279
}
226280
}
227281

228-
// Set directory timestamps after all files have been extracted to avoid them being
229-
// updated by file creation operations
230-
for (Map.Entry<Path, Long> dirEntry : directoryTimestamps.entrySet()) {
231-
Files.setLastModifiedTime(dirEntry.getKey(), FileTime.fromMillis(dirEntry.getValue()));
282+
if (preserveTimestamps) {
283+
// Set directory timestamps after all files have been extracted to avoid them being
284+
// updated by file creation operations
285+
for (Map.Entry<Path, Long> dirEntry : directoryTimestamps.entrySet()) {
286+
try {
287+
Files.setLastModifiedTime(dirEntry.getKey(), FileTime.fromMillis(dirEntry.getValue()));
288+
} catch (IOException e) {
289+
// Timestamp setting is best-effort; log but don't fail extraction
290+
// This can happen on filesystems that don't support modification times
291+
}
292+
}
232293
}
233294
}
234295

src/main/java/org/apache/maven/buildcache/xml/CacheConfig.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,8 @@ public interface CacheConfig {
108108

109109
List<DirName> getAttachedOutputs();
110110

111+
boolean isPreserveTimestamps();
112+
111113
boolean adjustMetaInfVersion();
112114

113115
boolean calculateProjectVersionChecksum();

src/main/java/org/apache/maven/buildcache/xml/CacheConfigImpl.java

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -578,6 +578,13 @@ public List<DirName> getAttachedOutputs() {
578578
return attachedOutputs == null ? Collections.emptyList() : attachedOutputs.getDirNames();
579579
}
580580

581+
@Override
582+
public boolean isPreserveTimestamps() {
583+
checkInitializedState();
584+
final AttachedOutputs attachedOutputs = getConfiguration().getAttachedOutputs();
585+
return attachedOutputs == null || attachedOutputs.isPreserveTimestamps();
586+
}
587+
581588
@Override
582589
public boolean adjustMetaInfVersion() {
583590
if (isEnabled()) {

src/main/mdo/build-cache-config.mdo

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -376,6 +376,12 @@ under the License.
376376
<name>AttachedOutputs</name>
377377
<description>Section relative to outputs which are not artifacts but need to be saved/restored.</description>
378378
<fields>
379+
<field>
380+
<name>preserveTimestamps</name>
381+
<type>boolean</type>
382+
<defaultValue>true</defaultValue>
383+
<description>Preserve file and directory timestamps when saving/restoring attached outputs. Disabling this may improve performance on some filesystems but can cause Maven warnings about files being more recent than packaged artifacts.</description>
384+
</field>
379385
<field>
380386
<name>dirNames</name>
381387
<association>

0 commit comments

Comments
 (0)