You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@struts.apache.org by GitBox <gi...@apache.org> on 2020/04/25 07:11:01 UTC
[GitHub] [struts] lukaszlenart opened a new pull request #406: [WW-5070] Adds more sophisticated logic to search for the Root
lukaszlenart opened a new pull request #406:
URL: https://github.com/apache/struts/pull/406
Resolves [WW-5070](https://issues.apache.org/jira/browse/WW-5070)
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] [struts] lukaszlenart commented on a change in pull request #406: [WW-5070] Adds more sophisticated logic to search for the Root
Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on a change in pull request #406:
URL: https://github.com/apache/struts/pull/406#discussion_r415539580
##########
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:
Right, good point :)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [struts] lukaszlenart commented on a change in pull request #406: [WW-5070] Adds more sophisticated logic to search for the Root
Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on a change in pull request #406:
URL: https://github.com/apache/struts/pull/406#discussion_r415540412
##########
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:
👍
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [struts] coveralls edited a comment on pull request #406: [WW-5070] Adds more sophisticated logic to search for the Root
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #406:
URL: https://github.com/apache/struts/pull/406#issuecomment-619337271
[](https://coveralls.io/builds/30359667)
Coverage increased (+0.003%) to 49.184% when pulling **d826efb9d09d2fc0b09a2eafde5bafc38ff3c6a8 on WW-5070-root-action-model** into **3ccbcc6f9c9f84f6f1e5644e1e266754d88a8017 on master**.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [struts] JCgH4164838Gh792C124B5 commented on a change in pull request #406: [WW-5070] Adds more sophisticated logic to search for the Root
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [struts] lukaszlenart commented on a change in pull request #406: [WW-5070] Adds more sophisticated logic to search for the Root
Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on a change in pull request #406:
URL: https://github.com/apache/struts/pull/406#discussion_r415537558
##########
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:
Yeah, the idea was as this is a larger change to the class the ID should be changed, then I thought _why do we need the ID at all here, nobody should ever serialize a instance of `Result`_. So probably the main case is to stop extending `Serializable` by `Result`. I will put back a new ID.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [struts] coveralls commented on pull request #406: [WW-5070] Adds more sophisticated logic to search for the Root
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #406:
URL: https://github.com/apache/struts/pull/406#issuecomment-619337271
[](https://coveralls.io/builds/30334750)
Coverage increased (+0.003%) to 49.184% when pulling **19c71ff1537711ce0b802545e8327d564ee8f769 on WW-5070-root-action-model** into **3ccbcc6f9c9f84f6f1e5644e1e266754d88a8017 on master**.
----------------------------------------------------------------
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:
users@infra.apache.org