[
https://issues.apache.org/jira/browse/WW-5701?focusedWorklogId=1038243&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1038243
]
ASF GitHub Bot logged work on WW-5701:
--------------------------------------
Author: ASF GitHub Bot
Created on: 27/Aug/26 09:14
Start Date: 27/Aug/26 09:14
Worklog Time Spent: 10m
Work Description: lukaszlenart opened a new pull request, #1874:
URL: https://github.com/apache/struts/pull/1874
Fixes [WW-5701](https://issues.apache.org/jira/browse/WW-5701)
### Problem
`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.
```
vs.setValue("names", new String[]{"alpha", "ognl.NoConversionPossible",
"omega"});
result: [alpha, omega]
expected: [alpha, ognl.NoConversionPossible, omega]
```
Nothing signalled the loss. No conversion had failed, so no conversion error
was
registered — the action simply saw a shorter collection.
The exposure is wider than collections declared to hold `String`: at
`CollectionConverter.java:48-50`, when no element type can be determined the
member type defaults to `String.class`, so untyped collections are affected
too.
### Fix
Compare by reference instead, at all three sites (lines 64, 75, 83).
**Please do not simplify this back to `equals()`** — that is what caused the
bug.
Identity is correct here rather than incidental:
- The constant is declared `Object`, **not** `String`
(`TypeConverter.java:49`), 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, third-party converters compiled elsewhere included.
- 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"*.
### Scope, stated precisely
This protects values arriving from a request, which is the case that matters.
It does **not** protect a value that happens to be interned — application
code
calling the converter programmatically with a String literal would still see
it
dropped, because all identical literals share one interned instance.
Closing that too would mean giving the marker an identity no user string can
share, which changes a published constant and is a binary-compatibility
question
rather than a bug fix. Out of scope here, and noted so the limit is not
misread as an oversight.
### Testing
New `CollectionConverterTest`, written test-first and mutation-checked: the
production change was stashed to confirm the test fails without it
(`[alpha, omega]`).
- `testElementWhoseTextEqualsTheMarkerIsKept` — the fixture is built at
runtime
rather than written as a literal, precisely because a literal would be
interned
to the same instance as the constant and would not represent a real
request.
An `assertNotSame` guards that, so the test cannot silently regress into
testing nothing.
- `testUnconvertibleElementIsStillDropped` — the guard must keep doing its
job;
this passed before and after the change.
Full core suite: **3199 tests, 0 failures**.
### Related
[WW-5700](https://issues.apache.org/jira/browse/WW-5700) / #1873 fixes the
mirror-image defect in the map and list property accessors, which *stored*
the
marker instead of skipping it, and uses identity comparison for the same
reason.
This one was found while reviewing that fix. The two are independent and can
merge in either order.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Issue Time Tracking
-------------------
Worklog Id: (was: 1038243)
Remaining Estimate: 0h
Time Spent: 10m
> 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
> Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> 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 an 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)
