[
https://issues.apache.org/jira/browse/WW-5070?focusedWorklogId=427379&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-427379
]
ASF GitHub Bot logged work on WW-5070:
--------------------------------------
Author: ASF GitHub Bot
Created on: 26/Apr/20 17:19
Start Date: 26/Apr/20 17:19
Worklog Time Spent: 10m
Work Description: JCgH4164838Gh792C124B5 commented on a change in pull
request #406:
URL: https://github.com/apache/struts/pull/406#discussion_r415353423
##########
File path: plugins/json/src/main/java/org/apache/struts2/json/JSONResult.java
##########
@@ -212,12 +211,22 @@ protected Object readRootObject(ActionInvocation
invocation) {
}
protected Object findRootObject(ActionInvocation invocation) {
+ ValueStack stack = invocation.getStack();
Review comment:
The new logic seems to make sense, and the updated comment should make
it clear what the behaviour will be in 2.6.
There might be some users with existing implementations depending on the
current 2.5.x behaviour (always `peek()` if root is `null`), but they could
implement a custom `JSONResult` with the old behaviour if they really needed it.
##########
File path: plugins/json/src/main/java/org/apache/struts2/json/JSONResult.java
##########
@@ -286,7 +294,9 @@ public String getRoot() {
}
/**
- * Sets the root object to be serialized, defaults to the Action
+ * Sets the root object to be serialized, defaults to the Action.
+ * If the Action implements {@link ModelDriven}, model will be used instead
+ * and assumptions is the Model was pushed on the top of the stack
Review comment:
The last comment might be a bit clearer as:
```
* Sets the root object to be serialized, defaults to the Action.
* If the Action implements {@link ModelDriven}, the Model will be used
instead,
* with the logic assuming the Model was pushed onto the top of the
stack.
```
##########
File path: plugins/json/src/main/java/org/apache/struts2/json/JSONResult.java
##########
@@ -68,8 +69,6 @@
*/
public class JSONResult implements Result {
- private static final long serialVersionUID = 8624350183189931165L;
Review comment:
Since `JSONResult` still extends `Serializable`, maybe the
`serialVersionUID` should be kept ?
By removing it, anyone who Java-serialized instances in 2.5.x (possible) and
then tries to Java-deserialize them in 2.6 could experience deserialization
failures.
If this change is intentional because of the logic change for 2.6, maybe a
brand new `serialVersionUID` value should be generated instead of just dropping
it ?
##########
File path: plugins/json/src/main/java/org/apache/struts2/json/JSONResult.java
##########
@@ -212,12 +211,22 @@ protected Object readRootObject(ActionInvocation
invocation) {
}
protected Object findRootObject(ActionInvocation invocation) {
+ ValueStack stack = invocation.getStack();
Object rootObject;
if (this.root != null) {
- ValueStack stack = invocation.getStack();
+ LOG.debug("Root was defined as [{}], searching stack for it",
this.root);
rootObject = stack.findValue(root);
} else {
- rootObject = invocation.getStack().peek(); // model overrides
action
+ LOG.debug("Root was not defined, searching for #action");
+ rootObject = stack.findValue("#action");
+ if (rootObject instanceof ModelDriven) {
+ LOG.debug("Action is an instance of ModelDriven, assuming
model is on the top of the stack and using it");
+ rootObject = stack.peek();
+ }
+ if (rootObject == null) {
Review comment:
Maybe the 2nd if check should be changed to:
```
else if (rootObject == null) {
```
because if the rootObject is a `ModelDriven` and somehow the `stack.peek()`
returns`null`, the PR's logic would attempt the same operation again (along
with an extra debug output).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
Issue Time Tracking
-------------------
Worklog Id: (was: 427379)
Time Spent: 0.5h (was: 20m)
> JSONResult default root object should be set explicitly, rather than from
> result of ValueStack.peek()
> -----------------------------------------------------------------------------------------------------
>
> Key: WW-5070
> URL: https://issues.apache.org/jira/browse/WW-5070
> Project: Struts 2
> Issue Type: Improvement
> Components: Core Results
> Affects Versions: 2.5.17
> Environment: Struts 2.5.17
> Tomcat 8.0.24
> Reporter: Michael Hum
> Priority: Minor
> Fix For: 2.6
>
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> JSONResult#setRoot() is documented as follows:
> {quote}
> Sets the root object to be serialized, defaults to the Action
> {quote}
> This is implemented in {{org.apache.struts2.json.JSONResult#findRootObject}}:
> {code:java}
> if (this.root != null) {
> ValueStack stack = invocation.getStack();
> rootObject = stack.findValue(root);
> } else {
> rootObject = invocation.getStack().peek(); // model overrides
> action
> }
> {code}
> We have just run into an issue with our application where this expectation
> did not turn out to be true, due to a race condition in some of our custom
> results/interceptors (triggered by multiple requests) that results in the top
> of the stack not being the action. We've mitigated the issue by explicitly
> setting the root for serialization as the action:
> {code:java}
> @Result(name = FooAction.JSON, type = "json", params = {"root", "#action",
> "noCache", "true", "ignoreHierarchy", "false"}),
> {code}
> While not a bug triggered by any struts code, it would make more sense in
> this case to explicitly find the action instead of assuming that the top of
> the stack is the action. The ValueStack is able to be manipulated freely by
> developers, so this would guarantee that the default be correct regardless of
> external manipulation.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
