diff --git a/core/src/main/java/org/apache/struts2/ognl/accessor/XWorkListPropertyAccessor.java b/core/src/main/java/org/apache/struts2/ognl/accessor/XWorkListPropertyAccessor.java index e741877cb0..591d822b61 100644 --- a/core/src/main/java/org/apache/struts2/ognl/accessor/XWorkListPropertyAccessor.java +++ b/core/src/main/java/org/apache/struts2/ognl/accessor/XWorkListPropertyAccessor.java @@ -20,6 +20,7 @@ import org.apache.struts2.ObjectFactory; import org.apache.struts2.conversion.ObjectTypeDeterminer; +import org.apache.struts2.conversion.TypeConverter; import org.apache.struts2.conversion.impl.XWorkConverter; import org.apache.struts2.inject.Inject; import org.apache.struts2.ognl.OgnlUtil; @@ -30,6 +31,8 @@ import ognl.PropertyAccessor; import org.apache.struts2.StrutsConstants; import org.apache.struts2.StrutsException; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import java.util.Collection; import java.util.List; @@ -43,6 +46,8 @@ */ public class XWorkListPropertyAccessor extends ListPropertyAccessor { + private static final Logger LOG = LogManager.getLogger(XWorkListPropertyAccessor.class); + private XWorkCollectionPropertyAccessor _sAcc = new XWorkCollectionPropertyAccessor(); private XWorkConverter xworkConverter; @@ -167,6 +172,10 @@ public void setProperty(OgnlContext context, Object target, Object name, Object } Object realValue = getRealValue(context, value, convertToClass); + if (realValue == TypeConverter.NO_CONVERSION_POSSIBLE) { + LOG.debug("Unable to convert value for index [{}] to the declared element type, skipping assignment", name); + return; + } if (target instanceof List list && name instanceof Number) { //make sure there are enough spaces in the List to set diff --git a/core/src/main/java/org/apache/struts2/ognl/accessor/XWorkMapPropertyAccessor.java b/core/src/main/java/org/apache/struts2/ognl/accessor/XWorkMapPropertyAccessor.java index f15223fdf9..5b5a6aa14c 100644 --- a/core/src/main/java/org/apache/struts2/ognl/accessor/XWorkMapPropertyAccessor.java +++ b/core/src/main/java/org/apache/struts2/ognl/accessor/XWorkMapPropertyAccessor.java @@ -20,6 +20,7 @@ import org.apache.struts2.ObjectFactory; import org.apache.struts2.conversion.ObjectTypeDeterminer; +import org.apache.struts2.conversion.TypeConverter; import org.apache.struts2.conversion.impl.XWorkConverter; import org.apache.struts2.inject.Inject; import org.apache.struts2.util.reflection.ReflectionContextState; @@ -127,8 +128,17 @@ public void setProperty(OgnlContext context, Object target, Object name, Object LOG.trace("Entering setProperty({},{},{},{})", context, target, name, value); Object key = getKey(context, name); + if (key == TypeConverter.NO_CONVERSION_POSSIBLE) { + LOG.debug("Unable to convert key [{}] to the declared key type, skipping assignment", name); + return; + } + Object convertedValue = getValue(context, value); + if (convertedValue == TypeConverter.NO_CONVERSION_POSSIBLE) { + LOG.debug("Unable to convert value for key [{}] to the declared element type, skipping assignment", key); + return; + } Map map = (Map) target; - map.put(key, getValue(context, value)); + map.put(key, convertedValue); } private Object getValue(OgnlContext context, Object value) { diff --git a/core/src/test/java/org/apache/struts2/interceptor/parameter/ParametersInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/parameter/ParametersInterceptorTest.java index ba2f48969d..2b1b4386cc 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/parameter/ParametersInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/parameter/ParametersInterceptorTest.java @@ -48,6 +48,7 @@ import org.apache.struts2.action.ParameterValueAware; import org.apache.struts2.config.StrutsXmlConfigurationProvider; import org.apache.struts2.dispatcher.HttpParameters; +import org.apache.struts2.util.Element; import org.junit.Assert; import java.io.File; @@ -1013,6 +1014,44 @@ protected void setUp() throws Exception { container.inject(config.getInterceptors().get(0).getInterceptor()); } + /** + * WW-5700: a value that cannot be converted to the map's element type must not be stored. + * An unchecked s:checkbox with submitUnchecked="true" submits the CheckboxInterceptor's + * uncheckedValue, "false", which cannot become an Integer. + */ + public void testUnconvertibleValueIsNotBoundIntoTypedMap() { + CheckboxAction action = new CheckboxAction(); + ValueStack vs = ActionContext.getContext().getValueStack(); + vs.push(action); + + ParametersInterceptor pi = new ParametersInterceptor(); + container.inject(pi); + + Map params = new HashMap<>(); + params.put("capDeferral[100]", "1"); + params.put("capDeferral[200]", "false"); + + pi.applyParameters(action, vs, HttpParameters.create(params).build()); + + Map capDeferral = action.getCapDeferral(); + assertEquals("sanity: the convertible value must still bind", Integer.valueOf(1), capDeferral.get(100L)); + for (Object entry : ((Map) capDeferral).entrySet()) { + Map.Entry e = (Map.Entry) entry; + assertTrue("key is not a Long: " + e.getKey(), e.getKey() instanceof Long); + assertTrue("value is not an Integer: " + e.getValue(), e.getValue() instanceof Integer); + } + } + + public static class CheckboxAction { + @Element(value = Integer.class) + private final Map capDeferral = new HashMap<>(); + + @StrutsParameter(depth = 1) + public Map getCapDeferral() { + return capDeferral; + } + } + } class ValidateAction implements ValidationAware { diff --git a/core/src/test/java/org/apache/struts2/ognl/accessor/XWorkListPropertyAccessorTest.java b/core/src/test/java/org/apache/struts2/ognl/accessor/XWorkListPropertyAccessorTest.java index c028c98a8e..c1a393ac42 100644 --- a/core/src/test/java/org/apache/struts2/ognl/accessor/XWorkListPropertyAccessorTest.java +++ b/core/src/test/java/org/apache/struts2/ognl/accessor/XWorkListPropertyAccessorTest.java @@ -64,6 +64,22 @@ public void testCanAccessListSizeProperty() { assertEquals(myList.size(), vs.findValue("strings.size")); } + public void testUnconvertibleElementIsNotStored() { + ValueStack vs = ActionContext.getContext().getValueStack(); + ListHolder listHolder = new ListHolder(); + listHolder.setLongs(new ArrayList<>()); + vs.push(listHolder); + + vs.setValue("longs[0]", "1"); + vs.setValue("longs[1]", "not-a-number"); + + assertEquals(Long.valueOf(1), listHolder.getLongs().get(0)); + for (Object element : (List) listHolder.getLongs()) { + assertTrue("list must not hold a non-Long element: " + element, + element == null || element instanceof Long); + } + } + public void testAutoGrowthCollectionLimit() { PropertyAccessor accessor = container.getInstance(PropertyAccessor.class, ArrayList.class.getName()); ((XWorkListPropertyAccessor) accessor).setAutoGrowCollectionLimit("2"); diff --git a/core/src/test/java/org/apache/struts2/ognl/accessor/XWorkMapPropertyAccessorTest.java b/core/src/test/java/org/apache/struts2/ognl/accessor/XWorkMapPropertyAccessorTest.java index 7a1d9cd9c5..8bc67fe4d4 100644 --- a/core/src/test/java/org/apache/struts2/ognl/accessor/XWorkMapPropertyAccessorTest.java +++ b/core/src/test/java/org/apache/struts2/ognl/accessor/XWorkMapPropertyAccessorTest.java @@ -25,6 +25,7 @@ import org.apache.struts2.util.reflection.ReflectionContextState; import java.util.Collections; +import java.util.HashMap; import java.util.Map; public class XWorkMapPropertyAccessorTest extends XWorkTestCase { @@ -57,6 +58,50 @@ public void testNullIsReturnedWhenCreateNullObjectsIsSpecifiedAsFalse() { assertNull(vs.findValue("map['key']")); } + public void testUnconvertibleValueIsNotStored() { + TypedMapHolder holder = new TypedMapHolder(); + ValueStack vs = ActionContext.getContext().getValueStack(); + vs.push(holder); + + vs.setValue("counts[1]", "5"); + vs.setValue("counts[2]", "not-a-number"); + + assertEquals(Integer.valueOf(5), holder.getCounts().get(1L)); + assertOnlyDeclaredTypes(holder.getCounts()); + } + + public void testUnconvertibleKeyIsNotStored() { + TypedMapHolder holder = new TypedMapHolder(); + ValueStack vs = ActionContext.getContext().getValueStack(); + vs.push(holder); + + vs.setValue("counts[1]", "5"); + vs.setValue("counts['abc']", "6"); + + assertEquals(Integer.valueOf(5), holder.getCounts().get(1L)); + assertOnlyDeclaredTypes(holder.getCounts()); + } + + /** + * A Map declared to hold Long keys and Integer values must never be left holding anything else. + */ + private static void assertOnlyDeclaredTypes(Map map) { + for (Object o : ((Map) map).entrySet()) { + Map.Entry entry = (Map.Entry) o; + assertTrue("key is not a Long: " + entry.getKey(), entry.getKey() instanceof Long); + assertTrue("value is not an Integer: " + entry.getValue(), entry.getValue() instanceof Integer); + } + } + + public static class TypedMapHolder { + @Element(value = Integer.class) + private final Map counts = new HashMap<>(); + + public Map getCounts() { + return counts; + } + } + private static class MapHolder { private final Map map;