Skip to content

Commit 3d534ee

Browse files
committed
Add preservePermissions config and include permissions in cache checksum
When preservePermissions=true (default), Unix file permissions are stored in ZIP entry headers, making them part of the ZIP file's binary content. This ensures that hashing the ZIP file (for cache keys) includes permission information, providing proper cache invalidation when file permissions change. This addresses the architectural correctness concern raised in PR apache#392 review: permissions should be in the checksum if they're being preserved, similar to how Git includes file mode in tree hashes. Changes: - Added preservePermissions boolean parameter to CacheUtils.zip() and unzip() - Added preservePermissions config option to build-cache-config.mdo (default: true) - Added isPreservePermissions() method to CacheConfig interface and implementation - Updated CacheControllerImpl to pass preservePermissions config to zip/unzip - Made permission preservation conditional based on the config flag - Fixed permissionsToMode() to include file type bits (0100000 for regular files) - Added comprehensive tests: * testPermissionsAffectFileHashWhenEnabled() - Verifies different permissions produce different ZIP hashes when preservation is enabled * testPermissionsDoNotAffectHashWhenDisabled() - Verifies permissions are NOT preserved when the flag is disabled - Added JavaDoc explaining that permissions become part of cache checksum Behavior: - preservePermissions=true: Permissions stored in ZIP, affect cache key (default) - preservePermissions=false: Permissions not preserved, files use system umask This is analogous to Git's treatment of file mode in tree hashes, ensuring that metadata included in cache restoration is also part of cache key computation. Optimize filesystem capability check to avoid repeated exceptions Addresses reviewer feedback: check once if filesystem supports POSIX file attributes instead of throwing and catching UnsupportedOperationException for every file during zip/unzip operations. Changes: - Added single filesystem capability check at the beginning of zip() and unzip() - Removed try-catch blocks from inside the file iteration loops - Improved performance on non-POSIX filesystems (e.g., Windows) Before: UnsupportedOperationException thrown and caught for every file After: Single supportedFileAttributeViews() check per zip/unzip operation This is more efficient and cleaner code while maintaining identical behavior.
1 parent 298f064 commit 3d534ee

File tree

6 files changed

+209
-17
lines changed

6 files changed

+209
-17
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.isPreservePermissions());
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.isPreservePermissions());
900900
}
901901

902902
// TODO: move to config

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

Lines changed: 28 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -159,11 +159,21 @@ public static boolean isArchive(File file) {
159159
* @param dir directory to zip
160160
* @param zip zip to populate
161161
* @param glob glob to apply to filenames
162+
* @param preservePermissions whether to preserve Unix file permissions in the zip.
163+
* <p><b>Important:</b> When {@code true}, permissions are stored in ZIP entry headers,
164+
* which means they become part of the ZIP file's binary content. As a result, hashing
165+
* the ZIP file (e.g., for cache keys) will include permission information, ensuring
166+
* cache invalidation when file permissions change. This behavior is similar to how Git
167+
* includes file mode in tree hashes.</p>
162168
* @return true if at least one file has been included in the zip.
163169
* @throws IOException
164170
*/
165-
public static boolean zip(final Path dir, final Path zip, final String glob) throws IOException {
171+
public static boolean zip(final Path dir, final Path zip, final String glob, boolean preservePermissions) throws IOException {
166172
final MutableBoolean hasFiles = new MutableBoolean();
173+
// Check once if filesystem supports POSIX permissions instead of catching exceptions for every file
174+
final boolean supportsPosix = preservePermissions
175+
&& dir.getFileSystem().supportedFileAttributeViews().contains("posix");
176+
167177
try (ZipArchiveOutputStream zipOutputStream = new ZipArchiveOutputStream(Files.newOutputStream(zip))) {
168178

169179
PathMatcher matcher =
@@ -178,12 +188,10 @@ public FileVisitResult visitFile(Path path, BasicFileAttributes basicFileAttribu
178188
final ZipArchiveEntry zipEntry =
179189
new ZipArchiveEntry(dir.relativize(path).toString());
180190

181-
// Preserve Unix permissions if available
182-
try {
191+
// Preserve Unix permissions if requested and filesystem supports it
192+
if (supportsPosix) {
183193
Set<PosixFilePermission> permissions = Files.getPosixFilePermissions(path);
184194
zipEntry.setUnixMode(permissionsToMode(permissions));
185-
} catch (UnsupportedOperationException e) {
186-
// Not a POSIX filesystem, permissions not available
187195
}
188196

189197
zipOutputStream.putArchiveEntry(zipEntry);
@@ -198,7 +206,11 @@ public FileVisitResult visitFile(Path path, BasicFileAttributes basicFileAttribu
198206
return hasFiles.booleanValue();
199207
}
200208

201-
public static void unzip(Path zip, Path out) throws IOException {
209+
public static void unzip(Path zip, Path out, boolean preservePermissions) throws IOException {
210+
// Check once if filesystem supports POSIX permissions instead of catching exceptions for every file
211+
final boolean supportsPosix = preservePermissions
212+
&& out.getFileSystem().supportedFileAttributeViews().contains("posix");
213+
202214
try (ZipArchiveInputStream zis = new ZipArchiveInputStream(Files.newInputStream(zip))) {
203215
ZipArchiveEntry entry = zis.getNextEntry();
204216
while (entry != null) {
@@ -215,14 +227,12 @@ public static void unzip(Path zip, Path out) throws IOException {
215227
}
216228
Files.setLastModifiedTime(file, FileTime.fromMillis(entry.getTime()));
217229

218-
// Restore Unix permissions if available
219-
int unixMode = entry.getUnixMode();
220-
if (unixMode != 0) {
221-
try {
230+
// Restore Unix permissions if requested and filesystem supports it
231+
if (supportsPosix) {
232+
int unixMode = entry.getUnixMode();
233+
if (unixMode != 0) {
222234
Set<PosixFilePermission> permissions = modeToPermissions(unixMode);
223235
Files.setPosixFilePermissions(file, permissions);
224-
} catch (UnsupportedOperationException e) {
225-
// Not a POSIX filesystem, cannot set permissions
226236
}
227237
}
228238

@@ -248,10 +258,12 @@ public static <T> void debugPrintCollection(
248258
* Convert POSIX file permissions to Unix mode integer.
249259
*
250260
* @param permissions POSIX file permissions
251-
* @return Unix mode as integer (e.g., 0755)
261+
* @return Unix mode as integer (e.g., {@code 0100755} for regular file with rwxr-xr-x)
252262
*/
253263
private static int permissionsToMode(Set<PosixFilePermission> permissions) {
254-
int mode = 0;
264+
// Start with regular file type (0100000 in octal)
265+
int mode = 0100000;
266+
255267
if (permissions.contains(PosixFilePermission.OWNER_READ)) {
256268
mode |= 0400;
257269
}
@@ -285,11 +297,12 @@ private static int permissionsToMode(Set<PosixFilePermission> permissions) {
285297
/**
286298
* Convert Unix mode integer to POSIX file permissions.
287299
*
288-
* @param mode Unix mode (e.g., 0755)
300+
* @param mode Unix mode (e.g., {@code 0100755} for regular file with rwxr-xr-x)
289301
* @return Set of POSIX file permissions
290302
*/
291303
private static Set<PosixFilePermission> modeToPermissions(int mode) {
292304
Set<PosixFilePermission> permissions = new HashSet<>();
305+
// Extract permission bits (lower 9 bits), ignoring file type bits
293306
if ((mode & 0400) != 0) {
294307
permissions.add(PosixFilePermission.OWNER_READ);
295308
}

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 isPreservePermissions();
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 isPreservePermissions() {
583+
checkInitializedState();
584+
final AttachedOutputs attachedOutputs = getConfiguration().getAttachedOutputs();
585+
return attachedOutputs == null || attachedOutputs.isPreservePermissions();
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>preservePermissions</name>
381+
<type>boolean</type>
382+
<defaultValue>true</defaultValue>
383+
<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>
384+
</field>
379385
<field>
380386
<name>dirNames</name>
381387
<association>
Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,164 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
package org.apache.maven.buildcache;
20+
21+
import org.junit.jupiter.api.Test;
22+
import org.junit.jupiter.api.io.TempDir;
23+
24+
import java.io.IOException;
25+
import java.io.OutputStream;
26+
import java.nio.charset.StandardCharsets;
27+
import java.nio.file.Files;
28+
import java.nio.file.Path;
29+
import java.nio.file.attribute.PosixFilePermission;
30+
import java.nio.file.attribute.PosixFilePermissions;
31+
import java.util.Arrays;
32+
import java.util.Set;
33+
34+
import static org.junit.jupiter.api.Assertions.assertFalse;
35+
import static org.junit.jupiter.api.Assertions.assertTrue;
36+
37+
/**
38+
* Tests for permission preservation in CacheUtils.zip() and CacheUtils.unzip() methods.
39+
* These tests verify that Unix file permissions affect ZIP file hashes when preservation
40+
* is enabled, and do not affect hashes when disabled.
41+
*/
42+
class CacheUtilsPermissionsTest {
43+
44+
@TempDir
45+
Path tempDir;
46+
47+
/**
48+
* Tests that ZIP file hash changes when permissions change (when preservePermissions=true).
49+
* This ensures that the cache invalidates when file permissions change, maintaining
50+
* cache correctness similar to how Git includes file mode in tree hashes.
51+
*/
52+
@Test
53+
void testPermissionsAffectFileHashWhenEnabled() throws IOException {
54+
// Skip test on non-POSIX filesystems (e.g., Windows)
55+
if (!tempDir.getFileSystem().supportedFileAttributeViews().contains("posix")) {
56+
return;
57+
}
58+
59+
// Given: Same directory content with different permissions
60+
Path sourceDir1 = tempDir.resolve("source1");
61+
Files.createDirectories(sourceDir1);
62+
Path file1 = sourceDir1.resolve("script.sh");
63+
writeString(file1, "#!/bin/bash\necho hello");
64+
65+
// Set executable permissions (755)
66+
Set<PosixFilePermission> execPermissions = PosixFilePermissions.fromString("rwxr-xr-x");
67+
Files.setPosixFilePermissions(file1, execPermissions);
68+
69+
// Create second directory with identical content but different permissions
70+
Path sourceDir2 = tempDir.resolve("source2");
71+
Files.createDirectories(sourceDir2);
72+
Path file2 = sourceDir2.resolve("script.sh");
73+
writeString(file2, "#!/bin/bash\necho hello"); // Identical content
74+
75+
// Set non-executable permissions (644)
76+
Set<PosixFilePermission> normalPermissions = PosixFilePermissions.fromString("rw-r--r--");
77+
Files.setPosixFilePermissions(file2, normalPermissions);
78+
79+
// When: Create ZIP files with preservePermissions=true
80+
Path zip1 = tempDir.resolve("cache1.zip");
81+
Path zip2 = tempDir.resolve("cache2.zip");
82+
CacheUtils.zip(sourceDir1, zip1, "*", true);
83+
CacheUtils.zip(sourceDir2, zip2, "*", true);
84+
85+
// Then: ZIP files should have different hashes despite identical content
86+
byte[] hash1 = Files.readAllBytes(zip1);
87+
byte[] hash2 = Files.readAllBytes(zip2);
88+
89+
boolean hashesAreDifferent = !Arrays.equals(hash1, hash2);
90+
assertTrue(hashesAreDifferent,
91+
"ZIP files with same content but different permissions should have different hashes " +
92+
"when preservePermissions=true. This ensures cache invalidation when permissions change " +
93+
"(executable vs non-executable files).");
94+
}
95+
96+
/**
97+
* Tests that ZIP file hash does NOT significantly vary when permissions change but
98+
* preservePermissions=false. While ZIP timestamps may still cause minor differences,
99+
* the key point is that permission information is NOT deterministically stored.
100+
*/
101+
@Test
102+
void testPermissionsDoNotAffectHashWhenDisabled() throws IOException {
103+
// Skip test on non-POSIX filesystems (e.g., Windows)
104+
if (!tempDir.getFileSystem().supportedFileAttributeViews().contains("posix")) {
105+
return;
106+
}
107+
108+
// Given: Same directory content with different permissions
109+
Path sourceDir1 = tempDir.resolve("source1");
110+
Files.createDirectories(sourceDir1);
111+
Path file1 = sourceDir1.resolve("script.sh");
112+
writeString(file1, "#!/bin/bash\necho hello");
113+
114+
// Set executable permissions (755)
115+
Set<PosixFilePermission> execPermissions = PosixFilePermissions.fromString("rwxr-xr-x");
116+
Files.setPosixFilePermissions(file1, execPermissions);
117+
118+
// Create second directory with identical content but different permissions
119+
Path sourceDir2 = tempDir.resolve("source2");
120+
Files.createDirectories(sourceDir2);
121+
Path file2 = sourceDir2.resolve("script.sh");
122+
writeString(file2, "#!/bin/bash\necho hello"); // Identical content
123+
124+
// Set non-executable permissions (644)
125+
Set<PosixFilePermission> normalPermissions = PosixFilePermissions.fromString("rw-r--r--");
126+
Files.setPosixFilePermissions(file2, normalPermissions);
127+
128+
// When: Create ZIP files with preservePermissions=false
129+
Path zip1 = tempDir.resolve("cache1.zip");
130+
Path zip2 = tempDir.resolve("cache2.zip");
131+
CacheUtils.zip(sourceDir1, zip1, "*", false);
132+
CacheUtils.zip(sourceDir2, zip2, "*", false);
133+
134+
// Unzip and verify permissions are NOT preserved
135+
Path extractDir1 = tempDir.resolve("extracted1");
136+
Path extractDir2 = tempDir.resolve("extracted2");
137+
Files.createDirectories(extractDir1);
138+
Files.createDirectories(extractDir2);
139+
CacheUtils.unzip(zip1, extractDir1, false);
140+
CacheUtils.unzip(zip2, extractDir2, false);
141+
142+
Path extractedFile1 = extractDir1.resolve("script.sh");
143+
Path extractedFile2 = extractDir2.resolve("script.sh");
144+
145+
Set<PosixFilePermission> perms1 = Files.getPosixFilePermissions(extractedFile1);
146+
Set<PosixFilePermission> perms2 = Files.getPosixFilePermissions(extractedFile2);
147+
148+
// Files should NOT retain their original different permissions
149+
// Both should have default permissions determined by umask
150+
assertFalse(perms1.equals(execPermissions) && perms2.equals(normalPermissions),
151+
"When preservePermissions=false, original permissions should NOT be preserved. " +
152+
"Files should use system default permissions (umask).");
153+
}
154+
155+
156+
/**
157+
* Java 8 compatible version of Files.writeString().
158+
*/
159+
private void writeString(Path path, String content) throws IOException {
160+
try (OutputStream out = Files.newOutputStream(path)) {
161+
out.write(content.getBytes(StandardCharsets.UTF_8));
162+
}
163+
}
164+
}

0 commit comments

Comments
 (0)