WW-3871 Derive ConversionRule prefixes for @TypeConversion keys#1812
Draft
lukaszlenart wants to merge 23 commits into
Draft
WW-3871 Derive ConversionRule prefixes for @TypeConversion keys#1812lukaszlenart wants to merge 23 commits into
lukaszlenart wants to merge 23 commits into
Conversation
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…ng later entries
…on lifecycle Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e 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.
… 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.
…, 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.
Contributor
There was a problem hiding this comment.
Pull request overview
Updates Struts’ annotation-based type conversion so @TypeConversion keys can be written as bare property names across class-, method-, and field-level declarations, with rule-specific prefixes derived consistently and without breaking already-prefixed keys.
Changes:
- Centralizes rule→prefix logic in
ConversionRule.prefix()and introducesXWorkConverter.resolveKey(...)to normalize/derive mapping keys across annotation passes. - Refactors converter-mapping registration into ordered passes and adds field-level
@TypeConversionsupport. - Adds targeted unit + end-to-end tests covering prefix derivation, collision handling, precedence, and action lifecycle binding.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/superpowers/specs/2026-07-25-WW-3871-typeconversion-key-derivation-design.md | Design spec describing key-derivation rules, precedence, and edge cases. |
| docs/superpowers/plans/2026-07-25-WW-3871-typeconversion-key-derivation.md | Implementation plan and verification checklist for the change set. |
| core/src/main/java/org/apache/struts2/conversion/annotations/ConversionRule.java | Adds prefix() to own the rule-to-prefix mapping. |
| core/src/main/java/org/apache/struts2/conversion/annotations/TypeConversion.java | Expands target to FIELD, updates Javadoc to describe derived prefixes and usage. |
| core/src/main/java/org/apache/struts2/conversion/impl/XWorkConverter.java | Adds resolveKey, routes class/method/field annotation processing through it, and adds field pass. |
| core/src/test/java/org/apache/struts2/conversion/annotations/ConversionRuleTest.java | Unit tests for ConversionRule.prefix(). |
| core/src/test/java/org/apache/struts2/conversion/impl/XWorkConverterTest.java | Unit tests for resolveKey and annotation mapping behavior (class/method/field, collisions, empty keys). |
| core/src/test/java/org/apache/struts2/util/BareKeyConversionAction.java | Test fixture for class-level bare keys. |
| core/src/test/java/org/apache/struts2/util/CollidingKeyConversionAction.java | Test fixture for collision behavior across sources. |
| core/src/test/java/org/apache/struts2/util/EmptyKeyConversionAction.java | Test fixture for empty class-level key handling. |
| core/src/test/java/org/apache/struts2/util/ExplicitKeyConversionAction.java | Test fixture for explicit, unprefixed method keys being normalized. |
| core/src/test/java/org/apache/struts2/util/FieldConversionAction.java | Test fixture for field-level @TypeConversion + precedence. |
| core/src/test/java/org/apache/struts2/util/MyBeanBareKeyAction.java | End-to-end fixture proving bare keys bind through the action lifecycle. |
| core/src/test/java/org/apache/struts2/util/MyBeanActionTest.java | Adds end-to-end test asserting bare-key and prefixed-key behavior match. |
| core/src/test/resources/org/apache/struts2/util/CollidingKeyConversionAction-conversion.properties | Properties fixture to prove collision handling doesn’t drop subsequent entries. |
| core/src/test/resources/xwork-sample.xml | Registers the new MyBeanBareKey action for the end-to-end test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…plicit 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.
…gn 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.
…eConversion 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.
…tation 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.
…_ 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.
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 <noreply@anthropic.com>
…ion 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.
…gging 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.
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:<true> but was:<false>); reverting it passes again. Mutation was not committed. Corrected both Javadocs, which overclaimed what the old assertion proved.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes WW-3871
What
@TypeConversionnow derives its conversion-mapping key prefix from the declaredConversionRule, so a bare property name works askeyat class, method and field level:Half of the reporter's request was already implemented in 2018 (77cbafb), which taught the method-level path to derive the key from the property name. This closes the remaining gaps.
Changes
ConversionRule.prefix()owns the rule-to-prefix table, previously duplicated inline. Theswitchis exhaustive with nodefault, so a future rule cannot silently ship without a prefix.XWorkConverter.resolveKey(type, rule, name)is the single resolver all annotation passes route through — explicit keys and derived property names alike. A key that already carries a rule prefix is returned untouched, so existing annotations keep working byte-for-byte.ConversionType.APPLICATIONkeys are class names and are never prefixed.addConverterMappingsplit into four ordered passes — properties file, class-level, method, field — keeping the existing first-writer-wins rule. The previous single method was ~45 lines of nested loops.@TypeConversionis now@Target({METHOD, FIELD}), which its Javadoc has always claimed. The field pass reads declared, non-static, non-synthetic fields only;buildConverterMappingalready walks the hierarchy.Three defects in the same code path are fixed as a byproduct:
breakwherecontinuewas meant: one already-mapped key aborted the remaining@TypeConversionentries in a@Conversionarray.keyregistered a mapping under"". It is now skipped with a WARN naming the class.APPLICATION-scoped annotation with no explicit key derived a member name and registered it in the global default-converter map, whichlookup(String, boolean)only ever reads by class name. That entry was unreachable; it is now skipped with a WARN.Upgrade impact
Worth a release note. Applications carrying a method-level annotation with a bare key and a non-
PROPERTYrule — for example:registered
somePropbefore this change, a key nothing reads. It now registersCreateIfNull_someProp, whichDefaultObjectTypeDeterminerdoes read. Such annotations therefore go from dormant to active on upgrade, and it bites in both directions:value = "true"enables collection auto-creation that was not happening, andvalue = "false"actively suppresses creation that previously occurred by default throughisIndexAccessed.The same applies to
KEY,ELEMENTandKEY_PROPERTY. Auto-growth stays bounded either way —XWorkListPropertyAccessorenforcesstruts.ognl.autoGrowthCollectionLimit.Turning these into no-ops was never intended; the previous behaviour was a silent misfire. Related: because
convertValuecasts the looked-up mapping with an unchecked(TypeConverter), a property literally namedsomePropon that class would previously have thrownClassCastException— the prefixing removes that latent crash.Compatibility
Source- and binary-compatible.
MyBeanActiondeliberately keeps its spelled-out prefixes and its tests are untouched — that is the backward-compatibility evidence, alongside a new bare-key twin asserted to produce an identical mapping.Notes for review
COLLECTIONandELEMENTare interchangeable —DefaultConversionAnnotationProcessorhandles both in one branch andDefaultObjectTypeDeterminer.getElementClassreadsElement_then falls back to the deprecatedCollection_— sokey = "Element_users", rule = COLLECTIONmust not becomeCollection_Element_users.getMethods()(inherited) while the field pass readsgetDeclaredFields(), a superclass's annotated setter beats a subclass's field annotation; this is documented onprocessFieldAnnotations.key().@Key,@Element,@KeyPropertyand@CreateIfNullstill win over an equivalent@TypeConversionon the same property —DefaultObjectTypeDeterminer.getAnnotationconsults them before the converter mapping. Newly reachable now that@TypeConversiontargets fields, so it is documented on the annotation.DefaultConversionFileProcessor:69has the identicalbreak-instead-of-continuedefect for-conversion.propertiesfiles. Happy to file a follow-up.Testing
mvn test -DskipAssembly -pl core— 3043/3043 passing.mvn javadoc:javadoc -pl coreclean.New coverage:
ConversionRuleTest;resolveKeyunit tests including prefix crossover and theAPPLICATIONcarve-out; class-level bare keys asserted equal to the spelled-out form; a-conversion.propertieskey collision proving later entries still register; field derivation and method-over-field precedence; inherited method annotations still registering through a subclass; and an end-to-end binding test through the action lifecycle.Design notes and the implementation plan are included under
docs/superpowers/.