Lukasz Lenart created WW-5660:
---------------------------------
Summary: Define interceptor parameters via dedicated params
classes instead of setters on the interceptor
Key: WW-5660
URL: https://issues.apache.org/jira/browse/WW-5660
Project: Struts 2
Issue Type: Improvement
Components: Core Interceptors
Reporter: Lukasz Lenart
Fix For: 8.0.0
h3. Current state
Interceptor {{<param>}} values are applied by reflection onto the interceptor
instance at
configuration-parse time:
{code:java}
// DefaultInterceptorFactory.java:86
reflectionProvider.setProperties(params, interceptor);
{code}
Every parameter an interceptor accepts is therefore a setter on the interceptor
itself, and
configuration-time state and request-time state share one object. The scale
in-tree today:
* 44 interceptor implementations across {{core}} and {{plugins}}
* 61 distinct {{public void setXxx(String)}} parameter setters in
{{org.apache.struts2.interceptor}} alone
* {{disabled}} is universal, held as a shared {{boolean}} on
{{AbstractInterceptor}} (field at
{{:28}}, written by {{setDisabled}} at {{:56-58}}, read by
{{shouldIntercept}} at {{:61-63}})
h3. Why this is worth changing
Interceptors are singletons: built once at configuration-parse time and reused
for every
request, and shared across every action referencing the same stack. Nothing in
the current
shape distinguishes a parameter that is fixed at configuration time from one
that varies per
request, so the type system offers no protection against writing request-scoped
state onto
process-wide state.
WW-5659 is the concrete demonstration. {{WithLazyParams}} resolved {{${...}}}
parameters per
request and applied them to the interceptor through exactly the setters above,
so concurrent
requests could observe one another's upload validation policy. The fix had to
change the
contract so resolved values land in a per-invocation object rather than on the
interceptor.
h3. What WW-5659 already established
WW-5659 introduced the vocabulary this issue would generalise, in
{{org.apache.struts2.interceptor}}:
* {{InterceptorParams}} - the general contract for a parameters holder
* {{DisableParams}} - opt-in holder state for the universal {{disabled}}
parameter
* {{UploadPolicy}} - the file upload holder, {{extends DisableParams}}
It also began moving {{disabled}} off {{AbstractInterceptor}}: on the lazy path
the singleton
field is no longer written, and the holder is authoritative.
h3. Proposal
Give every interceptor a dedicated parameters class rather than a set of
setters:
* the framework populates the params object rather than the interceptor
* the interceptor consumes it, and holds no mutable parameter state itself
* configuration-time and request-time state become separable by construction
* {{disabled}} moves off {{AbstractInterceptor}} entirely and onto
{{DisableParams}}
The natural population point already exists at
{{DefaultInterceptorFactory.java:86}}, which
would target the params object instead of the interceptor.
h3. Why 8.0.0
This is an ecosystem-wide breaking change. It touches all 44 in-tree
interceptors plus every
third-party interceptor that declares a {{<param>}}. It fits alongside the API
reshaping
already targeted at 8.0.0 - see WW-4759 ({{struts2-api}} extraction).
h3. Explicitly not a correctness fix
No known bug depends on this. WW-5659 established that {{disabled}} was racy
only on the lazy
parameter path, and that path is fixed. This issue is a design improvement that
makes the
whole class of bug unexpressible rather than fixed case by case, so it should
not inherit
WW-5659's urgency and should not be rushed into a minor release.
h3. Scope for a follow-up design
Worth deciding during design rather than up front:
* whether the params object is supplied by the interceptor (as
{{WithLazyParams.newLazyParams()}}
does) or resolved by the framework from a declared type
* how {{ConditionalInterceptor}} and {{MethodFilterInterceptor}} parameters fit
* whether a migration shim is offered for third-party interceptors, or whether
the compile
break is accepted as it was in WW-5659
* how much of the OGNL type conversion currently provided by reflection-based
population must
be preserved
h3. References
* WW-5659 - request-scoped resolution of lazy interceptor params; introduced
{{InterceptorParams}}, {{DisableParams}}, {{UploadPolicy}}
* WW-4759 - struts2-api extraction, also targeted at 8.0.0
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
