WW-5685 Stop dropping the rest of a -conversion.properties file after the first already-mapped key

core/src/main/java/org/apache/struts2/conversion/impl/DefaultConversionFileProcessor.java

@@ -69,7 +69,13 @@ public void process(Map<String, Object> 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)

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;
+    }
+}

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());

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 {
+}

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 {
+}

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

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