You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@struts.apache.org by lu...@apache.org on 2026/08/27 09:14:37 UTC
(struts) branch fix/WW-5701-collection-converter-identity created (now 9a9d1edce)
This is an automated email from the ASF dual-hosted git repository.
lukaszlenart pushed a change to branch fix/WW-5701-collection-converter-identity
in repository https://gitbox.apache.org/repos/asf/struts.git
at 9a9d1edce WW-5701 fix(conversion): compare the conversion marker by identity, not equals
This branch includes the following new commits:
new 9a9d1edce WW-5701 fix(conversion): compare the conversion marker by identity, not equals
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(struts) 01/01: WW-5701 fix(conversion): compare the conversion marker by identity, not equals
Posted by lu...@apache.org.
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 <lu...@apache.org>
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 <no...@anthropic.com>
---
.../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;
+ }
+ }
+}