Lukasz Lenart created WW-5661:
---------------------------------
Summary: Refactor DefaultActionInvocation into smaller
collaborators
Key: WW-5661
URL: https://issues.apache.org/jira/browse/WW-5661
Project: Struts 2
Issue Type: Improvement
Components: Core
Reporter: Lukasz Lenart
Fix For: 7.4.0
h3. Trigger
SonarCloud reports {{java:S6539}} against {{DefaultActionInvocation.java:59}}:
{quote}
Split this "Monster Class" into smaller and more specialized ones to reduce its
dependencies
on other classes from 22 to the maximum authorized 20 or less.
{quote}
The rule is {{INFO}} severity and the metric itself is not the point. The class
crossed the
threshold because WW-5659 added two imports, but the underlying condition long
predates that
change and is worth addressing on its own terms rather than as a metric chase.
h3. Current state
{{core/src/main/java/org/apache/struts2/DefaultActionInvocation.java}}
* 570 lines, 28 imports
* 18 instance fields, 15 of them {{protected}}
* ~30 methods
* implements {{ActionInvocation}}, which declares only 10 methods - the public
contract is
small relative to the implementation behind it
The class currently owns several unrelated concerns:
* *invocation state* - {{action}}, {{proxy}}, {{result}}, {{explicitResult}},
{{resultCode}},
{{executed}}, {{stack}}, {{invocationContext}}
* *dependency injection surface* - seven {{@Inject}} setters
({{unknownHandlerManager}}, {{valueStackFactory}}, {{objectFactory}},
{{container}},
{{actionEventListener}}, {{ognlUtil}}, {{asyncManager}})
* *interceptor dispatch* - {{invoke}}, {{mergedParams}}, {{executeConditional}},
{{createInterceptors}}, {{prepareLazyParamInjector}}
* *action instantiation* - {{createAction}}
* *context map assembly* - {{createContextMap}}
* *result lifecycle* - {{createResult}}, {{getResult}}, {{executeResult}},
{{saveResult}}
* *action method invocation and exception handling* - {{invokeAction}}
* *async coordination* - {{asyncManager}} interplay inside {{invoke}}
* *pre-result listener registry* - {{preResultListeners}},
{{addPreResultListener}}
{{invoke()}} alone spans roughly 90 lines and mixes interceptor iteration,
lazy-params
dispatch, conditional-interceptor dispatch, async short-circuiting and result
execution.
h3. The binding constraint
{{DefaultActionInvocation}} is not effectively sealed. {{RestActionInvocation}}
({{plugins/rest}}) extends it, overrides {{invoke}} and {{saveResult}}, and
reads or writes the
inherited {{protected}} fields {{explicitResult}}, {{resultCode}}, {{proxy}},
{{stack}},
{{result}} and {{container}} directly.
Any refactoring that relocates those fields breaks that subclass, and very
likely third-party
subclasses too, since the {{protected}} surface has been stable for a long
time. The class is
constructed in exactly one place ({{DefaultActionProxyFactory.java:65}}), so
the *construction*
path is easy to change; it is the *inheritance* surface that is expensive.
This is why the issue targets 8.0.0 rather than a minor release.
h3. Candidate seams
Offered as starting points for a design discussion, not as a settled plan:
* *result lifecycle* - {{createResult}} / {{getResult}} / {{executeResult}} /
{{saveResult}}
form a coherent group with a clear boundary. {{RestActionInvocation}}
overrides
{{saveResult}}, so this seam needs a migration story before it can move.
* *action instantiation* - {{createAction}} depends on {{objectFactory}},
{{actionEventListener}} and {{unknownHandlerManager}} and is otherwise
self-contained.
Probably the cheapest extraction.
* *context map assembly* - {{createContextMap}} is pure construction and
touches little state.
* *interceptor dispatch* - the most valuable extraction and the hardest,
because {{invoke()}}
is re-entrant: interceptors call {{invocation.invoke()}} recursively, so any
split must
preserve that.
* *async coordination* - currently interleaved with interceptor dispatch inside
{{invoke()}};
worth separating so each is readable alone.
h3. Non-goals
* Not a behaviour change. This is structural work and should be covered by the
existing test
suite passing unchanged, not by new assertions about new behaviour.
* Not a fix for {{mergedParams}}' inherited name-based lookup quirk - that is
tracked
separately.
* Not an attempt to reach a specific Sonar number. If a split lands the metric
under 20, good;
if the honest decomposition leaves it at 21, that is an acceptable outcome
and the rule
should be marked accordingly.
h3. Suggested approach
Given the inheritance constraint, extract collaborators incrementally and keep
{{DefaultActionInvocation}} as a thin delegating facade that preserves its
current
{{protected}} surface for as long as compatibility requires. That allows the
work to proceed in
reviewable steps rather than one large break, with the facade thinned out once
subclasses have
migrated.
h3. References
* SonarCloud rule {{java:S6539}} on {{DefaultActionInvocation.java:59}}
* WW-5659 - added the two imports that crossed the threshold; did not create
the condition
* WW-4759 - {{struts2-api}} extraction, also targeted at 8.0.0; worth
sequencing against
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
