This is an automated email from the ASF dual-hosted git repository.
lukaszlenart pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/struts.git
The following commit(s) were added to refs/heads/main by this push:
new b8aa6cb43 WW-5685 fix(conversion): skip the mapped key instead of
abandoning the file (#1860)
b8aa6cb43 is described below
commit b8aa6cb439fe786a005dd50aa2d25ead216d27a9
Author: Lukasz Lenart <[email protected]>
AuthorDate: Sun Aug 23 20:04:05 2026 +0200
WW-5685 fix(conversion): skip the mapped key instead of abandoning the file
(#1860)
DefaultConversionFileProcessor.process skipped keys already present in the
converter mapping with a break rather than a continue, so the first
already-mapped entry ended the loop and every remaining entry in that
-conversion.properties file was silently dropped. No warning, no error;
the affected properties just fell back to default conversion.
XWorkConverter.buildConverterMapping walks the class, then its interfaces,
then its superclass, passing one accumulating mapping into each call, so a
key claimed earlier in that walk aborted a later file outright. Annotation
derived entries land in the same map and could abort a file the same way.
Properties extends Hashtable and entrySet() has no defined order, so which
entries survived depended on hash order rather than file order.
WW-3871 fixed the same defect in the annotation path (PR #1812, 7.3.0);
this is the properties-file path that change did not touch.
Two tests, both mutation-checked by reverting continue to break:
DefaultConversionFileProcessorTest derives the colliding key from the
actual iteration order at run time and pre-maps whichever key comes first,
so break registers nothing at all. That makes it discriminating on any JDK
in any hash order, rather than depending on where a fixture's colliding key
happens to land.
XWorkConverterTest covers the realistic trigger through the hierarchy walk,
a subclass and superclass file sharing a key. This one does depend on the
fixture's iteration order, and the first version of it was vacuous -- the
shared key landed last, so break dropped nothing and the test passed
against the unfixed code. The key is now named so that it is read first
(verified identical on Temurin 17, 21 and 25), and the test asserts that
precondition, so a future reordering fails loudly instead of going quiet.
Co-authored-by: Claude Opus 5 <[email protected]>
---
.../impl/DefaultConversionFileProcessor.java | 8 +-
.../impl/DefaultConversionFileProcessorTest.java | 89 ++++++++++++++++++++++
.../conversion/impl/XWorkConverterTest.java | 42 ++++++++++
.../util/PropertiesCollisionBaseAction.java | 27 +++++++
.../struts2/util/PropertiesCollisionSubAction.java | 27 +++++++
...ertiesCollisionBaseAction-conversion.properties | 24 ++++++
...pertiesCollisionSubAction-conversion.properties | 19 +++++
7 files changed, 235 insertions(+), 1 deletion(-)
diff --git
a/core/src/main/java/org/apache/struts2/conversion/impl/DefaultConversionFileProcessor.java
b/core/src/main/java/org/apache/struts2/conversion/impl/DefaultConversionFileProcessor.java
index 194ebb28d..8d3ca581e 100644
---
a/core/src/main/java/org/apache/struts2/conversion/impl/DefaultConversionFileProcessor.java
+++
b/core/src/main/java/org/apache/struts2/conversion/impl/DefaultConversionFileProcessor.java
@@ -69,7 +69,13 @@ public class DefaultConversionFileProcessor implements
ConversionFileProcessor {
String key = (String) entry.getKey();
if (mapping.containsKey(key)) {
- break;
+ // Skip this entry only. Until WW-5685 this was a
break, which abandoned the
+ // rest of the file: a key claimed earlier in the
hierarchy walk silently
+ // dropped every remaining entry, and
Properties.entrySet() has no defined
+ // order, so which ones survived depended on hash
order.
+ LOG.debug("Skipping [{}] from [{}]: key is already
mapped by a higher precedence source",
+ key, converterFilename);
+ continue;
}
// for keyProperty of Set
if
(key.startsWith(DefaultObjectTypeDeterminer.KEY_PROPERTY_PREFIX)
diff --git
a/core/src/test/java/org/apache/struts2/conversion/impl/DefaultConversionFileProcessorTest.java
b/core/src/test/java/org/apache/struts2/conversion/impl/DefaultConversionFileProcessorTest.java
new file mode 100644
index 000000000..eb806827d
--- /dev/null
+++
b/core/src/test/java/org/apache/struts2/conversion/impl/DefaultConversionFileProcessorTest.java
@@ -0,0 +1,89 @@
+/*
+ * 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.XWorkTestCase;
+import org.apache.struts2.conversion.ConversionFileProcessor;
+import org.apache.struts2.util.ClassLoaderUtil;
+import org.apache.struts2.util.PropertiesCollisionBaseAction;
+
+import java.io.InputStream;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Properties;
+
+/**
+ * WW-5685: an already-mapped key must skip that one entry, not abandon the
rest of the file.
+ */
+public class DefaultConversionFileProcessorTest extends XWorkTestCase {
+
+ private static final String FILENAME =
+
"org/apache/struts2/util/PropertiesCollisionBaseAction-conversion.properties";
+
+ private static final String SENTINEL = "supplied by a higher precedence
source";
+
+ /**
+ * {@code Properties} extends {@code Hashtable}, so {@code entrySet()} has
no defined iteration
+ * order and a fixture cannot pin down which key is seen first.
Pre-mapping whichever key the
+ * iteration actually yields first makes this test discriminating on any
JDK and in any hash
+ * order: with the {@code break} this replaced, the loop stopped on that
first entry and
+ * registered nothing at all.
+ */
+ public void testEntriesAfterAnAlreadyMappedKeyAreStillRegistered() throws
Exception {
+ Properties fixture = loadFixture();
+ String firstKey = (String)
fixture.entrySet().iterator().next().getKey();
+
+ Map<String, Object> mapping = new HashMap<>();
+ mapping.put(firstKey, SENTINEL);
+
+ processor().process(mapping, PropertiesCollisionBaseAction.class,
FILENAME);
+
+ assertEquals("the already-mapped key must not be overwritten",
SENTINEL, mapping.get(firstKey));
+ for (Object key : fixture.keySet()) {
+ assertTrue("entry [" + key + "] was dropped after the collision on
[" + firstKey + "]",
+ mapping.containsKey(key));
+ }
+ }
+
+ /**
+ * The complementary case: with nothing pre-mapped, every entry registers.
Guards against a
+ * "fix" that skips too much rather than too little.
+ */
+ public void testAllEntriesRegisterWhenNothingIsAlreadyMapped() throws
Exception {
+ Properties fixture = loadFixture();
+
+ Map<String, Object> mapping = new HashMap<>();
+ processor().process(mapping, PropertiesCollisionBaseAction.class,
FILENAME);
+
+ assertEquals("every entry in the file must register", fixture.size(),
mapping.size());
+ }
+
+ private ConversionFileProcessor processor() {
+ return container.getInstance(ConversionFileProcessor.class);
+ }
+
+ private Properties loadFixture() throws Exception {
+ Properties properties = new Properties();
+ try (InputStream is = ClassLoaderUtil.getResourceAsStream(FILENAME,
getClass())) {
+ properties.load(is);
+ }
+ assertTrue("the fixture must hold more than one key to be
discriminating", properties.size() > 1);
+ return properties;
+ }
+}
diff --git
a/core/src/test/java/org/apache/struts2/conversion/impl/XWorkConverterTest.java
b/core/src/test/java/org/apache/struts2/conversion/impl/XWorkConverterTest.java
index eec40bd9e..753d28ef4 100644
---
a/core/src/test/java/org/apache/struts2/conversion/impl/XWorkConverterTest.java
+++
b/core/src/test/java/org/apache/struts2/conversion/impl/XWorkConverterTest.java
@@ -50,6 +50,8 @@ import org.apache.struts2.util.EmptyKeyConversionAction;
import org.apache.struts2.util.ExplicitKeyConversionAction;
import org.apache.struts2.util.FieldConversionAction;
import org.apache.struts2.util.InheritedMethodConversionSubAction;
+import org.apache.struts2.util.PropertiesCollisionSubAction;
+import org.apache.struts2.util.ClassLoaderUtil;
import org.apache.struts2.util.MyBean;
import org.apache.struts2.util.MyBeanAction;
@@ -911,6 +913,46 @@ public class XWorkConverterTest extends XWorkTestCase {
assertEquals("true",
freshConverter.getConverter(CollidingKeyConversionAction.class,
"CreateIfNull_afterTheCollision"));
}
+ private static final String PROPERTIES_COLLISION_BASE_FILE =
+
"org/apache/struts2/util/PropertiesCollisionBaseAction-conversion.properties";
+
+ /**
+ * The key a {@link Properties} load yields first for the given file.
{@code Properties} extends
+ * {@code Hashtable}, so the order is a function of the key strings rather
than the file, and a
+ * collision on any later key would leave the test above unable to detect
the defect.
+ */
+ private static String firstKeyOf(String filename) throws IOException {
+ Properties properties = new Properties();
+ try (InputStream is = ClassLoaderUtil.getResourceAsStream(filename,
XWorkConverterTest.class)) {
+ properties.load(is);
+ }
+ return (String) properties.keySet().iterator().next();
+ }
+
+ /**
+ * WW-5685, the properties-file counterpart of the test above and the
realistic trigger for it.
+ * The hierarchy walk reads the subclass file first and passes one
accumulating mapping down, so
+ * by the time the superclass file is read its shared key is taken. That
collision used to
+ * abandon the superclass file outright, dropping every other entry in it.
+ */
+ public void testPropertiesEntriesAfterAKeyCollisionAreStillRegistered()
throws Exception {
+ XWorkConverter freshConverter = container.inject(XWorkConverter.class);
+ freshConverter.setTypeConverterHolder(new StrutsTypeConverterHolder());
+
+ assertEquals("the fixture only discriminates while the shared key is
read first; "
+ + "Properties iteration order has changed and it must
be renamed again",
+ "CreateIfNull_overridden",
firstKeyOf(PROPERTIES_COLLISION_BASE_FILE));
+
+ // the subclass file is read first, so it keeps the shared key
+ assertEquals("fromSub",
+
freshConverter.getConverter(PropertiesCollisionSubAction.class,
"CreateIfNull_overridden"));
+
+ for (String key : new String[]{"CreateIfNull_alpha",
"CreateIfNull_bravo", "CreateIfNull_charlie"}) {
+ assertEquals("superclass entry [" + key + "] was dropped after the
collision", "true",
+
freshConverter.getConverter(PropertiesCollisionSubAction.class, key));
+ }
+ }
+
public void testClassLevelEmptyKeyRegistersNoMapping() throws Exception {
XWorkConverter freshConverter = container.inject(XWorkConverter.class);
freshConverter.setTypeConverterHolder(new StrutsTypeConverterHolder());
diff --git
a/core/src/test/java/org/apache/struts2/util/PropertiesCollisionBaseAction.java
b/core/src/test/java/org/apache/struts2/util/PropertiesCollisionBaseAction.java
new file mode 100644
index 000000000..75a84ebdc
--- /dev/null
+++
b/core/src/test/java/org/apache/struts2/util/PropertiesCollisionBaseAction.java
@@ -0,0 +1,27 @@
+/*
+ * 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.util;
+
+/**
+ * Superclass half of the WW-5685 fixture. Its {@code -conversion.properties}
file declares a key
+ * that {@link PropertiesCollisionSubAction}'s file has already claimed, plus
three keys of its own
+ * that must survive the collision.
+ */
+public class PropertiesCollisionBaseAction {
+}
diff --git
a/core/src/test/java/org/apache/struts2/util/PropertiesCollisionSubAction.java
b/core/src/test/java/org/apache/struts2/util/PropertiesCollisionSubAction.java
new file mode 100644
index 000000000..fe5fbc5ae
--- /dev/null
+++
b/core/src/test/java/org/apache/struts2/util/PropertiesCollisionSubAction.java
@@ -0,0 +1,27 @@
+/*
+ * 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.util;
+
+/**
+ * Subclass half of the WW-5685 fixture. Processed first by the hierarchy walk
in
+ * {@code XWorkConverter.buildConverterMapping}, so the key it declares is
already mapped by the
+ * time the superclass file is read.
+ */
+public class PropertiesCollisionSubAction extends
PropertiesCollisionBaseAction {
+}
diff --git
a/core/src/test/resources/org/apache/struts2/util/PropertiesCollisionBaseAction-conversion.properties
b/core/src/test/resources/org/apache/struts2/util/PropertiesCollisionBaseAction-conversion.properties
new file mode 100644
index 000000000..8b7a164bd
--- /dev/null
+++
b/core/src/test/resources/org/apache/struts2/util/PropertiesCollisionBaseAction-conversion.properties
@@ -0,0 +1,24 @@
+#
+# 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.
+#
+# CreateIfNull_overridden collides with
PropertiesCollisionSubAction-conversion.properties.
+# Until WW-5685 that collision abandoned the whole file, dropping the three
keys below.
+CreateIfNull_overridden=fromBase
+CreateIfNull_alpha=true
+CreateIfNull_bravo=true
+CreateIfNull_charlie=true
diff --git
a/core/src/test/resources/org/apache/struts2/util/PropertiesCollisionSubAction-conversion.properties
b/core/src/test/resources/org/apache/struts2/util/PropertiesCollisionSubAction-conversion.properties
new file mode 100644
index 000000000..c88e1b22a
--- /dev/null
+++
b/core/src/test/resources/org/apache/struts2/util/PropertiesCollisionSubAction-conversion.properties
@@ -0,0 +1,19 @@
+#
+# 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.
+#
+CreateIfNull_overridden=fromSub
