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) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
