You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@struts.apache.org by lu...@apache.org on 2026/07/21 18:55:53 UTC
(struts) 06/14: WW-5539 Add TypeConverterHolder#computeMappingIfAbsent
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 <lu...@apache.org>
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);
+ }
}