Fb jpms - fixes for illegal access to private information in java.* packages.

.gitignore

@@ -21,3 +21,6 @@ site-content
 
 # jenv's version file
 .java-version
+
+# Netbeans
+nb-configuration.xml

pom.xml

@@ -466,9 +466,7 @@
         <jdk>[9,)</jdk>
       </activation>
       <properties>
-        <!-- LANG-1265: allow tests to access private fields/methods of java.base classes via reflection -->
-        <!-- LANG-1667: allow tests to access private fields/methods of java.base/java.util such as ArrayList via reflection -->
-        <argLine>-Xmx512m --add-opens java.base/java.lang.reflect=ALL-UNNAMED --add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.util=ALL-UNNAMED --add-opens java.base/java.time=ALL-UNNAMED --add-opens java.base/java.time.chrono=ALL-UNNAMED</argLine>
+        <argLine>-Xmx512m</argLine>
       </properties>
     </profile>
     <profile>

src/main/java/org/apache/commons/lang3/builder/CompareToBuilder.java

@@ -115,6 +115,11 @@ private static void reflectionAppend(
         final boolean useTransients,
         final String[] excludeFields) {
 
+        if (Reflection.isNonIntrospectibleClass(lhs) && lhs instanceof Comparable) {
+            builder.append(lhs, rhs);
+            return;
+        }
+
         final Field[] fields = clazz.getDeclaredFields();
         AccessibleObject.setAccessible(fields, true);
         for (int i = 0; i < fields.length && builder.comparison == 0; i++) {

src/main/java/org/apache/commons/lang3/builder/EqualsBuilder.java

@@ -22,7 +22,9 @@
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashSet;
+import java.util.Iterator;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 
 import org.apache.commons.lang3.ArrayUtils;
@@ -731,7 +733,7 @@ public EqualsBuilder append(final Object lhs, final Object rhs) {
             // to be inlined
             appendArray(lhs, rhs);
         } else // The simple case, not an array, just test the element
-        if (testRecursive && !ClassUtils.isPrimitiveOrWrapper(lhsClass)) {
+        if (testRecursive && !ClassUtils.isPrimitiveOrWrapper(lhsClass) && !Reflection.isNonIntrospectibleClass(lhsClass)) {
             reflectionAppend(lhs, rhs);
         } else {
             isEquals = lhs.equals(rhs);
@@ -956,11 +958,47 @@ public EqualsBuilder reflectionAppend(final Object lhs, final Object rhs) {
         }
 
         try {
-            if (testClass.isArray()) {
+            if (testClass.isArray() || Reflection.isNonIntrospectibleClass(testClass)) {
                 append(lhs, rhs);
-            } else //If either class is being excluded, call normal object equals method on lhsClass.
-            if (bypassReflectionClasses != null
+            } else if (Collection.class.isAssignableFrom(testClass)) {
+                // Right now this also handles Sets.
+                // Which is likely fine for TreeSets and any other sorted Collection
+                // But it would not be easy to implement this for e.g. a HashSet
+                // The good thing is that this behaviour is backward compatible with
+                // pre-jpms handling where we did reflect into the internal structures.
+
+                Collection lColl = (Collection) lhs;
+                Collection rColl = (Collection) rhs;
+                if (lColl.size() != rColl.size()) {
+                    isEquals = false;
+                    return this;
+                }
+                final Iterator lIt = lColl.iterator();
+                final Iterator rIt = rColl.iterator();
+                while (lIt.hasNext() || rIt.hasNext()) {
+                    if (lIt.hasNext() != rIt.hasNext()) {
+                        // if one iterator has no more entries but the other one does, then it cannot be equal
+                        isEquals = false;
+                        return this;
+                    } else {
+                        reflectionAppend(lIt.next(), rIt.next());
+                    }
+                }
+            } else if (Map.class.isAssignableFrom(testClass)) {
+                Map<?, ?> lMap = (Map) lhs;
+                Map<?, ?> rMap = (Map) rhs;
+                if (lMap.size() != rMap.size()) {
+                    isEquals = false;
+                    return this;
+                }
+                for (Map.Entry e : lMap.entrySet()) {
+                    if (isEquals) {
+                        append(e.getValue(), rMap.get(e.getKey()));
+                    }
+                }
+            } else if (bypassReflectionClasses != null
                     && (bypassReflectionClasses.contains(lhsClass) || bypassReflectionClasses.contains(rhsClass))) {
+                //If either class is being excluded, call normal object equals method on lhsClass.
                 isEquals = lhs.equals(rhs);
             } else {
                 reflectionAppend(lhs, rhs, testClass);

src/main/java/org/apache/commons/lang3/builder/HashCodeBuilder.java

@@ -180,6 +180,10 @@ private static void reflectionAppend(final Object object, final Class<?> clazz,
         if (isRegistered(object)) {
             return;
         }
+        if (Reflection.isNonIntrospectibleClass(object)) {
+            builder.append(object);
+            return;
+        }
         try {
             register(object);
             // The elements in the returned array are not sorted and are not in any particular order.

src/main/java/org/apache/commons/lang3/builder/Reflection.java

@@ -18,13 +18,62 @@
 package org.apache.commons.lang3.builder;
 
 import java.lang.reflect.Field;
+import java.time.Duration;
+import java.time.Instant;
+import java.time.LocalDate;
+import java.time.LocalDateTime;
+import java.time.LocalTime;
+import java.time.OffsetDateTime;
+import java.time.OffsetTime;
+import java.time.Period;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Map;
 import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 
 /**
  * Package-private reflection code.
  */
 final class Reflection {
 
+    /**
+     * Some classes known to not be introspectable.
+     * Those are classes where we would blow up when we try to access internal information via reflection.
+     */
+    private static final Set<Class> KNOWN_NON_INTROSPECTIBLE_CLASSES = new HashSet<>();
+    {
+        KNOWN_NON_INTROSPECTIBLE_CLASSES.addAll(Arrays.asList(
+                Boolean.class,
+                Character.class,
+                String.class,
+                Byte.class,
+                Short.class,
+                Integer.class,
+                Long.class,
+                Float.class,
+                Double.class,
+                java.util.Date.class,
+                java.sql.Date.class,
+                LocalDate.class,
+                LocalDateTime.class,
+                LocalTime.class,
+                OffsetTime.class,
+                OffsetDateTime.class,
+                Instant.class,
+                Duration.class,
+                Period.class
+                ));
+    }
+
+    /**
+     * this is a cache for {@link #isNonIntrospectibleClass(Class)}.
+     * Downside: this has a Class as key, so it *might* lock a ClassLoader.
+     * TODO think about making this cache optional? Otoh we only cache classes we touch via the reflection methods.
+     */
+    private static Map<Class, Boolean> nonIntrospectibleClasses = new ConcurrentHashMap<>();
+
     /**
      * Delegates to {@link Field#get(Object)} and rethrows {@link IllegalAccessException} as {@link IllegalArgumentException}.
      *
@@ -41,4 +90,44 @@ static Object getUnchecked(final Field field, final Object obj) {
         }
     }
 
+    /**
+     * Check whether the class internas of the given object can be accessed. This might return false in Java9
+     * and thus lead to a difference in behaviour if modularity is activated or not.
+     * But it's the best we can do.
+     *
+     * @return {@code true} if the given class internas not accessible
+     */
+    static boolean isNonIntrospectibleClass(Object o) {
+        return o != null && isNonIntrospectibleClass(o.getClass());
+    }
+
+    /**
+     * Check whether the given class internas can be accessed. This might return false in Java9
+     * and thus lead to a difference in behaviour if modularity is activated or not.
+     * But it's the best we can do.
+     *
+     * @return {@code true} if the given class internas not accessible
+     */
+    static boolean isNonIntrospectibleClass(Class<?> clazz) {
+        if (KNOWN_NON_INTROSPECTIBLE_CLASSES.contains(clazz)) {
+            return true;
+        }
+        Boolean isAccessible = nonIntrospectibleClasses.get(clazz);
+        if (isAccessible != null) {
+            return isAccessible;
+        }
+        try {
+            final Field[] declaredFields = clazz.getDeclaredFields();
+            for (Field f : declaredFields) {
+                f.setAccessible(true);
+            }
+
+            nonIntrospectibleClasses.put(clazz, false);
+            return false;
+        } catch (Exception e) {
+            nonIntrospectibleClasses.put(clazz, true);
+            return true;
+        }
+    }
+
 }

src/main/java/org/apache/commons/lang3/builder/ReflectionToStringBuilder.java

@@ -23,6 +23,7 @@
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Comparator;
+import java.util.Map;
 import java.util.Objects;
 
 import org.apache.commons.lang3.ArraySorter;
@@ -848,8 +849,13 @@ public String toString() {
 
         validate();
 
+        if (handleNativeClasses()) {
+            return super.toString();
+        }
+
         Class<?> clazz = getObject().getClass();
         appendFieldsIn(clazz);
+
         while (clazz.getSuperclass() != null && clazz != getUpToClass()) {
             clazz = clazz.getSuperclass();
             appendFieldsIn(clazz);
@@ -867,4 +873,21 @@ private void validate() {
         }
     }
 
+    private boolean handleNativeClasses() {
+        Object value = getObject();
+        if (value.getClass().isArray() || value instanceof Collection || value instanceof Map) {
+            // we first need to unregister the value to be able to handle it in the style
+            ToStringStyle.unregister(value);
+            getStyle().append(getStringBuffer(), null, value, true);
+            return true;
+        }
+
+        if (Reflection.isNonIntrospectibleClass(value)) {
+            getStringBuffer().append("value=").append(getObject().toString());
+            return true;
+        }
+
+        return false;
+    }
+
 }

src/test/java/org/apache/commons/lang3/builder/EqualsBuilderTest.java

@@ -27,7 +27,9 @@
 import java.math.BigInteger;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 
 import org.apache.commons.lang3.AbstractLangTest;
 import org.apache.commons.lang3.reflect.MethodUtils;
@@ -1142,6 +1144,24 @@ public void testObjectsBypassReflectionClasses() {
         assertTrue(new EqualsBuilder().setBypassReflectionClasses(bypassReflectionClasses).isEquals());
     }
 
+    @Test
+    public void testArrayListComparison() {
+        final List<String> a = Arrays.asList("A", "B", "C");
+        final List<String> b = Arrays.asList("A", "B", "C");
+        assertTrue(new EqualsBuilder().setTestRecursive(true).append(a, b).isEquals());
+    }
+
+    @Test
+    public void testMapComparison() {
+        final Map<Integer, String> a = new HashMap<>();
+        final Map<Integer, String> b = new HashMap<>();
+        a.put(2, "B");
+        a.put(1, "A");
+        b.put(1, "A");
+        b.put(2, "B");
+        assertTrue(new EqualsBuilder().setTestRecursive(true).append(a, b).isEquals());
+    }
+
     @Test
     public void testRaggedArray() {
         final long[][] array1 = new long[2][];

src/test/java/org/apache/commons/lang3/builder/ToStringBuilderTest.java

@@ -22,6 +22,7 @@
 import static org.junit.jupiter.api.Assertions.assertThrows;
 
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.HashMap;
 import java.util.List;
 
@@ -229,21 +230,33 @@ public void assertReflectionArray(final String expected, final Object actual) {
      */
     @Test
     public void test_setUpToClass_invalid() {
-        final Integer val = Integer.valueOf(5);
+        final HirAFixture val = new HirAFixture();
         final ReflectionToStringBuilder test = new ReflectionToStringBuilder(val);
         assertThrows(IllegalArgumentException.class, () -> test.setUpToClass(String.class));
         test.toString();
     }
 
+    private static class HirAFixture extends HirBFixture {
+        int x = 1;
+    }
+
+    private static class HirBFixture extends HirCFixture {
+        int y = 2;
+    }
+
+    private static class HirCFixture {
+        int z = 3;
+    }
+
     /**
      * Tests ReflectionToStringBuilder setUpToClass().
      */
     @Test
     public void test_setUpToClass_valid() {
-        final Integer val = Integer.valueOf(5);
+        final HirAFixture val = new HirAFixture();
         final ReflectionToStringBuilder test = new ReflectionToStringBuilder(val);
-        test.setUpToClass(Number.class);
-        test.toString();
+        test.setUpToClass(HirBFixture.class);
+        assertEquals(val.toString() + "[x=1,y=2]", test.toString());
     }
 
     @Test
@@ -752,6 +765,12 @@ public void testObjectArray() {
         assertEquals(baseStr + "[<null>]", new ToStringBuilder(base).append((Object) array).toString());
     }
 
+    @Test
+    public void testArrayList() {
+        List<Integer> list = Arrays.asList(23, 12, 39);
+        assertEquals(baseStr + "[[23, 12, 39]]", new ToStringBuilder(base).append(list).toString());
+    }
+
     @Test
     public void testObjectBuild() {
         final Integer i3 = Integer.valueOf(3);
@@ -972,17 +991,14 @@ public void testReflectionHierarchyArrayList() {
         // LANG-1337 without this, the generated string can differ depending on the JVM version/vendor
         final List<Object> list = new ArrayList<>(ARRAYLIST_INITIAL_CAPACITY);
         final String baseString = toBaseString(list);
-        final String expectedWithTransients = baseString
-                + "[elementData={<null>,<null>,<null>,<null>,<null>,<null>,<null>,<null>,<null>,<null>},size=0,modCount=0]";
+        final String expected = baseString
+                + "[[]]";
         final String toStringWithTransients = ToStringBuilder.reflectionToString(list, null, true);
-        if (!expectedWithTransients.equals(toStringWithTransients)) {
-            assertEquals(expectedWithTransients, toStringWithTransients);
-        }
-        final String expectedWithoutTransients = baseString + "[size=0]";
+        assertEquals(expected, toStringWithTransients);
+
+        // no difference as Collections and Maps are not handled via reflection anymore
         final String toStringWithoutTransients = ToStringBuilder.reflectionToString(list, null, false);
-        if (!expectedWithoutTransients.equals(toStringWithoutTransients)) {
-            assertEquals(expectedWithoutTransients, toStringWithoutTransients);
-        }
+        assertEquals(expected, toStringWithoutTransients);
     }
 
     @Test