[
https://issues.apache.org/jira/browse/WW-5701?focusedWorklogId=1038336&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1038336
]
ASF GitHub Bot logged work on WW-5701:
--------------------------------------
Author: ASF GitHub Bot
Created on: 27/Aug/26 16:21
Start Date: 27/Aug/26 16:21
Worklog Time Spent: 10m
Work Description: lukaszlenart opened a new pull request, #1879:
URL: https://github.com/apache/struts/pull/1879
Backport of [#1874](https://github.com/apache/struts/pull/1874) to the 6.x
line.
`NO_CONVERSION_POSSIBLE` is an ordinary `String` constant, so
`CollectionConverter`'s
`!NO_CONVERSION_POSSIBLE.equals(convertedValue)` guard also matched a
*successfully* converted element
whose own text happens to be `"ognl.NoConversionPossible"` — and silently
dropped it. Only the constant
instance itself signals a failed conversion, so the comparison is now by
identity.
Verified affected on 6.x before fixing:
`testElementWhoseTextEqualsTheMarkerIsKept` fails on
`support/struts-6-x-x` with `expected:<[alpha, ognl.NoConversionPossible,
omega]> but was:<[alpha, omega]>`.
Note on the fixture, because it is easy to get wrong: the test value is
built at runtime with
`new String(...toCharArray())` and guarded by `assertNotSame`. A String
*literal* is interned to the very
same instance as the constant, so a literal-based test would pass vacuously
even against the unfixed code.
No request-derived parameter is ever that instance — a servlet container
builds parameter values from the
request bytes.
The companion test pins that the guard still works: a genuinely
unconvertible element is still dropped.
Full `core` suite green: 2719 tests, 0 failures.
Fixes [WW-5701](https://issues.apache.org/jira/browse/WW-5701)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Issue Time Tracking
-------------------
Worklog Id: (was: 1038336)
Time Spent: 0.5h (was: 20m)
> 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
> Assignee: Lukasz Lenart
> Priority: Major
> Fix For: 6.12.0, 7.4.0
>
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> h2. Summary
> CollectionConverter decides whether an element converted successfully by
> comparing the converted value to the conversion-failure marker declared in
> TypeConverter (line 49), using 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 comparing against the marker with equals():
> * 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 marker field is declared as Object, not String (TypeConverter.java line
> 49), so it is not a JLS constant variable and is not inlined at compile time
> - every reader loads the one shared instance at runtime, third-party
> converters included. A parameter value built by a servlet container from
> request bytes 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 a reference-comparison bytecode instruction against the same
> field, not an equals() call.
> To be precise about the limit: reference comparison protects values arriving
> from a request, which is the case that matters here. It does not protect a
> value that happens to be interned, since all identical String literals share
> one instance - application code calling the converter programmatically with
> such a literal would still lose it. Closing that as well 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.
> 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 reference
> 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.
> h2. Status
> Fixed in PR https://github.com/apache/struts/pull/1874 - two regression
> tests, written test-first and mutation-checked. The fixture for the
> kept-element test is built at runtime rather than written as a String
> literal, because a literal would be interned to the same instance as the
> marker and the test would pass vacuously; an assertNotSame guards that. Full
> core suite green.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
