Lukasz Lenart created WW-5657:
---------------------------------
Summary: Reduce cognitive complexity in XWorkConverter
Key: WW-5657
URL: https://issues.apache.org/jira/browse/WW-5657
Project: Struts 2
Issue Type: Improvement
Components: XML Validators
Reporter: Lukasz Lenart
Fix For: 7.3.0
h2. Summary
SonarCloud reports three methods in
{{core/src/main/java/org/apache/struts2/conversion/impl/XWorkConverter.java}}
above the cognitive-complexity threshold ({{java:S3776}}, limit 15), plus a
handful of smaller findings in the same file. None is a correctness bug; all
are maintainability debt in a class that sits on the type-conversion path.
h2. Current state
Measured from the SonarCloud API on 2026-07-25
({{componentKeys=apache_struts:core/src/main/java/org/apache/struts2/conversion/impl/XWorkConverter.java}}).
|| Method || Cognitive complexity || Rule || Origin ||
| {{convertValue(Map, Object, Member, String, Object, Class)}} | *34* |
java:S3776 CRITICAL | long-standing |
| {{processMethodAnnotations(Map, Class)}} | *26* | java:S3776 CRITICAL |
introduced by WW-3871 |
| {{processFieldAnnotations(Map, Class)}} | *21* | java:S3776 CRITICAL |
introduced by WW-3871 |
WW-3871 replaced a single {{addConverterMapping}} of complexity 34 with four
named passes. That was a net improvement in readability, but two of the passes
still exceed the threshold on their own, because each carries the same
five-step pipeline of guards.
Also open on this file:
|| Rule || Location || Note ||
| java:S135 MINOR (x3) | the three annotation passes | "at most one
break/continue per loop" — the guard-clause {{continue}}s |
| java:S1192 CRITICAL | {{convertValue}} | literal {{"Unable to convert value
using type converter [{}]"}} repeated 3 times |
| java:S1172 MAJOR | {{handleConversionException}} | unused parameter
{{object}} |
| java:S112 MAJOR | {{buildConverterMapping}} | throws generic {{Exception}} |
| java:S1181 MAJOR | {{getConverter}} | "Catch Exception instead of Throwable"
— *see the warning below, do not apply blindly* |
h2. Proposed work
h3. 1. Extract the shared annotation-registration pipeline
{{processMethodAnnotations}} and {{processFieldAnnotations}} are the same five
steps in the same order, differing only in how the fallback name is derived,
the wording of two WARNs, and whether logging is gated:
# skip non-{{@TypeConversion}} annotations
# skip APPLICATION-scoped annotations with no explicit key (WARN)
# derive the name: explicit {{key()}}, else property name / field name
# resolve the key, skip if unresolvable (WARN)
# skip if the key is already mapped, otherwise delegate to
{{annotationProcessor}}
Extracting steps 2-5 into one helper taking the {{TypeConversion}}, the
{{Member}}, the derived fallback name and a "should log" flag collapses both
passes to a loop plus a delegate call. The guards become early {{return}}s in
the helper, which drops both methods under the S3776 threshold *and* clears the
three S135 findings — the {{continue}} statements disappear along with the
duplication.
This is the higher-value half of the ticket: it removes a copy-paste pair,
which is worth more than the Sonar number.
h3. 2. Decompose {{convertValue}}
The 87-line body does five separable jobs: unwrap a single-element
{{String[]}}, find a member-level converter, fall back to a class-level
converter, invoke it, and translate failures into conversion errors. Each is
independently nameable and testable. This is the older and larger of the two
problems, and the riskier one — see the constraint below.
h3. 3. Smaller findings
{{S1192}} (extract the repeated literal into a constant) and {{S1172}} (drop
the unused {{object}} parameter — check callers first, it is {{protected}} and
may be overridden downstream) are cheap and can ride along.
h2. Constraints
* *No behavioural change.* The conversion mapping registered for a given class,
and the precedence between the four passes, must stay identical. WW-3871 added
tests that pin the key-derivation rules, the class > method > field
precedence and first-writer-wins; they must pass untouched, as must
{{MyBeanActionTest}}, which is the backward-compatibility evidence for the
annotation forms.
* *Do not "fix" {{java:S1181}} by narrowing {{catch (Throwable)}} to {{catch
(Exception)}} in {{getConverter}}.* That catch is load-bearing:
{{getDeclaredFields()}} and {{getMethods()}} can throw {{NoClassDefFoundError}}
for a class with a member whose type is absent from the classpath, and the
handler turns it into {{addNoMapping(clazz)}}. Narrowing it would let an
{{Error}} escape into the OGNL property-access path. If the rule is to be
silenced, silence it with a comment explaining why, not by changing the catch.
* {{convertValue}} is {{public}} and
{{getConverter}}/{{handleConversionException}}/{{buildConverterMapping}} are
{{protected}} — this class is extended in the wild. Signature changes belong in
a major release; prefer extracting {{private}} helpers.
* Core module tests are JUnit 3/4, never JUnit 5: classes extending
{{XWorkTestCase}} use {{public void testXxx()}} with no {{@Test}} annotation,
which silently never runs there.
h2. Acceptance criteria
* No {{java:S3776}} issue remains open on {{XWorkConverter.java}} in SonarCloud.
* {{mvn test -DskipAssembly -pl core}} passes with no test modified to
accommodate the refactor. A test that had to change is a signal that behaviour
changed.
* No public or protected signature changes.
h2. Notes
Found while reviewing [PR #1812|https://github.com/apache/struts/pull/1812]
(WW-3871). Item 1 covers debt that PR introduced; item 2 predates it. If item 1
is folded into WW-3871 before it merges, this ticket narrows to
{{convertValue}} and the smaller findings.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
