WW-5701 fix(conversion): compare the conversion marker by identity, not equals - #1874
Open
lukaszlenart wants to merge 2 commits into
Open
WW-5701 fix(conversion): compare the conversion marker by identity, not equals#1874lukaszlenart wants to merge 2 commits into
lukaszlenart wants to merge 2 commits into
Conversation
…ot equals 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 from the resulting collection. Nothing signalled the loss: no conversion had failed, so no conversion error was registered and the action simply saw a shorter collection. The exposure is not limited to collections declared to hold Strings - when no element type can be determined the member type defaults to String.class, so untyped collections are affected too. Compare by reference instead, at all three sites. Identity is correct here rather than incidental. The constant is declared Object, not String, 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, including in third-party converters compiled elsewhere. 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". Please do not simplify this back to equals(), which is what caused the bug. WW-5700 fixed the mirror-image defect in the map and list property accessors, which stored the marker instead of skipping it, and used identity comparison for the same reason. Found while reviewing that fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e guard paths The Sonar quality gate on the pull request failed at 77.8% coverage of new code: the marker guard was only exercised on the array-source path, leaving the false branch of the other two guards uncovered. Both added paths are reachable from a request - a Set-typed property fed from a List, and a single-valued parameter assigned to a collection property. The single-value holder is seeded before the assignment so that a setter which is never called cannot make the test pass vacuously. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes WW-5701 by preventing CollectionConverter from discarding legitimate user-supplied values that happen to equal the textual representation of the NO_CONVERSION_POSSIBLE marker.
Changes:
- Switch marker checks in
CollectionConverterfromequals()to reference comparison (!=) to correctly detect converter-failure signalling. - Add a new
CollectionConverterTestcovering the WW-5701 regression and related conversion-drop scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| core/src/main/java/org/apache/struts2/conversion/impl/CollectionConverter.java | Uses identity comparison against NO_CONVERSION_POSSIBLE to avoid false positives when user values equal the marker’s text. |
| core/src/test/java/org/apache/struts2/conversion/impl/CollectionConverterTest.java | Adds regression tests ensuring marker-text values are preserved and truly unconvertible values are still dropped. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| 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) { |
| 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) { |
| 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) { |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes WW-5701
Problem
CollectionConverterdecided whether an element had converted successfully bycomparing the result to
TypeConverter.NO_CONVERSION_POSSIBLEwithequals().The marker's value is the ordinary text
ognl.NoConversionPossible, so anelement that genuinely held that text converted fine and was then silently
discarded.
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: atCollectionConverter.java:48-50, when no element type can be determined themember 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:
Object, notString(
TypeConverter.java:49), so it is not a JLS constant variable and is notinlined into referencing class files. Every reference resolves to the one field
value at runtime, third-party converters compiled elsewhere included.
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: theproduction change was stashed to confirm the test fails without it
(
[alpha, omega]).testElementWhoseTextEqualsTheMarkerIsKept— the fixture is built at runtimerather 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
assertNotSameguards that, so the test cannot silently regress intotesting 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 / #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