diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessor.java b/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessor.java index 47a6404381..4e50485276 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessor.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessor.java @@ -20,6 +20,7 @@ import com.opensymphony.xwork2.ObjectFactory; import com.opensymphony.xwork2.conversion.ObjectTypeDeterminer; +import com.opensymphony.xwork2.conversion.TypeConverter; import com.opensymphony.xwork2.conversion.impl.XWorkConverter; import com.opensymphony.xwork2.inject.Inject; import com.opensymphony.xwork2.ognl.OgnlUtil; @@ -29,6 +30,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(Map context, Object target, Object name, Object value) } 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 && name instanceof Number) { //make sure there are enough spaces in the List to set diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkMapPropertyAccessor.java b/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkMapPropertyAccessor.java index bad7932cee..dd465fd48e 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkMapPropertyAccessor.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkMapPropertyAccessor.java @@ -20,6 +20,7 @@ import com.opensymphony.xwork2.ObjectFactory; import com.opensymphony.xwork2.conversion.ObjectTypeDeterminer; +import com.opensymphony.xwork2.conversion.TypeConverter; import com.opensymphony.xwork2.conversion.impl.XWorkConverter; import com.opensymphony.xwork2.inject.Inject; import com.opensymphony.xwork2.util.reflection.ReflectionContextState; @@ -126,8 +127,17 @@ public void setProperty(Map context, Object target, Object name, Object value) t 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(Map context, Object value) { diff --git a/core/src/test/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessorTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessorTest.java index 4a89d89354..9df833198a 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessorTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/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/com/opensymphony/xwork2/ognl/accessor/XWorkMapPropertyAccessorTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/accessor/XWorkMapPropertyAccessorTest.java index 1835e938ac..b0629c1efe 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/accessor/XWorkMapPropertyAccessorTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/accessor/XWorkMapPropertyAccessorTest.java @@ -25,6 +25,7 @@ import com.opensymphony.xwork2.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; 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 cf87495bc3..2101b701dd 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 @@ -38,6 +38,7 @@ import com.opensymphony.xwork2.ognl.OgnlValueStackFactory; import com.opensymphony.xwork2.ognl.accessor.CompoundRootAccessor; import com.opensymphony.xwork2.ognl.accessor.RootAccessor; +import com.opensymphony.xwork2.util.Element; import com.opensymphony.xwork2.util.ValueStack; import com.opensymphony.xwork2.util.ValueStackFactory; import com.opensymphony.xwork2.util.reflection.ReflectionContextState; @@ -1012,6 +1013,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 {