diff --git a/core/src/main/java/org/apache/struts2/conversion/annotations/ConversionRule.java b/core/src/main/java/org/apache/struts2/conversion/annotations/ConversionRule.java index fa75f69e46..db9793ecd9 100644 --- a/core/src/main/java/org/apache/struts2/conversion/annotations/ConversionRule.java +++ b/core/src/main/java/org/apache/struts2/conversion/annotations/ConversionRule.java @@ -18,6 +18,8 @@ */ package org.apache.struts2.conversion.annotations; +import org.apache.struts2.conversion.impl.DefaultObjectTypeDeterminer; + /** * ConversionRule * @@ -28,6 +30,30 @@ public enum ConversionRule { PROPERTY, COLLECTION, MAP, KEY, KEY_PROPERTY, ELEMENT, CREATE_IF_NULL; + /** + * The prefix a conversion mapping key carries for this rule, as read back by + * {@link DefaultObjectTypeDeterminer}. {@code PROPERTY} and {@code MAP} have no prefix of their + * own: map and collection metadata is read through the {@code Key_} and {@code Element_} keys. + * + *

{@code COLLECTION} deliberately derives {@code Collection_}, the deprecated spelling that + * {@link DefaultObjectTypeDeterminer} still falls back to (and logs an INFO about) for + * compatibility with existing annotations. Prefer {@link #ELEMENT}, whose {@code Element_} + * prefix is the current form.

+ * + * @return the mapping key prefix, never null; an empty string when the rule has none + * @since 7.3.0 + */ + public String prefix() { + return switch (this) { + case COLLECTION -> DefaultObjectTypeDeterminer.DEPRECATED_ELEMENT_PREFIX; + case CREATE_IF_NULL -> DefaultObjectTypeDeterminer.CREATE_IF_NULL_PREFIX; + case ELEMENT -> DefaultObjectTypeDeterminer.ELEMENT_PREFIX; + case KEY -> DefaultObjectTypeDeterminer.KEY_PREFIX; + case KEY_PROPERTY -> DefaultObjectTypeDeterminer.KEY_PROPERTY_PREFIX; + case PROPERTY, MAP -> ""; + }; + } + @Override public String toString() { return super.toString().toUpperCase(); diff --git a/core/src/main/java/org/apache/struts2/conversion/annotations/TypeConversion.java b/core/src/main/java/org/apache/struts2/conversion/annotations/TypeConversion.java index a28d8c9873..062fc167d9 100644 --- a/core/src/main/java/org/apache/struts2/conversion/annotations/TypeConversion.java +++ b/core/src/main/java/org/apache/struts2/conversion/annotations/TypeConversion.java @@ -48,9 +48,16 @@ *

Annotation usage:

* * - *

The TypeConversion annotation can be applied at property and method level.

+ *

The TypeConversion annotation can be applied at field and method level.

* * + *

The {@code org.apache.struts2.util} package also has dedicated {@code @Key}, + * {@code @Element}, {@code @KeyProperty} and {@code @CreateIfNull} annotations that {@link + * org.apache.struts2.conversion.impl.DefaultObjectTypeDeterminer} consults, on the field then its + * setter then its getter, before falling back to the converter mapping this annotation + * populates. If both a dedicated annotation and an equivalent {@code @TypeConversion} are declared + * for the same property, the dedicated annotation wins silently.

+ * *

Annotation parameters:

* * @@ -67,8 +74,11 @@ * * key * no - * The annotated property/key name - * The optional property name mostly used within TYPE level annotations. + * The resolved property name on a method; the field's own name on a field + * The property name the rule applies to. The matching prefix for the given rule + * (Key_, Element_, KeyProperty_, CreateIfNull_, or the deprecated + * Collection_) is prepended automatically unless the key already carries it. Required on TYPE level annotations, + * where there is no member name to derive it from. * * * type @@ -115,11 +125,10 @@ * private String convertInt; * * private String convertDouble; - * private List users = null; * * private HashMap keyValues = null; * - * @TypeConversion(type = ConversionType.APPLICATION) + * @TypeConversion() * public void setConvertInt( String convertInt ) { * this.convertInt = convertInt; * } @@ -129,6 +138,9 @@ * this.convertDouble = convertDouble; * } * + * @TypeConversion(rule = ConversionRule.CREATE_IF_NULL, value = "true") + * private List users = null; + * * @TypeConversion(rule = ConversionRule.COLLECTION, converterClass = String.class) * public void setUsers( List users ) { * this.users = users; @@ -139,7 +151,7 @@ * this.keyValues = keyValues; * } * - * @TypeConversion(type = ConversionType.APPLICATION, property = "java.util.Date", converterClass = XWorkBasicConverter.class) + * @TypeConversion(type = ConversionType.APPLICATION, key = "java.util.Date", converterClass = XWorkBasicConverter.class) * public String execute() throws Exception { * return SUCCESS; * } @@ -150,15 +162,26 @@ * @author Rainer Hermanns * @version $Id$ */ -@Target({ ElementType.METHOD}) +@Target({ElementType.METHOD, ElementType.FIELD}) @Retention(RetentionPolicy.RUNTIME) public @interface TypeConversion { /** - * The optional key name used within TYPE level annotations. - * Defaults to the property name. + * The property name this conversion applies to. Optional on a method, where it defaults to the + * resolved JavaBean property name; optional on a field, where it defaults to the field's own + * name instead - not necessarily the same thing. Required on TYPE level annotations. + * + *

The prefix matching the declared {@link ConversionRule} is prepended automatically, so + * {@code @TypeConversion(key = "users", rule = ConversionRule.CREATE_IF_NULL, value = "true")} + * and {@code @TypeConversion(key = "CreateIfNull_users", ...)} are equivalent.

+ * + *

If a field's name does not match the property it backs (for example a field {@code _users} + * exposed as property {@code users}), give an explicit {@code key} of {@code "users"} - a derived + * key of {@code CreateIfNull__users} is never looked up, since conversion metadata is read by + * property name, not field name.

* * @return key + * @since 7.3.0 the rule prefix is derived; previously the full key had to be spelled out */ String key() default ""; @@ -174,7 +197,7 @@ /** * The ConversionRule can be a PROPERTY, KEY, KEY_PROPERTY, ELEMENT, COLLECTION (deprecated) or a MAP. - * Note: Collection and Map conversion rules can be determined via org.apache.struts2.util.DefaultObjectTypeDeterminer. + * Note: Collection and Map conversion rules can be determined via org.apache.struts2.conversion.impl.DefaultObjectTypeDeterminer. * * @see DefaultObjectTypeDeterminer * diff --git a/core/src/main/java/org/apache/struts2/conversion/impl/XWorkConverter.java b/core/src/main/java/org/apache/struts2/conversion/impl/XWorkConverter.java index 5579a15d3e..7780f3fdaf 100644 --- a/core/src/main/java/org/apache/struts2/conversion/impl/XWorkConverter.java +++ b/core/src/main/java/org/apache/struts2/conversion/impl/XWorkConverter.java @@ -27,6 +27,8 @@ import org.apache.struts2.conversion.TypeConverter; import org.apache.struts2.conversion.TypeConverterHolder; import org.apache.struts2.conversion.annotations.Conversion; +import org.apache.struts2.conversion.annotations.ConversionRule; +import org.apache.struts2.conversion.annotations.ConversionType; import org.apache.struts2.conversion.annotations.TypeConversion; import org.apache.struts2.inject.Inject; import org.apache.struts2.util.AnnotationUtils; @@ -40,8 +42,10 @@ import org.apache.struts2.StrutsConstants; import java.lang.annotation.Annotation; +import java.lang.reflect.Field; import java.lang.reflect.Member; import java.lang.reflect.Method; +import java.lang.reflect.Modifier; import java.net.URL; import java.text.MessageFormat; import java.util.ArrayList; @@ -486,6 +490,52 @@ private Object[] getClassProperty(Map context) { return (lastClass != null && lastProperty != null) ? new Object[] {lastClass, lastProperty} : null; } + /** + * Resolves the conversion mapping key for an annotation: the given name carrying the + * {@link ConversionRule}'s prefix. A name that already starts with any known rule's + * prefix is returned unchanged, not only the prefix of the declared rule, so annotations that + * spell the prefix out keep working. This matters because {@link ConversionRule#COLLECTION} and + * {@link ConversionRule#ELEMENT} are interchangeable in both {@link DefaultConversionAnnotationProcessor} + * and {@link DefaultObjectTypeDeterminer}: a key such as {@code Element_users} declared with + * {@code rule = COLLECTION} must not become {@code Collection_Element_users}. + * + * @param type the annotation's {@link ConversionType}; APPLICATION keys are class names and are never prefixed + * @param rule the annotation's {@link ConversionRule} + * @param name an explicit key or a property name derived from a method or field + * @return the mapping key, or null when no name is available and the entry must be skipped + * @since 7.3.0 + */ + static String resolveKey(ConversionType type, ConversionRule rule, String name) { + if (StringUtils.isEmpty(name)) { + return null; + } + if (type == ConversionType.APPLICATION) { + return name; + } + String prefix = rule.prefix(); + if (name.startsWith(prefix)) { + return name; + } + for (ConversionRule other : ConversionRule.values()) { + String otherPrefix = other.prefix(); + if (!otherPrefix.isEmpty() && name.startsWith(otherPrefix)) { + return name; // already carries a rule prefix; leave it exactly as written + } + } + return prefix + name; + } + + /** + * True when a {@link TypeConversion} is APPLICATION-scoped but carries no explicit key. + * APPLICATION entries are stored in the global default converter map, which {@link + * #lookup(String, boolean)} and {@link #lookup(Class)} only ever read by class name. + * A member name derived from a method or field can never be looked up there, so such an entry + * must be skipped rather than registered under a key nothing can reach. + */ + private static boolean isApplicationScopedWithoutKey(TypeConversion tc) { + return tc.type() == ConversionType.APPLICATION && StringUtils.isEmpty(tc.key()); + } + /** * Looks for converter mappings for the specified class and adds it to an existing map. Only new converters are * added. If a converter is defined on a key that already exists, the converter is ignored. @@ -498,55 +548,134 @@ protected void addConverterMapping(Map mapping, Class clazz) { String converterFilename = buildConverterFilename(clazz); fileProcessor.process(mapping, clazz, converterFilename); - // Process annotations - Annotation[] annotations = clazz.getAnnotations(); + processClassLevelAnnotations(mapping, clazz); + processMethodAnnotations(mapping, clazz); + processFieldAnnotations(mapping, clazz); + } - for (Annotation annotation : annotations) { - if (annotation instanceof Conversion conversion) { - for (TypeConversion tc : conversion.conversions()) { - if (mapping.containsKey(tc.key())) { - break; - } - if (LOG.isDebugEnabled()) { - if (StringUtils.isEmpty(tc.key())) { - LOG.debug("WARNING! key of @TypeConversion [{}/{}] applied to [{}] is empty!", tc.converter(), tc.converterClass(), clazz.getName()); - } else { - LOG.debug("TypeConversion [{}/{}] with key: [{}]", tc.converter(), tc.converterClass(), tc.key()); - } - } - annotationProcessor.process(mapping, tc, tc.key()); + /** + * Registers the {@link TypeConversion} entries declared by a class level {@link Conversion} + * annotation. + */ + private void processClassLevelAnnotations(Map mapping, Class clazz) { + for (Annotation annotation : clazz.getAnnotations()) { + if (!(annotation instanceof Conversion conversion)) { + continue; + } + for (TypeConversion tc : conversion.conversions()) { + String key = resolveKey(tc.type(), tc.rule(), tc.key()); + if (key == null) { + LOG.warn("Ignoring @TypeConversion [{}/{}] declared on [{}]: no key was given and a class level annotation has no property name to derive one from", + tc.converter(), tc.converterClass(), clazz.getName()); + continue; } + if (mapping.containsKey(key)) { + continue; + } + LOG.debug("TypeConversion [{}/{}] declared on [{}] resolved to key [{}]", + tc.converter(), tc.converterClass(), clazz.getName(), key); + annotationProcessor.process(mapping, tc, key); } } + } - // Process annotated methods + /** + * Registers {@link TypeConversion} annotations found on the class' methods. + */ + private void processMethodAnnotations(Map mapping, Class clazz) { for (Method method : clazz.getMethods()) { - annotations = method.getAnnotations(); - for (Annotation annotation : annotations) { + // clazz.getMethods() returns inherited public methods too, and buildConverterMapping + // calls this method once per class in the hierarchy, so a single annotation declared + // on a base class method is visited once per subclass level: for C extends B extends A, + // an annotation on a method declared in A is seen three times, all with the same + // method.getDeclaringClass(). Only log skips on the pass whose clazz is that declaring + // class, so a misconfigured annotation is reported exactly once instead of once per + // subclass. This gates logging only - the derivation/registration pipeline in + // registerAnnotatedMember still runs on every visit, which first-writer-wins relies on. + // Note buildConverterMapping only visits each class' direct interfaces, never + // super-interfaces, so for "class C implements B" where "interface B extends A" and A + // declares the annotated method, no visited level ever satisfies declaringClass == clazz + // and a misconfigured annotation there logs nothing at all. Registration is unaffected. + boolean logSkips = method.getDeclaringClass() == clazz; + for (Annotation annotation : method.getAnnotations()) { if (annotation instanceof TypeConversion tc) { - String key = tc.key(); - // Default to the property name with prefix - if (StringUtils.isEmpty(key)) { - key = AnnotationUtils.resolvePropertyName(method); - key = switch (tc.rule()) { - case COLLECTION -> DefaultObjectTypeDeterminer.DEPRECATED_ELEMENT_PREFIX + key; - case CREATE_IF_NULL -> DefaultObjectTypeDeterminer.CREATE_IF_NULL_PREFIX + key; - case ELEMENT -> DefaultObjectTypeDeterminer.ELEMENT_PREFIX + key; - case KEY -> DefaultObjectTypeDeterminer.KEY_PREFIX + key; - case KEY_PROPERTY -> DefaultObjectTypeDeterminer.KEY_PROPERTY_PREFIX + key; - default -> key; - }; - LOG.debug("Retrieved key [{}] from method name [{}]", key, method.getName()); - } - if (mapping.containsKey(key)) { - break; - } - annotationProcessor.process(mapping, tc, key); + registerAnnotatedMember(mapping, tc, method, AnnotationUtils.resolvePropertyName(method), logSkips); + } + } + } + } + + /** + * Registers {@link TypeConversion} annotations found on the class' own fields. Only declared + * fields are read: {@link #buildConverterMapping(Class)} already walks the class hierarchy and + * calls this method once per class. Static and synthetic fields are skipped, which also makes + * this a no-op for interfaces. + * + *

The stated precedence "class > method > field" is per-class, not per-hierarchy-level: + * {@link #processMethodAnnotations(Map, Class)} sees {@link Class#getMethods()}, which includes + * inherited public methods, so a superclass's annotated setter claims its key before this pass + * ever looks at a subclass's field for that same class. A subclass field annotation only wins + * when no method anywhere in the hierarchy already claimed its key.

+ */ + private void processFieldAnnotations(Map mapping, Class clazz) { + for (Field field : clazz.getDeclaredFields()) { + if (Modifier.isStatic(field.getModifiers()) || field.isSynthetic()) { + continue; + } + for (Annotation annotation : field.getAnnotations()) { + if (annotation instanceof TypeConversion tc) { + // getDeclaredFields() is visited once per class, never revisited from a + // subclass level, so there is no multi-visit noise to gate against here. + registerAnnotatedMember(mapping, tc, field, field.getName(), true); } } } } + /** + * Shared pipeline for a single {@link TypeConversion} annotation found by {@link + * #processMethodAnnotations(Map, Class)} or {@link #processFieldAnnotations(Map, Class)}: skip + * APPLICATION-scoped annotations with no explicit key, derive the name, resolve it to a mapping + * key, and register it unless something already claimed that key. + * + * @param mapping the map being built for the current class + * @param tc the annotation to register + * @param member the annotated {@link Method} or {@link Field}; used only for its name and + * declaring class, both needed for logging + * @param fallbackName the name to use when {@code tc.key()} is empty - a resolved property name + * for a method, or the field's own name for a field + * @param logSkips whether to log skipped entries; see {@link #processMethodAnnotations(Map, Class)} + * for why a method pass gates this and a field pass does not + */ + private void registerAnnotatedMember(Map mapping, TypeConversion tc, Member member, String fallbackName, boolean logSkips) { + if (isApplicationScopedWithoutKey(tc)) { + if (logSkips) { + LOG.warn("Ignoring @TypeConversion on [{}#{}]: an application-scoped conversion needs an explicit class-name key, not a derived property name", + member.getDeclaringClass().getName(), member.getName()); + } + return; + } + String name = StringUtils.isEmpty(tc.key()) ? fallbackName : tc.key(); + String key = resolveKey(tc.type(), tc.rule(), name); + if (key == null) { + if (logSkips) { + LOG.warn("Ignoring @TypeConversion on [{}#{}]: no key was given and no property name could be derived", + member.getDeclaringClass().getName(), member.getName()); + } + return; + } + if (mapping.containsKey(key)) { + if (logSkips) { + LOG.debug("Skipping @TypeConversion on [{}#{}]: key [{}] is already mapped, either by a higher " + + "precedence source or by this same annotation seen at a lower level of the hierarchy", + member.getDeclaringClass().getName(), member.getName(), key); + } + return; + } + LOG.debug("TypeConversion [{}/{}] on [{}#{}] resolved to key [{}]", + tc.converter(), tc.converterClass(), member.getDeclaringClass().getName(), member.getName(), key); + annotationProcessor.process(mapping, tc, key); + } /** * Looks for converter mappings for the specified class, traversing up its class hierarchy and interfaces and adding diff --git a/core/src/test/java/org/apache/struts2/conversion/ConversionTestAction.java b/core/src/test/java/org/apache/struts2/conversion/ConversionTestAction.java index a425a202b0..a54417d23f 100644 --- a/core/src/test/java/org/apache/struts2/conversion/ConversionTestAction.java +++ b/core/src/test/java/org/apache/struts2/conversion/ConversionTestAction.java @@ -53,7 +53,7 @@ public String getConvertInt() { return convertInt; } - @TypeConversion(type = ConversionType.APPLICATION) + @TypeConversion() public void setConvertInt( String convertInt ) { this.convertInt = convertInt; } diff --git a/core/src/test/java/org/apache/struts2/conversion/annotations/ConversionRuleTest.java b/core/src/test/java/org/apache/struts2/conversion/annotations/ConversionRuleTest.java new file mode 100644 index 0000000000..04e63e1eca --- /dev/null +++ b/core/src/test/java/org/apache/struts2/conversion/annotations/ConversionRuleTest.java @@ -0,0 +1,37 @@ +/* + * 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.annotations; + +import org.junit.Test; + +import static org.junit.Assert.assertEquals; + +public class ConversionRuleTest { + + @Test + public void prefixIsDefinedForEveryRule() { + assertEquals("", ConversionRule.PROPERTY.prefix()); + assertEquals("", ConversionRule.MAP.prefix()); + assertEquals("Collection_", ConversionRule.COLLECTION.prefix()); + assertEquals("CreateIfNull_", ConversionRule.CREATE_IF_NULL.prefix()); + assertEquals("Element_", ConversionRule.ELEMENT.prefix()); + assertEquals("Key_", ConversionRule.KEY.prefix()); + assertEquals("KeyProperty_", ConversionRule.KEY_PROPERTY.prefix()); + } +} 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 9be7c8c482..3f107c8a07 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 @@ -37,8 +37,21 @@ import org.apache.struts2.util.ValueStack; import org.apache.struts2.util.reflection.ReflectionContextState; import ognl.OgnlRuntime; +import org.apache.struts2.conversion.ConversionAnnotationProcessor; import org.apache.struts2.conversion.TypeConverter; +import org.apache.struts2.conversion.TypeConverterHolder; import org.apache.struts2.conversion.StrutsTypeConverterHolder; +import org.apache.struts2.conversion.annotations.ConversionRule; +import org.apache.struts2.conversion.annotations.ConversionType; +import org.apache.struts2.util.ApplicationScopedWithoutKeyConversionAction; +import org.apache.struts2.util.BareKeyConversionAction; +import org.apache.struts2.util.CollidingKeyConversionAction; +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.MyBean; +import org.apache.struts2.util.MyBeanAction; import java.io.IOException; import java.io.InputStream; @@ -814,6 +827,172 @@ public void testCollectionConversion() { assertEquals(converted, Arrays.asList(1, 2, 3)); } + public void testResolveKeyPrependsTheRulePrefix() { + assertEquals("KeyProperty_annotatedBeanMap", + XWorkConverter.resolveKey(ConversionType.CLASS, ConversionRule.KEY_PROPERTY, "annotatedBeanMap")); + assertEquals("Element_annotatedBeanList", + XWorkConverter.resolveKey(ConversionType.CLASS, ConversionRule.ELEMENT, "annotatedBeanList")); + assertEquals("CreateIfNull_users", + XWorkConverter.resolveKey(ConversionType.CLASS, ConversionRule.CREATE_IF_NULL, "users")); + } + + public void testResolveKeyLeavesAnAlreadyPrefixedKeyAlone() { + assertEquals("KeyProperty_annotatedBeanMap", + XWorkConverter.resolveKey(ConversionType.CLASS, ConversionRule.KEY_PROPERTY, "KeyProperty_annotatedBeanMap")); + assertEquals("Key_beanMap", + XWorkConverter.resolveKey(ConversionType.CLASS, ConversionRule.KEY, "Key_beanMap")); + + // COLLECTION and ELEMENT are interchangeable in DefaultConversionAnnotationProcessor and + // DefaultObjectTypeDeterminer, so a key already carrying either prefix must be left alone + // regardless of which of the two rules is declared. + assertEquals("Element_users", + XWorkConverter.resolveKey(ConversionType.CLASS, ConversionRule.COLLECTION, "Element_users")); + assertEquals("Collection_users", + XWorkConverter.resolveKey(ConversionType.CLASS, ConversionRule.ELEMENT, "Collection_users")); + } + + public void testResolveKeyDoesNotPrefixPropertyOrMapRules() { + assertEquals("someProperty", + XWorkConverter.resolveKey(ConversionType.CLASS, ConversionRule.PROPERTY, "someProperty")); + assertEquals("keyValues", + XWorkConverter.resolveKey(ConversionType.CLASS, ConversionRule.MAP, "keyValues")); + } + + public void testResolveKeyNeverPrefixesApplicationScopedKeys() { + assertEquals("java.util.Date", + XWorkConverter.resolveKey(ConversionType.APPLICATION, ConversionRule.PROPERTY, "java.util.Date")); + assertEquals("java.util.Date", + XWorkConverter.resolveKey(ConversionType.APPLICATION, ConversionRule.ELEMENT, "java.util.Date")); + } + + public void testResolveKeyReturnsNullWhenNoNameIsAvailable() { + assertNull(XWorkConverter.resolveKey(ConversionType.CLASS, ConversionRule.PROPERTY, null)); + assertNull(XWorkConverter.resolveKey(ConversionType.CLASS, ConversionRule.KEY, "")); + } + + public void testExplicitMethodKeyGetsTheRulePrefix() { + XWorkConverter freshConverter = container.inject(XWorkConverter.class); + freshConverter.setTypeConverterHolder(new StrutsTypeConverterHolder()); + + assertEquals("true", freshConverter.getConverter(ExplicitKeyConversionAction.class, "CreateIfNull_bareList")); + assertNull(freshConverter.getConverter(ExplicitKeyConversionAction.class, "bareList")); + } + + public void testClassLevelBareKeysGetTheRulePrefix() { + XWorkConverter freshConverter = container.inject(XWorkConverter.class); + freshConverter.setTypeConverterHolder(new StrutsTypeConverterHolder()); + + assertEquals("id", freshConverter.getConverter(BareKeyConversionAction.class, "KeyProperty_annotatedBeanMap")); + assertEquals(MyBean.class, freshConverter.getConverter(BareKeyConversionAction.class, "Element_annotatedBeanMap")); + assertEquals("id", freshConverter.getConverter(BareKeyConversionAction.class, "KeyProperty_annotatedBeanList")); + assertEquals(MyBean.class, freshConverter.getConverter(BareKeyConversionAction.class, "Element_annotatedBeanList")); + } + + public void testClassLevelBareKeysMatchTheSpelledOutForm() { + XWorkConverter freshConverter = container.inject(XWorkConverter.class); + freshConverter.setTypeConverterHolder(new StrutsTypeConverterHolder()); + + for (String key : new String[]{"KeyProperty_annotatedBeanMap", "Element_annotatedBeanMap", + "KeyProperty_annotatedBeanList", "Element_annotatedBeanList"}) { + assertEquals("mismatch for " + key, + freshConverter.getConverter(MyBeanAction.class, key), + freshConverter.getConverter(BareKeyConversionAction.class, key)); + } + } + + public void testClassLevelEntriesAfterAKeyCollisionAreStillRegistered() { + XWorkConverter freshConverter = container.inject(XWorkConverter.class); + freshConverter.setTypeConverterHolder(new StrutsTypeConverterHolder()); + + // supplied by the -conversion.properties file, so the annotation must not overwrite it + assertEquals("true", freshConverter.getConverter(CollidingKeyConversionAction.class, "CreateIfNull_fromProperties")); + // the entry after the collision used to be dropped by `break` + assertEquals("true", freshConverter.getConverter(CollidingKeyConversionAction.class, "CreateIfNull_afterTheCollision")); + } + + public void testClassLevelEmptyKeyRegistersNoMapping() throws Exception { + XWorkConverter freshConverter = container.inject(XWorkConverter.class); + freshConverter.setTypeConverterHolder(new StrutsTypeConverterHolder()); + + Map mapping = freshConverter.buildConverterMapping(EmptyKeyConversionAction.class); + + assertFalse("an empty class-level key must not register a \"\" mapping", mapping.containsKey("")); + assertTrue("no mapping should have been registered at all", mapping.isEmpty()); + } + + public void testFieldLevelAnnotationDerivesKeyFromTheFieldName() { + XWorkConverter freshConverter = container.inject(XWorkConverter.class); + freshConverter.setTypeConverterHolder(new StrutsTypeConverterHolder()); + + assertEquals("true", freshConverter.getConverter(FieldConversionAction.class, "CreateIfNull_fieldOnlyList")); + } + + public void testMethodAnnotationWinsOverFieldAnnotation() { + XWorkConverter freshConverter = container.inject(XWorkConverter.class); + freshConverter.setTypeConverterHolder(new StrutsTypeConverterHolder()); + + assertEquals(Long.class, freshConverter.getConverter(FieldConversionAction.class, "Key_contestedMap")); + } + + /** + * {@code processMethodAnnotations} gates its WARN logging to the hierarchy level whose {@code + * clazz} matches {@code method.getDeclaringClass()}, so a misconfigured annotation on an + * inherited method logs once instead of once per subclass. Gating logging alone is only safe + * because registration still happens at the subclass level - before that subclass's own field + * pass runs - as documented on {@code XWorkConverter#processFieldAnnotations}. {@link + * InheritedMethodConversionSubAction} declares a contesting field annotation for the same + * property the inherited setter claims; this asserts the inherited method annotation still wins, + * which would break if registration were mistakenly gated along with logging. + */ + public void testInheritedMethodAnnotationStillRegistersThroughASubclass() { + XWorkConverter freshConverter = container.inject(XWorkConverter.class); + freshConverter.setTypeConverterHolder(new StrutsTypeConverterHolder()); + + assertEquals("true", freshConverter.getConverter(InheritedMethodConversionSubAction.class, "CreateIfNull_inheritedList")); + } + + /** + * Wires a fresh {@link StrutsTypeConverterHolder} into both {@code freshConverter} and its + * {@link ConversionAnnotationProcessor}. The two are injected as independent singletons: an + * {@code APPLICATION}-scoped {@link TypeConversion} is registered by the annotation processor + * calling {@code TypeConverterHolder.addDefaultMapping} directly (bypassing the {@code mapping} + * map {@code XWorkConverter} caches per-class), so swapping only {@code XWorkConverter}'s own + * holder - as the other {@code freshConverter.setTypeConverterHolder(...)} tests in this file do + * for class/method/field mappings - would silently observe the shared container-wide holder + * instead of the one the test controls. + */ + private XWorkConverter freshConverterWithIsolatedHolder(TypeConverterHolder holder) { + XWorkConverter freshConverter = container.inject(XWorkConverter.class); + DefaultConversionAnnotationProcessor freshProcessor = container.inject(DefaultConversionAnnotationProcessor.class); + freshProcessor.setTypeConverterHolder(holder); + freshConverter.setConversionAnnotationProcessor(freshProcessor); + freshConverter.setTypeConverterHolder(holder); + return freshConverter; + } + + public void testApplicationScopedMethodWithoutKeyRegistersNoDefaultMapping() { + StrutsTypeConverterHolder holder = new StrutsTypeConverterHolder(); + XWorkConverter freshConverter = freshConverterWithIsolatedHolder(holder); + + // forces the mapping to be built; the property itself does not need to resolve to anything + freshConverter.getConverter(ApplicationScopedWithoutKeyConversionAction.class, "anything"); + + assertFalse("a method-level APPLICATION conversion with no key must not register a default " + + "mapping under the derived member name, which lookup(String,boolean)/lookup(Class) never read", + holder.containsDefaultMapping("applicationScopedMethod")); + } + + public void testApplicationScopedFieldWithoutKeyRegistersNoDefaultMapping() { + StrutsTypeConverterHolder holder = new StrutsTypeConverterHolder(); + XWorkConverter freshConverter = freshConverterWithIsolatedHolder(holder); + + freshConverter.getConverter(ApplicationScopedWithoutKeyConversionAction.class, "anything"); + + assertFalse("a field-level APPLICATION conversion with no key must not register a default " + + "mapping under the derived member name, which lookup(String,boolean)/lookup(Class) never read", + holder.containsDefaultMapping("applicationScopedField")); + } + public static class CountingXWorkConverter extends XWorkConverter { final AtomicInteger builds = new AtomicInteger(); diff --git a/core/src/test/java/org/apache/struts2/util/ApplicationScopedWithoutKeyConversionAction.java b/core/src/test/java/org/apache/struts2/util/ApplicationScopedWithoutKeyConversionAction.java new file mode 100644 index 0000000000..cc93ae7fff --- /dev/null +++ b/core/src/test/java/org/apache/struts2/util/ApplicationScopedWithoutKeyConversionAction.java @@ -0,0 +1,54 @@ +/* + * 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; + +import org.apache.struts2.conversion.annotations.ConversionType; +import org.apache.struts2.conversion.annotations.TypeConversion; + +/** + * An {@code APPLICATION}-scoped {@link TypeConversion} is stored in the global default converter + * map, which is keyed by class name. With no explicit {@code key}, a method or field pass would + * otherwise derive a member name (e.g. {@code applicationScopedMethod}) and register it there, + * where nothing can ever look it up. Both the method-level and field-level annotation below must + * be skipped rather than registered under their derived member names. + */ +public class ApplicationScopedWithoutKeyConversionAction { + + @TypeConversion(type = ConversionType.APPLICATION) + private String applicationScopedField; + + private String applicationScopedMethod; + + public String getApplicationScopedField() { + return applicationScopedField; + } + + public void setApplicationScopedField(String applicationScopedField) { + this.applicationScopedField = applicationScopedField; + } + + public String getApplicationScopedMethod() { + return applicationScopedMethod; + } + + @TypeConversion(type = ConversionType.APPLICATION) + public void setApplicationScopedMethod(String applicationScopedMethod) { + this.applicationScopedMethod = applicationScopedMethod; + } +} diff --git a/core/src/test/java/org/apache/struts2/util/BareKeyConversionAction.java b/core/src/test/java/org/apache/struts2/util/BareKeyConversionAction.java new file mode 100644 index 0000000000..de8e3152c8 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/util/BareKeyConversionAction.java @@ -0,0 +1,61 @@ +/* + * 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; + +import org.apache.struts2.conversion.annotations.Conversion; +import org.apache.struts2.conversion.annotations.ConversionRule; +import org.apache.struts2.conversion.annotations.TypeConversion; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +/** + * The class level counterpart of {@link MyBeanAction}, declaring the same four conversions with + * bare property names instead of spelled-out prefixes. + */ +@Conversion( + conversions = { + @TypeConversion(key = "annotatedBeanMap", rule = ConversionRule.KEY_PROPERTY, value = "id"), + @TypeConversion(key = "annotatedBeanMap", rule = ConversionRule.ELEMENT, converterClass = MyBean.class), + @TypeConversion(key = "annotatedBeanList", rule = ConversionRule.KEY_PROPERTY, value = "id"), + @TypeConversion(key = "annotatedBeanList", rule = ConversionRule.ELEMENT, converterClass = MyBean.class) + }) +public class BareKeyConversionAction { + + private Map annotatedBeanMap = new HashMap(); + private List annotatedBeanList = new ArrayList(); + + public Map getAnnotatedBeanMap() { + return annotatedBeanMap; + } + + public void setAnnotatedBeanMap(Map annotatedBeanMap) { + this.annotatedBeanMap = annotatedBeanMap; + } + + public List getAnnotatedBeanList() { + return annotatedBeanList; + } + + public void setAnnotatedBeanList(List annotatedBeanList) { + this.annotatedBeanList = annotatedBeanList; + } +} diff --git a/core/src/test/java/org/apache/struts2/util/CollidingKeyConversionAction.java b/core/src/test/java/org/apache/struts2/util/CollidingKeyConversionAction.java new file mode 100644 index 0000000000..e8d30f91d5 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/util/CollidingKeyConversionAction.java @@ -0,0 +1,48 @@ +/* + * 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; + +import org.apache.struts2.conversion.annotations.Conversion; +import org.apache.struts2.conversion.annotations.ConversionRule; +import org.apache.struts2.conversion.annotations.TypeConversion; + +import java.util.ArrayList; +import java.util.List; + +/** + * The first conversion entry collides with a key already supplied by + * {@code CollidingKeyConversionAction-conversion.properties}; the second must still be registered. + */ +@Conversion( + conversions = { + @TypeConversion(key = "fromProperties", rule = ConversionRule.CREATE_IF_NULL, value = "false"), + @TypeConversion(key = "afterTheCollision", rule = ConversionRule.CREATE_IF_NULL, value = "true") + }) +public class CollidingKeyConversionAction { + + private List afterTheCollision = new ArrayList(); + + public List getAfterTheCollision() { + return afterTheCollision; + } + + public void setAfterTheCollision(List afterTheCollision) { + this.afterTheCollision = afterTheCollision; + } +} diff --git a/core/src/test/java/org/apache/struts2/util/EmptyKeyConversionAction.java b/core/src/test/java/org/apache/struts2/util/EmptyKeyConversionAction.java new file mode 100644 index 0000000000..267df549bc --- /dev/null +++ b/core/src/test/java/org/apache/struts2/util/EmptyKeyConversionAction.java @@ -0,0 +1,34 @@ +/* + * 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; + +import org.apache.struts2.conversion.annotations.Conversion; +import org.apache.struts2.conversion.annotations.ConversionRule; +import org.apache.struts2.conversion.annotations.TypeConversion; + +/** + * A class level {@link TypeConversion} has no property name to derive a key from, so an unset + * (empty) {@code key} must be skipped rather than registered under {@code ""}. + */ +@Conversion( + conversions = { + @TypeConversion(rule = ConversionRule.CREATE_IF_NULL, value = "true") + }) +public class EmptyKeyConversionAction { +} diff --git a/core/src/test/java/org/apache/struts2/util/ExplicitKeyConversionAction.java b/core/src/test/java/org/apache/struts2/util/ExplicitKeyConversionAction.java new file mode 100644 index 0000000000..8997d06963 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/util/ExplicitKeyConversionAction.java @@ -0,0 +1,43 @@ +/* + * 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; + +import org.apache.struts2.conversion.annotations.ConversionRule; +import org.apache.struts2.conversion.annotations.TypeConversion; + +import java.util.ArrayList; +import java.util.List; + +/** + * Declares a method level {@link TypeConversion} with an explicit, unprefixed key. Before WW-3871 + * this registered a bare {@code bareList} mapping that nothing ever read. + */ +public class ExplicitKeyConversionAction { + + private List bareList = new ArrayList(); + + public List getBareList() { + return bareList; + } + + @TypeConversion(key = "bareList", rule = ConversionRule.CREATE_IF_NULL, value = "true") + public void setBareList(List bareList) { + this.bareList = bareList; + } +} diff --git a/core/src/test/java/org/apache/struts2/util/FieldConversionAction.java b/core/src/test/java/org/apache/struts2/util/FieldConversionAction.java new file mode 100644 index 0000000000..9207519888 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/util/FieldConversionAction.java @@ -0,0 +1,58 @@ +/* + * 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; + +import org.apache.struts2.conversion.annotations.ConversionRule; +import org.apache.struts2.conversion.annotations.TypeConversion; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +/** + * Exercises field level {@link TypeConversion}: {@code fieldOnlyList} is annotated on the field + * alone, while {@code contestedMap} is annotated on both the field and its setter so the + * class > method > field precedence can be asserted. + */ +public class FieldConversionAction { + + @TypeConversion(rule = ConversionRule.CREATE_IF_NULL, value = "true") + private List fieldOnlyList = new ArrayList(); + + @TypeConversion(rule = ConversionRule.KEY, converterClass = String.class) + private Map contestedMap = new HashMap(); + + public List getFieldOnlyList() { + return fieldOnlyList; + } + + public void setFieldOnlyList(List fieldOnlyList) { + this.fieldOnlyList = fieldOnlyList; + } + + public Map getContestedMap() { + return contestedMap; + } + + @TypeConversion(rule = ConversionRule.KEY, converterClass = Long.class) + public void setContestedMap(Map contestedMap) { + this.contestedMap = contestedMap; + } +} diff --git a/core/src/test/java/org/apache/struts2/util/InheritedMethodConversionAction.java b/core/src/test/java/org/apache/struts2/util/InheritedMethodConversionAction.java new file mode 100644 index 0000000000..fc668d0f88 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/util/InheritedMethodConversionAction.java @@ -0,0 +1,48 @@ +/* + * 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; + +import org.apache.struts2.conversion.annotations.ConversionRule; +import org.apache.struts2.conversion.annotations.TypeConversion; + +import java.util.ArrayList; +import java.util.List; + +/** + * Declares a bare-key, method level {@link TypeConversion} on a setter that {@link + * InheritedMethodConversionSubAction} inherits without overriding. {@code Class#getMethods()} on the + * subclass still returns this method, so + * {@link org.apache.struts2.conversion.impl.XWorkConverter#buildConverterMapping} visits it once per + * class in the hierarchy, registering it at the subclass level before that subclass's own field pass + * runs - used together with {@link InheritedMethodConversionSubAction}'s contesting field annotation + * to assert that precedence, independent of how many times the (gated) WARN logging fires. + */ +public class InheritedMethodConversionAction { + + private List inheritedList = new ArrayList(); + + public List getInheritedList() { + return inheritedList; + } + + @TypeConversion(rule = ConversionRule.CREATE_IF_NULL, value = "true") + public void setInheritedList(List inheritedList) { + this.inheritedList = inheritedList; + } +} diff --git a/core/src/test/java/org/apache/struts2/util/InheritedMethodConversionSubAction.java b/core/src/test/java/org/apache/struts2/util/InheritedMethodConversionSubAction.java new file mode 100644 index 0000000000..feb8fdcce5 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/util/InheritedMethodConversionSubAction.java @@ -0,0 +1,41 @@ +/* + * 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; + +import org.apache.struts2.conversion.annotations.ConversionRule; +import org.apache.struts2.conversion.annotations.TypeConversion; + +import java.util.List; + +/** + * Extends {@link InheritedMethodConversionAction} without overriding its annotated setter, so {@code + * XWorkConverter.buildConverterMapping}'s walk up the hierarchy - this class, then its parent, then + * stopping at {@code Object} - visits the inherited annotated setter once per class it processes. + * + *

Declares its own {@code inheritedList} field, annotated with a contesting {@code + * CREATE_IF_NULL} value, for the same property the inherited setter already claims. This is used to + * assert that the inherited method annotation - registered at this subclass level, before this + * class's own field pass ever runs - keeps winning over the field annotation declared here, per the + * precedence documented on {@code XWorkConverter#processFieldAnnotations}.

+ */ +public class InheritedMethodConversionSubAction extends InheritedMethodConversionAction { + + @TypeConversion(rule = ConversionRule.CREATE_IF_NULL, value = "false") + private List inheritedList; +} diff --git a/core/src/test/java/org/apache/struts2/util/MyBeanActionTest.java b/core/src/test/java/org/apache/struts2/util/MyBeanActionTest.java index 98c836d5b3..bd9d1da440 100644 --- a/core/src/test/java/org/apache/struts2/util/MyBeanActionTest.java +++ b/core/src/test/java/org/apache/struts2/util/MyBeanActionTest.java @@ -126,6 +126,34 @@ public void testIndexedMap() { } } + public void testBareConversionKeysBindTheSameWayAsPrefixedOnes() throws Exception { + HashMap params = new HashMap<>(); + params.put("annotatedBeanList(1234567890).name", "This is the bla bean by annotation"); + params.put("annotatedBeanMap[1234567891].id", "1234567891"); + params.put("annotatedBeanMap[1234567891].name", "This is the 2nd bla bean by annotation"); + + ActionContext extraContext = ActionContext.of().withParameters(HttpParameters.create(params).build()); + + ActionProxy proxy = actionProxyFactory.createActionProxy("", "MyBeanBareKey", null, extraContext.getContextMap()); + proxy.execute(); + MyBeanBareKeyAction action = (MyBeanBareKeyAction) proxy.getInvocation().getAction(); + + // CreateIfNull_annotatedBeanList + Element_annotatedBeanList + assertEquals(1, action.getAnnotatedBeanList().size()); + assertEquals(MyBean.class, action.getAnnotatedBeanList().get(0).getClass()); + assertEquals("This is the bla bean by annotation", + proxy.getInvocation().getStack().findValue("annotatedBeanList.get(0).name")); + // KeyProperty_annotatedBeanList (value = "id"): the index used to address the list, + // 1234567890, is bound onto the created bean's own "id" property. + assertEquals(Long.valueOf(1234567890L), ((MyBean) action.getAnnotatedBeanList().get(0)).getId()); + + // Key_annotatedBeanMap makes the key a Long, Element_annotatedBeanMap makes the value a MyBean + assertTrue(action.getAnnotatedBeanMap().containsKey(1234567891L)); + assertEquals(MyBean.class, action.getAnnotatedBeanMap().get(1234567891L).getClass()); + assertEquals("This is the 2nd bla bean by annotation", + proxy.getInvocation().getStack().findValue("annotatedBeanMap.get(1234567891L).name")); + } + @Override protected void setUp() throws Exception { super.setUp(); diff --git a/core/src/test/java/org/apache/struts2/util/MyBeanBareKeyAction.java b/core/src/test/java/org/apache/struts2/util/MyBeanBareKeyAction.java new file mode 100644 index 0000000000..ebc232f033 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/util/MyBeanBareKeyAction.java @@ -0,0 +1,68 @@ +/* + * 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; + +import org.apache.struts2.action.Action; +import org.apache.struts2.conversion.annotations.Conversion; +import org.apache.struts2.conversion.annotations.ConversionRule; +import org.apache.struts2.conversion.annotations.TypeConversion; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +/** + * {@link MyBeanAction} restated with bare property names as conversion keys. Both must bind + * identically; {@code MyBeanAction} keeps the spelled-out prefixes so the old form stays covered. + */ +@Conversion( + conversions = { + @TypeConversion(key = "annotatedBeanMap", rule = ConversionRule.KEY_PROPERTY, value = "id"), + @TypeConversion(key = "annotatedBeanMap", rule = ConversionRule.ELEMENT, converterClass = MyBean.class), + @TypeConversion(key = "annotatedBeanList", rule = ConversionRule.KEY_PROPERTY, value = "id"), + @TypeConversion(key = "annotatedBeanList", rule = ConversionRule.ELEMENT, converterClass = MyBean.class) + }) +public class MyBeanBareKeyAction implements Action { + + private Map annotatedBeanMap = new HashMap(); + private List annotatedBeanList = new ArrayList(); + + public Map getAnnotatedBeanMap() { + return annotatedBeanMap; + } + + @TypeConversion(rule = ConversionRule.KEY, converterClass = Long.class) + public void setAnnotatedBeanMap(Map annotatedBeanMap) { + this.annotatedBeanMap = annotatedBeanMap; + } + + public List getAnnotatedBeanList() { + return annotatedBeanList; + } + + @TypeConversion(rule = ConversionRule.CREATE_IF_NULL, value = "true") + public void setAnnotatedBeanList(List annotatedBeanList) { + this.annotatedBeanList = annotatedBeanList; + } + + public String execute() throws Exception { + return SUCCESS; + } +} diff --git a/core/src/test/resources/org/apache/struts2/util/CollidingKeyConversionAction-conversion.properties b/core/src/test/resources/org/apache/struts2/util/CollidingKeyConversionAction-conversion.properties new file mode 100644 index 0000000000..f034532945 --- /dev/null +++ b/core/src/test/resources/org/apache/struts2/util/CollidingKeyConversionAction-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_fromProperties=true diff --git a/core/src/test/resources/xwork-sample.xml b/core/src/test/resources/xwork-sample.xml index 5bc189d821..456dc6dade 100644 --- a/core/src/test/resources/xwork-sample.xml +++ b/core/src/test/resources/xwork-sample.xml @@ -131,6 +131,12 @@ + + + + + + expectedFoo diff --git a/docs/superpowers/plans/2026-07-25-WW-3871-typeconversion-key-derivation.md b/docs/superpowers/plans/2026-07-25-WW-3871-typeconversion-key-derivation.md new file mode 100644 index 0000000000..b0351c147f --- /dev/null +++ b/docs/superpowers/plans/2026-07-25-WW-3871-typeconversion-key-derivation.md @@ -0,0 +1,1026 @@ +# WW-3871 `@TypeConversion` Key Derivation Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Let `@TypeConversion` accept a bare property name as its `key` at class, method and field level, deriving the `ConversionRule` prefix automatically, without breaking annotations that already spell the prefix out. + +**Architecture:** `ConversionRule` gains a `prefix()` method owning the rule-to-prefix table. `XWorkConverter` gains one static `resolveKey(type, rule, name)` used by all three annotation passes, and `addConverterMapping` splits into four named passes (properties file, class-level, method, field) with first-writer-wins precedence. `@TypeConversion` gains `ElementType.FIELD`. + +**Tech Stack:** Java 17, Maven, JUnit (see constraints), Log4j2, `org.apache.commons.lang3.StringUtils`. + +**Spec:** `docs/superpowers/specs/2026-07-25-WW-3871-typeconversion-key-derivation-design.md` + +## Global Constraints + +- **Branch:** work on `WW-3871-typeconversion-key-derivation` (already created, based on `main` @ `8a5323fbd`). Never commit to `main`. +- **Commit messages:** must be prefixed with the ticket: `WW-3871 (): `. Types: `feat`, `fix`, `refactor`, `test`, `docs`, `chore`. +- **Test framework:** core tests are JUnit 3/4, **never** JUnit 5. Classes extending `XWorkTestCase` (which extends `junit.framework.TestCase`) use `public void testXxx()` with **no** `@Test` annotation — an `@Test` there silently never runs. Standalone JUnit 4 classes (no superclass, `org.junit.Test` + `org.junit.Assert`) are also fine and are used in this module, e.g. `core/src/test/java/org/apache/struts2/LocaleProviderTest.java`. +- **Build command:** `mvn test -DskipAssembly -pl core -Dtest=ClassName#methodName` for a single test; `mvn test -DskipAssembly -pl core` for the module. +- **Licence header:** every new `.java` and `.properties` file must start with the Apache licence header — copy it verbatim from a neighbouring file in the same directory. +- **Target version for `@since` tags:** `7.3.0`. +- **No new dependencies.** + +--- + +### Task 1: `ConversionRule.prefix()` + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/conversion/annotations/ConversionRule.java` +- Test (create): `core/src/test/java/org/apache/struts2/conversion/annotations/ConversionRuleTest.java` + +**Interfaces:** +- Consumes: `DefaultObjectTypeDeterminer.KEY_PREFIX`, `ELEMENT_PREFIX`, `KEY_PROPERTY_PREFIX`, `CREATE_IF_NULL_PREFIX`, `DEPRECATED_ELEMENT_PREFIX` — public String constants in `org.apache.struts2.conversion.impl.DefaultObjectTypeDeterminer` (`"Key_"`, `"Element_"`, `"KeyProperty_"`, `"CreateIfNull_"`, `"Collection_"`). +- Produces: `public String ConversionRule.prefix()` — returns the mapping-key prefix for the rule, `""` for `PROPERTY` and `MAP`. Never null. + +- [ ] **Step 1: Write the failing test** + +Create `core/src/test/java/org/apache/struts2/conversion/annotations/ConversionRuleTest.java` (Apache licence header first, copied from `ConversionRule.java`): + +```java +package org.apache.struts2.conversion.annotations; + +import org.junit.Test; + +import static org.junit.Assert.assertEquals; + +public class ConversionRuleTest { + + @Test + public void prefixIsDefinedForEveryRule() { + assertEquals("", ConversionRule.PROPERTY.prefix()); + assertEquals("", ConversionRule.MAP.prefix()); + assertEquals("Collection_", ConversionRule.COLLECTION.prefix()); + assertEquals("CreateIfNull_", ConversionRule.CREATE_IF_NULL.prefix()); + assertEquals("Element_", ConversionRule.ELEMENT.prefix()); + assertEquals("Key_", ConversionRule.KEY.prefix()); + assertEquals("KeyProperty_", ConversionRule.KEY_PROPERTY.prefix()); + } +} +``` + +- [ ] **Step 2: Run it to make sure it fails** + +Run: `mvn test -DskipAssembly -pl core -Dtest=ConversionRuleTest` +Expected: compilation failure — `cannot find symbol: method prefix()`. + +- [ ] **Step 3: Implement `prefix()`** + +In `ConversionRule.java`, add the import and the method. The enum body becomes: + +```java +package org.apache.struts2.conversion.annotations; + +import org.apache.struts2.conversion.impl.DefaultObjectTypeDeterminer; + +/** + * ConversionRule + * + * @author Rainer Hermanns + * @version $Id$ + */ +public enum ConversionRule { + + PROPERTY, COLLECTION, MAP, KEY, KEY_PROPERTY, ELEMENT, CREATE_IF_NULL; + + /** + * The prefix a conversion mapping key carries for this rule, as read back by + * {@link DefaultObjectTypeDeterminer}. {@code PROPERTY} and {@code MAP} have no prefix of their + * own: map and collection metadata is read through the {@code Key_} and {@code Element_} keys. + * + * @return the mapping key prefix, never null; an empty string when the rule has none + * @since 7.3.0 + */ + public String prefix() { + return switch (this) { + case COLLECTION -> DefaultObjectTypeDeterminer.DEPRECATED_ELEMENT_PREFIX; + case CREATE_IF_NULL -> DefaultObjectTypeDeterminer.CREATE_IF_NULL_PREFIX; + case ELEMENT -> DefaultObjectTypeDeterminer.ELEMENT_PREFIX; + case KEY -> DefaultObjectTypeDeterminer.KEY_PREFIX; + case KEY_PROPERTY -> DefaultObjectTypeDeterminer.KEY_PROPERTY_PREFIX; + case PROPERTY, MAP -> ""; + }; + } + + @Override + public String toString() { + return super.toString().toUpperCase(); + } +} +``` + +The `switch` is deliberately exhaustive with no `default` branch: adding a rule then fails to compile until its prefix is decided. + +- [ ] **Step 4: Run the test and make sure it passes** + +Run: `mvn test -DskipAssembly -pl core -Dtest=ConversionRuleTest` +Expected: PASS, 1 test. + +- [ ] **Step 5: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/conversion/annotations/ConversionRule.java \ + core/src/test/java/org/apache/struts2/conversion/annotations/ConversionRuleTest.java +git commit -m "WW-3871 feat(core): add ConversionRule#prefix() owning the rule-to-prefix table" +``` + +--- + +### Task 2: Split `addConverterMapping` into named passes (pure refactor) + +No behaviour changes in this task — including the `break` statements, which stay wrong until Task 4. This exists so the later tasks touch small, readable methods. + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/conversion/impl/XWorkConverter.java:496-548` + +**Interfaces:** +- Produces: `private void processClassLevelAnnotations(Map mapping, Class clazz)` and `private void processMethodAnnotations(Map mapping, Class clazz)`, both called from `addConverterMapping`. + +- [ ] **Step 1: Run the existing tests to establish the baseline** + +Run: `mvn test -DskipAssembly -pl core -Dtest='XWorkConverterTest+AnnotationXWorkConverterTest+MyBeanActionTest'` +Expected: PASS. Record the test counts — the same tests must still pass at Step 3. + +- [ ] **Step 2: Extract the two passes** + +Replace the whole body of `addConverterMapping` (currently lines 496-548) with: + +```java + protected void addConverterMapping(Map mapping, Class clazz) { + // Process -conversion.properties file + String converterFilename = buildConverterFilename(clazz); + fileProcessor.process(mapping, clazz, converterFilename); + + processClassLevelAnnotations(mapping, clazz); + processMethodAnnotations(mapping, clazz); + } + + /** + * Registers the {@link TypeConversion} entries declared by a class level {@link Conversion} + * annotation. + */ + private void processClassLevelAnnotations(Map mapping, Class clazz) { + for (Annotation annotation : clazz.getAnnotations()) { + if (annotation instanceof Conversion conversion) { + for (TypeConversion tc : conversion.conversions()) { + if (mapping.containsKey(tc.key())) { + break; + } + if (LOG.isDebugEnabled()) { + if (StringUtils.isEmpty(tc.key())) { + LOG.debug("WARNING! key of @TypeConversion [{}/{}] applied to [{}] is empty!", tc.converter(), tc.converterClass(), clazz.getName()); + } else { + LOG.debug("TypeConversion [{}/{}] with key: [{}]", tc.converter(), tc.converterClass(), tc.key()); + } + } + annotationProcessor.process(mapping, tc, tc.key()); + } + } + } + } + + /** + * Registers {@link TypeConversion} annotations found on the class' methods. + */ + private void processMethodAnnotations(Map mapping, Class clazz) { + for (Method method : clazz.getMethods()) { + for (Annotation annotation : method.getAnnotations()) { + if (annotation instanceof TypeConversion tc) { + String key = tc.key(); + // Default to the property name with prefix + if (StringUtils.isEmpty(key)) { + key = AnnotationUtils.resolvePropertyName(method); + key = switch (tc.rule()) { + case COLLECTION -> DefaultObjectTypeDeterminer.DEPRECATED_ELEMENT_PREFIX + key; + case CREATE_IF_NULL -> DefaultObjectTypeDeterminer.CREATE_IF_NULL_PREFIX + key; + case ELEMENT -> DefaultObjectTypeDeterminer.ELEMENT_PREFIX + key; + case KEY -> DefaultObjectTypeDeterminer.KEY_PREFIX + key; + case KEY_PROPERTY -> DefaultObjectTypeDeterminer.KEY_PROPERTY_PREFIX + key; + default -> key; + }; + LOG.debug("Retrieved key [{}] from method name [{}]", key, method.getName()); + } + if (mapping.containsKey(key)) { + break; + } + annotationProcessor.process(mapping, tc, key); + } + } + } + } +``` + +Note: the original code reused a local `Annotation[] annotations` variable across both loops; the extracted methods each iterate directly, so that variable disappears. Check whether `Annotation` and `Method` imports are still needed — they are, both extracted methods use them. + +- [ ] **Step 3: Run the same tests and confirm identical results** + +Run: `mvn test -DskipAssembly -pl core -Dtest='XWorkConverterTest+AnnotationXWorkConverterTest+MyBeanActionTest'` +Expected: PASS, same counts as Step 1. + +- [ ] **Step 4: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/conversion/impl/XWorkConverter.java +git commit -m "WW-3871 refactor(core): split addConverterMapping into per-source passes" +``` + +--- + +### Task 3: `resolveKey` helper, wired into the method pass + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/conversion/impl/XWorkConverter.java` +- Test (modify): `core/src/test/java/org/apache/struts2/conversion/impl/XWorkConverterTest.java` +- Test fixture (create): `core/src/test/java/org/apache/struts2/util/ExplicitKeyConversionAction.java` + +**Interfaces:** +- Consumes: `ConversionRule.prefix()` from Task 1; `processMethodAnnotations` from Task 2. +- Produces: `static String resolveKey(ConversionType type, ConversionRule rule, String name)` on `XWorkConverter` — package-private static, returns the prefixed mapping key, or `null` when `name` is null or empty. Callers in Tasks 4 and 5 rely on this exact signature and on `null` meaning "skip this entry". + +- [ ] **Step 1: Write the failing unit tests for `resolveKey`** + +Append to `core/src/test/java/org/apache/struts2/conversion/impl/XWorkConverterTest.java` (a `XWorkTestCase` subclass — plain `testXxx` methods, no `@Test`): + +```java + public void testResolveKeyPrependsTheRulePrefix() { + assertEquals("KeyProperty_annotatedBeanMap", + XWorkConverter.resolveKey(ConversionType.CLASS, ConversionRule.KEY_PROPERTY, "annotatedBeanMap")); + assertEquals("Element_annotatedBeanList", + XWorkConverter.resolveKey(ConversionType.CLASS, ConversionRule.ELEMENT, "annotatedBeanList")); + assertEquals("CreateIfNull_users", + XWorkConverter.resolveKey(ConversionType.CLASS, ConversionRule.CREATE_IF_NULL, "users")); + } + + public void testResolveKeyLeavesAnAlreadyPrefixedKeyAlone() { + assertEquals("KeyProperty_annotatedBeanMap", + XWorkConverter.resolveKey(ConversionType.CLASS, ConversionRule.KEY_PROPERTY, "KeyProperty_annotatedBeanMap")); + assertEquals("Key_beanMap", + XWorkConverter.resolveKey(ConversionType.CLASS, ConversionRule.KEY, "Key_beanMap")); + } + + public void testResolveKeyDoesNotPrefixPropertyOrMapRules() { + assertEquals("someProperty", + XWorkConverter.resolveKey(ConversionType.CLASS, ConversionRule.PROPERTY, "someProperty")); + assertEquals("keyValues", + XWorkConverter.resolveKey(ConversionType.CLASS, ConversionRule.MAP, "keyValues")); + } + + public void testResolveKeyNeverPrefixesApplicationScopedKeys() { + assertEquals("java.util.Date", + XWorkConverter.resolveKey(ConversionType.APPLICATION, ConversionRule.PROPERTY, "java.util.Date")); + assertEquals("java.util.Date", + XWorkConverter.resolveKey(ConversionType.APPLICATION, ConversionRule.ELEMENT, "java.util.Date")); + } + + public void testResolveKeyReturnsNullWhenNoNameIsAvailable() { + assertNull(XWorkConverter.resolveKey(ConversionType.CLASS, ConversionRule.PROPERTY, null)); + assertNull(XWorkConverter.resolveKey(ConversionType.CLASS, ConversionRule.KEY, "")); + } +``` + +Add these imports to that test class if not already present: + +```java +import org.apache.struts2.conversion.annotations.ConversionRule; +import org.apache.struts2.conversion.annotations.ConversionType; +``` + +- [ ] **Step 2: Run them to make sure they fail** + +Run: `mvn test -DskipAssembly -pl core -Dtest=XWorkConverterTest` +Expected: compilation failure — `cannot find symbol: method resolveKey(...)`. + +- [ ] **Step 3: Implement `resolveKey` and use it in the method pass** + +Add to `XWorkConverter` (place it just above `addConverterMapping`): + +```java + /** + * Resolves the conversion mapping key for an annotation: the given name carrying the + * {@link ConversionRule}'s prefix. A name that already starts with that prefix is returned + * unchanged, so annotations that spell the prefix out keep working. + * + * @param type the annotation's {@link ConversionType}; APPLICATION keys are class names and are never prefixed + * @param rule the annotation's {@link ConversionRule} + * @param name an explicit key or a property name derived from a method or field + * @return the mapping key, or null when no name is available and the entry must be skipped + * @since 7.3.0 + */ + static String resolveKey(ConversionType type, ConversionRule rule, String name) { + if (StringUtils.isEmpty(name)) { + return null; + } + if (type == ConversionType.APPLICATION) { + return name; + } + String prefix = rule.prefix(); + return name.startsWith(prefix) ? name : prefix + name; + } +``` + +Add the imports `org.apache.struts2.conversion.annotations.ConversionRule` and `org.apache.struts2.conversion.annotations.ConversionType`. + +Then replace the body of `processMethodAnnotations` (from Task 2) with: + +```java + private void processMethodAnnotations(Map mapping, Class clazz) { + for (Method method : clazz.getMethods()) { + for (Annotation annotation : method.getAnnotations()) { + if (!(annotation instanceof TypeConversion tc)) { + continue; + } + String name = StringUtils.isEmpty(tc.key()) ? AnnotationUtils.resolvePropertyName(method) : tc.key(); + String key = resolveKey(tc.type(), tc.rule(), name); + if (key == null) { + LOG.warn("Ignoring @TypeConversion on [{}#{}]: no key was given and no property name could be derived from the method", + clazz.getName(), method.getName()); + continue; + } + if (mapping.containsKey(key)) { + continue; + } + LOG.debug("TypeConversion [{}/{}] on method [{}] resolved to key [{}]", + tc.converter(), tc.converterClass(), method.getName(), key); + annotationProcessor.process(mapping, tc, key); + } + } + } +``` + +Two behaviour changes land here: an explicit method-level key is now normalised through `resolveKey`, and the `break` becomes `continue` so one already-mapped key no longer aborts the method's remaining annotations. `DefaultObjectTypeDeterminer` may no longer be referenced by this class — remove the import only if the compiler says it is unused. + +- [ ] **Step 4: Run the unit tests and make sure they pass** + +Run: `mvn test -DskipAssembly -pl core -Dtest=XWorkConverterTest` +Expected: PASS. + +- [ ] **Step 5: Write the failing fixture test for explicit-key normalisation** + +Create `core/src/test/java/org/apache/struts2/util/ExplicitKeyConversionAction.java` (Apache licence header copied from `MyBeanAction.java`): + +```java +package org.apache.struts2.util; + +import org.apache.struts2.conversion.annotations.ConversionRule; +import org.apache.struts2.conversion.annotations.TypeConversion; + +import java.util.ArrayList; +import java.util.List; + +/** + * Declares a method level {@link TypeConversion} with an explicit, unprefixed key. Before WW-3871 + * this registered a bare {@code bareList} mapping that nothing ever read. + */ +public class ExplicitKeyConversionAction { + + private List bareList = new ArrayList(); + + public List getBareList() { + return bareList; + } + + @TypeConversion(key = "bareList", rule = ConversionRule.CREATE_IF_NULL, value = "true") + public void setBareList(List bareList) { + this.bareList = bareList; + } +} +``` + +Append to `XWorkConverterTest`: + +```java + public void testExplicitMethodKeyGetsTheRulePrefix() { + XWorkConverter freshConverter = container.inject(XWorkConverter.class); + freshConverter.setTypeConverterHolder(new StrutsTypeConverterHolder()); + + assertEquals("true", freshConverter.getConverter(ExplicitKeyConversionAction.class, "CreateIfNull_bareList")); + assertNull(freshConverter.getConverter(ExplicitKeyConversionAction.class, "bareList")); + } +``` + +Add the import `org.apache.struts2.util.ExplicitKeyConversionAction` if the test class does not already import that package wholesale. + +- [ ] **Step 6: Run it and confirm it passes** + +Run: `mvn test -DskipAssembly -pl core -Dtest=XWorkConverterTest#testExplicitMethodKeyGetsTheRulePrefix` +Expected: PASS. + +- [ ] **Step 7: Run the wider conversion suite for regressions** + +Run: `mvn test -DskipAssembly -pl core -Dtest='XWorkConverterTest+AnnotationXWorkConverterTest+MyBeanActionTest+XWorkBasicConverterTest'` +Expected: PASS. + +- [ ] **Step 8: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/conversion/impl/XWorkConverter.java \ + core/src/test/java/org/apache/struts2/conversion/impl/XWorkConverterTest.java \ + core/src/test/java/org/apache/struts2/util/ExplicitKeyConversionAction.java +git commit -m "WW-3871 feat(core): derive conversion mapping keys through a single resolver" +``` + +--- + +### Task 4: Class-level bare keys, `continue` fix, empty-key warning + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/conversion/impl/XWorkConverter.java` (`processClassLevelAnnotations`) +- Test fixture (create): `core/src/test/java/org/apache/struts2/util/BareKeyConversionAction.java` +- Test fixture (create): `core/src/test/java/org/apache/struts2/util/CollidingKeyConversionAction.java` +- Test resource (create): `core/src/test/resources/org/apache/struts2/util/CollidingKeyConversionAction-conversion.properties` +- Test (modify): `core/src/test/java/org/apache/struts2/conversion/impl/XWorkConverterTest.java` + +**Interfaces:** +- Consumes: `XWorkConverter.resolveKey(ConversionType, ConversionRule, String)` from Task 3. +- Produces: nothing new; changes `processClassLevelAnnotations` behaviour only. + +- [ ] **Step 1: Write the failing tests** + +Create `core/src/test/java/org/apache/struts2/util/BareKeyConversionAction.java` (licence header first): + +```java +package org.apache.struts2.util; + +import org.apache.struts2.conversion.annotations.Conversion; +import org.apache.struts2.conversion.annotations.ConversionRule; +import org.apache.struts2.conversion.annotations.TypeConversion; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +/** + * The class level counterpart of {@link MyBeanAction}, declaring the same four conversions with + * bare property names instead of spelled-out prefixes. + */ +@Conversion( + conversions = { + @TypeConversion(key = "annotatedBeanMap", rule = ConversionRule.KEY_PROPERTY, value = "id"), + @TypeConversion(key = "annotatedBeanMap", rule = ConversionRule.ELEMENT, converterClass = MyBean.class), + @TypeConversion(key = "annotatedBeanList", rule = ConversionRule.KEY_PROPERTY, value = "id"), + @TypeConversion(key = "annotatedBeanList", rule = ConversionRule.ELEMENT, converterClass = MyBean.class) + }) +public class BareKeyConversionAction { + + private Map annotatedBeanMap = new HashMap(); + private List annotatedBeanList = new ArrayList(); + + public Map getAnnotatedBeanMap() { + return annotatedBeanMap; + } + + public void setAnnotatedBeanMap(Map annotatedBeanMap) { + this.annotatedBeanMap = annotatedBeanMap; + } + + public List getAnnotatedBeanList() { + return annotatedBeanList; + } + + public void setAnnotatedBeanList(List annotatedBeanList) { + this.annotatedBeanList = annotatedBeanList; + } +} +``` + +Note the same bare key appears twice with different rules — that is the point: the rule, not the key, disambiguates them. + +Create `core/src/test/java/org/apache/struts2/util/CollidingKeyConversionAction.java` (licence header first): + +```java +package org.apache.struts2.util; + +import org.apache.struts2.conversion.annotations.Conversion; +import org.apache.struts2.conversion.annotations.ConversionRule; +import org.apache.struts2.conversion.annotations.TypeConversion; + +import java.util.ArrayList; +import java.util.List; + +/** + * The first conversion entry collides with a key already supplied by + * {@code CollidingKeyConversionAction-conversion.properties}; the second must still be registered. + */ +@Conversion( + conversions = { + @TypeConversion(key = "fromProperties", rule = ConversionRule.CREATE_IF_NULL, value = "false"), + @TypeConversion(key = "afterTheCollision", rule = ConversionRule.CREATE_IF_NULL, value = "true") + }) +public class CollidingKeyConversionAction { + + private List afterTheCollision = new ArrayList(); + + public List getAfterTheCollision() { + return afterTheCollision; + } + + public void setAfterTheCollision(List afterTheCollision) { + this.afterTheCollision = afterTheCollision; + } +} +``` + +Create `core/src/test/resources/org/apache/struts2/util/CollidingKeyConversionAction-conversion.properties` (comment-style Apache licence header copied verbatim from `core/src/test/resources/org/apache/struts2/test/DataAware-conversion.properties`, then): + +```properties +CreateIfNull_fromProperties=true +``` + +Append to `XWorkConverterTest`: + +```java + public void testClassLevelBareKeysGetTheRulePrefix() { + XWorkConverter freshConverter = container.inject(XWorkConverter.class); + freshConverter.setTypeConverterHolder(new StrutsTypeConverterHolder()); + + assertEquals("id", freshConverter.getConverter(BareKeyConversionAction.class, "KeyProperty_annotatedBeanMap")); + assertEquals(MyBean.class, freshConverter.getConverter(BareKeyConversionAction.class, "Element_annotatedBeanMap")); + assertEquals("id", freshConverter.getConverter(BareKeyConversionAction.class, "KeyProperty_annotatedBeanList")); + assertEquals(MyBean.class, freshConverter.getConverter(BareKeyConversionAction.class, "Element_annotatedBeanList")); + } + + public void testClassLevelBareKeysMatchTheSpelledOutForm() { + XWorkConverter freshConverter = container.inject(XWorkConverter.class); + freshConverter.setTypeConverterHolder(new StrutsTypeConverterHolder()); + + for (String key : new String[]{"KeyProperty_annotatedBeanMap", "Element_annotatedBeanMap", + "KeyProperty_annotatedBeanList", "Element_annotatedBeanList"}) { + assertEquals("mismatch for " + key, + freshConverter.getConverter(MyBeanAction.class, key), + freshConverter.getConverter(BareKeyConversionAction.class, key)); + } + } + + public void testClassLevelEntriesAfterAKeyCollisionAreStillRegistered() { + XWorkConverter freshConverter = container.inject(XWorkConverter.class); + freshConverter.setTypeConverterHolder(new StrutsTypeConverterHolder()); + + // supplied by the -conversion.properties file, so the annotation must not overwrite it + assertEquals("true", freshConverter.getConverter(CollidingKeyConversionAction.class, "CreateIfNull_fromProperties")); + // the entry after the collision used to be dropped by `break` + assertEquals("true", freshConverter.getConverter(CollidingKeyConversionAction.class, "CreateIfNull_afterTheCollision")); + } +``` + +Add imports for `org.apache.struts2.util.BareKeyConversionAction`, `org.apache.struts2.util.CollidingKeyConversionAction`, `org.apache.struts2.util.MyBean` and `org.apache.struts2.util.MyBeanAction` as needed. + +- [ ] **Step 2: Run them to make sure they fail** + +Run: `mvn test -DskipAssembly -pl core -Dtest='XWorkConverterTest#testClassLevelBareKeysGetTheRulePrefix+XWorkConverterTest#testClassLevelEntriesAfterAKeyCollisionAreStillRegistered'` +Expected: FAIL — bare keys resolve to null (no prefix applied), and the post-collision entry is missing. + +- [ ] **Step 3: Rewrite `processClassLevelAnnotations`** + +```java + private void processClassLevelAnnotations(Map mapping, Class clazz) { + for (Annotation annotation : clazz.getAnnotations()) { + if (!(annotation instanceof Conversion conversion)) { + continue; + } + for (TypeConversion tc : conversion.conversions()) { + String key = resolveKey(tc.type(), tc.rule(), tc.key()); + if (key == null) { + LOG.warn("Ignoring @TypeConversion [{}/{}] declared on [{}]: no key was given and a class level annotation has no property name to derive one from", + tc.converter(), tc.converterClass(), clazz.getName()); + continue; + } + if (mapping.containsKey(key)) { + continue; + } + LOG.debug("TypeConversion [{}/{}] declared on [{}] resolved to key [{}]", + tc.converter(), tc.converterClass(), clazz.getName(), key); + annotationProcessor.process(mapping, tc, key); + } + } + } +``` + +Three changes: `resolveKey` is applied, the `containsKey` guard now tests the *resolved* key and `continue`s instead of `break`ing, and an unresolvable key is skipped with a WARN rather than registering a `""` mapping. + +- [ ] **Step 4: Run the new tests and make sure they pass** + +Run: `mvn test -DskipAssembly -pl core -Dtest=XWorkConverterTest` +Expected: PASS. + +- [ ] **Step 5: Run the conversion suite for regressions** + +Run: `mvn test -DskipAssembly -pl core -Dtest='XWorkConverterTest+AnnotationXWorkConverterTest+MyBeanActionTest+XWorkBasicConverterTest+StringConverterTest'` +Expected: PASS. `MyBeanActionTest` passing here is the backward-compatibility proof: its fixture still spells out every prefix. + +- [ ] **Step 6: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/conversion/impl/XWorkConverter.java \ + core/src/test/java/org/apache/struts2/conversion/impl/XWorkConverterTest.java \ + core/src/test/java/org/apache/struts2/util/BareKeyConversionAction.java \ + core/src/test/java/org/apache/struts2/util/CollidingKeyConversionAction.java \ + core/src/test/resources/org/apache/struts2/util/CollidingKeyConversionAction-conversion.properties +git commit -m "WW-3871 fix(core): derive class level conversion keys and stop dropping later entries" +``` + +--- + +### Task 5: Field-level `@TypeConversion` + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/conversion/annotations/TypeConversion.java:153` (`@Target`) +- Modify: `core/src/main/java/org/apache/struts2/conversion/impl/XWorkConverter.java` (`addConverterMapping`, new `processFieldAnnotations`) +- Test fixture (create): `core/src/test/java/org/apache/struts2/util/FieldConversionAction.java` +- Test (modify): `core/src/test/java/org/apache/struts2/conversion/impl/XWorkConverterTest.java` + +**Interfaces:** +- Consumes: `XWorkConverter.resolveKey(ConversionType, ConversionRule, String)` from Task 3. +- Produces: `private void processFieldAnnotations(Map mapping, Class clazz)`, called last from `addConverterMapping`. + +- [ ] **Step 1: Write the failing test** + +Create `core/src/test/java/org/apache/struts2/util/FieldConversionAction.java` (licence header first): + +```java +package org.apache.struts2.util; + +import org.apache.struts2.conversion.annotations.ConversionRule; +import org.apache.struts2.conversion.annotations.TypeConversion; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +/** + * Exercises field level {@link TypeConversion}: {@code fieldOnlyList} is annotated on the field + * alone, while {@code contestedMap} is annotated on both the field and its setter so the + * class > method > field precedence can be asserted. + */ +public class FieldConversionAction { + + @TypeConversion(rule = ConversionRule.CREATE_IF_NULL, value = "true") + private List fieldOnlyList = new ArrayList(); + + @TypeConversion(rule = ConversionRule.KEY, converterClass = String.class) + private Map contestedMap = new HashMap(); + + public List getFieldOnlyList() { + return fieldOnlyList; + } + + public void setFieldOnlyList(List fieldOnlyList) { + this.fieldOnlyList = fieldOnlyList; + } + + public Map getContestedMap() { + return contestedMap; + } + + @TypeConversion(rule = ConversionRule.KEY, converterClass = Long.class) + public void setContestedMap(Map contestedMap) { + this.contestedMap = contestedMap; + } +} +``` + +Append to `XWorkConverterTest`: + +```java + public void testFieldLevelAnnotationDerivesKeyFromTheFieldName() { + XWorkConverter freshConverter = container.inject(XWorkConverter.class); + freshConverter.setTypeConverterHolder(new StrutsTypeConverterHolder()); + + assertEquals("true", freshConverter.getConverter(FieldConversionAction.class, "CreateIfNull_fieldOnlyList")); + } + + public void testMethodAnnotationWinsOverFieldAnnotation() { + XWorkConverter freshConverter = container.inject(XWorkConverter.class); + freshConverter.setTypeConverterHolder(new StrutsTypeConverterHolder()); + + assertEquals(Long.class, freshConverter.getConverter(FieldConversionAction.class, "Key_contestedMap")); + } +``` + +Add the import for `org.apache.struts2.util.FieldConversionAction`. + +- [ ] **Step 2: Run it to make sure it fails** + +Run: `mvn test-compile -DskipAssembly -pl core` +Expected: compilation failure — `annotation type org.apache.struts2.conversion.annotations.TypeConversion is not applicable to this kind of declaration` on the two annotated fields. + +- [ ] **Step 3: Widen the annotation target** + +In `TypeConversion.java`, change line 153: + +```java +@Target({ElementType.METHOD, ElementType.FIELD}) +``` + +- [ ] **Step 4: Run the test again — it must now fail on the assertion, not on compilation** + +Run: `mvn test -DskipAssembly -pl core -Dtest=XWorkConverterTest#testFieldLevelAnnotationDerivesKeyFromTheFieldName` +Expected: FAIL — `expected: but was:`; the annotation compiles but nothing reads it yet. + +- [ ] **Step 5: Add the field pass** + +In `XWorkConverter`, add the call at the end of `addConverterMapping`: + +```java + protected void addConverterMapping(Map mapping, Class clazz) { + // Process -conversion.properties file + String converterFilename = buildConverterFilename(clazz); + fileProcessor.process(mapping, clazz, converterFilename); + + processClassLevelAnnotations(mapping, clazz); + processMethodAnnotations(mapping, clazz); + processFieldAnnotations(mapping, clazz); + } +``` + +and the new method after `processMethodAnnotations`: + +```java + /** + * Registers {@link TypeConversion} annotations found on the class' own fields. Only declared + * fields are read: {@link #buildConverterMapping(Class)} already walks the class hierarchy and + * calls this method once per class. Static and synthetic fields are skipped, which also makes + * this a no-op for interfaces. + */ + private void processFieldAnnotations(Map mapping, Class clazz) { + for (Field field : clazz.getDeclaredFields()) { + if (Modifier.isStatic(field.getModifiers()) || field.isSynthetic()) { + continue; + } + for (Annotation annotation : field.getAnnotations()) { + if (!(annotation instanceof TypeConversion tc)) { + continue; + } + String name = StringUtils.isEmpty(tc.key()) ? field.getName() : tc.key(); + String key = resolveKey(tc.type(), tc.rule(), name); + if (key == null) { + // defensive: a field always has a name, so this is unreachable in practice + LOG.warn("Ignoring @TypeConversion on field [{}#{}]: the key could not be resolved", + clazz.getName(), field.getName()); + continue; + } + if (mapping.containsKey(key)) { + LOG.debug("Skipping @TypeConversion on field [{}#{}]: key [{}] is already mapped by a higher precedence source", + clazz.getName(), field.getName(), key); + continue; + } + LOG.debug("TypeConversion [{}/{}] on field [{}] resolved to key [{}]", + tc.converter(), tc.converterClass(), field.getName(), key); + annotationProcessor.process(mapping, tc, key); + } + } + } +``` + +Add the imports `java.lang.reflect.Field` and `java.lang.reflect.Modifier`. + +- [ ] **Step 6: Run the new tests and make sure they pass** + +Run: `mvn test -DskipAssembly -pl core -Dtest=XWorkConverterTest` +Expected: PASS, including `testMethodAnnotationWinsOverFieldAnnotation` — the method pass runs first and claims `Key_contestedMap` with `Long.class`. + +- [ ] **Step 7: Run the full core module** + +Run: `mvn test -DskipAssembly -pl core` +Expected: PASS. This is the first full run; field scanning now touches every class the converter maps, so a broad run is warranted here rather than at the end. + +- [ ] **Step 8: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/conversion/annotations/TypeConversion.java \ + core/src/main/java/org/apache/struts2/conversion/impl/XWorkConverter.java \ + core/src/test/java/org/apache/struts2/conversion/impl/XWorkConverterTest.java \ + core/src/test/java/org/apache/struts2/util/FieldConversionAction.java +git commit -m "WW-3871 feat(core): support @TypeConversion on fields" +``` + +--- + +### Task 6: End-to-end proof through the action lifecycle + +The unit tests assert mapping contents; this asserts that bare keys actually bind request parameters, through the same path `MyBeanActionTest` exercises. + +**Files:** +- Test fixture (create): `core/src/test/java/org/apache/struts2/util/MyBeanBareKeyAction.java` +- Test resource (modify): `core/src/test/resources/xwork-sample.xml:128-132` (add a sibling action) +- Test (modify): `core/src/test/java/org/apache/struts2/util/MyBeanActionTest.java` + +**Interfaces:** +- Consumes: everything from Tasks 3-5. +- Produces: action name `MyBeanBareKey` in `xwork-sample.xml`. + +- [ ] **Step 1: Write the failing test** + +Create `core/src/test/java/org/apache/struts2/util/MyBeanBareKeyAction.java` (licence header first) — a copy of `MyBeanAction` with bare class-level keys and no `beanList`/`beanMap` properties: + +```java +package org.apache.struts2.util; + +import org.apache.struts2.action.Action; +import org.apache.struts2.conversion.annotations.Conversion; +import org.apache.struts2.conversion.annotations.ConversionRule; +import org.apache.struts2.conversion.annotations.TypeConversion; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +/** + * {@link MyBeanAction} restated with bare property names as conversion keys. Both must bind + * identically; {@code MyBeanAction} keeps the spelled-out prefixes so the old form stays covered. + */ +@Conversion( + conversions = { + @TypeConversion(key = "annotatedBeanMap", rule = ConversionRule.KEY_PROPERTY, value = "id"), + @TypeConversion(key = "annotatedBeanMap", rule = ConversionRule.ELEMENT, converterClass = MyBean.class), + @TypeConversion(key = "annotatedBeanList", rule = ConversionRule.KEY_PROPERTY, value = "id"), + @TypeConversion(key = "annotatedBeanList", rule = ConversionRule.ELEMENT, converterClass = MyBean.class) + }) +public class MyBeanBareKeyAction implements Action { + + private Map annotatedBeanMap = new HashMap(); + private List annotatedBeanList = new ArrayList(); + + public Map getAnnotatedBeanMap() { + return annotatedBeanMap; + } + + @TypeConversion(rule = ConversionRule.KEY, converterClass = Long.class) + public void setAnnotatedBeanMap(Map annotatedBeanMap) { + this.annotatedBeanMap = annotatedBeanMap; + } + + public List getAnnotatedBeanList() { + return annotatedBeanList; + } + + @TypeConversion(rule = ConversionRule.CREATE_IF_NULL, value = "true") + public void setAnnotatedBeanList(List annotatedBeanList) { + this.annotatedBeanList = annotatedBeanList; + } + + public String execute() throws Exception { + return SUCCESS; + } +} +``` + +Register it in `core/src/test/resources/xwork-sample.xml`, immediately after the existing `MyBean` action (line 132): + +```xml + + + + + +``` + +Append to `MyBeanActionTest`: + +```java + public void testBareConversionKeysBindTheSameWayAsPrefixedOnes() throws Exception { + HashMap params = new HashMap<>(); + params.put("annotatedBeanList(1234567890).name", "This is the bla bean by annotation"); + params.put("annotatedBeanMap[1234567891].id", "1234567891"); + params.put("annotatedBeanMap[1234567891].name", "This is the 2nd bla bean by annotation"); + + ActionContext extraContext = ActionContext.of().withParameters(HttpParameters.create(params).build()); + + ActionProxy proxy = actionProxyFactory.createActionProxy("", "MyBeanBareKey", null, extraContext.getContextMap()); + proxy.execute(); + MyBeanBareKeyAction action = (MyBeanBareKeyAction) proxy.getInvocation().getAction(); + + // CreateIfNull_annotatedBeanList + Element_annotatedBeanList + assertEquals(1, action.getAnnotatedBeanList().size()); + assertEquals(MyBean.class, action.getAnnotatedBeanList().get(0).getClass()); + assertEquals("This is the bla bean by annotation", + proxy.getInvocation().getStack().findValue("annotatedBeanList.get(0).name")); + + // Key_annotatedBeanMap makes the key a Long, Element_annotatedBeanMap makes the value a MyBean + assertTrue(action.getAnnotatedBeanMap().containsKey(1234567891L)); + assertEquals(MyBean.class, action.getAnnotatedBeanMap().get(1234567891L).getClass()); + assertEquals("This is the 2nd bla bean by annotation", + proxy.getInvocation().getStack().findValue("annotatedBeanMap.get(1234567891L).name")); + } +``` + +Unlike the existing tests in this class, this one lets exceptions propagate rather than catching and calling `fail()` — a stack trace from the runner is more useful than a bare failure. + +- [ ] **Step 2: Run it — it must pass on the first try** + +This task adds no production code, so there is no red phase: Tasks 3-5 already implement the behaviour and this test only proves it reaches the action. A failure here is a real defect in those tasks, not a missing feature — debug it there rather than adjusting this test. + +Run: `mvn test -DskipAssembly -pl core -Dtest=MyBeanActionTest#testBareConversionKeysBindTheSameWayAsPrefixedOnes` +Expected: PASS. + +- [ ] **Step 3: Confirm the old form still binds** + +Run: `mvn test -DskipAssembly -pl core -Dtest=MyBeanActionTest` +Expected: PASS — all three tests, including the two pre-existing ones against the spelled-out fixture. + +- [ ] **Step 4: Commit** + +```bash +git add core/src/test/java/org/apache/struts2/util/MyBeanBareKeyAction.java \ + core/src/test/java/org/apache/struts2/util/MyBeanActionTest.java \ + core/src/test/resources/xwork-sample.xml +git commit -m "WW-3871 test(core): assert bare conversion keys bind through the action lifecycle" +``` + +--- + +### Task 7: Documentation + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/conversion/annotations/TypeConversion.java` (class Javadoc: usage snippet, parameter table, example) + +**Interfaces:** +- Consumes: the behaviour built in Tasks 1-5. +- Produces: nothing code-facing. + +- [ ] **Step 1: Update the usage snippet** + +Replace the `usage` snippet body (line 51) with: + +```java + *

The TypeConversion annotation can be applied at field and method level.

+``` + +- [ ] **Step 2: Update the `key` row of the parameter table** + +Replace the `key` row (lines 67-72) with: + +```java + * + * key + * no + * The annotated property/field name + * The property name the rule applies to. The matching prefix for the given rule + * (Key_, Element_, KeyProperty_, CreateIfNull_) + * is prepended automatically unless the key already carries it. Required on TYPE level annotations, + * where there is no member name to derive it from. + * +``` + +- [ ] **Step 3: Update the `key()` attribute Javadoc** + +Replace the Javadoc on `key()` (lines 157-163) with: + +```java + /** + * The property name this conversion applies to. Optional on fields and methods, where it + * defaults to the property name; required on TYPE level annotations. + * + *

The prefix matching the declared {@link ConversionRule} is prepended automatically, so + * {@code @TypeConversion(key = "users", rule = ConversionRule.CREATE_IF_NULL, value = "true")} + * and {@code @TypeConversion(key = "CreateIfNull_users", ...)} are equivalent.

+ * + * @return key + * @since 7.3.0 the rule prefix is derived; previously the full key had to be spelled out + */ +``` + +- [ ] **Step 4: Simplify the example** + +In the example snippet, add a field-level case and drop a redundant prefix. Replace the `setUsers` example (lines 132-135) with: + +```java + * @TypeConversion(rule = ConversionRule.CREATE_IF_NULL, value = "true") + * private List users = null; + * + * @TypeConversion(rule = ConversionRule.COLLECTION, converterClass = String.class) + * public void setUsers( List users ) { + * this.users = users; + * } +``` + +- [ ] **Step 5: Verify the Javadoc builds** + +Run: `mvn javadoc:javadoc -DskipAssembly -pl core -q` +Expected: no errors on `TypeConversion.java` or `ConversionRule.java`. Pre-existing warnings elsewhere in the module are not this task's concern. + +- [ ] **Step 6: Run the full core suite one last time** + +Run: `mvn test -DskipAssembly -pl core` +Expected: PASS. + +- [ ] **Step 7: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/conversion/annotations/TypeConversion.java +git commit -m "WW-3871 docs(core): document conversion key derivation and field level support" +``` + +--- + +## Verification Checklist + +Before opening the PR: + +- [ ] `mvn test -DskipAssembly -pl core` passes in full +- [ ] `MyBeanActionTest` still passes with its original spelled-out prefixes untouched — the backward-compatibility guarantee +- [ ] `git log --oneline main..HEAD` shows every commit prefixed `WW-3871` +- [ ] PR title: `WW-3871 Derive ConversionRule prefixes for @TypeConversion keys` +- [ ] PR body links the ticket: `Fixes [WW-3871](https://issues.apache.org/jira/browse/WW-3871)` +- [ ] This is not a security patch (no OGNL evaluation, parameter filtering, upload or auth path is touched), so a public PR is appropriate diff --git a/docs/superpowers/specs/2026-07-25-WW-3871-typeconversion-key-derivation-design.md b/docs/superpowers/specs/2026-07-25-WW-3871-typeconversion-key-derivation-design.md new file mode 100644 index 0000000000..c3cc34b247 --- /dev/null +++ b/docs/superpowers/specs/2026-07-25-WW-3871-typeconversion-key-derivation-design.md @@ -0,0 +1,229 @@ +# WW-3871 — `@TypeConversion` key derivation + +**Ticket:** [WW-3871](https://issues.apache.org/jira/browse/WW-3871) — TypeConversion annotation support improvement +**Target version:** 7.3.0 +**Date:** 2026-07-25 + +## Problem + +The reporter asked that `@TypeConversion` build its own key from the property name once a +`ConversionRule` is given, instead of forcing: + +```java +@TypeConversion(key = "CreateIfNull_users", rule = ConversionRule.CreateIfNull, value = "true") +``` + +Half of this already works. Commit `77cbafb74` (2018) taught `XWorkConverter` to derive the key for +**method-level** annotations: with no `key`, the property name is resolved from the method and the +rule's prefix is prepended (`XWorkConverter.java:526-545`). + +Three gaps remain: + +1. **Class-level `@Conversion(conversions = {...})` has no derivation at all.** The `key` is used + verbatim (`XWorkConverter.java:504-519`), so callers still spell out prefixes — see + `core/src/test/java/org/apache/struts2/util/MyBeanAction.java:38-41`. +2. **`@TypeConversion` is `@Target({METHOD})`**, while its own Javadoc claims it "can be applied at + property and method level" (`TypeConversion.java:51`). +3. **Explicit keys are never normalised.** `@TypeConversion(key = "foo", rule = CREATE_IF_NULL)` on a + setter registers `foo`, a mapping nothing reads — `DefaultObjectTypeDeterminer` looks up + `CreateIfNull_foo`. + +Two defects live in the same code block and are fixed here rather than left behind: + +- `break` where `continue` is meant (`XWorkConverter.java:508` and `:542`). One already-mapped key + aborts the **remaining** `@TypeConversion` entries in a `@Conversion` array. +- An empty class-level `key` registers a mapping under `""`. `DefaultConversionAnnotationProcessor` + guards only `null` (`:58`). + +## Goals + +- A bare property name in `key` works at class, method and field level, for every `ConversionRule`. +- Existing annotations that spell out the prefix keep working byte-for-byte. +- `@TypeConversion` becomes usable on fields, matching its documentation. +- The rule-to-prefix table lives in one place. + +## Non-goals + +- `ConversionRule.COLLECTION` / the `Collection_` prefix stays deprecated-as-is. +- `ConversionRule.MAP` keeps having no prefix of its own. +- No changes to `struts-conversion.properties` or `-conversion.properties` parsing. This + ticket is annotations only. + +## Design + +### 1. Rule-to-prefix mapping moves onto `ConversionRule` + +`org.apache.struts2.conversion.annotations.ConversionRule` gains: + +```java +public String prefix() { + return switch (this) { + case COLLECTION -> DefaultObjectTypeDeterminer.DEPRECATED_ELEMENT_PREFIX; // "Collection_" + case CREATE_IF_NULL -> DefaultObjectTypeDeterminer.CREATE_IF_NULL_PREFIX; // "CreateIfNull_" + case ELEMENT -> DefaultObjectTypeDeterminer.ELEMENT_PREFIX; // "Element_" + case KEY -> DefaultObjectTypeDeterminer.KEY_PREFIX; // "Key_" + case KEY_PROPERTY -> DefaultObjectTypeDeterminer.KEY_PROPERTY_PREFIX; // "KeyProperty_" + case PROPERTY, MAP -> ""; + }; +} +``` + +`PROPERTY` and `MAP` returning `""` preserves today's behaviour: `DefaultObjectTypeDeterminer` reads +map and collection metadata through the `Key_` and `Element_` keys, never through a `Map_` key. + +The `annotations` package already depends on `conversion.impl` (`TypeConversion.converterClass()` +defaults to `XWorkBasicConverter.class`), so referencing the prefix constants adds no new coupling. +An exhaustive `switch` over the enum means a future rule cannot silently miss a prefix. + +### 2. One key resolver in `XWorkConverter` + +```java +static String resolveKey(ConversionType type, ConversionRule rule, String name) { + if (name == null || name.isEmpty()) { + return null; // caller skips the entry and logs WARN + } + if (type == ConversionType.APPLICATION) { + return name; // key is a class name, never prefixed + } + String prefix = rule.prefix(); + if (name.startsWith(prefix)) { + return name; + } + for (ConversionRule other : ConversionRule.values()) { + String otherPrefix = other.prefix(); + if (!otherPrefix.isEmpty() && name.startsWith(otherPrefix)) { + return name; // already carries a rule prefix; leave it exactly as written + } + } + return prefix + name; +} +``` + +It takes the two annotation attributes rather than the `TypeConversion` instance so it can be unit +tested directly — annotation instances are awkward to construct in a test. + +All three annotation passes route both explicit keys and derived property names through this +function, so the three call sites cannot drift apart. + +**The `APPLICATION` carve-out** is belt-and-braces: application-scope entries use the default +`PROPERTY` rule in practice, where the prefix is `""` anyway. Without it, +`@TypeConversion(type = APPLICATION, key = "java.util.Date", rule = ELEMENT)` would register +`Element_java.util.Date` into the global converter map, which nothing reads. + +`APPLICATION`-scoped entries are stored in the global default converter map, which is keyed by +**class name** (`DefaultConversionAnnotationProcessor` calls `converterHolder.addDefaultMapping(key, +converter)`, and the only readers, `XWorkConverter.lookup(String, boolean)` and `lookup(Class)`, both +key off a class name). A method or field annotation has no class name to offer, only a member name, +so `resolveKey` cannot derive one: `processMethodAnnotations` and `processFieldAnnotations` skip an +`APPLICATION`-typed entry with an empty `key` before deriving a name, logging a WARN that names the +declaring class and the member. The class-level pass needs no equivalent guard - `key` is required +there for an unrelated reason (no member to derive a property name from at all), and an empty key is +already skipped by the existing null-from-`resolveKey` path. + +**The "already prefixed" guard checks every rule's prefix, not just the declared rule's.** An +already-prefixed key is returned untouched, so `key = "KeyProperty_annotatedBeanMap"` and +`key = "annotatedBeanMap"` both resolve to `KeyProperty_annotatedBeanMap` under `rule = KEY_PROPERTY`. +Checking only the declared rule's prefix is not enough: `COLLECTION` and `ELEMENT` are interchangeable +in both `DefaultConversionAnnotationProcessor` (same branch handles both) and +`DefaultObjectTypeDeterminer.getElementClass` (reads `Element_`, falls back to the deprecated +`Collection_`), so `key = "Element_users", rule = COLLECTION` must not become +`Collection_Element_users`. Matching against every rule's prefix keeps that crossover working. It +misfires only for a property literally named `KeyProperty_foo` (or another prefix), which is legal +Java but effectively nonexistent; such a property gets exactly today's behaviour. + +### 3. Field-level support + +`@TypeConversion` becomes `@Target({ElementType.METHOD, ElementType.FIELD})`. + +`addConverterMapping` splits into four ordered passes, keeping today's first-writer-wins rule: + +```java +protected void addConverterMapping(Map mapping, Class clazz) { + fileProcessor.process(mapping, clazz, buildConverterFilename(clazz)); // 1 + processClassLevelAnnotations(mapping, clazz); // 2 + processMethodAnnotations(mapping, clazz); // 3 + processFieldAnnotations(mapping, clazz); // 4 new +} +``` + +This yields the precedence **class > method > field**, which preserves current behaviour exactly: +class-level `@Conversion` already outranks methods, and field annotations — none of which exist +today — only fill gaps. + +Extracting the three passes into named private methods is the targeted cleanup this ticket earns. +The current single method is roughly 45 lines of nested loops concealing the `break` defect, and a +fourth pass would push it past readable. + +The field pass iterates `clazz.getDeclaredFields()` — declared, not inherited, because +`buildConverterMapping` already walks the class hierarchy and calls `addConverterMapping` per class. +It skips `static` and synthetic fields, which also makes the interface case a no-op +(`getDeclaredFields()` on an interface returns its constants). A field's own name is the property +name; no getter/setter parsing is involved. + +### 4. Interaction with the 7.3.0 mapping cache + +`addConverterMapping` runs inside the builder that `XWorkConverter.buildConverterMapping` hands to +`TypeConverterHolder.computeMappingIfAbsent` (WW-5539). That builder deliberately executes **outside** +any lock — `StrutsTypeConverterHolder.java:171-188` documents why: it instantiates, and under +`SpringObjectFactory` autowires, arbitrary user-supplied `TypeConverter`s, so running it under a +`ConcurrentHashMap` bin lock would risk `IllegalStateException("Recursive update")` or self-deadlock +if any of that re-enters conversion. + +The new field pass instantiates converters exactly as the existing method pass does, so it inherits +that safety and adds no new hazard. The one consequence to respect: under first-access contention the +builder may run more than once for the same class, so every pass must stay free of side effects +outside the `mapping` map it is handed. + +### 5. Error handling + +| Situation | Today | After | +|---|---|---| +| Class-level entry with `key = ""` | registers a `""` mapping | skipped, `WARN` naming class and annotation | +| `@TypeConversion` on a non-property method (`execute()`) with no key | silently dropped at DEBUG | skipped, `WARN` naming class and method | +| Field annotation whose key a method already claimed | n/a | skipped, `DEBUG` — the documented precedence, made visible | +| Second entry in a `@Conversion` array after a key collision | **dropped** (`break`) | processed (`continue`) | + +The class-level `mapping.containsKey(...)` check moves **after** key resolution; it currently tests +the raw key, which post-change would be the wrong string. `DefaultConversionAnnotationProcessor.process` +keeps its `key == null` guard as defence in depth even though callers no longer pass null. + +## Testing + +Both affected test classes extend `XWorkTestCase`, i.e. JUnit 3 style — new tests use +`public void testXxx()` with no `@Test` annotation, which would silently never run there. + +**`XWorkConverterTest` / `AnnotationXWorkConverterTest`:** + +- bare key + rule resolves to the prefixed key +- already-prefixed key + same rule is unchanged (idempotence) +- `PROPERTY` and `MAP` rules leave the key untouched +- `type = APPLICATION` leaves the key untouched regardless of rule +- empty key and non-property method produce no mapping entry, and specifically no `""` key + +**`MyBeanActionTest` — end-to-end:** `MyBeanAction` keeps its four spelled-out class-level prefixes +untouched, which is what proves existing applications don't break. A second fixture action declares +the same four conversions with **bare** keys; the test asserts both produce identical converter +mappings and identical bound results. + +**Field support:** a fixture with `@TypeConversion` on a private field, asserting its derived key +matches the setter form's, plus a test that a method annotation wins when both a field and its +setter are annotated for the same key. + +**`continue` regression:** a fixture whose `@Conversion` array has an early entry colliding with a +key already in the mapping, asserting the later entries still register. This regression is currently +invisible. + +## Documentation + +- `TypeConversion` Javadoc: the parameter table's `key` row gains the derivation rule (it currently + says only "Defaults to the property name", which understates it); the class-level example drops its + now-redundant prefixes; an `@since 7.3.0` note records that bare keys are accepted at class and + field level, and that fields are a supported target. +- `ConversionRule` Javadoc: document `prefix()` and which rules have none. + +## Compatibility + +Source- and binary-compatible. The only behavioural change to existing code is that an explicit +method-level key carrying a non-`PROPERTY` rule without its prefix now resolves to the prefixed key. +That mapping is unreachable today, so the change turns a silent no-op into the behaviour the author +intended.