This is an automated email from the ASF dual-hosted git repository.
lukaszlenart pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/struts.git
The following commit(s) were added to refs/heads/main by this push:
new 655310bd1 WW-5701 fix(conversion): compare the conversion marker by
identity, not equals (#1874)
655310bd1 is described below
commit 655310bd1cabf53825005c989f7b4853811cd839
Author: Lukasz Lenart <[email protected]>
AuthorDate: Mon Aug 31 20:03:26 2026 +0200
WW-5701 fix(conversion): compare the conversion marker by identity, not
equals (#1874)
* 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]>
* 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 <[email protected]>
---------
Co-authored-by: Claude Opus 5 <[email protected]>
---
.../conversion/impl/CollectionConverter.java | 6 +-
.../conversion/impl/CollectionConverterTest.java | 127 +++++++++++++++++++++
2 files changed, 130 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..8e30b52e0
--- /dev/null
+++
b/core/src/test/java/org/apache/struts2/conversion/impl/CollectionConverterTest.java
@@ -0,0 +1,127 @@
+/*
+ * 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.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.
+ * <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());
+ }
+
+ /**
+ * 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<String> names = new ArrayList<>();
+ private List<Long> numbers = new ArrayList<>();
+ private Set<Long> numberSet = new LinkedHashSet<>();
+
+ 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;
+ }
+
+ public Set<Long> getNumberSet() {
+ return numberSet;
+ }
+
+ public void setNumberSet(Set<Long> numberSet) {
+ this.numberSet = numberSet;
+ }
+ }
+}
