From c55fb566982b42ef394b693078859cb79fcb83af Mon Sep 17 00:00:00 2001 From: Romain Manni-Bucau Date: Tue, 25 Aug 2026 23:01:59 +0200 Subject: [PATCH 1/2] Simplify PerHierarchyAndLexicographicalOrderFieldComparator and make its cache stable per type --- ...ndLexicographicalOrderFieldComparator.java | 92 +++++++----- ...xicographicalOrderFieldComparatorTest.java | 134 ++++++++++++++++++ 2 files changed, 190 insertions(+), 36 deletions(-) create mode 100644 johnzon-jsonb/src/test/java/org/apache/johnzon/jsonb/order/PerHierarchyAndLexicographicalOrderFieldComparatorTest.java diff --git a/johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/order/PerHierarchyAndLexicographicalOrderFieldComparator.java b/johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/order/PerHierarchyAndLexicographicalOrderFieldComparator.java index 9c4b2327..bf834a6b 100644 --- a/johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/order/PerHierarchyAndLexicographicalOrderFieldComparator.java +++ b/johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/order/PerHierarchyAndLexicographicalOrderFieldComparator.java @@ -18,13 +18,17 @@ */ 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 { private final Class clazz; private final Map distances = new ConcurrentHashMap<>(); + private final AtomicBoolean populated = new AtomicBoolean(); public PerHierarchyAndLexicographicalOrderFieldComparator(final Class clazz) { this.clazz = clazz; @@ -32,53 +36,69 @@ public PerHierarchyAndLexicographicalOrderFieldComparator(final Class 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); } -} +} \ No newline at end of file diff --git a/johnzon-jsonb/src/test/java/org/apache/johnzon/jsonb/order/PerHierarchyAndLexicographicalOrderFieldComparatorTest.java b/johnzon-jsonb/src/test/java/org/apache/johnzon/jsonb/order/PerHierarchyAndLexicographicalOrderFieldComparatorTest.java new file mode 100644 index 00000000..aa38b433 --- /dev/null +++ b/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 comparator = new PerHierarchyAndLexicographicalOrderFieldComparator(Sub.class); + final List 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 comparator = new PerHierarchyAndLexicographicalOrderFieldComparator(Sub.class); + assertEquals(0, comparator.compare("aField", "aField")); + } + + @Test + public void onlyOneInCacheComparesStrings() { + final Comparator comparator = new PerHierarchyAndLexicographicalOrderFieldComparator(Sub.class); + final List order = Arrays.asList("notAMember", "aField"); + Collections.sort(order, comparator); + assertEquals(Arrays.asList("aField", "notAMember"), order); + } + + @Test + public void bothUnknownCompareStrings() { + final Comparator comparator = new PerHierarchyAndLexicographicalOrderFieldComparator(Sub.class); + final List order = Arrays.asList("zeta", "alpha"); + Collections.sort(order, comparator); + assertEquals(Arrays.asList("alpha", "zeta"), order); + } + + @Test + public void nullHandling() { + final Comparator 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 comparator = new PerHierarchyAndLexicographicalOrderFieldComparator(BooleanHolder.class); + assertEquals(0, comparator.compare("flag", "flag")); + final List 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; + } + } +} \ No newline at end of file From aedff10700b659c84f676aea11e3af2b012f9dfc Mon Sep 17 00:00:00 2001 From: Markus Jung Date: Wed, 26 Aug 2026 18:00:22 +0200 Subject: [PATCH 2/2] cosmetics --- ...ndLexicographicalOrderFieldComparator.java | 37 ++++++------------- 1 file changed, 11 insertions(+), 26 deletions(-) diff --git a/johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/order/PerHierarchyAndLexicographicalOrderFieldComparator.java b/johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/order/PerHierarchyAndLexicographicalOrderFieldComparator.java index bf834a6b..c5840ea2 100644 --- a/johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/order/PerHierarchyAndLexicographicalOrderFieldComparator.java +++ b/johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/order/PerHierarchyAndLexicographicalOrderFieldComparator.java @@ -22,10 +22,15 @@ import java.lang.reflect.Method; import java.util.Comparator; import java.util.Map; +import java.util.Objects; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicBoolean; +import org.apache.johnzon.mapper.util.BeanUtil; + public class PerHierarchyAndLexicographicalOrderFieldComparator implements Comparator { + private static final Comparator NULLS_LAST = Comparator.nullsLast(Comparator.naturalOrder()); + private final Class clazz; private final Map distances = new ConcurrentHashMap<>(); private final AtomicBoolean populated = new AtomicBoolean(); @@ -36,18 +41,18 @@ public PerHierarchyAndLexicographicalOrderFieldComparator(final Class clazz) @Override public int compare(final String o1, final String o2) { - if (o1 != null ? o1.equals(o2) : o2 == null) { + if (Objects.equals(o1, o2)) { return 0; } 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); + return NULLS_LAST.compare(o1, o2); } final int res = d2 - d1; // reversed! if (res == 0) { - return compareStrings(o1, o2); + return NULLS_LAST.compare(o1, o2); } return res; } @@ -68,10 +73,10 @@ private void populateDistances() { } final String name = method.getName(); if (name.length() > 3 && name.startsWith("get")) { - distances.putIfAbsent(decapitalize(name.substring(3)), level); + distances.putIfAbsent(BeanUtil.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); + distances.putIfAbsent(BeanUtil.decapitalize(name.substring(2)), level); } } level++; @@ -81,24 +86,4 @@ private void populateDistances() { } } } - - 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 compareStrings(final String o1, final String o2) { - if (o1 == null) { - return o2 == null ? 0 : 1; - } - if (o2 == null) { - return -1; - } - return o1.compareTo(o2); - } -} \ No newline at end of file +}