mirror of
https://github.com/apache/struts.git
synced 2026-09-11 00:29:40 +00:00
WW-5701 fix(conversion): compare the conversion marker by identity, not equals (#1874)
* WW-5701 fix(conversion): compare the conversion marker by identity, not 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> * WW-5701 test(conversion): cover the collection-source and single-value 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> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Lukasz Lenart
@@ -61,7 +61,7 @@ public class CollectionConverter extends DefaultTypeConverter {
|
||||
|
||||
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) {
|
||||
result.add(convertedValue);
|
||||
}
|
||||
}
|
||||
@@ -72,7 +72,7 @@ public class CollectionConverter extends DefaultTypeConverter {
|
||||
|
||||
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);
|
||||
}
|
||||
}
|
||||
@@ -80,7 +80,7 @@ public class CollectionConverter extends DefaultTypeConverter {
|
||||
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) {
|
||||
result.add(convertedValue);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,127 @@
|
||||
/*
|
||||
* Licensed to the Apache Software Foundation (ASF) under one
|
||||
* or more contributor license agreements. See the NOTICE file
|
||||
* distributed with this work for additional information
|
||||
* regarding copyright ownership. The ASF licenses this file
|
||||
* to you under the Apache License, Version 2.0 (the
|
||||
* "License"); you may not use this file except in compliance
|
||||
* with the License. You may obtain a copy of the License at
|
||||
*
|
||||
* http://www.apache.org/licenses/LICENSE-2.0
|
||||
*
|
||||
* Unless required by applicable law or agreed to in writing,
|
||||
* software distributed under the License is distributed on an
|
||||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
|
||||
* KIND, either express or implied. See the License for the
|
||||
* specific language governing permissions and limitations
|
||||
* under the License.
|
||||
*/
|
||||
package org.apache.struts2.conversion.impl;
|
||||
|
||||
import org.apache.struts2.ActionContext;
|
||||
import org.apache.struts2.conversion.TypeConverter;
|
||||
import org.apache.struts2.XWorkTestCase;
|
||||
import org.apache.struts2.util.ValueStack;
|
||||
|
||||
import java.util.ArrayList;
|
||||
import java.util.Arrays;
|
||||
import java.util.HashSet;
|
||||
import java.util.LinkedHashSet;
|
||||
import java.util.List;
|
||||
import java.util.Set;
|
||||
|
||||
public class CollectionConverterTest extends XWorkTestCase {
|
||||
|
||||
/**
|
||||
* WW-5701: the marker constant's value is ordinary text, so an element that genuinely holds
|
||||
* that text converts successfully and must be kept.
|
||||
* <p>
|
||||
* The value is built at runtime rather than written as a literal on purpose: a literal would be
|
||||
* interned to the very same instance as the constant's value, which no request-derived
|
||||
* parameter ever is. A servlet container builds parameter values from the request bytes.
|
||||
*/
|
||||
public void testElementWhoseTextEqualsTheMarkerIsKept() {
|
||||
String asSubmittedByAUser = new String("ognl.NoConversionPossible".toCharArray());
|
||||
assertNotSame("fixture must not be interned", TypeConverter.NO_CONVERSION_POSSIBLE, asSubmittedByAUser);
|
||||
|
||||
Holder holder = new Holder();
|
||||
ValueStack vs = ActionContext.getContext().getValueStack();
|
||||
vs.push(holder);
|
||||
|
||||
vs.setValue("names", new String[]{"alpha", asSubmittedByAUser, "omega"});
|
||||
|
||||
assertEquals(Arrays.asList("alpha", "ognl.NoConversionPossible", "omega"), holder.getNames());
|
||||
}
|
||||
|
||||
/**
|
||||
* The guard must still do its job: a genuinely unconvertible element is dropped.
|
||||
*/
|
||||
public void testUnconvertibleElementIsStillDropped() {
|
||||
Holder holder = new Holder();
|
||||
ValueStack vs = ActionContext.getContext().getValueStack();
|
||||
vs.push(holder);
|
||||
|
||||
vs.setValue("numbers", new String[]{"1", "not-a-number", "3"});
|
||||
|
||||
assertEquals(Arrays.asList(1L, 3L), holder.getNumbers());
|
||||
}
|
||||
|
||||
/**
|
||||
* The same guard on the path taken when the submitted value is itself a collection rather than
|
||||
* an array - here a List feeding a Set-typed property.
|
||||
*/
|
||||
public void testUnconvertibleElementIsDroppedFromACollectionSource() {
|
||||
Holder holder = new Holder();
|
||||
ValueStack vs = ActionContext.getContext().getValueStack();
|
||||
vs.push(holder);
|
||||
|
||||
vs.setValue("numberSet", Arrays.asList("1", "not-a-number", "3"));
|
||||
|
||||
assertEquals(new HashSet<>(Arrays.asList(1L, 3L)), holder.getNumberSet());
|
||||
}
|
||||
|
||||
/**
|
||||
* The same guard on the path taken when a single value is assigned to a collection property.
|
||||
* The property is seeded first so that a setter which is never called cannot pass vacuously.
|
||||
*/
|
||||
public void testUnconvertibleSingleValueIsDropped() {
|
||||
Holder holder = new Holder();
|
||||
holder.setNumbers(new ArrayList<>(Arrays.asList(99L)));
|
||||
ValueStack vs = ActionContext.getContext().getValueStack();
|
||||
vs.push(holder);
|
||||
|
||||
vs.setValue("numbers", "not-a-number");
|
||||
|
||||
assertEquals(List.of(), holder.getNumbers());
|
||||
}
|
||||
|
||||
public static class Holder {
|
||||
private List<String> names = new ArrayList<>();
|
||||
private List<Long> numbers = new ArrayList<>();
|
||||
private Set<Long> numberSet = new LinkedHashSet<>();
|
||||
|
||||
public List<String> getNames() {
|
||||
return names;
|
||||
}
|
||||
|
||||
public void setNames(List<String> names) {
|
||||
this.names = names;
|
||||
}
|
||||
|
||||
public List<Long> getNumbers() {
|
||||
return numbers;
|
||||
}
|
||||
|
||||
public void setNumbers(List<Long> numbers) {
|
||||
this.numbers = numbers;
|
||||
}
|
||||
|
||||
public Set<Long> getNumberSet() {
|
||||
return numberSet;
|
||||
}
|
||||
|
||||
public void setNumberSet(Set<Long> numberSet) {
|
||||
this.numberSet = numberSet;
|
||||
}
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user