From 9a9d1edce086f7e9ade461fee601c2cc3f402e08 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 27 Aug 2026 11:10:14 +0200 Subject: [PATCH 1/2] 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 --- .../conversion/impl/CollectionConverter.java | 6 +- .../impl/CollectionConverterTest.java | 86 +++++++++++++++++++ 2 files changed, 89 insertions(+), 3 deletions(-) create mode 100644 core/src/test/java/org/apache/struts2/conversion/impl/CollectionConverterTest.java 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 815380927b..26bb865651 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 Object convertValue(Map context, Object target, Member me 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 Object convertValue(Map context, Object target, Member me 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 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 (!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 0000000000..34f4717cc1 --- /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. + *

+ * 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 names = new ArrayList<>(); + private List numbers = new ArrayList<>(); + + 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; + } + } +} From 68d0886374b310081d059fc34b05b95f722e4056 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 27 Aug 2026 18:52:48 +0200 Subject: [PATCH 2/2] WW-5701 test(conversion): cover the collection-source and single-value guard paths The Sonar quality gate on the pull request failed at 77.8% coverage of new code: the marker guard was only exercised on the array-source path, leaving the false branch of the other two guards uncovered. Both added paths are reachable from a request - a Set-typed property fed from a List, and a single-valued parameter assigned to a collection property. The single-value holder is seeded before the assignment so that a setter which is never called cannot make the test pass vacuously. Co-Authored-By: Claude Opus 5 --- .../impl/CollectionConverterTest.java | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) 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 index 34f4717cc1..8e30b52e04 100644 --- a/core/src/test/java/org/apache/struts2/conversion/impl/CollectionConverterTest.java +++ b/core/src/test/java/org/apache/struts2/conversion/impl/CollectionConverterTest.java @@ -25,7 +25,10 @@ import java.util.ArrayList; import java.util.Arrays; +import java.util.HashSet; +import java.util.LinkedHashSet; import java.util.List; +import java.util.Set; public class CollectionConverterTest extends XWorkTestCase { @@ -63,9 +66,39 @@ public void testUnconvertibleElementIsStillDropped() { 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(List.of(), 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; @@ -82,5 +115,13 @@ public List getNumbers() { public void setNumbers(List numbers) { this.numbers = numbers; } + + public Set getNumberSet() { + return numberSet; + } + + public void setNumberSet(Set numberSet) { + this.numberSet = numberSet; + } } }