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