RFR: 8317678: Fix up hashCode() for ZipFile.Source.Key [v9]

[Jaikiran Pai]

On Mon, 23 Oct 2023 09:14:06 GMT, Sean Coffey <coffeys at openjdk.org> wrote:

>> Fix up java.util.zip.ZipFile$Source hashCode() impl so that duplicate Source objects aren't created for the same zip file.
>
> Sean Coffey has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update code comments

Hello Sean, thank you for the updates. The fix now really boils down to fixing the `hashCode()` implementation of `java.util.zip.ZipFile$Source$Key` so that it honours the `equals()` and `hashCode()` contract which mandates that the `hashCode()` returned by instances which are `equals()` must be the same.

The change you have in the `Key` class now looks good to me.

Coming to the newly introduced `ZipSourceCache.java` test case, I went through it. What's being done in that test looks OK to me, but I felt that it's a bit too complex for what I think we should be testing here - asserting that `hashCode()` returned by `Key` instances are same if `Key` instances are `equals()`.

I decided to attempt a different way to test this and this is what the test looks like:


/*
 * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
 * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
 *
 * This code is free software; you can redistribute it and/or modify it
 * under the terms of the GNU General Public License version 2 only, as
 * published by the Free Software Foundation.
 *
 * This code is distributed in the hope that it will be useful, but WITHOUT
 * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
 * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
 * version 2 for more details (a copy is included in the LICENSE file that
 * accompanied this code).
 *
 * You should have received a copy of the GNU General Public License version
 * 2 along with this work; if not, write to the Free Software Foundation,
 * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
 *
 * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
 * or visit www.oracle.com if you need additional information or have any
 * questions.
 */

/* @test
 * @bug 8317678
 * @modules java.base/java.util.zip:open
 * @summary Fix up hashCode() for ZipFile.Source.Key
 * @run junit/othervm ZipSourceCache
 */

import java.io.File;
import java.io.FileOutputStream;
import java.io.IOException;
import java.lang.reflect.Constructor;
import java.lang.reflect.Method;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
import java.nio.file.FileSystem;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.attribute.BasicFileAttributes;

import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;

public class ZipSourceCache {
    private static final String TEST_FILE_NAME = "foobar.zip";

    @BeforeAll
    public static void beforeAll() throws Exception {
        // create the file to run our test
        try (final FileOutputStream fos = new FileOutputStream(TEST_FILE_NAME)) {
            // no need to write anything to the file, just the presence of the file
            // is enough
        }
    }

    @AfterAll
    public static void afterAll() throws Exception {
        Files.deleteIfExists(Path.of(TEST_FILE_NAME));
    }

    /*
     * Verifies that instances of java.util.zip.ZipFile$Source$Key which are "equal()"
     * return back the same "hashCode()"
     */
    @Test
    public void testKeyEqualsHashCode() throws Exception {
        // create an instance of java.util.zip.ZipCoder for UTF-8 charset
        final Class<?> zipCoderClass = Class.forName("java.util.zip.ZipCoder");
        final Method m = zipCoderClass.getDeclaredMethod("get", Charset.class);
        m.setAccessible(true);
        final Object utf8ZipCoder = m.invoke(null, StandardCharsets.UTF_8);


        // get the java.util.zip.ZipFile$Source$Key class
        final Class<?> keyClass = Class.forName("java.util.zip.ZipFile$Source$Key");
        final Constructor<?> ctr = keyClass.getConstructor(File.class, BasicFileAttributes.class,
                zipCoderClass);
        ctr.setAccessible(true);

        final File relFile = new File(TEST_FILE_NAME);
        final File absFile = new File(TEST_FILE_NAME).getAbsoluteFile();
        System.out.println("Running test on " + keyClass.getName() + " with relative File path "
                + relFile.getPath() + " and absolute File path " + absFile.getPath());
        // create an instance of java.util.zip.ZipFile$Source$Key passing it a relative file path
        // to "foobar.zip"
        final Object relKey = ctr.newInstance(relFile, getAttrs(relFile), utf8ZipCoder);
        System.out.println("relKey, hashCode = " + relKey.hashCode());

        // create an instance of java.util.zip.ZipFile$Source$Key passing it an absolute file path
        // to "foobar.zip"
        final Object absKey = ctr.newInstance(absFile, getAttrs(absFile), utf8ZipCoder);
        System.out.println("absKey, hashCode = " + absKey.hashCode());
        // these 2 Key instances, one created with a relative File path and other with a absolute
        // File path, to the same file, are expected to be "equal()"
        Assertions.assertTrue(absKey.equals(relKey), "Key instances were expected to be equal");
        // verify that the instances which are "equal()" return the same "hashCode()"
        Assertions.assertEquals(relKey.hashCode(), absKey.hashCode(), "Differing hashCode() for" +
                " Keys that were equal");
    }

    private static BasicFileAttributes getAttrs(final File file) throws IOException {
        final FileSystem fs = file.toPath().getFileSystem();
        return Files.readAttributes(fs.getPath(file.getPath()),
                BasicFileAttributes.class);
    }
}

All it does is asserts the `hashCode()` values of `Key` instances created out of a relative file and absolute file pointing to the same underlying file. This test consistently fails without your fix for the `Key` class and passes with your fix. I haven't run it on Windows, but I think it shouldn't be any different.

Do you think we could simplify the `ZipSourceCache.java` test in this PR to instead use this alternate approach or some form of it?

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

PR Comment: https://git.openjdk.org/jdk/pull/16115#issuecomment-1774868391