This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch WW-3871-typeconversion-key-derivation in repository https://gitbox.apache.org/repos/asf/struts.git
commit 955e3d9d10653ec3f8062160c6a7b9142d764809 Author: Lukasz Lenart <[email protected]> AuthorDate: Sat Jul 25 17:11:25 2026 +0200 WW-3871 fix(core): widen resolveKey idempotence guard against any rule prefix resolveKey only recognized a key as already-prefixed if it started with its own declared rule's prefix. COLLECTION and ELEMENT are interchangeable throughout the conversion pipeline (DefaultConversionAnnotationProcessor handles them in the same branch, DefaultObjectTypeDeterminer.getElementClass reads Element_ then falls back to the deprecated Collection_), so key="Element_users" with rule=COLLECTION silently doubled to Collection_Element_users instead of being left alone, losing the mapping. Match against every known rule's prefix instead. Also documents two related precedence subtleties surfaced during review: processFieldAnnotations' Javadoc now notes that an inherited method can claim a key before a subclass's own field annotation is considered, since getMethods() includes inherited methods and runs first; and the unresolvable-key WARN in processMethodAnnotations now names the method's declaring class rather than the class being scanned, since getMethods() can surface the same inherited method at every level of the hierarchy. Design spec section 2 updated to match the implementation. --- .../struts2/conversion/impl/XWorkConverter.java | 30 +++++++++++++++++++--- ...WW-3871-typeconversion-key-derivation-design.md | 26 ++++++++++++++----- 2 files changed, 46 insertions(+), 10 deletions(-) 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 0dcd4fcaf..d85689d3f 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 @@ -492,8 +492,12 @@ public class XWorkConverter extends DefaultTypeConverter { /** * 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. + * {@link ConversionRule}'s prefix. A name that already starts with <em>any</em> 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} @@ -509,7 +513,16 @@ public class XWorkConverter extends DefaultTypeConverter { return name; } String prefix = rule.prefix(); - return name.startsWith(prefix) ? name : prefix + name; + 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; } /** @@ -567,8 +580,11 @@ public class XWorkConverter extends DefaultTypeConverter { String name = StringUtils.isEmpty(tc.key()) ? AnnotationUtils.resolvePropertyName(method) : tc.key(); String key = resolveKey(tc.type(), tc.rule(), name); if (key == null) { + // method.getDeclaringClass(), not clazz: getMethods() returns inherited methods too, + // so an annotation on one superclass method can otherwise log once per subclass in + // the hierarchy, each naming a different, misleading class. LOG.warn("Ignoring @TypeConversion on [{}#{}]: no key was given and no property name could be derived from the method", - clazz.getName(), method.getName()); + method.getDeclaringClass().getName(), method.getName()); continue; } if (mapping.containsKey(key)) { @@ -586,6 +602,12 @@ public class XWorkConverter extends DefaultTypeConverter { * 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. + * + * <p>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.</p> */ private void processFieldAnnotations(Map<String, Object> mapping, Class clazz) { for (Field field : clazz.getDeclaredFields()) { 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 index 150d861a4..30b301c24 100644 --- 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 @@ -86,7 +86,16 @@ static String resolveKey(ConversionType type, ConversionRule rule, String name) return name; // key is a class name, never prefixed } String prefix = rule.prefix(); - return name.startsWith(prefix) ? name : prefix + name; + 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; } ``` @@ -101,11 +110,16 @@ function, so the three call sites cannot drift apart. `@TypeConversion(type = APPLICATION, key = "java.util.Date", rule = ELEMENT)` would register `Element_java.util.Date` into the global converter map, which nothing reads. -**`name.startsWith(prefix)`** is the backward-compatibility guarantee: an already-prefixed key is -returned untouched, so `key = "KeyProperty_annotatedBeanMap"` and `key = "annotatedBeanMap"` both -resolve to `KeyProperty_annotatedBeanMap` under `rule = KEY_PROPERTY`. 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. +**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_<prop>`, falls back to the deprecated +`Collection_<prop>`), 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
