Simplify PerHierarchyAndLexicographicalOrderFieldComparator and make …

johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/order/PerHierarchyAndLexicographicalOrderFieldComparator.java

@@ -18,67 +18,87 @@
  */
 package org.apache.johnzon.jsonb.order;
 
+import java.lang.reflect.Field;
+import java.lang.reflect.Method;
 import java.util.Comparator;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 public class PerHierarchyAndLexicographicalOrderFieldComparator implements Comparator<String> {
     private final Class<?> clazz;
     private final Map<String, Integer> distances = new ConcurrentHashMap<>();
+    private final AtomicBoolean populated = new AtomicBoolean();
 
     public PerHierarchyAndLexicographicalOrderFieldComparator(final Class<?> clazz) {
         this.clazz = clazz;
     }
 
     @Override
     public int compare(final String o1, final String o2) {
-        if (o1.equals(o2)) {
+        if (o1 != null ? o1.equals(o2) : o2 == null) {
             return 0;
         }
-        final int d1 = distance(o1);
-        final int d2 = distance(o2);
+        populateDistances();
+        final Integer d1 = o1 == null ? null : distances.get(o1);
+        final Integer d2 = o2 == null ? null : distances.get(o2);
+        if (d1 == null || d2 == null) {
+            return compareStrings(o1, o2);
+        }
         final int res = d2 - d1; // reversed!
         if (res == 0) {
-            return o1.compareTo(o2);
+            return compareStrings(o1, o2);
         }
         return res;
     }
 
-    private int distance(final String o1) {
-        return distances.getOrDefault(o1, slowDistance(o1));
+    private void populateDistances() {
+        if (!populated.get()) {
+            synchronized (this) {
+                if (populated.compareAndSet(false, true)) {
+                    Class<?> current = clazz;
+                    int level = 0;
+                    while (current != null && current != Object.class) {
+                        for (final Field field : current.getDeclaredFields()) {
+                            distances.putIfAbsent(field.getName(), level);
+                        }
+                        for (final Method method : current.getDeclaredMethods()) {
+                            if (method.getParameterCount() != 0 || method.getReturnType() == void.class) {
+                                continue;
+                            }
+                            final String name = method.getName();
+                            if (name.length() > 3 && name.startsWith("get")) {
+                                distances.putIfAbsent(decapitalize(name.substring(3)), level);
+                            } else if (name.length() > 2 && name.startsWith("is")
+                                    && (method.getReturnType() == boolean.class || method.getReturnType() == Boolean.class)) {
+                                distances.putIfAbsent(decapitalize(name.substring(2)), level);
+                            }
+                        }
+                        level++;
+                        current = current.getSuperclass();
+                    }
+                }
+            }
+        }
     }
 
-    private int cache(final String o1, final int distance) {
-        distances.putIfAbsent(o1, distance);
-        return distance;
+    private String decapitalize(final String value) {
+        if (value.isEmpty()) {
+            return value;
+        }
+        if (value.length() > 1 && Character.isUpperCase(value.charAt(0)) && Character.isUpperCase(value.charAt(1))) {
+            return value;
+        }
+        return Character.toLowerCase(value.charAt(0)) + value.substring(1);
     }
 
-    private int slowDistance(String o1) {
-        Class<?> current = clazz;
-        int i = 0;
-        while (current != null && current != Object.class) {
-            try {
-                current.getDeclaredField(o1);
-                return cache(o1, i);
-            } catch (final NoSuchFieldException e) {
-                // no-op
-            }
-            final String methodSuffix = Character.toUpperCase(o1.charAt(0)) + (o1.length() > 1 ? o1.substring(1) : "");
-            try {
-                current.getDeclaredMethod("get" + methodSuffix);
-                return cache(o1, i);
-            } catch (final Exception e) {
-                // no-op
-            }
-            try {
-                current.getDeclaredMethod("is" + methodSuffix);
-                return cache(o1, i);
-            } catch (final Exception e) {
-                // no-op
-            }
-            i++;
-            current = current.getSuperclass();
+    private int compareStrings(final String o1, final String o2) {
+        if (o1 == null) {
+            return o2 == null ? 0 : 1;
+        }
+        if (o2 == null) {
+            return -1;
         }
-        return i;
+        return o1.compareTo(o2);
     }
-}
+}

johnzon-jsonb/src/test/java/org/apache/johnzon/jsonb/order/PerHierarchyAndLexicographicalOrderFieldComparatorTest.java

@@ -0,0 +1,134 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.johnzon.jsonb.order;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.List;
+
+import org.junit.Test;
+
+public class PerHierarchyAndLexicographicalOrderFieldComparatorTest {
+
+    @Test
+    public void inheritedFieldsComeFirstThenLexicographicalWithinLevel() {
+        final Comparator<String> comparator = new PerHierarchyAndLexicographicalOrderFieldComparator(Sub.class);
+        final List<String> order = Arrays.asList("subField", "aField", "superField", "zField");
+        Collections.sort(order, comparator);
+        assertEquals(Arrays.asList("superField", "zField", "aField", "subField"), order);
+    }
+
+    @Test
+    public void equalNamesCompareZero() {
+        final Comparator<String> comparator = new PerHierarchyAndLexicographicalOrderFieldComparator(Sub.class);
+        assertEquals(0, comparator.compare("aField", "aField"));
+    }
+
+    @Test
+    public void onlyOneInCacheComparesStrings() {
+        final Comparator<String> comparator = new PerHierarchyAndLexicographicalOrderFieldComparator(Sub.class);
+        final List<String> order = Arrays.asList("notAMember", "aField");
+        Collections.sort(order, comparator);
+        assertEquals(Arrays.asList("aField", "notAMember"), order);
+    }
+
+    @Test
+    public void bothUnknownCompareStrings() {
+        final Comparator<String> comparator = new PerHierarchyAndLexicographicalOrderFieldComparator(Sub.class);
+        final List<String> order = Arrays.asList("zeta", "alpha");
+        Collections.sort(order, comparator);
+        assertEquals(Arrays.asList("alpha", "zeta"), order);
+    }
+
+    @Test
+    public void nullHandling() {
+        final Comparator<String> comparator = new PerHierarchyAndLexicographicalOrderFieldComparator(Sub.class);
+        assertTrue(comparator.compare(null, "aField") > 0);
+        assertTrue(comparator.compare("aField", null) < 0);
+        assertEquals(0, comparator.compare(null, null));
+        assertEquals(0, comparator.compare("aField", "aField"));
+    }
+
+    @Test
+    public void booleanIsAccessorIsIdentified() {
+        final Comparator<String> comparator = new PerHierarchyAndLexicographicalOrderFieldComparator(BooleanHolder.class);
+        assertEquals(0, comparator.compare("flag", "flag"));
+        final List<String> order = Arrays.asList("other", "flag");
+        Collections.sort(order, comparator);
+        assertEquals(Arrays.asList("flag", "other"), order);
+    }
+
+    public static class Super {
+        private String superField;
+        private String zField;
+
+        public String getSuperField() {
+            return superField;
+        }
+
+        public void setSuperField(final String superField) {
+            this.superField = superField;
+        }
+
+        public String getzField() {
+            return zField;
+        }
+
+        public void setzField(final String zField) {
+            this.zField = zField;
+        }
+    }
+
+    public static class Sub extends Super {
+        private String subField;
+        private String aField;
+
+        public String getSubField() {
+            return subField;
+        }
+
+        public void setSubField(final String subField) {
+            this.subField = subField;
+        }
+
+        public String getaField() {
+            return aField;
+        }
+
+        public void setaField(final String aField) {
+            this.aField = aField;
+        }
+    }
+
+    public static class BooleanHolder {
+        private boolean flag;
+
+        public boolean isFlag() {
+            return flag;
+        }
+
+        public void setFlag(final boolean flag) {
+            this.flag = flag;
+        }
+    }
+}