[
https://issues.apache.org/jira/browse/WW-5657?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Lukasz Lenart reassigned WW-5657:
---------------------------------
Assignee: Lukasz Lenart
> 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
> Assignee: Lukasz Lenart
> Priority: Major
> 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)
