On Mon, 23 Oct 2023 09:14:06 GMT, Sean Coffey <coff...@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
