From 11f5591b743e8c5ad925cccb4ba07580eb7d1702 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 23 Aug 2026 19:44:40 +0200 Subject: [PATCH] WW-5685 fix(conversion): skip the mapped key instead of abandoning the file 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 --- .../impl/DefaultConversionFileProcessor.java | 8 +- .../DefaultConversionFileProcessorTest.java | 89 +++++++++++++++++++ .../conversion/impl/XWorkConverterTest.java | 42 +++++++++ .../util/PropertiesCollisionBaseAction.java | 27 ++++++ .../util/PropertiesCollisionSubAction.java | 27 ++++++ ...sCollisionBaseAction-conversion.properties | 24 +++++ ...esCollisionSubAction-conversion.properties | 19 ++++ 7 files changed, 235 insertions(+), 1 deletion(-) create mode 100644 core/src/test/java/org/apache/struts2/conversion/impl/DefaultConversionFileProcessorTest.java create mode 100644 core/src/test/java/org/apache/struts2/util/PropertiesCollisionBaseAction.java create mode 100644 core/src/test/java/org/apache/struts2/util/PropertiesCollisionSubAction.java create mode 100644 core/src/test/resources/org/apache/struts2/util/PropertiesCollisionBaseAction-conversion.properties create mode 100644 core/src/test/resources/org/apache/struts2/util/PropertiesCollisionSubAction-conversion.properties 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 194ebb28dc..8d3ca581ee 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 void process(Map mapping, Class clazz, String converterFi 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 0000000000..eb806827d3 --- /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 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 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 eec40bd9e4..753d28ef4f 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.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 void testClassLevelEntriesAfterAKeyCollisionAreStillRegistered() { 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 0000000000..75a84ebdc6 --- /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 0000000000..fe5fbc5aef --- /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 0000000000..8b7a164bd1 --- /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 0000000000..c88e1b22a4 --- /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