[
https://issues.apache.org/jira/browse/WW-3871?focusedWorklogId=1032182&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1032182
]
ASF GitHub Bot logged work on WW-3871:
--------------------------------------
Author: ASF GitHub Bot
Created on: 25/Jul/26 17:07
Start Date: 25/Jul/26 17:07
Worklog Time Spent: 10m
Work Description: Copilot commented on code in PR #1812:
URL: https://github.com/apache/struts/pull/1812#discussion_r3650593030
##########
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) {
Review Comment:
`ConversionType.APPLICATION` entries are stored in the global default
converter map keyed by *class name* (see DefaultConversionAnnotationProcessor).
When `tc.key()` is empty, this method derives a property name (e.g.
`convertInt`) and registers an unreachable default mapping under that
non-class-name key, which can silently pollute the global map. Consider
requiring an explicit `key` for `APPLICATION` on methods and skipping otherwise.
##########
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:
The Javadoc example now declares `users` twice (once unannotated earlier in
the snippet, and again here with `@TypeConversion`), which makes the sample
invalid/confusing. Remove the earlier unannotated `users` field so the example
shows only the field-level `@TypeConversion`.
##########
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:
Field annotations can also be declared with `type = APPLICATION`. If `key`
is omitted, this pass derives the *field name* and registers an `APPLICATION`
default mapping under that non-class-name key, which is unreachable and
unnecessarily pollutes the global converter map. Consider requiring an explicit
`key` for `APPLICATION` on fields and skipping otherwise.
##########
core/src/main/java/org/apache/struts2/conversion/annotations/TypeConversion.java:
##########
@@ -139,7 +145,7 @@
* this.keyValues = keyValues;
Review Comment:
The example shows `type = ConversionType.APPLICATION` on
`setConvertInt(...)` without providing a `key`. `APPLICATION` conversions are
stored under the given key in the global default converter map (typically keyed
by target class name), so deriving/omitting the key here is misleading and
likely results in an unreachable mapping. Consider changing this example to a
class-scoped conversion (or specify an explicit class-name key).
Issue Time Tracking
-------------------
Worklog Id: (was: 1032182)
Time Spent: 0.5h (was: 20m)
> 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: 0.5h
> 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)
