On Mon, 12 May 2025 09:40:56 GMT, David Beaumont <d...@openjdk.org> wrote:
> Adding read-only support to ZipFileSystem.
>
> The new `accessMode` environment property allows for readOnly and readWrite
> values, and ensures that the requested mode is consistent with what's
> returned.
>
> This involved a little refactoring to ensure that "read only" state was set
> initially and only unset at the end of initialization if appropriate.
>
> By making 2 methods return values (rather than silently set non-final fields
> as a side effect) it's now clear in what order fields are initialized and
> which are final (sadly there are still non-final fields, but only a split of
> this class into two types can fix that, since determining multi-jar support
> requires reading the file system).
Preloading some explanatory comment.
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 83:
> 81: private static final boolean isWindows = System.getProperty("os.name")
> 82: .startsWith("Windows");
> 83: private static final byte[] ROOTPATH = new byte[]{'/'};
Whitespace format changes from my editor - will revert (sorry hadn't spotted).
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 89:
> 87:
> 88: // Posix file permissions allow per-file access control in a
> posix-like fashion.
> 89: // Note that using a "readOnly" access mode will force all files, and
> the
Not correct now - will fix.
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 198:
> 196: this.defaultOwner = supportPosix ? initOwner(zfpath, env) : null;
> 197: this.defaultGroup = supportPosix ? initGroup(zfpath, env) : null;
> 198: this.defaultPermissions = supportPosix ?
> Collections.unmodifiableSet(initPermissions(env)) : null;
Ensuring this is unmodifiable to avoid risk of returning a modifiable enum set.
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 221:
> 219: }
> 220: // sm and existence check
> 221: zfpath.getFileSystem().provider().checkAccess(zfpath,
> java.nio.file.AccessMode.READ);
Type name clash with existing AccessMode class. Since new enum is private,
happy to rename.
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 241:
> 239: // is why they are the only non-final fields), and it requires
> that the
> 240: // inode map has been initialized.
> 241: Optional<Integer> multiReleaseVersion =
> determineReleaseVersion(env);
Now release version and entry lookup determination always happen on a read-only
file system.
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 259:
> 257:
> 258: // Pass "this" as a parameter after everything else is set up.
> 259: this.rootdir = new ZipPath(this, new byte[]{'/'});
This could probably be set above release version etc. but it's a choice of
either:
1. waiting until everything is set up before passing "this"
2. letting an incomplete "this" instance get passed to another class (possible
escape risk?)
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 264:
> 262: /**
> 263: * Return the compression method to use (STORED or DEFLATED). If the
> 264: * property {@code compressionMethod} is set use its value to
> determine
Typo fix (too many m's).
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 377:
> 375: Object o = env.get(PROPERTY_DEFAULT_PERMISSIONS);
> 376: if (o == null) {
> 377: return PosixFilePermissions.fromString("rwxrwxrwx");
Since DEFAULT_PERMISSIONS was mutable and only used exactly once, it seems
better to just inline it where it's clear what it actually is based off. Better
readability.
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 1458:
> 1456: * use its value to determine the requested version. If not use the
> value of the "multi-release" property.
> 1457: */
> 1458: private Optional<Integer> determineReleaseVersion(Map<String, ?>
> env) throws IOException {
Refactored to return the value rather than side-effect instance fields.
test/jdk/jdk/nio/zipfs/NewFileSystemTests.java line 180:
> 178: */
> 179: @Test
> 180: public void testNoSuchFileFailure() {
In theory some of these could be split more, but they are uncomplicated sanity
checks.
test/jdk/jdk/nio/zipfs/Utils.java line 52:
> 50: */
> 51: static Path createJarFile(String name, String... entries) throws
> IOException {
> 52: Path jarFile = Paths.get(name);
Previous tests always had the test file called the same thing.
Note that in jtreg tests, the file of the same name often already exists in the
test directory.
I tried adding a "ensure file doesn't already exist" check and it fails lots of
tests.
-------------
PR Review: https://git.openjdk.org/jdk/pull/25178#pullrequestreview-2832583353
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2084284309
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2084288335
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2084292488
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2084296094
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2084297859
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2084301697
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2084302415
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2084306071
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2084307536
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2084311454
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2084317768
