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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
