This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch WW-5539 in repository https://gitbox.apache.org/repos/asf/struts.git
commit ff24097447eca6f995b1d1ed80f97b434b6a475f Author: Lukasz Lenart <[email protected]> AuthorDate: Tue Jul 21 17:06:54 2026 +0200 WW-5539 Add TypeConverterHolder#computeMappingIfAbsent Adds an atomic build-once-and-cache operation so callers no longer need check-then-act around the class mapping cache, and deprecates the three primitives it subsumes: getMapping, addMapping and containsNoMapping. The method is a default method delegating to those primitives, so third-party TypeConverterHolder implementations keep working unchanged. --- .../conversion/StrutsTypeConverterHolder.java | 21 +++++ .../struts2/conversion/TypeConverterHolder.java | 46 +++++++++++ .../conversion/StrutsTypeConverterHolderTest.java | 92 ++++++++++++++++++++++ 3 files changed, 159 insertions(+) diff --git a/core/src/main/java/org/apache/struts2/conversion/StrutsTypeConverterHolder.java b/core/src/main/java/org/apache/struts2/conversion/StrutsTypeConverterHolder.java index 09f5766bc..247f2badc 100644 --- a/core/src/main/java/org/apache/struts2/conversion/StrutsTypeConverterHolder.java +++ b/core/src/main/java/org/apache/struts2/conversion/StrutsTypeConverterHolder.java @@ -21,9 +21,11 @@ package org.apache.struts2.conversion; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import java.util.Collections; import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; +import java.util.function.Function; /** * Default implementation of {@link TypeConverterHolder} @@ -99,20 +101,39 @@ public class StrutsTypeConverterHolder implements TypeConverterHolder { } @Override + @Deprecated public Map<String, Object> getMapping(Class clazz) { return mappings.get(clazz); } @Override + @Deprecated public void addMapping(Class clazz, Map<String, Object> mapping) { mappings.put(clazz, mapping); } @Override + @Deprecated public boolean containsNoMapping(Class clazz) { return noMapping.contains(clazz); } + @Override + public Map<String, Object> computeMappingIfAbsent(Class clazz, Function<Class, Map<String, Object>> builder) { + if (noMapping.contains(clazz)) { + return Collections.emptyMap(); + } + Map<String, Object> mapping = mappings.computeIfAbsent(clazz, c -> { + Map<String, Object> built = builder.apply(c); + return (built == null || built.isEmpty()) ? null : built; + }); + if (mapping == null) { + noMapping.add(clazz); + return Collections.emptyMap(); + } + return mapping; + } + @Override public void addNoMapping(Class clazz) { noMapping.add(clazz); diff --git a/core/src/main/java/org/apache/struts2/conversion/TypeConverterHolder.java b/core/src/main/java/org/apache/struts2/conversion/TypeConverterHolder.java index d20324719..eb710e408 100644 --- a/core/src/main/java/org/apache/struts2/conversion/TypeConverterHolder.java +++ b/core/src/main/java/org/apache/struts2/conversion/TypeConverterHolder.java @@ -18,7 +18,9 @@ */ package org.apache.struts2.conversion; +import java.util.Collections; import java.util.Map; +import java.util.function.Function; /** * Holds all mappings related to {@link TypeConverter}s @@ -54,7 +56,10 @@ public interface TypeConverterHolder { * * @param clazz class to convert to/from * @return {@link TypeConverter} for given class + * @deprecated since 7.3.0, use {@link #computeMappingIfAbsent(Class, Function)} which resolves + * and caches the mapping atomically instead of requiring a check-then-act at the call site. */ + @Deprecated Map<String, Object> getMapping(Class clazz); /** @@ -62,7 +67,10 @@ public interface TypeConverterHolder { * * @param clazz class to convert to/from * @param mapping property converters + * @deprecated since 7.3.0, use {@link #computeMappingIfAbsent(Class, Function)} which stores + * the built mapping itself. */ + @Deprecated void addMapping(Class clazz, Map<String, Object> mapping); /** @@ -70,7 +78,10 @@ public interface TypeConverterHolder { * * @param clazz class to convert to/from * @return true if mapping couldn't be found + * @deprecated since 7.3.0, use {@link #computeMappingIfAbsent(Class, Function)} which returns + * an empty map for classes known to have no mapping. */ + @Deprecated boolean containsNoMapping(Class clazz); /** @@ -97,4 +108,39 @@ public interface TypeConverterHolder { */ void addUnknownMapping(String className); + /** + * Returns the property-converter mapping for the given class, building and caching it on first + * use. Never returns {@code null}: a class known to have no mapping yields + * {@link Collections#emptyMap()}. + * + * <p>If the builder returns {@code null} or an empty map, the class is recorded in the negative + * cache so the builder is not invoked for it again.</p> + * + * <p>Implementations are expected to make this atomic so that the builder runs at most once per + * class. The default implementation is a non-atomic check-then-act using the deprecated + * primitives, preserving pre-7.3.0 behaviour for third-party holders that do not override it.</p> + * + * @param clazz class to convert to/from + * @param builder builds the property-converter mapping for the class when it is not yet cached + * @return the mapping for the class, or an empty map if it has none + * @since 7.3.0 + */ + @SuppressWarnings("deprecation") + default Map<String, Object> computeMappingIfAbsent(Class clazz, Function<Class, Map<String, Object>> builder) { + if (containsNoMapping(clazz)) { + return Collections.emptyMap(); + } + Map<String, Object> mapping = getMapping(clazz); + if (mapping != null) { + return mapping; + } + mapping = builder.apply(clazz); + if (mapping == null || mapping.isEmpty()) { + addNoMapping(clazz); + return Collections.emptyMap(); + } + addMapping(clazz, mapping); + return mapping; + } + } diff --git a/core/src/test/java/org/apache/struts2/conversion/StrutsTypeConverterHolderTest.java b/core/src/test/java/org/apache/struts2/conversion/StrutsTypeConverterHolderTest.java index be931af8a..efb7f0fc1 100644 --- a/core/src/test/java/org/apache/struts2/conversion/StrutsTypeConverterHolderTest.java +++ b/core/src/test/java/org/apache/struts2/conversion/StrutsTypeConverterHolderTest.java @@ -21,12 +21,16 @@ package org.apache.struts2.conversion; import org.apache.struts2.XWorkTestCase; import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; import static org.assertj.core.api.Assertions.assertThat; @@ -130,4 +134,92 @@ public class StrutsTypeConverterHolderTest extends XWorkTestCase { assertThat(holder.containsUnknownMapping("stub.Later")).isFalse(); assertThat(holder.getDefaultMapping("stub.Later")).isNotNull(); } + + public void testComputeMappingIfAbsentBuildsOnceAndCaches() { + StrutsTypeConverterHolder holder = new StrutsTypeConverterHolder(); + AtomicInteger builds = new AtomicInteger(); + + Map<String, Object> first = holder.computeMappingIfAbsent(String.class, clazz -> { + builds.incrementAndGet(); + Map<String, Object> built = new HashMap<>(); + built.put("someProperty", "someConverter"); + return built; + }); + Map<String, Object> second = holder.computeMappingIfAbsent(String.class, clazz -> { + builds.incrementAndGet(); + return new HashMap<>(); + }); + + assertThat(builds.get()).isEqualTo(1); + assertThat(first).containsEntry("someProperty", "someConverter"); + assertThat(second).isSameAs(first); + } + + public void testComputeMappingIfAbsentNegativeCachesEmptyResult() { + StrutsTypeConverterHolder holder = new StrutsTypeConverterHolder(); + AtomicInteger builds = new AtomicInteger(); + + Map<String, Object> first = holder.computeMappingIfAbsent(String.class, clazz -> { + builds.incrementAndGet(); + return Collections.emptyMap(); + }); + Map<String, Object> second = holder.computeMappingIfAbsent(String.class, clazz -> { + builds.incrementAndGet(); + return Collections.emptyMap(); + }); + + assertThat(first).isEmpty(); + assertThat(second).isEmpty(); + assertThat(builds.get()).as("empty result must be negative cached").isEqualTo(1); + assertThat(holder.containsNoMapping(String.class)).isTrue(); + } + + public void testComputeMappingIfAbsentNegativeCachesNullResult() { + StrutsTypeConverterHolder holder = new StrutsTypeConverterHolder(); + + Map<String, Object> result = holder.computeMappingIfAbsent(String.class, clazz -> null); + + assertThat(result).isNotNull().isEmpty(); + assertThat(holder.containsNoMapping(String.class)).isTrue(); + } + + public void testComputeMappingIfAbsentShortCircuitsOnKnownNoMapping() { + StrutsTypeConverterHolder holder = new StrutsTypeConverterHolder(); + holder.addNoMapping(String.class); + + Map<String, Object> result = holder.computeMappingIfAbsent(String.class, clazz -> { + throw new AssertionError("builder must not run for a negative-cached class"); + }); + + assertThat(result).isNotNull().isEmpty(); + } + + public void testComputeMappingIfAbsentBuildsOnceUnderConcurrency() throws Exception { + StrutsTypeConverterHolder holder = new StrutsTypeConverterHolder(); + AtomicInteger builds = new AtomicInteger(); + ExecutorService pool = Executors.newFixedThreadPool(THREADS); + CountDownLatch start = new CountDownLatch(1); + List<Future<Map<String, Object>>> futures = new ArrayList<>(); + + for (int t = 0; t < THREADS; t++) { + futures.add(pool.submit(() -> { + start.await(); + return holder.computeMappingIfAbsent(String.class, clazz -> { + builds.incrementAndGet(); + Map<String, Object> built = new HashMap<>(); + built.put("someProperty", "someConverter"); + return built; + }); + })); + } + + start.countDown(); + Map<String, Object> expected = futures.get(0).get(60, TimeUnit.SECONDS); + for (Future<Map<String, Object>> future : futures) { + assertThat(future.get(60, TimeUnit.SECONDS)).isSameAs(expected); + } + pool.shutdown(); + + assertThat(builds.get()).as("mapping must be built exactly once").isEqualTo(1); + } }
