[
https://issues.apache.org/jira/browse/WW-5675?focusedWorklogId=1035805&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1035805
]
ASF GitHub Bot logged work on WW-5675:
--------------------------------------
Author: ASF GitHub Bot
Created on: 14/Aug/26 13:51
Start Date: 14/Aug/26 13:51
Worklog Time Spent: 10m
Work Description: lukaszlenart opened a new pull request, #1847:
URL: https://github.com/apache/struts/pull/1847
Fixes [WW-5675](https://issues.apache.org/jira/browse/WW-5675) — sub-task of
[WW-5667](https://issues.apache.org/jira/browse/WW-5667).
## The problem
`SecurityMemberAccess` is a `Scope.PROTOTYPE` bean, so a fresh instance is
constructed once per value stack and again for every OGNL context — several
times per request. Each construction re-ran all sixteen `@Inject` configuration
setters, each re-parsing a raw comma-delimited string from scratch: comma
splitting, `strip`, classloader validation, `Pattern.compile`, and `HashSet`
accumulation. With the stock `struts-excluded-classes.xml` that is roughly 90
configuration entries rebuilt per instantiation.
`OgnlUtil.copy` alone calls `createDefaultContext` twice, so a single copy
paid for two full configuration rebuilds.
This is the dominant half of WW-5667. The sibling ticket WW-5674 (merged as
`81b34c295`) addressed the per-*access* allocations; **this** is the one
expected to move the reporter's 9%.
Note that the fix originally proposed on WW-5667 — caching the parsed set in
a `SecurityMemberAccess` field — cannot work, because the instance holding that
field is itself discarded and rebuilt each time.
## The change
A new container-singleton bean, `SecurityMemberAccessConfig`, owns all
configuration parsing and does it once per container. `SecurityMemberAccess`
stays `Scope.PROTOTYPE` and receives that bean through a single `@Inject`
setter, copying immutable set references — no parsing, no `HashSet`
construction, no `Pattern.compile` per instantiation.
Also in this PR:
- The lazy dev-mode flip (`useDevModeConfiguration()` plus a `volatile`
guard) is gone from the OGNL access path. Dev-mode now resolves once, in the
config bean's `Initializable.init()`.
- The two-set allowlist walk added by WW-5674 collapses into a single
precomputed union, resolving WW-5678's first item.
- `ConfigParseUtil.validatePackageNames` no longer recompiles
`Pattern.compile("\\s")` once per package name (~58 recompiles per
instantiation under the default config).
### Why setter injection rather than constructor injection
Deliberate, and load-bearing. `ContainerImpl.addInjectors` recurses into
superclasses first, so an existing user subclass calling
`super(providerAllowlist, threadAllowlist)` keeps compiling *and* still
receives the configuration. Had the configuration arrived via the constructor
with a deprecated two-argument overload retained, such a subclass would have
compiled cleanly and run with empty exclusions — a silent fail-open. Setter
injection removes that failure mode rather than documenting it.
`useConfig` is a mandatory `@Inject`, so a container missing the binding
throws at build time instead of running with weaker exclusions.
### Registration in two places
`SecurityMemberAccessConfig` is registered in both
`DefaultConfiguration.bootstrapFactories` and `struts-beans.xml`, mirroring
`ProviderAllowlist` and `ThreadAllowlist`. Both are load-bearing:
- `Dispatcher.init()` installs its own provider list and never adds
`StrutsDefaultConfigurationProvider`, so the main container is built from
`StrutsBeanSelectionProvider` plus `struts-beans.xml`.
- `DefaultConfiguration.reloadContainer` builds a bootstrap container from
`bootstrapFactories` and instantiates `SecurityMemberAccess` through it before
the main container exists.
Production throws at startup without either.
## ⚠️ Breaking change in a minor release
**Five `public` methods are deleted from `SecurityMemberAccess`:**
`useDevMode`, `useDevModeExcludedClasses`,
`useDevModeExcludedPackageNamePatterns`, `useDevModeExcludedPackageNames`,
`useDevModeExcludedPackageExemptClasses`.
They are only ever container-injected — a repo-wide search finds no direct
caller in core, plugins, apps, or tests. Retaining them faithfully would mean
keeping `isDevMode` plus the four dev-mode set fields on the instance and
reinstating the lazy flip, i.e. keeping precisely the code this change exists
to delete. Retaining them in simplified form was rejected because the current
semantics are subtle enough that any simplification would silently change them:
a manual `useDevModeExcludedClasses` call accumulates into the dev-mode set,
which then *replaces* — rather than unions with — `excludedClasses` on first
access.
The failure mode for anyone affected is a compile error on upgrade, which is
loud and has an obvious fix.
**This needs a Version Notes and Migration Guide entry for the release. That
entry does not exist yet.**
The other eleven configuration setters are *not* removed. They keep their
exact bodies and still mutate the instance, so the ~110 existing direct call
sites are unaffected. They are annotated `@Deprecated(since = "7.4.0",
forRemoval = true)` and scheduled for removal in
[WW-5682](https://issues.apache.org/jira/browse/WW-5682) (8.0.0).
## Behaviour
OGNL allow/deny semantics are unchanged. One visible difference: the
`"DevMode enabled, using DevMode excluded classes and packages..."` warning now
fires when the configuration singleton is built rather than on the first OGNL
access — a deterministic startup signal instead of one contingent on traffic.
## Testing
Full `core` suite: **3181 tests, 0 failures, 0 errors**. `plugins/spring`
61/61, `plugins/cdi` 17/17.
New coverage, beyond the existing suites passing untouched:
- **Sharing proof** — several `SecurityMemberAccess` instances from one
container hold reference-identical configuration sets (`assertSame`, since any
re-parse necessarily allocates fresh). The container is loaded with every
relevant constant set away from its default, so the assertions cannot pass on
shared defaults.
- **Instance isolation** — a deprecated setter call on one instance perturbs
neither a sibling nor the singleton.
- **Subclass injection** — a subclass using the two-argument constructor
still receives the configuration, guarding the fail-open hole described above.
- **Production registration** — a `StrutsInternalTestCase` boot (real
`Dispatcher.init()`, loads `struts-beans.xml`) asserts the bean is a singleton
on the path production actually uses. Verified by temporarily setting
`scope="prototype"` and confirming the test fails.
- **Differential parsing** — the config bean's output is compared against a
frozen copy of the accumulation logic it replaces, including the
`useAllowStaticFieldAccess` → `useExcludedClasses` side effect and the
commutativity that makes the setters safe against the container's unspecified
`getDeclaredMethods()` order.
The design document is included in the diff at
`docs/superpowers/specs/2026-08-14-WW-5675-security-member-access-config-sharing-design.md`.
Issue Time Tracking
-------------------
Worklog Id: (was: 1035805)
Remaining Estimate: 0h
Time Spent: 10m
> Stop re-parsing OGNL security config on every SecurityMemberAccess
> instantiation
> --------------------------------------------------------------------------------
>
> Key: WW-5675
> URL: https://issues.apache.org/jira/browse/WW-5675
> Project: Struts 2
> Issue Type: Sub-task
> Reporter: Lukasz Lenart
> Assignee: Lukasz Lenart
> Priority: Major
> Fix For: 7.4.0
>
> Time Spent: 10m
> Remaining Estimate: 0h
>
> Sub-task of WW-5667.
> {{SecurityMemberAccess}} is a {{Scope.PROTOTYPE}} bean
> ({{StrutsBeanSelectionProvider}}, since WW-5343). Every
> {{container.getInstance(SecurityMemberAccess.class)}} therefore constructs a
> fresh instance and re-runs all of its {{@Inject}} setters, each of which
> re-parses a raw comma-delimited configuration string from scratch.
> With the stock {{struts-excluded-classes.xml}}, a single instantiation
> re-parses roughly:
> * 16 excluded class names from {{struts.excludedClasses}} and another 16 from
> {{struts.devMode.excludedClasses}} — comma split plus classloader validation
> * 29 excluded package names from {{struts.excludedPackageNames}} and another
> 29 from {{struts.devMode.excludedPackageNames}} — comma split, {{strip}}, and
> {{validatePackageNames}}
> * the allowlist class and package sets
> * any configured excluded-package-name patterns, which are re-compiled via
> {{Pattern.compile}}
> New instances are created on the request path from at least:
> * {{OgnlValueStackFactory.createValueStack(...)}} — once per value stack;
> {{ParametersInterceptor.toNewStack}} creates an additional stack per request
> * {{OgnlUtil.createDefaultContext(Object, ClassResolver)}} — reached from
> {{setProperties}}, {{copy}}, {{getBeanMap}} and friends
> so the full configuration is rebuilt several times per request.
> This matches JFR sample 2 on the parent ticket:
> {noformat}
> HashMap.put / HashSet.add
> ConfigParseUtil.toNewPackageNamesSet(Collection, String) :138
> SecurityMemberAccess.useExcludedPackageNames(String) :446
> {noformat}
> Note that the fix proposed on the parent ticket — caching the parsed set in a
> {{SecurityMemberAccess}} field — does not address this, because the instance
> holding the field is itself discarded and rebuilt each time.
> h2. Also in scope: validatePackageNames recompiles a constant pattern per
> entry
> {{ConfigParseUtil.validatePackageNames}} evaluates {{Pattern.compile("\\s")}}
> once per package name rather than once overall:
> {code:java}
> public static void validatePackageNames(Collection packageNames) {
> if (packageNames.stream().anyMatch(s ->
> Pattern.compile("\\s").matcher(s).find())) {
> throw new ConfigurationException("Excluded package names could not be
> parsed due to erroneous whitespace characters: " + packageNames);
> }
> }
> {code}
> With the default configuration that is roughly 58 recompiles of a trivial
> pattern per {{SecurityMemberAccess}} instantiation, on top of the parsing
> work above. Hoisting the pattern to a static constant — or replacing the
> check with a plain character scan — is a small, self-contained fix, and it
> shares this ticket's root cause of doing per-instantiation work that should
> be done once.
> h2. Design note: this ticket unblocks collapsing the allowlist two-set walk
> WW-5674 merged the two allowlist walks in {{isClassAllowlisted}} by adding a
> two-set helper:
> {code:java}
> static boolean isPackageBelongsToPackages(String packageName, Set first, Set
> second)
> {code}
> The cleaner shape is a single set holding the union of
> {{ALLOWLIST_REQUIRED_PACKAGES}} and {{allowlistPackageNames}}, precomputed
> once. That would delete the two-set parameter and the three-argument overload
> entirely, leaving one set and one walk.
> It was rejected in WW-5674 for two reasons, and _this ticket removes the
> first and largely removes the second_:
> h1. Under the current prototype scope, a precomputed union field is rebuilt
> on every instantiation — several times per request — adding to exactly the
> cost this ticket is about. Once the parsed configuration is shared rather
> than per-instance, the union becomes free.
> h1. The union would need initialising in two places: at the field
> declaration, for the case where the {{@Inject}} setter never fires because no
> allowlist is configured, and again in the setter. Duplicated initialisation
> here is a _fail-open_ hazard — get it wrong and
> {{ALLOWLIST_REQUIRED_PACKAGES}} silently drops out of the allowlist with
> nothing failing loudly. Whatever this ticket does to give the configuration a
> single well-defined construction point makes that much safer to get right.
> Worth doing as part of this ticket rather than leaving it to WW-5678, since
> the enabling change lands here. Note that it would also resolve WW-5678's
> first item for free, by removing the package-private overload that shares a
> name with a public method.
> h2. Constraints
> The OGNL allow/deny semantics must not change. The dev-mode configuration
> switchover in {{useDevModeConfiguration()}} must keep working. Per-instance
> mutable state set at request time by
> {{ParametersInterceptor.applyMemberAccessProperties}}
> ({{useAcceptProperties}} / {{useExcludeProperties}}) must stay per-instance —
> this is why the bean was made prototype-scoped in the first place, so any
> move back towards sharing has to account for it.
> h2. Related
> * WW-5674 — the per-access half of the parent report
> ({{isClassBelongsToPackages}} allocations). Handled separately; it does not
> move this cost.
> * WW-5677 — remaining redundant {{getPackage()}} lookups on the per-access
> path. Same file, different root cause.
> * WW-5678 — helper naming and visibility cleanup for 8.0.0. Its first item
> may be resolved for free here, see the design note above.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
