From cf0cb2e224dde8af92ad3865551192e591a33a18 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 25 Jul 2026 13:49:07 +0200 Subject: [PATCH 01/23] WW-3871 docs: add design spec for @TypeConversion key derivation Specifies deriving the ConversionRule prefix for @TypeConversion keys at class, method and field level via a single resolver, adds ElementType.FIELD as a target, and records the break/continue and empty-key fixes in the same code block. Co-Authored-By: Claude Opus 5 --- ...71-typeconversion-key-derivation-design.md | 188 ++++++++++++++++++ 1 file changed, 188 insertions(+) create mode 100644 docs/superpowers/specs/2026-07-25-WW-3871-typeconversion-key-derivation-design.md 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..340968138c --- /dev/null +++ b/docs/superpowers/specs/2026-07-25-WW-3871-typeconversion-key-derivation-design.md @@ -0,0 +1,188 @@ +# 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(TypeConversion tc, String name) { + if (name == null || name.isEmpty()) { + return null; // caller skips the entry and logs WARN + } + if (tc.type() == ConversionType.APPLICATION) { + return name; // key is a class name, never prefixed + } + String prefix = tc.rule().prefix(); + return name.startsWith(prefix) ? name : prefix + name; +} +``` + +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. + +**`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. + +### 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. 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. From f3952e487165f9ab5500ea0cb55dd0167e098743 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 25 Jul 2026 15:00:57 +0200 Subject: [PATCH 02/23] WW-3871 docs: note interaction with the 7.3.0 converter mapping cache Records that addConverterMapping runs inside the computeMappingIfAbsent builder introduced by WW-5539, which executes outside any lock, so the new field pass adds no deadlock risk but must stay side-effect free. Co-Authored-By: Claude Opus 5 --- ...-3871-typeconversion-key-derivation-design.md | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) 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 340968138c..5e957a7e6d 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 @@ -133,7 +133,21 @@ It skips `static` and synthetic fields, which also makes the interface case a no (`getDeclaredFields()` on an interface returns its constants). A field's own name is the property name; no getter/setter parsing is involved. -### 4. Error handling +### 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 | |---|---|---| From a06198e4e8c57d3267a03baa35cba6eb1eedb750 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 25 Jul 2026 15:06:32 +0200 Subject: [PATCH 03/23] WW-3871 docs: add implementation plan for @TypeConversion key derivation Seven TDD tasks covering ConversionRule#prefix(), the shared resolveKey helper, class- and field-level derivation, the break/continue and empty-key fixes, an end-to-end binding proof and the Javadoc updates. Refines the spec's resolveKey signature to take the two annotation attributes rather than the annotation instance, so it can be unit tested directly. Co-Authored-By: Claude Opus 5 --- ...5-WW-3871-typeconversion-key-derivation.md | 1026 +++++++++++++++++ ...71-typeconversion-key-derivation-design.md | 9 +- 2 files changed, 1032 insertions(+), 3 deletions(-) create mode 100644 docs/superpowers/plans/2026-07-25-WW-3871-typeconversion-key-derivation.md 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 index 5e957a7e6d..150d861a40 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 @@ -78,18 +78,21 @@ An exhaustive `switch` over the enum means a future rule cannot silently miss a ### 2. One key resolver in `XWorkConverter` ```java -static String resolveKey(TypeConversion tc, String name) { +static String resolveKey(ConversionType type, ConversionRule rule, String name) { if (name == null || name.isEmpty()) { return null; // caller skips the entry and logs WARN } - if (tc.type() == ConversionType.APPLICATION) { + if (type == ConversionType.APPLICATION) { return name; // key is a class name, never prefixed } - String prefix = tc.rule().prefix(); + String prefix = rule.prefix(); return name.startsWith(prefix) ? name : 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. From 734f85982359d5b4dff0a9d7414a0cc3f38ed130 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 25 Jul 2026 15:09:41 +0200 Subject: [PATCH 04/23] WW-3871 feat(core): add ConversionRule#prefix() owning the rule-to-prefix table --- .../annotations/ConversionRule.java | 21 +++++++++++ .../annotations/ConversionRuleTest.java | 37 +++++++++++++++++++ 2 files changed, 58 insertions(+) create mode 100644 core/src/test/java/org/apache/struts2/conversion/annotations/ConversionRuleTest.java 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..090a6709a4 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,25 @@ 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(); 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()); + } +} From 43d7e9b6b37a012846bcc52a3ce42a2651564cd5 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 25 Jul 2026 15:13:20 +0200 Subject: [PATCH 05/23] WW-3871 refactor(core): split addConverterMapping into per-source passes --- .../conversion/impl/XWorkConverter.java | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 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 5579a15d3e..8685e9d711 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 @@ -498,10 +498,16 @@ 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); + } - for (Annotation annotation : annotations) { + /** + * 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())) { @@ -518,11 +524,14 @@ protected void addConverterMapping(Map mapping, Class clazz) { } } } + } - // 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) { + for (Annotation annotation : method.getAnnotations()) { if (annotation instanceof TypeConversion tc) { String key = tc.key(); // Default to the property name with prefix From 5c2a964a5c5f7ade95a9609d155d62ccab7747bf Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 25 Jul 2026 15:20:12 +0200 Subject: [PATCH 06/23] WW-3871 feat(core): derive conversion mapping keys through a single resolver --- .../conversion/impl/XWorkConverter.java | 58 +++++++++++++------ .../conversion/impl/XWorkConverterTest.java | 46 +++++++++++++++ .../util/ExplicitKeyConversionAction.java | 43 ++++++++++++++ 3 files changed, 128 insertions(+), 19 deletions(-) create mode 100644 core/src/test/java/org/apache/struts2/util/ExplicitKeyConversionAction.java 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 8685e9d711..b3aa0e4731 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; @@ -486,6 +488,28 @@ 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 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; + } + /** * 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. @@ -532,26 +556,22 @@ private void processClassLevelAnnotations(Map mapping, Class cla 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); + 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); } } } 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..5b3fea454f 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 @@ -39,6 +39,9 @@ import ognl.OgnlRuntime; import org.apache.struts2.conversion.TypeConverter; 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.ExplicitKeyConversionAction; import java.io.IOException; import java.io.InputStream; @@ -814,6 +817,49 @@ 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")); + } + + 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 static class CountingXWorkConverter extends XWorkConverter { final AtomicInteger builds = new AtomicInteger(); 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; + } +} From b487d6640cef469a1cbb60385c34a5fdba9715a1 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 25 Jul 2026 15:26:50 +0200 Subject: [PATCH 07/23] WW-3871 fix(core): derive class level conversion keys and stop dropping later entries --- .../conversion/impl/XWorkConverter.java | 28 +++++---- .../conversion/impl/XWorkConverterTest.java | 36 +++++++++++ .../struts2/util/BareKeyConversionAction.java | 61 +++++++++++++++++++ .../util/CollidingKeyConversionAction.java | 48 +++++++++++++++ ...gKeyConversionAction-conversion.properties | 19 ++++++ 5 files changed, 179 insertions(+), 13 deletions(-) create mode 100644 core/src/test/java/org/apache/struts2/util/BareKeyConversionAction.java create mode 100644 core/src/test/java/org/apache/struts2/util/CollidingKeyConversionAction.java create mode 100644 core/src/test/resources/org/apache/struts2/util/CollidingKeyConversionAction-conversion.properties 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 b3aa0e4731..b0787735a2 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 @@ -532,20 +532,22 @@ protected void addConverterMapping(Map mapping, Class clazz) { */ 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()); + 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); } } } 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 5b3fea454f..b791fc941f 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 @@ -41,7 +41,11 @@ 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.BareKeyConversionAction; +import org.apache.struts2.util.CollidingKeyConversionAction; import org.apache.struts2.util.ExplicitKeyConversionAction; +import org.apache.struts2.util.MyBean; +import org.apache.struts2.util.MyBeanAction; import java.io.IOException; import java.io.InputStream; @@ -860,6 +864,38 @@ public void testExplicitMethodKeyGetsTheRulePrefix() { 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 static class CountingXWorkConverter extends XWorkConverter { final AtomicInteger builds = new AtomicInteger(); 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/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 From 217f74c75f378b8b0fb3d508d0715d8b1bfd5122 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 25 Jul 2026 15:34:28 +0200 Subject: [PATCH 08/23] WW-3871 feat(core): support @TypeConversion on fields --- .../annotations/TypeConversion.java | 2 +- .../conversion/impl/XWorkConverter.java | 38 ++++++++++++ .../conversion/impl/XWorkConverterTest.java | 15 +++++ .../struts2/util/FieldConversionAction.java | 58 +++++++++++++++++++ 4 files changed, 112 insertions(+), 1 deletion(-) create mode 100644 core/src/test/java/org/apache/struts2/util/FieldConversionAction.java 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..749d715c99 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 @@ -150,7 +150,7 @@ * @author Rainer Hermanns * @version $Id$ */ -@Target({ ElementType.METHOD}) +@Target({ElementType.METHOD, ElementType.FIELD}) @Retention(RetentionPolicy.RUNTIME) public @interface TypeConversion { 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 b0787735a2..0dcd4fcaf1 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 @@ -42,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; @@ -524,6 +526,7 @@ protected void addConverterMapping(Map mapping, Class clazz) { processClassLevelAnnotations(mapping, clazz); processMethodAnnotations(mapping, clazz); + processFieldAnnotations(mapping, clazz); } /** @@ -578,6 +581,41 @@ private void processMethodAnnotations(Map mapping, Class clazz) } } + /** + * 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); + } + } + } + /** * 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/impl/XWorkConverterTest.java b/core/src/test/java/org/apache/struts2/conversion/impl/XWorkConverterTest.java index b791fc941f..1ca4bc161b 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 @@ -44,6 +44,7 @@ import org.apache.struts2.util.BareKeyConversionAction; import org.apache.struts2.util.CollidingKeyConversionAction; import org.apache.struts2.util.ExplicitKeyConversionAction; +import org.apache.struts2.util.FieldConversionAction; import org.apache.struts2.util.MyBean; import org.apache.struts2.util.MyBeanAction; @@ -896,6 +897,20 @@ public void testClassLevelEntriesAfterAKeyCollisionAreStillRegistered() { assertEquals("true", freshConverter.getConverter(CollidingKeyConversionAction.class, "CreateIfNull_afterTheCollision")); } + 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")); + } + public static class CountingXWorkConverter extends XWorkConverter { final AtomicInteger builds = new AtomicInteger(); 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; + } +} From af603fa692841ae593bb846dbb24217f25aeba6b Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 25 Jul 2026 15:38:59 +0200 Subject: [PATCH 09/23] WW-3871 test(core): assert bare conversion keys bind through the action lifecycle Co-Authored-By: Claude Opus 5 --- .../apache/struts2/util/MyBeanActionTest.java | 25 +++++++ .../struts2/util/MyBeanBareKeyAction.java | 68 +++++++++++++++++++ core/src/test/resources/xwork-sample.xml | 6 ++ 3 files changed, 99 insertions(+) create mode 100644 core/src/test/java/org/apache/struts2/util/MyBeanBareKeyAction.java 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..3f19efbfe7 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,31 @@ 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")); + + // 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/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 From 0c86a3a1e0b28e413a389853cd1c053baf8c19de Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 25 Jul 2026 15:45:29 +0200 Subject: [PATCH 10/23] WW-3871 docs(core): document conversion key derivation and field level support --- .../annotations/TypeConversion.java | 21 ++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) 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 749d715c99..c65b5d08ec 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,7 +48,7 @@ *

Annotation usage:

* * - *

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

+ *

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

* * *

Annotation parameters:

@@ -67,8 +67,11 @@ * * key * no - * The annotated property/key name - * The optional property name mostly used within TYPE level annotations. + * 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. * * * type @@ -129,6 +132,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; @@ -155,10 +161,15 @@ 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 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 */ String key() default ""; From f5ce53646d79b41164a07de9cfe77f8523467cfa Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 25 Jul 2026 16:54:51 +0200 Subject: [PATCH 11/23] WW-3871 docs(core): add deprecated Collection_ prefix to parameter table --- .../apache/struts2/conversion/annotations/TypeConversion.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 c65b5d08ec..e4f86b965a 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 @@ -69,8 +69,8 @@ * 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, + * (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. * * From 955e3d9d10653ec3f8062160c6a7b9142d764809 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 25 Jul 2026 17:11:25 +0200 Subject: [PATCH 12/23] 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. --- .../conversion/impl/XWorkConverter.java | 30 ++++++++++++++++--- ...71-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 0dcd4fcaf1..d85689d3fb 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 @@ private Object[] getClassProperty(Map context) { /** * 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 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} @@ -509,7 +513,16 @@ static String resolveKey(ConversionType type, ConversionRule rule, String name) 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 @@ private void processMethodAnnotations(Map mapping, Class clazz) 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 @@ private void processMethodAnnotations(Map mapping, Class clazz) * 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()) { 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 150d861a40..30b301c240 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_`, 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 From 919bbd1380476cddf6f5c9ac33376b266dd2994d Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 25 Jul 2026 17:11:30 +0200 Subject: [PATCH 13/23] WW-3871 docs(core): correct TypeConversion Javadoc property attribute and determiner package Two pre-existing errors in the block this ticket's commits already touch: the APPLICATION example used a non-existent "property" attribute where "key" is the working form (see ConversionTestAction.java:97), and the rule() Javadoc pointed at org.apache.struts2.util.DefaultObjectTypeDeterminer instead of the actual org.apache.struts2.conversion.impl package. --- .../apache/struts2/conversion/annotations/TypeConversion.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 e4f86b965a..efd6b77c6b 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 @@ -145,7 +145,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; * } @@ -185,7 +185,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 * From a1c2193af4f6f636289ce8e62bcd36257e6cea06 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 25 Jul 2026 17:11:36 +0200 Subject: [PATCH 14/23] WW-3871 test(core): cover key-prefix crossover, empty class-level key, and KeyProperty_ end-to-end binding - testResolveKeyLeavesAnAlreadyPrefixedKeyAlone: add the COLLECTION/ELEMENT crossover cases that demonstrate the resolveKey guard fix (fail before, pass after). - New EmptyKeyConversionAction fixture plus testClassLevelEmptyKeyRegistersNoMapping: a class-level @TypeConversion with no key must be skipped, not registered under "". This was the one behavioural bullet in the spec's test plan with no coverage. - MyBeanActionTest.testBareConversionKeysBindTheSameWayAsPrefixedOnes: add an assertion that the bare KeyProperty_ derivation actually binds the list index onto the created bean's id property end to end, not just that a converter mapping exists. --- .../conversion/impl/XWorkConverterTest.java | 19 +++++++++++ .../util/EmptyKeyConversionAction.java | 34 +++++++++++++++++++ .../apache/struts2/util/MyBeanActionTest.java | 3 ++ 3 files changed, 56 insertions(+) create mode 100644 core/src/test/java/org/apache/struts2/util/EmptyKeyConversionAction.java 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 1ca4bc161b..db16433b1a 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 @@ -43,6 +43,7 @@ import org.apache.struts2.conversion.annotations.ConversionType; 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.MyBean; @@ -836,6 +837,14 @@ public void testResolveKeyLeavesAnAlreadyPrefixedKeyAlone() { 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() { @@ -897,6 +906,16 @@ public void testClassLevelEntriesAfterAKeyCollisionAreStillRegistered() { 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()); 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/MyBeanActionTest.java b/core/src/test/java/org/apache/struts2/util/MyBeanActionTest.java index 3f19efbfe7..bd9d1da440 100644 --- a/core/src/test/java/org/apache/struts2/util/MyBeanActionTest.java +++ b/core/src/test/java/org/apache/struts2/util/MyBeanActionTest.java @@ -143,6 +143,9 @@ public void testBareConversionKeysBindTheSameWayAsPrefixedOnes() throws Exceptio 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)); From c3f9b077180ced29d463801679d2a4cbb25b8c2c Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 25 Jul 2026 21:04:04 +0200 Subject: [PATCH 15/23] WW-3871 fix(core): skip APPLICATION-scoped @TypeConversion with no explicit key Method- and field-level @TypeConversion(type = APPLICATION) with no key previously derived a member name (e.g. a setter's property name) and registered it in the global default converter map via addDefaultMapping. That map is only ever read by class name (lookup(String, boolean) and lookup(Class)), so the entry was permanently unreachable. Skip it before deriving a name, logging a WARN naming the declaring class and member; the class-level pass already handled this correctly via resolveKey returning null. Adds a fixture and tests proving no default mapping is registered under the derived member name in either pass. --- .../conversion/impl/XWorkConverter.java | 23 ++++++++ .../conversion/impl/XWorkConverterTest.java | 45 ++++++++++++++++ ...ationScopedWithoutKeyConversionAction.java | 54 +++++++++++++++++++ 3 files changed, 122 insertions(+) create mode 100644 core/src/test/java/org/apache/struts2/util/ApplicationScopedWithoutKeyConversionAction.java 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 d85689d3fb..1f76df7f5d 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 @@ -525,6 +525,17 @@ static String resolveKey(ConversionType type, ConversionRule rule, String name) 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. @@ -577,6 +588,13 @@ private void processMethodAnnotations(Map mapping, Class clazz) if (!(annotation instanceof TypeConversion tc)) { continue; } + if (isApplicationScopedWithoutKey(tc)) { + // method.getDeclaringClass(), not clazz: getMethods() returns inherited methods + // too (see the resolveKey-null branch below for the same reasoning). + LOG.warn("Ignoring @TypeConversion on [{}#{}]: an application-scoped conversion needs an explicit class-name key, not a derived property name", + method.getDeclaringClass().getName(), method.getName()); + continue; + } String name = StringUtils.isEmpty(tc.key()) ? AnnotationUtils.resolvePropertyName(method) : tc.key(); String key = resolveKey(tc.type(), tc.rule(), name); if (key == null) { @@ -618,6 +636,11 @@ private void processFieldAnnotations(Map mapping, Class clazz) { if (!(annotation instanceof TypeConversion tc)) { continue; } + if (isApplicationScopedWithoutKey(tc)) { + LOG.warn("Ignoring @TypeConversion on field [{}#{}]: an application-scoped conversion needs an explicit class-name key, not a derived property name", + clazz.getName(), field.getName()); + continue; + } String name = StringUtils.isEmpty(tc.key()) ? field.getName() : tc.key(); String key = resolveKey(tc.type(), tc.rule(), name); if (key == null) { 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 db16433b1a..e444813c5e 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,10 +37,13 @@ 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; @@ -930,6 +933,48 @@ public void testMethodAnnotationWinsOverFieldAnnotation() { assertEquals(Long.class, freshConverter.getConverter(FieldConversionAction.class, "Key_contestedMap")); } + /** + * 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; + } +} From 48c6f82570aab2087dd061f46b066d3a7681e2f6 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 25 Jul 2026 21:04:15 +0200 Subject: [PATCH 16/23] WW-3871 docs(core): fix broken TypeConversion Javadoc example and align spec TypeConversion's example class declared `users` twice (once unannotated, once again at its annotated field), so the sample no longer compiled as written; drop the earlier, redundant declaration. The same example's setConvertInt showed @TypeConversion(type = APPLICATION) with no key - exactly the case the previous commit's XWorkConverter fix now skips. Drop the type attribute so it reads as a class-scoped conversion, matching the corrected ConversionTestAction fixture. The correct APPLICATION example further down (execute(), key = "java.util.Date") is untouched. Also records the APPLICATION no-key skip rule in the design spec's carve-out paragraph so spec and code agree. --- .../struts2/conversion/annotations/TypeConversion.java | 3 +-- .../struts2/conversion/ConversionTestAction.java | 2 +- ...-25-WW-3871-typeconversion-key-derivation-design.md | 10 ++++++++++ 3 files changed, 12 insertions(+), 3 deletions(-) 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 efd6b77c6b..5db1ab6a15 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 @@ -118,11 +118,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; * } 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/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 30b301c240..c3cc34b247 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 @@ -110,6 +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. +`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`. From dc254387755b03c812b9f112431918e21c0a33ba Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 25 Jul 2026 21:30:07 +0200 Subject: [PATCH 17/23] WW-3871 fix(core): dedupe method-pass WARN logging for inherited @TypeConversion processMethodAnnotations iterates clazz.getMethods(), which includes inherited public methods, and buildConverterMapping calls it once per class in the hierarchy. A single misconfigured @TypeConversion on a base class method was therefore logging its WARN once per subclass level. Gate both WARN call sites on method.getDeclaringClass() == clazz so each fires exactly once, at the level that owns the method; the derivation/registration logic keeps running on every visit unchanged. Adds a small permanent test proving the gate is logging-only: an inherited annotated setter still resolves and registers through a subclass that overrides nothing. --- .../conversion/impl/XWorkConverter.java | 26 ++++++---- .../conversion/impl/XWorkConverterTest.java | 15 ++++++ .../util/InheritedMethodConversionAction.java | 47 +++++++++++++++++++ .../InheritedMethodConversionSubAction.java | 27 +++++++++++ 4 files changed, 106 insertions(+), 9 deletions(-) create mode 100644 core/src/test/java/org/apache/struts2/util/InheritedMethodConversionAction.java create mode 100644 core/src/test/java/org/apache/struts2/util/InheritedMethodConversionSubAction.java 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 1f76df7f5d..16554b9f08 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 @@ -584,25 +584,33 @@ private void processClassLevelAnnotations(Map mapping, Class cla */ private void processMethodAnnotations(Map mapping, Class clazz) { for (Method method : clazz.getMethods()) { + // 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 a WARN 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 logic below still runs + // on every visit, which first-writer-wins relies on. + boolean logIfSkipped = method.getDeclaringClass() == clazz; for (Annotation annotation : method.getAnnotations()) { if (!(annotation instanceof TypeConversion tc)) { continue; } if (isApplicationScopedWithoutKey(tc)) { - // method.getDeclaringClass(), not clazz: getMethods() returns inherited methods - // too (see the resolveKey-null branch below for the same reasoning). - LOG.warn("Ignoring @TypeConversion on [{}#{}]: an application-scoped conversion needs an explicit class-name key, not a derived property name", - method.getDeclaringClass().getName(), method.getName()); + if (logIfSkipped) { + LOG.warn("Ignoring @TypeConversion on [{}#{}]: an application-scoped conversion needs an explicit class-name key, not a derived property name", + method.getDeclaringClass().getName(), method.getName()); + } continue; } 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", - method.getDeclaringClass().getName(), method.getName()); + if (logIfSkipped) { + LOG.warn("Ignoring @TypeConversion on [{}#{}]: no key was given and no property name could be derived from the method", + method.getDeclaringClass().getName(), method.getName()); + } continue; } if (mapping.containsKey(key)) { 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 e444813c5e..08d9d6caf7 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 @@ -49,6 +49,7 @@ 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; @@ -933,6 +934,20 @@ public void testMethodAnnotationWinsOverFieldAnnotation() { 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. This asserts that gate is + * logging-only: an inherited, correctly-configured method annotation must still resolve and + * register on every visit, proven here through a subclass that overrides nothing. + */ + 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 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..882509acb0 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/util/InheritedMethodConversionAction.java @@ -0,0 +1,47 @@ +/* + * 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 - used to assert the derivation/registration still happens on every visit + * even though the corresponding WARN logging is gated to fire once. + */ +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..58bd2b258f --- /dev/null +++ b/core/src/test/java/org/apache/struts2/util/InheritedMethodConversionSubAction.java @@ -0,0 +1,27 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.util; + +/** + * Extends {@link InheritedMethodConversionAction} without overriding anything, 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. + */ +public class InheritedMethodConversionSubAction extends InheritedMethodConversionAction { +} From 5dc5823b65617ce1d8aa0b09b0ad29ecdff4ca87 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 25 Jul 2026 21:30:12 +0200 Subject: [PATCH 18/23] WW-3871 docs(core): clarify field-name key default and dedicated-annotation precedence Two gaps in the @TypeConversion Javadoc, both newly relevant now that the annotation targets fields: - The key() default on a field is the field name, not the JavaBean property name (processFieldAnnotations uses field.getName()). A field like _users backing property users would otherwise derive CreateIfNull__users, a key DefaultObjectTypeDeterminer never looks up. - org.apache.struts2.util's dedicated field annotations (@Key, @Element, @KeyProperty, @CreateIfNull) are consulted by DefaultObjectTypeDeterminer before it falls back to the converter mapping @TypeConversion populates, so a dedicated annotation silently wins over an equivalent @TypeConversion on the same property. Verified against getAnnotation/getElementClass/ getKeyProperty in DefaultObjectTypeDeterminer before documenting it. --- .../conversion/annotations/TypeConversion.java | 13 +++++++++++++ 1 file changed, 13 insertions(+) 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 5db1ab6a15..e50105a9ce 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 @@ -51,6 +51,13 @@ *

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

* * + *

The {@code org.apache.struts2.util} package also has dedicated field annotations - {@code @Key}, + * {@code @Element}, {@code @KeyProperty} and {@code @CreateIfNull} - 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:

* * @@ -167,6 +174,12 @@ * {@code @TypeConversion(key = "users", rule = ConversionRule.CREATE_IF_NULL, value = "true")} * and {@code @TypeConversion(key = "CreateIfNull_users", ...)} are equivalent.

* + *

On a field, the default is the field name, not the JavaBean property name. 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 */ From 11ff53c0f53396b3acbbd201dba7c0dbece99dff Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 25 Jul 2026 21:30:16 +0200 Subject: [PATCH 19/23] WW-3871 docs(core): note COLLECTION derives the deprecated Collection_ prefix ConversionRule.COLLECTION.prefix() intentionally returns Collection_, the spelling DefaultObjectTypeDeterminer treats as deprecated and logs an INFO about on every fallback hit, kept for compatibility with existing annotations. Document that the derivation is deliberate and point readers at ELEMENT as the current form. --- .../struts2/conversion/annotations/ConversionRule.java | 5 +++++ 1 file changed, 5 insertions(+) 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 090a6709a4..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 @@ -35,6 +35,11 @@ public enum ConversionRule { * {@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 */ From 6e2ea3aea05e01039a587d353c0c0f8594e13ec6 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 25 Jul 2026 21:55:53 +0200 Subject: [PATCH 20/23] WW-3871 refactor(core): extract shared annotation-registration pipeline processMethodAnnotations and processFieldAnnotations were the same five-step pipeline (skip non-@TypeConversion, skip APPLICATION-scoped without a key, derive the name, resolve the key, register unless already mapped) written twice, driving SonarCloud S3776 cognitive complexity to 26 and 21 respectively and triggering three S135 multiple-break/continue findings. Extract steps 2-5 into a private registerAnnotatedMember(mapping, tc, Member, fallbackName, logSkips) helper that both passes delegate to. Each pass is now just its loop plus one instanceof check. The method pass keeps its per-declaring-class log gate (getMethods() revisits inherited methods once per hierarchy level); the field pass always logs, since getDeclaredFields() is visited once per class. The two WARN wordings, which differed only in a trailing clause, are merged into one message accurate for both a method and a field. No change to the registered mapping, pass order, or precedence for any class - verified via the existing XWorkConverterTest, AnnotationXWorkConverterTest, MyBeanActionTest, and ConversionRuleTest suites (92 tests, same count and same triggering warnings before and after) plus the full core module suite (3043 tests). Co-Authored-By: Claude Opus 5 --- .../conversion/impl/XWorkConverter.java | 105 +++++++++--------- 1 file changed, 54 insertions(+), 51 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 16554b9f08..f4afbc77b2 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 @@ -588,37 +588,15 @@ private void processMethodAnnotations(Map mapping, Class clazz) // 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 a WARN on the pass whose clazz is that declaring + // 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 logic below still runs - // on every visit, which first-writer-wins relies on. - boolean logIfSkipped = method.getDeclaringClass() == clazz; + // subclass. This gates logging only - the derivation/registration pipeline in + // registerAnnotatedMember still runs on every visit, which first-writer-wins relies on. + boolean logSkips = method.getDeclaringClass() == clazz; for (Annotation annotation : method.getAnnotations()) { - if (!(annotation instanceof TypeConversion tc)) { - continue; + if (annotation instanceof TypeConversion tc) { + registerAnnotatedMember(mapping, tc, method, AnnotationUtils.resolvePropertyName(method), logSkips); } - if (isApplicationScopedWithoutKey(tc)) { - if (logIfSkipped) { - LOG.warn("Ignoring @TypeConversion on [{}#{}]: an application-scoped conversion needs an explicit class-name key, not a derived property name", - method.getDeclaringClass().getName(), method.getName()); - } - continue; - } - String name = StringUtils.isEmpty(tc.key()) ? AnnotationUtils.resolvePropertyName(method) : tc.key(); - String key = resolveKey(tc.type(), tc.rule(), name); - if (key == null) { - if (logIfSkipped) { - LOG.warn("Ignoring @TypeConversion on [{}#{}]: no key was given and no property name could be derived from the method", - method.getDeclaringClass().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); } } } @@ -641,34 +619,59 @@ private void processFieldAnnotations(Map mapping, Class clazz) { continue; } for (Annotation annotation : field.getAnnotations()) { - if (!(annotation instanceof TypeConversion tc)) { - continue; + 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); } - if (isApplicationScopedWithoutKey(tc)) { - LOG.warn("Ignoring @TypeConversion on field [{}#{}]: an application-scoped conversion needs an explicit class-name key, not a derived property name", - clazz.getName(), field.getName()); - 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); } } } + /** + * 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 by a higher precedence source", + member.getDeclaringClass().getName(), member.getName(), key); + } + return; + } + LOG.debug("TypeConversion [{}/{}] on [{}] resolved to key [{}]", + tc.converter(), tc.converterClass(), 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 From 15665c1c762a6196122baff33175f37bd84244c0 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 25 Jul 2026 22:12:23 +0200 Subject: [PATCH 21/23] WW-3871 docs(core): fix inaccurate and self-contradicting TypeConversion key Javadoc @Key, @Element, @KeyProperty and @CreateIfNull are @Target({FIELD, METHOD}), not field-only, and the same paragraph already says they are read from the field, setter and getter - drop "field" from "dedicated field annotations". Fold the field-vs-property-name correction into key()'s opening sentence instead of stating "defaults to the property name" and rebutting it three lines later, and align the parameters table row for key with the same rule. --- .../annotations/TypeConversion.java | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) 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 e50105a9ce..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 @@ -51,8 +51,8 @@ *

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

* * - *

The {@code org.apache.struts2.util} package also has dedicated field annotations - {@code @Key}, - * {@code @Element}, {@code @KeyProperty} and {@code @CreateIfNull} - that {@link + *

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 @@ -74,7 +74,7 @@ * * key * no - * The annotated property/field name + * 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, @@ -167,18 +167,18 @@ public @interface TypeConversion { /** - * 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 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.

* - *

On a field, the default is the field name, not the JavaBean property name. 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.

+ *

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 From e8b81031eba48e68b8acbb5ae4597c2030bb9ec2 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 25 Jul 2026 22:12:30 +0200 Subject: [PATCH 22/23] WW-3871 docs(core): clarify XWorkConverter annotation-registration logging Give the success DEBUG the same [declaringClass#member] shape the three skip messages already use, instead of logging the bare member name that identifies neither the class nor whether it was a method or a field. Reword the "already mapped" DEBUG so it covers its commonest trigger - the same annotation seen one hierarchy level down, not just a genuinely higher-precedence source. Note in the logSkips comment that buildConverterMapping only visits each class' direct interfaces, so a misconfigured annotation declared on a super-interface method never gets logged at all, even though registration is unaffected. Also drop a stray extra blank line. No behavioural change: registration/derivation logic is untouched. --- .../struts2/conversion/impl/XWorkConverter.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 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 f4afbc77b2..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 @@ -592,6 +592,10 @@ private void processMethodAnnotations(Map mapping, Class clazz) // 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) { @@ -662,17 +666,17 @@ private void registerAnnotatedMember(Map mapping, TypeConversion } if (mapping.containsKey(key)) { if (logSkips) { - LOG.debug("Skipping @TypeConversion on [{}#{}]: key [{}] is already mapped by a higher precedence source", + 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.getName(), key); + 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 * any additional mappings it may find. Mappings lower in the hierarchy have priority over those higher in the From 14040dfd0428d91e14034c7ca4838d4fce6abe02 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 25 Jul 2026 22:12:39 +0200 Subject: [PATCH 23/23] WW-3871 test(core): make inherited-method-annotation test diagnostic testInheritedMethodAnnotationStillRegistersThroughASubclass previously asserted nothing the logSkips gate could break: the hierarchy walk always reaches InheritedMethodConversionAction itself, where declaringClass == clazz, so the key registers there regardless of whether registration is (wrongly) gated alongside logging. The test passed identically with logSkips hardcoded true or false. Give InheritedMethodConversionSubAction a contesting field annotation for the same property the inherited setter claims. The inherited method annotation registers at the subclass level - before the subclass's own field pass runs - so its value must keep winning; that is the invariant documented on processFieldAnnotations, and it is exactly what gating registration would break, since the subclass field would start winning over the inherited method annotation instead. Verified: temporarily wrapping the registerAnnotatedMember call in processMethodAnnotations with `if (logSkips)` makes this test fail (expected: but was:); reverting it passes again. Mutation was not committed. Corrected both Javadocs, which overclaimed what the old assertion proved. --- .../conversion/impl/XWorkConverterTest.java | 9 ++++++--- .../util/InheritedMethodConversionAction.java | 5 +++-- .../util/InheritedMethodConversionSubAction.java | 16 +++++++++++++++- 3 files changed, 24 insertions(+), 6 deletions(-) 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 08d9d6caf7..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 @@ -937,9 +937,12 @@ public void testMethodAnnotationWinsOverFieldAnnotation() { /** * {@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. This asserts that gate is - * logging-only: an inherited, correctly-configured method annotation must still resolve and - * register on every visit, proven here through a subclass that overrides nothing. + * 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); diff --git a/core/src/test/java/org/apache/struts2/util/InheritedMethodConversionAction.java b/core/src/test/java/org/apache/struts2/util/InheritedMethodConversionAction.java index 882509acb0..fc668d0f88 100644 --- a/core/src/test/java/org/apache/struts2/util/InheritedMethodConversionAction.java +++ b/core/src/test/java/org/apache/struts2/util/InheritedMethodConversionAction.java @@ -29,8 +29,9 @@ * 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 - used to assert the derivation/registration still happens on every visit - * even though the corresponding WARN logging is gated to fire once. + * 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 { diff --git a/core/src/test/java/org/apache/struts2/util/InheritedMethodConversionSubAction.java b/core/src/test/java/org/apache/struts2/util/InheritedMethodConversionSubAction.java index 58bd2b258f..feb8fdcce5 100644 --- a/core/src/test/java/org/apache/struts2/util/InheritedMethodConversionSubAction.java +++ b/core/src/test/java/org/apache/struts2/util/InheritedMethodConversionSubAction.java @@ -18,10 +18,24 @@ */ 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 anything, so {@code + * 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; }