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

[David Beaumont]

On Fri, 16 May 2025 14:23:38 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> David Beaumont has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Changes based on review feedback.
>
> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 114:
> 
>> 112:     private final ZipPath rootdir;
>> 113:     // Starts in readOnly (safe mode), but might be reset at the end of initialization.
>> 114:     private boolean readOnly = true;
> 
> If `readOnly` gets used by some code when a `ZipFileSystem` instance is being constructed (which is why I believe this can't be a `final` field), then I think we should not change this value to `true`. In other words, would this change now have a chance of introducing a `ReadOnlyFileSystemException` when constructing the `ZipFileSystem` whereas before it wouldn't?

""would this change now have a chance of introducing a ReadOnlyFileSystemException when constructing the ZipFileSystem whereas before it wouldn't?""

If there was ever a "ReadOnlyFileSystemException" when initializing the ZipFileSystem, we have a very serious bug already. Since we already have the opening of Zip working for cases where the underlying file is read-only in the file system, it has to be true that no attempt is made to modify the file contents.

While I accept that this new code now calls out to functions with this flag in a different state to before, I believe this is an absolute improvement since:

1. It is not acceptable to say to users "we support a read-only mode" if during initialization we might modify the file in any way (even including changing last-modification times etc.).

2. All the evidence points to whichever operations are being done during init as being fundamentally "read-only" (both due to it working on read-only zip files, and there being no conceptual need to modify anything during setup).

I'll happily do a thorough audit of everything which could be affected by this change if that will give you confidence, but I would not want to change this code back to its default "read-write until stated otherwise" behaviour.

> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 245:
> 
>> 243:                     : "The underlying ZIP file is not writable";
>> 244:             throw new IOException(
>> 245:                     "A writable ZIP file system could not be opened for: " + zfpath + "\n" + reason);
> 
> The JDK coding guidelines recommend excluding file paths from exception messages. So in this case the `zfpath` should be left out from the exception message. Additionally, I haven't seen us using newline characters in exception messages that we construct in the JDK code. So I think we should leave that out too. 
> 
> Given this, I think it might be simpler to just change this `if` block to something like:
> 
> 
> 
> if (...) {
>     String reason = multiReleaseVersion.isPresent()
>         ? "the multi-release JAR file is not writable"
>         : "the ZIP file is not writable";
> 
>     throw new IOException(reason);
> }

Thanks for letting me know. Now I think about the "no filename" things is very sensible for core libraries.
Done.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095517691
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095520598