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

[David Beaumont]

On Mon, 19 May 2025 12:39:22 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> David Beaumont has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fixed test.
>
> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 210:
> 
>> 208:             }
>> 209:         }
>> 210:         // sm and existence check
> 
> Nit - this is pre-existing, but would be good to cleanup. The "sm" here refers to SecurityManager. In JDK mainline, the SecurityManager is no longer present. So it would be good to change this comment to just "existence check" and remove reference to "sm".

Done.

> src/jdk.zipfs/share/classes/module-info.java line 290:
> 
>> 288:  *               already exist, and is incompatible with {@code "create"=true}.
>> 289:  *               Specifying both will cause an {@code IllegalArgumentException}
>> 290:  *               to be thrown when the Zip filesystem is created
> 
> To be precise, perhaps this last sentence should be reworded to:
> 
>> Specifying {@code create} as {@code true} and {@code accessMode} as {@code readOnly} will cause an {@code IllegalArgumentException} to be thrown when the ZIP filesystem is created.

Done.

> src/jdk.zipfs/share/classes/module-info.java line 304:
> 
>> 302:  *           </li>
>> 303:  *       </ul>
>> 304:  *       The access mode has no effect on reported POSIX file permissions (in cases
> 
> For consistency, I think this should be:
> 
>> The {@code accessMode} has no ....

Done.

> test/jdk/jdk/nio/zipfs/NewFileSystemTests.java line 208:
> 
>> 206:         // Multi-release JARs, when opened with a specified version are inherently read-only.
>> 207:         Path multiReleaseJar = createMultiReleaseJar();
>> 208:         try (FileSystem fs = FileSystems.newFileSystem(multiReleaseJar, Map.of("accessMode", "readWrite"))) {
> 
> I wonder if the ZIP filesystem implementation should throw an exception in this case. In the case where the application code has explicitly stated `accessMode=readWrite`, if the underlying file `Path` isn't writable, then we currently throw an `IOException`. I think we should specify and implement the same for the case where `accessMode=readWrite` and the file is a multi-release JAR file.

It does throw if accessMode=readWrite and the result is not writable for any reason.


this.readOnly = forceReadOnly || multiReleaseVersion.isPresent() || !Files.isWritable(zfpath);
if (readOnly && accessMode == ZipAccessMode.READ_WRITE) {

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095718428
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095714913
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095715684
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095723172