Hi, Both the fix and test have been modified accordingly. Please help review 'em. I'd appreciate your thoughts on it.
*BUG DESCRIPTION :* JDK-8245694 <https://bugs.openjdk.java.net/browse/JDK-8245694> *PATCH : * # HG changeset patch # User Yu Li <liyu.it...@gmail.com> # Date 1591541947 -28800 # Sun Jun 07 22:59:07 2020 +0800 # Node ID f5715d275e4f683f5c4bf39d457293904f27330e # Parent d0d06b8be6789994881dc81d50573f60ad2ef844 8245694: java.util.Properties.entrySet() does not override Object methods Summary: Add missing override methods Contributed-by: Yu Li <liyu.it...@gmail.com> diff -r d0d06b8be678 -r f5715d275e4f src/java.base/share/classes/java/util/Properties.java --- a/src/java.base/share/classes/java/util/Properties.java Fri May 29 11:58:00 2020 +0200 +++ b/src/java.base/share/classes/java/util/Properties.java Sun Jun 07 22:59:07 2020 +0800 @@ -1405,6 +1405,21 @@ } @Override + public boolean equals(Object o) { + return o == this || entrySet.equals(o); + } + + @Override + public int hashCode() { + return entrySet.hashCode(); + } + + @Override + public String toString() { + return entrySet.toString(); + } + + @Override public boolean removeAll(Collection<?> c) { return entrySet.removeAll(c); } diff -r d0d06b8be678 -r f5715d275e4f test/jdk/java/util/Properties/PropertiesEntrySetTest.java --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/test/jdk/java/util/Properties/PropertiesEntrySetTest.java Sun Jun 07 22:59:07 2020 +0800 @@ -0,0 +1,201 @@ +/* + * Copyright (c) 2020, 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 8245694 + * @summary tests the entrySet() method of Properties class + * @author Yu Li + * @run testng PropertiesEntrySetTest + */ + +import org.testng.annotations.Test; + +import java.util.Properties; + +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertFalse; +import static org.testng.Assert.assertThrows; +import static org.testng.Assert.assertTrue; + +public class PropertiesEntrySetTest { + + @Test + public void testEquals() { + Properties a = new Properties(); + var aEntrySet = a.entrySet(); + assertFalse(aEntrySet.equals(null)); + assertTrue(aEntrySet.equals(aEntrySet)); + + Properties b = new Properties(); + var bEntrySet = b.entrySet(); + assertTrue(bEntrySet.equals(aEntrySet)); + assertTrue(bEntrySet.hashCode() == aEntrySet.hashCode()); + + a.setProperty("p1", "1"); + assertFalse(bEntrySet.equals(aEntrySet)); + assertFalse(bEntrySet.hashCode() == aEntrySet.hashCode()); + + b.setProperty("p1", "1"); + assertTrue(aEntrySet.equals(bEntrySet)); + assertTrue(bEntrySet.hashCode() == aEntrySet.hashCode()); + + Properties c = new Properties(); + c.setProperty("p1", "2"); + var cEntrySet = c.entrySet(); + assertFalse(cEntrySet.equals(bEntrySet)); + assertFalse(bEntrySet.hashCode() == cEntrySet.hashCode()); + assertFalse(cEntrySet.equals(aEntrySet)); + assertFalse(aEntrySet.hashCode() == cEntrySet.hashCode()); + + a.setProperty("p2", "2"); + Properties d = new Properties(); + d.setProperty("p2", "2"); + d.setProperty("p1", "1"); + var dEntrySet = d.entrySet(); + assertTrue(dEntrySet.equals(aEntrySet)); + assertTrue(aEntrySet.hashCode() == dEntrySet.hashCode()); + + a.remove("p1"); + assertFalse(aEntrySet.equals(dEntrySet)); + assertFalse(aEntrySet.hashCode() == dEntrySet.hashCode()); + + d.remove("p1", "1"); + assertTrue(dEntrySet.equals(aEntrySet)); + assertTrue(aEntrySet.hashCode() == dEntrySet.hashCode()); + + a.clear(); + assertFalse(aEntrySet.equals(dEntrySet)); + assertFalse(aEntrySet.hashCode() == dEntrySet.hashCode()); + assertTrue(aEntrySet.isEmpty()); + + d.clear(); + assertTrue(dEntrySet.equals(aEntrySet)); + assertTrue(aEntrySet.hashCode() == dEntrySet.hashCode()); + assertTrue(dEntrySet.isEmpty()); + } + + @Test + public void testToString() { + Properties a = new Properties(); + var aEntrySet = a.entrySet(); + assertEquals(aEntrySet.toString(), "[]"); + + a.setProperty("p1", "1"); + assertEquals(aEntrySet.toString(), "[p1=1]"); + + a.setProperty("p2", "2"); + assertEquals(aEntrySet.size(), 2); + assertTrue(aEntrySet.toString().trim().startsWith("[")); + assertTrue(aEntrySet.toString().contains("p1=1")); + assertTrue(aEntrySet.toString().contains("p2=2")); + assertTrue(aEntrySet.toString().trim().endsWith("]")); + + Properties b = new Properties(); + b.setProperty("p2", "2"); + b.setProperty("p1", "1"); + var bEntrySet = b.entrySet(); + assertEquals(bEntrySet.size(), 2); + assertTrue(bEntrySet.toString().trim().startsWith("[")); + assertTrue(bEntrySet.toString().contains("p1=1")); + assertTrue(bEntrySet.toString().contains("p2=2")); + assertTrue(bEntrySet.toString().trim().endsWith("]")); + + b.setProperty("p0", "0"); + assertEquals(bEntrySet.size(), 3); + assertTrue(bEntrySet.toString().contains("p0=0")); + + b.remove("p1"); + assertEquals(bEntrySet.size(), 2); + assertFalse(bEntrySet.toString().contains("p1=1")); + assertTrue(bEntrySet.toString().trim().startsWith("[")); + assertTrue(bEntrySet.toString().contains("p0=0")); + assertTrue(bEntrySet.toString().contains("p2=2")); + assertTrue(bEntrySet.toString().trim().endsWith("]")); + + b.remove("p0", "0"); + assertEquals(bEntrySet.size(), 1); + assertFalse(bEntrySet.toString().contains("p0=0")); + assertTrue(bEntrySet.toString().trim().startsWith("[")); + assertTrue(bEntrySet.toString().contains("p2=2")); + assertTrue(bEntrySet.toString().trim().endsWith("]")); + + b.clear(); + assertTrue(bEntrySet.isEmpty()); + assertTrue(bEntrySet.toString().equals("[]")); + } + + @Test + public void testEntrySetWithoutException() { + Properties a = new Properties(); + a.setProperty("p1", "1"); + a.setProperty("p2", "2"); + var aEntrySet = a.entrySet(); + assertEquals(aEntrySet.size(), 2); + + var i = aEntrySet.iterator(); + var e1 = i.next(); + i.remove(); + assertFalse(aEntrySet.contains(e1)); + assertEquals(aEntrySet.size(), 1); + + var e2 = i.next(); + aEntrySet.remove(e2); + assertFalse(aEntrySet.contains(e2)); + assertTrue(aEntrySet.isEmpty()); + + a.setProperty("p1", "1"); + a.setProperty("p3", "3"); + Properties b = new Properties(); + b.setProperty("p2", "2"); + b.setProperty("p1", "1"); + var bEntrySet = b.entrySet(); + + assertFalse(bEntrySet.containsAll(aEntrySet)); + assertEquals(bEntrySet.size(), 2); + + assertTrue(bEntrySet.removeAll(aEntrySet)); + assertEquals(bEntrySet.size(), 1); + + assertTrue(bEntrySet.retainAll(aEntrySet)); + assertTrue(bEntrySet.isEmpty()); + assertEquals(aEntrySet.size(), 2); + + aEntrySet.clear(); + assertTrue(aEntrySet.isEmpty()); + } + + @Test + public void testEntrySetExceptionWhenAdd() { + Properties a = new Properties(); + a.setProperty("p1", "1"); + var aEntrySet = a.entrySet(); + + Properties b = new Properties(); + b.setProperty("p2", "2"); + var bEntrySet = b.entrySet(); + + assertThrows(UnsupportedOperationException.class, () -> aEntrySet.addAll(bEntrySet)); + assertThrows(UnsupportedOperationException.class, () -> aEntrySet.add(bEntrySet.iterator().next())); + } +} Best regards, Yu L. On Wed, Jun 3, 2020 at 1:37 PM Lisa Li <liyu.it...@gmail.com> wrote: > 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 >> >
