From 952882e9e44a6775889266329d408dd227df96af Mon Sep 17 00:00:00 2001 From: Gus Brodman Date: Mon, 2 Nov 2026 16:14:07 -0500 Subject: [PATCH] Use ClassValue for ModelUtils.ALL_FIELDS_CACHE Replace the Caffeine LoadingCache in ModelUtils.ALL_FIELDS_CACHE with java.lang.ClassValue and narrow getAllFields return type to ImmutableMap Using a ClassValue instead of a cache avoids boilerplate, avoids possible memory leak issues (because it stores the values on the classes themselves), and avoids having to hash or look up anything --- .../google/registry/model/ModelUtils.java | 57 ++++++++----------- .../google/registry/model/ModelUtilsTest.java | 21 ++----- 2 files changed, 28 insertions(+), 50 deletions(-) diff --git a/core/src/main/java/google/registry/model/ModelUtils.java b/core/src/main/java/google/registry/model/ModelUtils.java index 3246529f7e2..c2058ed3f31 100644 --- a/core/src/main/java/google/registry/model/ModelUtils.java +++ b/core/src/main/java/google/registry/model/ModelUtils.java @@ -21,8 +21,6 @@ import static java.util.stream.Collectors.toCollection; import static java.util.stream.Collectors.toList; -import com.github.benmanes.caffeine.cache.LoadingCache; -import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Predicate; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSortedMap; @@ -31,9 +29,7 @@ import java.lang.reflect.Field; import java.lang.reflect.Modifier; import java.util.AbstractList; -import java.util.ArrayDeque; import java.util.Collection; -import java.util.Deque; import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; @@ -46,33 +42,31 @@ public class ModelUtils { /** Caches all instance fields on an object, including non-public and inherited fields. */ - private static final LoadingCache, ImmutableMap> ALL_FIELDS_CACHE = - CacheUtils.newCacheBuilder() - .build( - clazz -> { - Deque> hierarchy = new ArrayDeque<>(); - // Walk the hierarchy up to but not including ImmutableObject (to ignore - // hashCode). - for (; clazz != ImmutableObject.class; clazz = clazz.getSuperclass()) { - // Add to the front, so that shadowed fields show up later in the list. - // This will mean that getFieldValues will show the most derived value. - hierarchy.addFirst(clazz); - } - Map fields = new LinkedHashMap<>(); - for (Class hierarchyClass : hierarchy) { - // Don't use hierarchyClass.getFields() because it only picks up public fields. - for (Field field : hierarchyClass.getDeclaredFields()) { - if (!Modifier.isStatic(field.getModifiers())) { - field.setAccessible(true); - fields.put(field.getName(), field); - } - } - } - return ImmutableMap.copyOf(fields); - }); + private static final ClassValue> ALL_FIELDS_CACHE = + new ClassValue<>() { + @Override + protected ImmutableMap computeValue(Class clazz) { + // Walk the hierarchy up to but not including ImmutableObject (to ignore hashCode). + if (clazz == null || clazz == ImmutableObject.class) { + return ImmutableMap.of(); + } + // Start with superclass fields so shadowed fields are overwritten by the subclass while + // preserving declaration order from the top of the hierarchy down. + Map fields = + new LinkedHashMap<>(ALL_FIELDS_CACHE.get(clazz.getSuperclass())); + // Don't use clazz.getFields() because it only picks up public fields. + for (Field field : clazz.getDeclaredFields()) { + if (!Modifier.isStatic(field.getModifiers())) { + field.setAccessible(true); + fields.put(field.getName(), field); + } + } + return ImmutableMap.copyOf(fields); + } + }; /** Lists all instance fields on an object, including non-public and inherited fields. */ - public static Map getAllFields(Class clazz) { + public static ImmutableMap getAllFields(Class clazz) { return ALL_FIELDS_CACHE.get(clazz); } @@ -187,9 +181,4 @@ private static Object cloneEmptyToNullRecursive(Object obj) { protected static T cloneEmptyToNull(T obj) { return (T) cloneEmptyToNullRecursive(obj); } - - @VisibleForTesting - static void resetCaches() { - ALL_FIELDS_CACHE.invalidateAll(); - } } diff --git a/core/src/test/java/google/registry/model/ModelUtilsTest.java b/core/src/test/java/google/registry/model/ModelUtilsTest.java index ca609d4a414..1b1f8acaa1f 100644 --- a/core/src/test/java/google/registry/model/ModelUtilsTest.java +++ b/core/src/test/java/google/registry/model/ModelUtilsTest.java @@ -18,8 +18,6 @@ import com.google.common.collect.ImmutableMap; import java.lang.reflect.Field; -import java.util.Map; -import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; /** Unit tests for {@link ModelUtils}. */ @@ -69,26 +67,17 @@ public Builder asBuilder() { } } - @BeforeEach - void resetCaches() { - ModelUtils.resetCaches(); - } - @Test void testGetAllFields() throws Exception { - Map expected = + ImmutableMap expected = ImmutableMap.of( "id", TestClass.class.getDeclaredField("id"), "a", TestClass.class.getDeclaredField("a"), "b", TestClass.class.getDeclaredField("b")); - // More complicated version of isEqualTo() so that we check for ordering. - assertThat(ModelUtils.getAllFields(TestClass.class).entrySet()) - .containsExactlyElementsIn(expected.entrySet()) - .inOrder(); - // Test again, to make sure we hit the cache. - assertThat(ModelUtils.getAllFields(TestClass.class).entrySet()) - .containsExactlyElementsIn(expected.entrySet()) - .inOrder(); + ImmutableMap firstLookup = ModelUtils.getAllFields(TestClass.class); + assertThat(firstLookup).containsExactlyEntriesIn(expected).inOrder(); + // Verify repeated lookups return the same cached instance. + assertThat(ModelUtils.getAllFields(TestClass.class)).isSameInstanceAs(firstLookup); } @Test