RFR: 8350880: (zipfs) Add support for read-only zip file systems [v3]

[Lance Andersen]

On Mon, 12 May 2025 10:16:33 GMT, David Beaumont <duke at 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).
>
> David Beaumont has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix comment based on current behaviour.

Thank. you David for your work here

A few comments on a quick pass in addition to those from Alan & Jai

The copyright will also need to be updated

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 95:

> 93: 
> 94:     private static final Set<PosixFilePermission> DEFAULT_PERMISSIONS =
> 95:         PosixFilePermissions.fromString("rwxrwxrwx");

I am not convinced this needs to be changed as it becomes a  personal style preference

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 113:

> 111:     final ZipCoder zc;
> 112:     private final ZipPath rootdir;
> 113:     // Start readOnly (safe mode) and maybe reset at end of initialization.

maybe -> may be

test/jdk/jdk/nio/zipfs/NewFileSystemTests.java line 219:

> 217:             assertFalse(fs.isReadOnly());
> 218:             if (!"Default version".equals(Files.readString(fs.getPath("file.txt"), UTF_8))) {
> 219:                 throw new RuntimeException("unexpected file content");

could also consider

fail("unexpected file content");

test/jdk/jdk/nio/zipfs/TestPosix.java line 434:

> 432:         createTestZipFile(ZIP_FILE, ENV_DEFAULT).close();
> 433:         // check entries on zipfs with default options
> 434:         try (FileSystem zip = FileSystems.newFileSystem(ZIP_FILE, ENV_DEFAULT)) {

This tests an empty Map and should still be a test as it is different from ENV_READ_ONLY

-------------

PR Review: https://git.openjdk.org/jdk/pull/25178#pullrequestreview-2840686940
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2089239326
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2089240538
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2089339910
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2089344316