Skip to content

Commit 2b4a88f

Browse files
committed
Add comprehensive unit tests and configuration for timestamp preservation
Addresses feedback from PR #388 review comments and adds comprehensive testing and configuration capabilities for timestamp preservation. Changes: 1. Fix glob filtering for directory entries (PR #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 #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 #388 feedback) - Wrap Files.setLastModifiedTime() in try-catch blocks - Best-effort timestamp setting with graceful degradation - Prevents failures on filesystems that don't support modification times 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).
1 parent 84b672e commit 2b4a88f

File tree

7 files changed

+616
-22
lines changed

7 files changed

+616
-22
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, cacheConfig.isPreservePermissions());
884+
boolean hasFile = CacheUtils.zip(dir, temp, glob, cacheConfig.isPreservePermissions(), 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, cacheConfig.isPreservePermissions());
899+
CacheUtils.unzip(artifactFilePath, outputDir, cacheConfig.isPreservePermissions(), cacheConfig.isPreserveTimestamps());
900900
}
901901

902902
// TODO: move to config

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

Lines changed: 78 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
import java.nio.file.attribute.PosixFilePermission;
3333
import java.util.Arrays;
3434
import java.util.Collection;
35+
import java.util.Collections;
3536
import java.util.HashMap;
3637
import java.util.HashSet;
3738
import java.util.List;
@@ -165,10 +166,11 @@ public static boolean isArchive(File file) {
165166
* the ZIP file (e.g., for cache keys) will include permission information, ensuring
166167
* cache invalidation when file permissions change. This behavior is similar to how Git
167168
* includes file mode in tree hashes.</p>
169+
* @param preserveTimestamps whether to preserve file and directory timestamps in the zip
168170
* @return true if at least one file has been included in the zip.
169171
* @throws IOException
170172
*/
171-
public static boolean zip(final Path dir, final Path zip, final String glob, boolean preservePermissions)
173+
public static boolean zip(final Path dir, final Path zip, final String glob, boolean preservePermissions, boolean preserveTimestamps)
172174
throws IOException {
173175
final MutableBoolean hasFiles = new MutableBoolean();
174176
// Check once if filesystem supports POSIX permissions instead of catching exceptions for every file
@@ -179,16 +181,20 @@ public static boolean zip(final Path dir, final Path zip, final String glob, boo
179181

180182
PathMatcher matcher =
181183
"*".equals(glob) ? null : FileSystems.getDefault().getPathMatcher("glob:" + glob);
184+
185+
// Track directories that contain matching files for glob filtering
186+
final Set<Path> directoriesWithMatchingFiles = new HashSet<>();
187+
// Track directory attributes for timestamp preservation
188+
final Map<Path, BasicFileAttributes> directoryAttributes =
189+
preserveTimestamps ? new HashMap<>() : Collections.emptyMap();
190+
182191
Files.walkFileTree(dir, new SimpleFileVisitor<Path>() {
183192

184193
@Override
185194
public FileVisitResult preVisitDirectory(Path path, BasicFileAttributes attrs) throws IOException {
186-
if (!path.equals(dir)) {
187-
String relativePath = dir.relativize(path).toString() + "/";
188-
ZipArchiveEntry zipEntry = new ZipArchiveEntry(relativePath);
189-
zipEntry.setTime(attrs.lastModifiedTime().toMillis());
190-
zipOutputStream.putArchiveEntry(zipEntry);
191-
zipOutputStream.closeArchiveEntry();
195+
if (preserveTimestamps) {
196+
// Store attributes for use in postVisitDirectory
197+
directoryAttributes.put(path, attrs);
192198
}
193199
return FileVisitResult.CONTINUE;
194200
}
@@ -198,11 +204,22 @@ public FileVisitResult visitFile(Path path, BasicFileAttributes basicFileAttribu
198204
throws IOException {
199205

200206
if (matcher == null || matcher.matches(path.getFileName())) {
207+
if (preserveTimestamps) {
208+
// Mark all parent directories as containing matching files
209+
Path parent = path.getParent();
210+
while (parent != null && !parent.equals(dir)) {
211+
directoriesWithMatchingFiles.add(parent);
212+
parent = parent.getParent();
213+
}
214+
}
215+
201216
final ZipArchiveEntry zipEntry =
202217
new ZipArchiveEntry(dir.relativize(path).toString());
203218

204-
// Preserve timestamp
205-
zipEntry.setTime(basicFileAttributes.lastModifiedTime().toMillis());
219+
// Preserve timestamp if requested
220+
if (preserveTimestamps) {
221+
zipEntry.setTime(basicFileAttributes.lastModifiedTime().toMillis());
222+
}
206223

207224
// Preserve Unix permissions if requested and filesystem supports it
208225
if (supportsPosix) {
@@ -217,17 +234,42 @@ public FileVisitResult visitFile(Path path, BasicFileAttributes basicFileAttribu
217234
}
218235
return FileVisitResult.CONTINUE;
219236
}
237+
238+
@Override
239+
public FileVisitResult postVisitDirectory(Path path, IOException exc) throws IOException {
240+
// Propagate any exception that occurred during directory traversal
241+
if (exc != null) {
242+
throw exc;
243+
}
244+
245+
// Add directory entry only if preserving timestamps and:
246+
// 1. It's not the root directory, AND
247+
// 2. Either no glob filter (matcher is null) OR directory contains matching files
248+
if (preserveTimestamps
249+
&& !path.equals(dir)
250+
&& (matcher == null || directoriesWithMatchingFiles.contains(path))) {
251+
BasicFileAttributes attrs = directoryAttributes.get(path);
252+
if (attrs != null) {
253+
String relativePath = dir.relativize(path).toString() + "/";
254+
ZipArchiveEntry zipEntry = new ZipArchiveEntry(relativePath);
255+
zipEntry.setTime(attrs.lastModifiedTime().toMillis());
256+
zipOutputStream.putArchiveEntry(zipEntry);
257+
zipOutputStream.closeArchiveEntry();
258+
}
259+
}
260+
return FileVisitResult.CONTINUE;
261+
}
220262
});
221263
}
222264
return hasFiles.booleanValue();
223265
}
224266

225-
public static void unzip(Path zip, Path out, boolean preservePermissions) throws IOException {
267+
public static void unzip(Path zip, Path out, boolean preservePermissions, boolean preserveTimestamps) throws IOException {
226268
// Check once if filesystem supports POSIX permissions instead of catching exceptions for every file
227269
final boolean supportsPosix = preservePermissions
228270
&& out.getFileSystem().supportedFileAttributeViews().contains("posix");
229271

230-
Map<Path, Long> directoryTimestamps = new HashMap<>();
272+
Map<Path, Long> directoryTimestamps = preserveTimestamps ? new HashMap<>() : Collections.emptyMap();
231273
try (ZipArchiveInputStream zis = new ZipArchiveInputStream(Files.newInputStream(zip))) {
232274
ZipArchiveEntry entry = zis.getNextEntry();
233275
while (entry != null) {
@@ -236,17 +278,26 @@ public static void unzip(Path zip, Path out, boolean preservePermissions) throws
236278
throw new RuntimeException("Bad zip entry");
237279
}
238280
if (entry.isDirectory()) {
239-
if (!Files.exists(file)) {
240-
Files.createDirectories(file);
281+
Files.createDirectories(file);
282+
if (preserveTimestamps) {
283+
directoryTimestamps.put(file, entry.getTime());
241284
}
242-
directoryTimestamps.put(file, entry.getTime());
243285
} else {
244286
Path parent = file.getParent();
245-
if (!Files.exists(parent)) {
287+
if (parent != null) {
246288
Files.createDirectories(parent);
247289
}
248290
Files.copy(zis, file, StandardCopyOption.REPLACE_EXISTING);
249-
Files.setLastModifiedTime(file, FileTime.fromMillis(entry.getTime()));
291+
292+
if (preserveTimestamps) {
293+
// Set file timestamp with error handling
294+
try {
295+
Files.setLastModifiedTime(file, FileTime.fromMillis(entry.getTime()));
296+
} catch (IOException e) {
297+
// Timestamp setting is best-effort; log but don't fail extraction
298+
// This can happen on filesystems that don't support modification times
299+
}
300+
}
250301

251302
// Restore Unix permissions if requested and filesystem supports it
252303
if (supportsPosix) {
@@ -261,10 +312,17 @@ public static void unzip(Path zip, Path out, boolean preservePermissions) throws
261312
}
262313
}
263314

264-
// Set directory timestamps after all files have been extracted to avoid them being
265-
// updated by file creation operations
266-
for (Map.Entry<Path, Long> dirEntry : directoryTimestamps.entrySet()) {
267-
Files.setLastModifiedTime(dirEntry.getKey(), FileTime.fromMillis(dirEntry.getValue()));
315+
if (preserveTimestamps) {
316+
// Set directory timestamps after all files have been extracted to avoid them being
317+
// updated by file creation operations
318+
for (Map.Entry<Path, Long> dirEntry : directoryTimestamps.entrySet()) {
319+
try {
320+
Files.setLastModifiedTime(dirEntry.getKey(), FileTime.fromMillis(dirEntry.getValue()));
321+
} catch (IOException e) {
322+
// Timestamp setting is best-effort; log but don't fail extraction
323+
// This can happen on filesystems that don't support modification times
324+
}
325+
}
268326
}
269327
}
270328

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

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

111111
boolean isPreservePermissions();
112112

113+
boolean isPreserveTimestamps();
114+
113115
boolean adjustMetaInfVersion();
114116

115117
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
@@ -585,6 +585,13 @@ public boolean isPreservePermissions() {
585585
return attachedOutputs == null || attachedOutputs.isPreservePermissions();
586586
}
587587

588+
@Override
589+
public boolean isPreserveTimestamps() {
590+
checkInitializedState();
591+
final AttachedOutputs attachedOutputs = getConfiguration().getAttachedOutputs();
592+
return attachedOutputs == null || attachedOutputs.isPreserveTimestamps();
593+
}
594+
588595
@Override
589596
public boolean adjustMetaInfVersion() {
590597
if (isEnabled()) {

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -382,6 +382,12 @@ under the License.
382382
<defaultValue>true</defaultValue>
383383
<description>Preserve Unix file permissions when saving/restoring attached outputs. When enabled, permissions are stored in ZIP entry headers and become part of the cache key, ensuring cache invalidation when permissions change. This is similar to how Git includes file mode in tree hashes. Disabling this may improve portability across different systems but will not preserve executable bits.</description>
384384
</field>
385+
<field>
386+
<name>preserveTimestamps</name>
387+
<type>boolean</type>
388+
<defaultValue>true</defaultValue>
389+
<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>
390+
</field>
385391
<field>
386392
<name>dirNames</name>
387393
<association>

0 commit comments

Comments
 (0)