Copilot commented on code in PR #1782:
URL: https://github.com/apache/struts/pull/1782#discussion_r3577359179
##########
plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java:
##########
@@ -98,6 +99,7 @@ public String intercept(ActionInvocation invocation) throws
Exception {
String requestContentType = readContentType(request);
String requestContentTypeEncoding = readContentTypeEncoding(request);
+ JSONUtil jsonUtil = getJSONUtil();
Review Comment:
`getJSONUtil()` is called unconditionally, which creates a fresh
JSONUtil/reader/writer even for requests that don't have a JSON content type.
This adds avoidable per-request overhead and also makes the interceptor depend
on a configured Container even when it ends up ignoring the request due to
content type.
##########
plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java:
##########
@@ -905,6 +890,18 @@ public void
testExcludePropertiesNotAppliedToInputByDefault() throws Exception {
assertEquals("b", action.getBar());
}
+ public void testObtainsFreshJSONUtilAndReaderPerInvocation() {
+ JSONInterceptor interceptor = new JSONInterceptor();
+ interceptor.setContainer(container); // StrutsTestCase-provided
container
+
+ JSONUtil first = interceptor.getJSONUtil();
+ JSONUtil second = interceptor.getJSONUtil();
+
+ assertNotSame("interceptor must obtain a fresh JSONUtil per request",
first, second);
+ assertNotSame("each fresh JSONUtil must carry its own reader (no
shared parse state)",
+ first.getReader(), second.getReader());
+ }
Review Comment:
The freshness test only asserts distinct JSONUtil instances and distinct
readers. Since this PR also relies on obtaining a fresh writer per request
(because StrutsJSONWriter is now explicitly not thread-safe), the test should
also assert that each acquired JSONUtil carries a distinct writer instance.
##########
plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java:
##########
@@ -744,8 +746,12 @@ public void setJsonRpcContentType(String
jsonRpcContentType) {
}
@Inject
- public void setJsonUtil(JSONUtil jsonUtil) {
- this.jsonUtil = jsonUtil;
+ public void setContainer(Container container) {
+ this.container = container;
+ }
+
+ protected JSONUtil getJSONUtil() {
+ return container.getInstance(JSONUtil.class);
}
Review Comment:
`getJSONUtil()` will throw a NullPointerException if `setContainer()` was
not called (e.g., manual instantiation in tests/tools). Failing fast with a
clear exception makes misconfiguration easier to diagnose and avoids an NPE
stack trace pointing at `container.getInstance(...)`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
