Hi， Thanks a lot Julia! I have signed OCA and emailed it to Oracle last week. But I haven't seen my name on the contributor list yet. I guess I'll have to wait.
And thanks a lot for all the advice, Brent! It's really useful. I'll do as you said. Best regards, Yu On Wed, Jun 3, 2020 at 7:51 AM Brent Christian <brent.christ...@oracle.com> wrote: > Thank you for submitting the fix, Yu Li. (And thanks for the sponsor, > Julia). This will be good to fix. I have a few changes to recommend. > > src/java.base/share/classes/java/util/Properties.java > > The code change looks fine to me. > > The ending Copyright year just needs to be changed from 2019 -> 2020. > > > test/jdk/java/util/Properties/PropertiesEntrySetTest.java > > + * Copyright (c) 2018, 2019, Oracle and/or its affiliates. All rights > > For a newly added file, only the current year is listed, so: > > Copyright (c) 2020, Oracle and/or its affiliates. All rights > reserved. > > +/* > + * @test > + * @summary tests the entrySet() method of Properties class > + * @author Yu Li > + * @bug 8245694 > + */ > > Please also add an @run tag. Using testng would prevent needing to add > assert[True|False] methods. So that would be: > > @run testng PropertiesEntrySetTest > > Then you should be able to: > > import org.testng.Assert; > > and use the testng Assert.assertTrue() and Assert.assertFalse(). > > > + Set<Map.Entry<Object, Object>> aEntrySet = a.entrySet(); > > You might consider using 'var' to simplify declarations of aEntrySet, > bEntrySet, etc > > > Please also add testing of EntrySet.toString(). > > Along with testing equals/hashCode/toString after properties are added, > I think it would be good to also test after properties are removed. > There's Properties.remove()/clear(), and also maybe some of the removal > methods listed in Hashtable.entrySet(): Iterator.remove(), Set.remove(), > etc. > > > (And just as a point of interest, there is a test, > test/jdk/java/util/Properties/CheckOverrides.java, > to ensure that the Properties class overrides all necessary methods of > its superclasses. Checks were not included for Properties.EntrySet > (hence this bug), though I don't think it makes sense to add any at this > point.) > > Thanks a lot, > -Brent > > On 6/2/20 8:43 AM, Julia Boes wrote: > > Hi Yu Li, > > > > I'm happy to sponsor your fix once it's reviewed. Could you just confirm > > that you have signed the Oracle Contributor Agreement? > > > > https://www.oracle.com/technetwork/community/oca-486395.html > > > > Cheers, > > > > Julia > > > > On 30/05/2020 21:00, Rob Spoor wrote: > >> There's still a little regression there since Java 8, if you add the > >> keySet(), values() or entrySet() to the Properties object as either > >> key or value; in Java 8 this would cause "(this Collection)" to be > >> included, while even with this fix that would cause a > >> StackOverflowError. However, given that it's only a corner case for > >> most Collections and Maps in the first place, and with Properties > >> objects even more so, I don't think anyone would mind this regression. > >> > >> Rob > >> > >> On 30/05/2020 20:31, Lisa Li wrote: > >>> Hi, > >>> > >>> Please help review the fix for JDK-8245694 > >>> <https://bugs.openjdk.java.net/browse/JDK-8245694>. And this is my > very > >>> first patch submission. I know it's not perfect. > >>> <https://bugs.openjdk.java.net/browse/JDK-8245694> > >>> > >>> > >>> *BUG DESCRIPTION*: > >>> > >>> JDK-8245694 <https://bugs.openjdk.java.net/browse/JDK-8245694> : > >>> java.util.Properties.entrySet() > >>> does not override java.lang.Object methods since java 9. > >>> > >>> > >>> *PROPOSED FIX*: > >>> > >>> Add delegating overrides for equals(), hashCode(), and toString(). > >>> > >>> > >>> *PATCH*: > >>> > >>> # HG changeset patch > >>> # User yuli > >>> # Date 1590841711 -28800 > >>> # Sat May 30 20:28:31 2020 +0800 > >>> # Node ID 7dc946e5b5863291fa070bfdbdce48aefbe87b7b > >>> # Parent 8e28aae5068069e853673148e4d3f44cb50350a7 > >>> 8245694: java.util.Properties.entrySet() does not override Object > >>> methods > >>> Summary: Add missing override methods > >>> Contributed-by: Yu Li <liyu.it...@gmail.com> > >>> > >>> diff --git a/src/java.base/share/classes/java/util/Properties.java > >>> b/src/java.base/share/classes/java/util/Properties.java > >>> --- a/src/java.base/share/classes/java/util/Properties.java > >>> +++ b/src/java.base/share/classes/java/util/Properties.java > >>> @@ -1414,6 +1414,21 @@ > >>> return entrySet.retainAll(c); > >>> } > >>> > >>> + @Override > >>> + public boolean equals(Object o) { > >>> + return entrySet.equals(o); > >>> + } > >>> + > >>> + @Override > >>> + public int hashCode() { > >>> + return entrySet.hashCode(); > >>> + } > >>> + > >>> + @Override > >>> + public String toString() { > >>> + return entrySet.toString(); > >>> + } > >>> + > >>> @Override > >>> public Iterator<Map.Entry<Object, Object>> iterator() { > >>> return entrySet.iterator(); > >>> diff --git a/test/jdk/java/util/Properties/PropertiesEntrySetTest.java > >>> b/test/jdk/java/util/Properties/PropertiesEntrySetTest.java > >>> new file mode 100644 > >>> --- /dev/null > >>> +++ b/test/jdk/java/util/Properties/PropertiesEntrySetTest.java > >>> @@ -0,0 +1,90 @@ > >>> +/* > >>> + * Copyright (c) 2018, 2019, 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 > >>> + * @summary tests the entrySet() method of Properties class > >>> + * @author Yu Li > >>> + * @bug 8245694 > >>> + */ > >>> + > >>> +import java.util.Map; > >>> +import java.util.Properties; > >>> +import java.util.Set; > >>> + > >>> +public class PropertiesEntrySetTest { > >>> + > >>> + public static void main(String[] args) { > >>> + > >>> + testEntrySet(); > >>> + > >>> + } > >>> + > >>> + private static void assertTrue(boolean value) { > >>> + if (!value) { > >>> + throw new RuntimeException("Failure in Properties > >>> testing."); > >>> + } > >>> + } > >>> + > >>> + private static void assertFalse(boolean value) { > >>> + if (value) { > >>> + throw new RuntimeException("Failure in Properties > >>> testing."); > >>> + } > >>> + } > >>> + > >>> + private static void testEntrySet() { > >>> + Properties a = new Properties(); > >>> + Set<Map.Entry<Object, Object>> aEntrySet = a.entrySet(); > >>> + assertFalse(aEntrySet.equals(null)); > >>> + assertTrue(aEntrySet.equals(aEntrySet)); > >>> + > >>> + Properties b = new Properties(); > >>> + Set<Map.Entry<Object, Object>> bEntrySet = b.entrySet(); > >>> + assertTrue(aEntrySet.equals(bEntrySet)); > >>> + assertTrue(aEntrySet.hashCode() == bEntrySet.hashCode()); > >>> + > >>> + a.setProperty("p1", "1"); > >>> + assertFalse(aEntrySet.equals(bEntrySet)); > >>> + assertFalse(bEntrySet.equals(aEntrySet)); > >>> + assertFalse(aEntrySet.hashCode() == bEntrySet.hashCode()); > >>> + > >>> + b.setProperty("p1", "1"); > >>> + assertTrue(aEntrySet.equals(bEntrySet)); > >>> + assertTrue(aEntrySet.hashCode() == bEntrySet.hashCode()); > >>> + > >>> + Properties c = new Properties(); > >>> + c.setProperty("p1", "2"); > >>> + Set<Map.Entry<Object, Object>> cEntrySet = c.entrySet(); > >>> + assertFalse(cEntrySet.equals(bEntrySet)); > >>> + assertFalse(cEntrySet.hashCode() == bEntrySet.hashCode()); > >>> + assertFalse(cEntrySet.equals(aEntrySet)); > >>> + assertFalse(cEntrySet.hashCode() == aEntrySet.hashCode()); > >>> + > >>> + a.setProperty("p2", "2"); > >>> + Properties d = new Properties(); > >>> + d.setProperty("p2", "2"); > >>> + d.setProperty("p1", "1"); > >>> + Set<Map.Entry<Object, Object>> dEntrySet = d.entrySet(); > >>> + assertTrue(dEntrySet.equals(aEntrySet)); > >>> + } > >>> +} > >>> > >>> Best regards, > >>> Yu Li >
