[
https://issues.apache.org/jira/browse/WW-5661?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Lukasz Lenart updated WW-5661:
------------------------------
Fix Version/s: 7.5.0
(was: 7.4.0)
> 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
> Priority: Minor
> Fix For: 7.5.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)
