[
https://issues.apache.org/jira/browse/WW-3871?focusedWorklogId=1032238&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1032238
]
ASF GitHub Bot logged work on WW-3871:
--------------------------------------
Author: ASF GitHub Bot
Created on: 25/Jul/26 19:12
Start Date: 25/Jul/26 19:12
Worklog Time Spent: 10m
Work Description: lukaszlenart commented on code in PR #1812:
URL: https://github.com/apache/struts/pull/1812#discussion_r3650845985
##########
core/src/main/java/org/apache/struts2/conversion/impl/XWorkConverter.java:
##########
@@ -498,51 +537,103 @@ protected void addConverterMapping(Map<String, Object>
mapping, Class clazz) {
String converterFilename = buildConverterFilename(clazz);
fileProcessor.process(mapping, clazz, converterFilename);
- // Process annotations
- Annotation[] annotations = clazz.getAnnotations();
+ processClassLevelAnnotations(mapping, clazz);
+ processMethodAnnotations(mapping, clazz);
+ processFieldAnnotations(mapping, clazz);
+ }
- for (Annotation annotation : annotations) {
- if (annotation instanceof Conversion conversion) {
- for (TypeConversion tc : conversion.conversions()) {
- if (mapping.containsKey(tc.key())) {
- break;
- }
- if (LOG.isDebugEnabled()) {
- if (StringUtils.isEmpty(tc.key())) {
- LOG.debug("WARNING! key of @TypeConversion [{}/{}]
applied to [{}] is empty!", tc.converter(), tc.converterClass(),
clazz.getName());
- } else {
- LOG.debug("TypeConversion [{}/{}] with key: [{}]",
tc.converter(), tc.converterClass(), tc.key());
- }
- }
- annotationProcessor.process(mapping, tc, tc.key());
+ /**
+ * Registers the {@link TypeConversion} entries declared by a class level
{@link Conversion}
+ * annotation.
+ */
+ private void processClassLevelAnnotations(Map<String, Object> mapping,
Class clazz) {
+ for (Annotation annotation : clazz.getAnnotations()) {
+ if (!(annotation instanceof Conversion conversion)) {
+ continue;
+ }
+ for (TypeConversion tc : conversion.conversions()) {
+ String key = resolveKey(tc.type(), tc.rule(), tc.key());
+ if (key == null) {
+ LOG.warn("Ignoring @TypeConversion [{}/{}] declared on
[{}]: no key was given and a class level annotation has no property name to
derive one from",
+ tc.converter(), tc.converterClass(),
clazz.getName());
+ continue;
+ }
+ if (mapping.containsKey(key)) {
+ continue;
}
+ LOG.debug("TypeConversion [{}/{}] declared on [{}] resolved to
key [{}]",
+ tc.converter(), tc.converterClass(), clazz.getName(),
key);
+ annotationProcessor.process(mapping, tc, key);
}
}
+ }
- // Process annotated methods
+ /**
+ * Registers {@link TypeConversion} annotations found on the class'
methods.
+ */
+ private void processMethodAnnotations(Map<String, Object> mapping, Class
clazz) {
for (Method method : clazz.getMethods()) {
- annotations = method.getAnnotations();
- for (Annotation annotation : annotations) {
- 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);
+ 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) {
+ // 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());
+ 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);
+ }
+ }
+ }
+
+ /**
+ * 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.
+ *
+ * <p>The stated precedence "class > method > field" is per-class,
not per-hierarchy-level:
+ * {@link #processMethodAnnotations(Map, Class)} sees {@link
Class#getMethods()}, which includes
+ * inherited public methods, so a superclass's annotated setter claims its
key before this pass
+ * ever looks at a subclass's field for that same class. A subclass field
annotation only wins
+ * when no method anywhere in the hierarchy already claimed its key.</p>
+ */
+ private void processFieldAnnotations(Map<String, Object> mapping, Class
clazz) {
+ for (Field field : clazz.getDeclaredFields()) {
+ 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) {
Review Comment:
Correct, and fixed in c3f9b0771 — the field pass now applies the same guard,
skipping an APPLICATION-scoped `@TypeConversion` with no explicit key and
logging a WARN naming the class and field.
This one is genuinely new surface, since the field pass did not exist before
this branch, so thanks for catching it rather than only its method-level twin.
Both passes share a small `isApplicationScopedWithoutKey` helper so they cannot
drift apart.
New coverage in `XWorkConverterTest` asserts no default mapping is
registered under the derived name, for both the method and field forms.
##########
core/src/main/java/org/apache/struts2/conversion/annotations/TypeConversion.java:
##########
@@ -129,6 +132,9 @@
* this.convertDouble = convertDouble;
* }
*
+ * @TypeConversion(rule = ConversionRule.CREATE_IF_NULL, value = "true")
+ * private List users = null;
+ *
Review Comment:
Correct — this was a real defect introduced by this branch, not a
pre-existing one. Fixed in 48c6f8257 by removing the earlier unannotated
declaration, leaving the annotated field-level `users` and its setter.
##########
core/src/main/java/org/apache/struts2/conversion/annotations/TypeConversion.java:
##########
@@ -139,7 +145,7 @@
* this.keyValues = keyValues;
Review Comment:
Correct, and fixed in 48c6f8257 — the example now shows a class-scoped
conversion, since `APPLICATION` entries are keyed by class name and a derived
`convertInt` key can never be read back. The existing `APPLICATION` example on
`execute()` with `key = "java.util.Date"` stays as the reference for the
correct form.
`ConversionTestAction:56` carried the same misleading declaration and has
been aligned; nothing depended on it.
Issue Time Tracking
-------------------
Worklog Id: (was: 1032238)
Time Spent: 50m (was: 40m)
> TypeConversion annotation support improvement
> ---------------------------------------------
>
> Key: WW-3871
> URL: https://issues.apache.org/jira/browse/WW-3871
> Project: Struts 2
> Issue Type: Improvement
> Components: Plugin - Convention
> Affects Versions: 2.3.4.1
> Reporter: Pavan Ananth
> Assignee: Lukasz Lenart
> Priority: Major
> Fix For: 7.3.0
>
> Time Spent: 50m
> Remaining Estimate: 0h
>
> The annotation support for TypeConversion in Struts 2 is too literal an
> interpretation of the XML support. For instance, I am required to supply this
> if I have choose the CreateIfNull feature at a property level :
> @TypeConversion(key="CreateIfNull_users", rule=ConversionRule.CreateIfNull,
> value="true")
> List<User> users;
> Given that the rule is CreateIfNull, the key can be constructed implicitly
> using property name - why is it asked of an user to type in the full key.
> This holds good for the other supported ConversionRules as well
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
