This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch fix/WW-5701-collection-converter-identity in repository https://gitbox.apache.org/repos/asf/struts.git
commit 9a9d1edce086f7e9ade461fee601c2cc3f402e08 Author: Lukasz Lenart <[email protected]> AuthorDate: Thu Aug 27 11:10:14 2026 +0200 WW-5701 fix(conversion): compare the conversion marker by identity, not equals CollectionConverter decided whether an element had converted successfully by comparing the result to TypeConverter.NO_CONVERSION_POSSIBLE with equals(). The marker's value is the ordinary text "ognl.NoConversionPossible", so an element that genuinely held that text converted fine and was then silently discarded from the resulting collection. Nothing signalled the loss: no conversion had failed, so no conversion error was registered and the action simply saw a shorter collection. The exposure is not limited to collections declared to hold Strings - when no element type can be determined the member type defaults to String.class, so untyped collections are affected too. Compare by reference instead, at all three sites. Identity is correct here rather than incidental. The constant is declared Object, not String, so it is not a JLS constant variable and is not inlined into referencing class files; every reference resolves to the one field value at runtime, including in third-party converters compiled elsewhere. A parameter value built by a servlet container from request bytes is a distinct object, so reference comparison separates "the converter signalled failure" from "the user submitted this text". Please do not simplify this back to equals(), which is what caused the bug. WW-5700 fixed the mirror-image defect in the map and list property accessors, which stored the marker instead of skipping it, and used identity comparison for the same reason. Found while reviewing that fix. Co-Authored-By: Claude Opus 5 <[email protected]> --- .../conversion/impl/CollectionConverter.java | 6 +- .../conversion/impl/CollectionConverterTest.java | 86 ++++++++++++++++++++++ 2 files changed, 89 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/conversion/impl/CollectionConverter.java b/core/src/main/java/org/apache/struts2/conversion/impl/CollectionConverter.java index 815380927..26bb86565 100644 --- a/core/src/main/java/org/apache/struts2/conversion/impl/CollectionConverter.java +++ b/core/src/main/java/org/apache/struts2/conversion/impl/CollectionConverter.java @@ -61,7 +61,7 @@ public class CollectionConverter extends DefaultTypeConverter { for (Object anObjArray : objArray) { Object convertedValue = converter.convertValue(context, target, member, propertyName, anObjArray, memberType); - if (!NO_CONVERSION_POSSIBLE.equals(convertedValue)) { + if (convertedValue != NO_CONVERSION_POSSIBLE) { result.add(convertedValue); } } @@ -72,7 +72,7 @@ public class CollectionConverter extends DefaultTypeConverter { for (Object aCol : col) { Object convertedValue = converter.convertValue(context, target, member, propertyName, aCol, memberType); - if (!NO_CONVERSION_POSSIBLE.equals(convertedValue)) { + if (convertedValue != NO_CONVERSION_POSSIBLE) { result.add(convertedValue); } } @@ -80,7 +80,7 @@ public class CollectionConverter extends DefaultTypeConverter { result = createCollection(toType, memberType, -1); TypeConverter converter = getTypeConverter(context); Object convertedValue = converter.convertValue(context, target, member, propertyName, value, memberType); - if (!NO_CONVERSION_POSSIBLE.equals(convertedValue)) { + if (convertedValue != NO_CONVERSION_POSSIBLE) { result.add(convertedValue); } } diff --git a/core/src/test/java/org/apache/struts2/conversion/impl/CollectionConverterTest.java b/core/src/test/java/org/apache/struts2/conversion/impl/CollectionConverterTest.java new file mode 100644 index 000000000..34f4717cc --- /dev/null +++ b/core/src/test/java/org/apache/struts2/conversion/impl/CollectionConverterTest.java @@ -0,0 +1,86 @@ +/* + * 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 org.apache.struts2.conversion.impl; + +import org.apache.struts2.ActionContext; +import org.apache.struts2.conversion.TypeConverter; +import org.apache.struts2.XWorkTestCase; +import org.apache.struts2.util.ValueStack; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; + +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. + * <p> + * 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()); + } + + public static class Holder { + private List<String> names = new ArrayList<>(); + private List<Long> numbers = new ArrayList<>(); + + public List<String> getNames() { + return names; + } + + public void setNames(List<String> names) { + this.names = names; + } + + public List<Long> getNumbers() { + return numbers; + } + + public void setNumbers(List<Long> numbers) { + this.numbers = numbers; + } + } +}
