[
https://issues.apache.org/jira/browse/WW-5701?focusedWorklogId=1038343&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1038343
]
ASF GitHub Bot logged work on WW-5701:
--------------------------------------
Author: ASF GitHub Bot
Created on: 27/Aug/26 17:03
Start Date: 27/Aug/26 17:03
Worklog Time Spent: 10m
Work Description: Copilot commented on code in PR #1874:
URL: https://github.com/apache/struts/pull/1874#discussion_r3873973969
##########
core/src/main/java/org/apache/struts2/conversion/impl/CollectionConverter.java:
##########
@@ -61,7 +61,7 @@ public Object convertValue(Map<String, Object> context,
Object target, Member me
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) {
Review Comment:
Consider adding a short inline comment next to these identity checks
explaining that reference comparison is intentional (the marker’s value can
equal a legitimate user-submitted string). This helps prevent a future
‘cleanup’ that reintroduces the WW-5701 bug by switching back to `equals()`.
##########
core/src/main/java/org/apache/struts2/conversion/impl/CollectionConverter.java:
##########
@@ -72,15 +72,15 @@ public Object convertValue(Map<String, Object> context,
Object target, Member me
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);
}
}
} else {
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) {
Review Comment:
Consider adding a short inline comment next to these identity checks
explaining that reference comparison is intentional (the marker’s value can
equal a legitimate user-submitted string). This helps prevent a future
‘cleanup’ that reintroduces the WW-5701 bug by switching back to `equals()`.
##########
core/src/main/java/org/apache/struts2/conversion/impl/CollectionConverter.java:
##########
@@ -72,15 +72,15 @@ public Object convertValue(Map<String, Object> context,
Object target, Member me
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) {
Review Comment:
Consider adding a short inline comment next to these identity checks
explaining that reference comparison is intentional (the marker’s value can
equal a legitimate user-submitted string). This helps prevent a future
‘cleanup’ that reintroduces the WW-5701 bug by switching back to `equals()`.
Issue Time Tracking
-------------------
Worklog Id: (was: 1038343)
Time Spent: 50m (was: 40m)
> 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: 50m
> 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)
