WW-5650 Obtain a fresh JSON reader/writer per request in JSONInterceptor

[lukaszlenart]

What

Follow-up refactor of the JSON plugin's reader/writer thread-safety handling.

The interim fixes in #1775 / #1776 confined StrutsJSONReader / StrutsJSONWriter per-operation
state to a ThreadLocal, created fresh per call and cleared in a finally. That works, but it
relies on a per-request cleanup contract and keeps a single reader/writer instance shared across
threads.

This change removes that machinery in favour of a simpler, structural approach: the singleton
JSONInterceptor now obtains a fresh JSONUtil (and thus a fresh reader + writer) per request
via the container, so no parse/serialize state is ever shared across threads in the first place.
With instances no longer shared, StrutsJSONReader / StrutsJSONWriter revert to plain,
single-use instance fields and are documented as not thread-safe.

Why this is safe / minimal

• JSONResult was already built per request (it injects a prototype JSONUtil) and is
  untouched — it never shared state.
• JSONReader, JSONWriter and JSONUtil are already registered scope="prototype", so
  container.getInstance(JSONUtil.class) hands back a fresh graph each call and continues to honour
  the struts.json.reader / struts.json.writer overrides.
• This mirrors an existing in-tree pattern (AbstractFileUploadInterceptor injects Container and
  resolves collaborators per request).

Changes

• JSONInterceptor: drop the cached @Inject JSONUtil field; inject Container and resolve a
  fresh JSONUtil per intercept() (new protected getJSONUtil() seam).
• StrutsJSONReader / StrutsJSONWriter: revert ThreadLocal state back to plain instance fields;
  add a "not thread-safe — obtain a fresh instance per operation" class note.
• Tests: the old shared-single-instance concurrency tests asserted an invariant this design
  intentionally drops (a single shared instance being safe), so they are replaced by an
  interceptor-level test asserting a distinct JSONUtil/reader is obtained per acquisition.

Testing

mvn test -DskipAssembly -pl plugins/json → all green (141 tests). JSONWriterOverrideTest passes,
confirming struts.json.reader / struts.json.writer overrides still resolve through the container.

Notes

• Public/protected extension-method signatures of the reader/writer are unchanged; custom
  subclasses and reader/writer overrides continue to work.
• The interceptor freshness test asserts distinctness via getJSONUtil() (JSONUtil exposes no
  writer getter); this guards against re-caching/singleton-scoping the util.

Fixes WW-5650

🤖 Generated with Claude Code

[lukaszlenart]

Code Review

Overview

Follow-up to the earlier thread-safety fixes (#1775/#1776). Those confined per-operation parse/serialize state to a ThreadLocal because a single StrutsJSONReader/StrutsJSONWriter was shared across concurrent requests via the singleton JSONInterceptor. This PR takes a cleaner structural approach: the interceptor now resolves a fresh JSONUtil (and thus fresh reader + writer) per request from the container, so no state is ever shared across threads. The ThreadLocal machinery reverts to plain instance fields, and the classes are documented as not thread-safe.

Correctness — solid

• Prototype scopes confirmed. struts-plugin.xml registers JSONWriter, JSONReader, and JSONUtil all as scope="prototype", so container.getInstance(JSONUtil.class) genuinely returns a fresh object graph each call. JSONUtil.setReader/setWriter are @Inject, so the fresh util carries a fresh reader/writer. The design premise holds.
• getJSONUtil() called once per intercept() and threaded through applyLimitsToReader(jsonUtil) and the SMD serialize path — no leftover reference to the removed cached field.
• JSONResult path untouched and already safe. Results are instantiated per invocation, each with its own @Inject JSONUtil, so it never shared state. Correctly left alone.
• Defensive reset preserved. write(...) resets buf/stack/exprStack/root/buildExpr at entry and read(String) resets it/c/depth, so even sequential reuse of one instance stays clean.
• No stragglers. No remaining setJsonUtil callers on the interceptor; all tests migrated to setContainer.

Style / conventions

• Mirrors the in-tree AbstractFileUploadInterceptor pattern (inject Container, resolve collaborators per request).
• Javadoc updates accurately describe the new contract and reference WW-5650.
• The protected getJSONUtil() seam is a clean, testable extension point; public/protected reader/writer signatures are unchanged, so subclasses and struts.json.reader/writer overrides keep working.

Performance

Each request now constructs a fresh JSONUtil + reader + writer via container.getInstance. A modest per-request cost versus the previous single injected instance — acceptable for eliminating shared mutable state. The expensive part, StrutsJSONWriter.BEAN_INFO_CACHE, remains static, so introspection results are still cached across instances.

Test coverage — minor gap

• Removing the concurrency tests is correct: they asserted an invariant the design intentionally drops (a single shared instance being safe).
• testObtainsFreshJSONUtilAndReaderPerInvocation is a good structural guard against re-caching/singleton-scoping (distinct JSONUtil + distinct getReader() per call).
• Gap: nothing exercises writer freshness per request the same way. The util exposes no writer getter (acknowledged in the PR), so it's understandable, but the writer was the more dangerous of the two (cross-response data leakage). Consider a small integration-level assertion if a getter can reasonably be exposed. Not blocking.

Security

Safe resolution of the original vulnerability class (one request's serialized data surfacing in another's response). Since instances are no longer shared, the race is structurally impossible rather than mitigated by cleanup discipline. Net improvement.

Nit

• StrutsJSONWriter adds a trailing blank line before the closing brace — trivial.

Verdict

Clean, well-reasoned refactor that removes complexity while strengthening the safety guarantee. Correctness verified against the plugin config and call sites. LGTM after considering the optional writer-freshness test.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR refactors the Struts JSON plugin to avoid sharing JSON parse/serialize state across concurrent requests by having JSONInterceptor resolve a fresh JSONUtil (and thus fresh reader/writer instances) per interception, instead of relying on per-call ThreadLocal state in StrutsJSONReader/StrutsJSONWriter.

Changes:

• JSONInterceptor now injects Container and obtains a fresh JSONUtil per invocation (with a getJSONUtil() seam), and applies limits to that per-invocation reader.
• StrutsJSONReader / StrutsJSONWriter revert from ThreadLocal state back to instance fields and are documented as not thread-safe (prototype-per-operation expectation).
• Concurrency-focused reader/writer tests are removed and replaced with an interceptor-level “fresh instance” test.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java	Switches from cached JSONUtil injection to per-invocation resolution via Container, and threads that instance through reader-limit application and JSON-RPC handling.
plugins/json/src/main/java/org/apache/struts2/json/StrutsJSONReader.java	Removes ThreadLocal parse state and returns to per-instance parse fields; updates class documentation to non-thread-safe/prototype usage.
plugins/json/src/main/java/org/apache/struts2/json/StrutsJSONWriter.java	Removes ThreadLocal write state and returns to per-instance write fields; updates class documentation to non-thread-safe/prototype usage.
plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java	Updates interceptor construction to use Container and adds a freshness test for per-acquisition JSONUtil/reader instances.
plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONReaderTest.java	Removes concurrency reuse regression test that’s no longer aligned with the new “fresh instance per operation” design.
plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONWriterTest.java	Removes concurrency reuse regression test that’s no longer aligned with the new “fresh instance per operation” design.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
89.9% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

[g0w6y]

The per-request prototype approach looks sound and preserves the fix from #1775/#1776, so this is a non-blocking suggestion, not a gate.

One coverage point worth considering: testObtainsFreshJSONUtilAndReaderPerInvocation asserts a fresh reader per invocation, but not a fresh writer, and nothing asserts that JSONResult obtains a fresh writer per request. Since StrutsJSONWriter now reverts to plain instance fields and is explicitly documented not thread-safe (including the lazily-initialized SimpleDateFormat in date()), the response-side protection from WW-5644 now depends entirely on the prototype lifecycle. There is no live bug today (JSONResult is instantiated per request), but if the bean were ever changed to singleton, or a writer cached, the cross-request response leak would return with no failing test to catch it.

Suggestion: (1) assert that two fresh JSONUtil instances carry distinct writers (would need a getWriter() accessor, which JSONUtil does not currently expose), and (2) a JSONResult-level test confirming a fresh writer per result. Happy to send a patch for these if useful.