From 727c23cded486f3ad74e9d831df777e1de3b215d Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 27 Aug 2026 18:20:40 +0200 Subject: [PATCH 1/2] WW-5701 fix(conversion): compare the conversion marker by identity, not equals Backport of the 7.4.0 fix (#1874) to the 6.x line. NO_CONVERSION_POSSIBLE is an ordinary String constant, so comparing with equals() also matched a genuinely converted element whose own text happens to be "ognl.NoConversionPossible" - and silently dropped it from the collection. Only the constant instance itself signals a failed conversion, so compare by identity. 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/com/opensymphony/xwork2/conversion/impl/CollectionConverterTest.java 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..ac62ff5b2e --- /dev/null +++ b/core/src/test/java/com/opensymphony/xwork2/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 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.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 0fb61893693581d349ca2ef15656cca8b0411f07 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 27 Aug 2026 19:06:28 +0200 Subject: [PATCH 2/2] WW-5701 test(conversion): cover the collection-source and single-value guard paths Backport of the coverage tests added on main, where the Sonar quality gate 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 | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) 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 index ac62ff5b2e..25b9f951db 100644 --- a/core/src/test/java/com/opensymphony/xwork2/conversion/impl/CollectionConverterTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/conversion/impl/CollectionConverterTest.java @@ -25,7 +25,11 @@ 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 { @@ -63,9 +67,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(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; @@ -82,5 +116,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; + } } }