Lukasz Lenart created WW-5701:
---------------------------------
Summary: CollectionConverter silently drops a legitimate element
whose text equals the NO_CONVERSION_POSSIBLE marker
Key: WW-5701
URL: https://issues.apache.org/jira/browse/WW-5701
Project: Struts 2
Issue Type: Bug
Reporter: Lukasz Lenart
h2. Summary
CollectionConverter decides whether an element converted successfully by
comparing the converted
value to the marker constant TypeConverter.NO_CONVERSION_POSSIBLE with
equals() rather than
reference identity. The marker's value is the ordinary text
"ognl.NoConversionPossible", so a
legitimate element whose content happens to be that text is mistaken for a
conversion failure and
silently dropped from the resulting collection.
h2. Reproduced
On main at 05ad78a06. A plain action property declared as a List of String,
bound from a normal
multi-valued request parameter:
{noformat}
vs.setValue("names", new String[]{"alpha", "ognl.NoConversionPossible",
"omega"});
result: [alpha, omega] expected: [alpha, ognl.NoConversionPossible, omega]
{noformat}
The middle element is discarded. No conversion error is registered, because no
conversion actually
failed - the element converted fine and was then thrown away by the guard. So
the loss is entirely
silent: the action sees a shorter collection with no indication anything
happened.
h2. Where
Three sites in
core/src/main/java/org/apache/struts2/conversion/impl/CollectionConverter.java,
all
using equals() against the marker:
* line 64 - the array branch
* line 75 - the Collection branch
* line 83 - the single-value branch
Note the exposure is wider than "collections explicitly declared to hold
Strings". At lines 48-50 of
the same file, when no element type can be determined the member type defaults
to String.class, so
an untyped collection is affected too.
h2. Why identity is correct
The constant is declared as Object, not String (TypeConverter.java:49), so it
is not a JLS constant
variable and is not inlined at compile time - every reader loads the one shared
instance. A value
arriving from an HTTP request is a distinct object even when its content is
identical, so reference
comparison distinguishes "the converter signalled failure" from "the user
submitted this text",
which is exactly the distinction being made here.
OGNL itself compares this marker by identity - its own conversion loop compiles
to if_acmpne
against the same field.
h2. Proposed fix
Replace equals() with reference comparison at all three sites.
h2. Relationship to WW-5700
WW-5700 fixed the mirror-image defect in XWorkMapPropertyAccessor and
XWorkListPropertyAccessor,
where the marker was _stored_ into a typed collection instead of being skipped;
that fix
deliberately used identity comparison for the reason given above.
CollectionConverter is the older
code that already had a guard, but used the weaker comparison. The two were
found together during
review of WW-5700 (PR #1873) and are separate defects: WW-5700 stores something
it should not,
this one drops something it should keep.
h2. Backward compatibility
Narrow, and strictly a fix. The only behaviour that changes is that an element
whose text is exactly
"ognl.NoConversionPossible" is now kept rather than discarded. Nothing can
reasonably depend on the
current silent removal.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
