diff --git a/core/src/main/java/com/opensymphony/xwork2/conversion/impl/CollectionConverter.java b/core/src/main/java/com/opensymphony/xwork2/conversion/impl/CollectionConverter.java index b7f707f404..32a32accbd 100644 --- a/core/src/main/java/com/opensymphony/xwork2/conversion/impl/CollectionConverter.java +++ b/core/src/main/java/com/opensymphony/xwork2/conversion/impl/CollectionConverter.java @@ -61,7 +61,7 @@ public Object convertValue(Map context, Object target, Member me for (Object anObjArray : objArray) { Object convertedValue = converter.convertValue(context, target, member, propertyName, anObjArray, memberType); - if (!TypeConverter.NO_CONVERSION_POSSIBLE.equals(convertedValue)) { + if (convertedValue != TypeConverter.NO_CONVERSION_POSSIBLE) { result.add(convertedValue); } } @@ -72,7 +72,7 @@ public Object convertValue(Map context, Object target, Member me for (Object aCol : col) { Object convertedValue = converter.convertValue(context, target, member, propertyName, aCol, memberType); - if (!TypeConverter.NO_CONVERSION_POSSIBLE.equals(convertedValue)) { + if (convertedValue != TypeConverter.NO_CONVERSION_POSSIBLE) { result.add(convertedValue); } } @@ -80,7 +80,7 @@ public Object convertValue(Map context, Object target, Member me result = createCollection(toType, memberType, -1); TypeConverter converter = getTypeConverter(context); Object convertedValue = converter.convertValue(context, target, member, propertyName, value, memberType); - if (!TypeConverter.NO_CONVERSION_POSSIBLE.equals(convertedValue)) { + if (convertedValue != TypeConverter.NO_CONVERSION_POSSIBLE) { result.add(convertedValue); } } diff --git a/core/src/test/java/com/opensymphony/xwork2/conversion/impl/CollectionConverterTest.java b/core/src/test/java/com/opensymphony/xwork2/conversion/impl/CollectionConverterTest.java new file mode 100644 index 0000000000..25b9f951db --- /dev/null +++ b/core/src/test/java/com/opensymphony/xwork2/conversion/impl/CollectionConverterTest.java @@ -0,0 +1,128 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package com.opensymphony.xwork2.conversion.impl; + +import com.opensymphony.xwork2.ActionContext; +import com.opensymphony.xwork2.XWorkTestCase; +import com.opensymphony.xwork2.conversion.TypeConverter; +import com.opensymphony.xwork2.util.ValueStack; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashSet; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Set; + +public class CollectionConverterTest extends XWorkTestCase { + + /** + * WW-5701: the marker constant's value is ordinary text, so an element that genuinely holds + * that text converts successfully and must be kept. + *

+ * The value is built at runtime rather than written as a literal on purpose: a literal would be + * interned to the very same instance as the constant's value, which no request-derived + * parameter ever is. A servlet container builds parameter values from the request bytes. + */ + public void testElementWhoseTextEqualsTheMarkerIsKept() { + String asSubmittedByAUser = new String("ognl.NoConversionPossible".toCharArray()); + assertNotSame("fixture must not be interned", TypeConverter.NO_CONVERSION_POSSIBLE, asSubmittedByAUser); + + Holder holder = new Holder(); + ValueStack vs = ActionContext.getContext().getValueStack(); + vs.push(holder); + + vs.setValue("names", new String[]{"alpha", asSubmittedByAUser, "omega"}); + + assertEquals(Arrays.asList("alpha", "ognl.NoConversionPossible", "omega"), holder.getNames()); + } + + /** + * The guard must still do its job: a genuinely unconvertible element is dropped. + */ + public void testUnconvertibleElementIsStillDropped() { + Holder holder = new Holder(); + ValueStack vs = ActionContext.getContext().getValueStack(); + vs.push(holder); + + vs.setValue("numbers", new String[]{"1", "not-a-number", "3"}); + + assertEquals(Arrays.asList(1L, 3L), holder.getNumbers()); + } + + /** + * The same guard on the path taken when the submitted value is itself a collection rather than + * an array - here a List feeding a Set-typed property. + */ + public void testUnconvertibleElementIsDroppedFromACollectionSource() { + Holder holder = new Holder(); + ValueStack vs = ActionContext.getContext().getValueStack(); + vs.push(holder); + + vs.setValue("numberSet", Arrays.asList("1", "not-a-number", "3")); + + assertEquals(new HashSet<>(Arrays.asList(1L, 3L)), holder.getNumberSet()); + } + + /** + * The same guard on the path taken when a single value is assigned to a collection property. + * The property is seeded first so that a setter which is never called cannot pass vacuously. + */ + public void testUnconvertibleSingleValueIsDropped() { + Holder holder = new Holder(); + holder.setNumbers(new ArrayList<>(Arrays.asList(99L))); + ValueStack vs = ActionContext.getContext().getValueStack(); + vs.push(holder); + + vs.setValue("numbers", "not-a-number"); + + assertEquals(Collections.emptyList(), holder.getNumbers()); + } + + public static class Holder { + private List names = new ArrayList<>(); + private List numbers = new ArrayList<>(); + private Set numberSet = new LinkedHashSet<>(); + + public List getNames() { + return names; + } + + public void setNames(List names) { + this.names = names; + } + + public List getNumbers() { + return numbers; + } + + public void setNumbers(List numbers) { + this.numbers = numbers; + } + + public Set getNumberSet() { + return numberSet; + } + + public void setNumberSet(Set numberSet) { + this.numberSet = numberSet; + } + } +}