[GitHub] [struts] JCgH4164838Gh792C124B5 commented on a change in pull request #397: [WW-4789] [WW-3788] ActionContext refactoring

[GitBox <gi...@apache.org>]

JCgH4164838Gh792C124B5 commented on a change in pull request #397:
URL: https://github.com/apache/struts/pull/397#discussion_r415405262



##########
File path: core/src/main/java/org/apache/struts2/ServletActionContext.java
##########
@@ -26,23 +26,18 @@
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 import javax.servlet.jsp.PageContext;
-import java.util.Map;
 
 /**
  * Web-specific context information for actions. This class subclasses <tt>ActionContext</tt> which
  * provides access to things like the action name, value stack, etc. This class adds access to
  * web objects like servlet parameters, request attributes and things like the HTTP session.
  */
-public class ServletActionContext extends ActionContext implements StrutsStatics {
-
-    private static final long serialVersionUID = -666854718275106687L;

Review comment:
       Since `ServletActionContext` still implements `Serializable` (by extension), should it retain a `serialVersionUID` ?
   
   A new `serialVersionUID` could be generated to make it clear any previously serialized instances are not compatible.

##########
File path: core/src/main/java/org/apache/struts2/result/PlainTextResult.java
##########
@@ -32,13 +32,13 @@
 
 /**
  * <!-- START SNIPPET: description -->
- *
+ * <p>
  * A result that send the content out as plain text. Useful typically when needed
  * to display the raw content of a JSP or Html file for example.
- *
+ * <p>

Review comment:
       Should this be `</p>` ?

##########
File path: core/src/main/java/org/apache/struts2/result/PlainTextResult.java
##########
@@ -32,13 +32,13 @@
 
 /**
  * <!-- START SNIPPET: description -->
- *
+ * <p>
  * A result that send the content out as plain text. Useful typically when needed
  * to display the raw content of a JSP or Html file for example.
- *
+ * <p>
  * <!-- END SNIPPET: description -->
- *
- *
+ * <p>

Review comment:
       Should the two `<p>` lines be deleted and one added after the "START SNIPPET" line below (to match the above paragraph) ?

##########
File path: core/src/main/java/org/apache/struts2/interceptor/ScopeInterceptor.java
##########
@@ -262,7 +262,7 @@ static void unlock(Object o) {
     }
 
     protected void after(ActionInvocation invocation, String result) throws Exception {
-        Map ses = ActionContext.getContext().getSession();
+        Map<String, Object> ses = ActionContext.getContext().getSession();

Review comment:
       Elsewhere local variable `ses` was replaced with `session` (e.g. as in `before()`).  Maybe it should be the same here:
   ```
           Map<String, Object> session = ActionContext.getContext().getSession();
           if (session != null) {
               unlock(session);
           }
   ```

##########
File path: core/src/main/java/org/apache/struts2/interceptor/ScopeInterceptor.java
##########
@@ -271,18 +271,18 @@ protected void after(ActionInvocation invocation, String result) throws Exceptio
 
     protected void before(ActionInvocation invocation) throws Exception {
         invocation.addPreResultListener(this);
-        Map ses = ActionContext.getContext().getSession();
-        if (ses == null && autoCreateSession) {
-            ses = new SessionMap(ServletActionContext.getRequest());
-            ActionContext.getContext().setSession(ses);
+        Map<String, Object> session = ActionContext.getContext().getSession();
+        if (session == null && autoCreateSession) {
+            session = new SessionMap<>(ServletActionContext.getRequest());
+            ActionContext.getContext().withSession(session);
         }
 
-        if ( ses != null) {
-            lock(ses, invocation);
+        if ( session != null) {
+            lock(session, invocation);
         }
 
         String key = getKey(invocation);
-        Map app = ActionContext.getContext().getApplication();
+        Map<String, Object> app = ActionContext.getContext().getApplication();

Review comment:
       Similar note for local variable `app` renamed to` application` in other methods.  Maybe it should be:
   ```
   Map<String, Object> application = ActionContext.getContext().getApplication();
   ```
   That would mean changing existing references to `application` into `this.application` (like in `beforeResult()`).

##########
File path: core/src/main/java/org/apache/struts2/result/ServletRedirectResult.java
##########
@@ -59,22 +62,22 @@
  * <b>This result type takes the following parameters:</b>
  * </p>
  * <!-- START SNIPPET: params -->
- * 
+ *
  * <ul>
- * 
+ *
  * <li><b>location (default)</b> - the location to go to after execution.</li>
- * 
+ *
  * <li><b>parse</b> - true by default. If set to false, the location param will
  * not be parsed for Ognl expressions.</li>
- * 
- * <li><b>anchor</b> - Optional.  Also known as "fragment" or colloquially as 
+ *
+ * <li><b>anchor</b> - Optional.  Also known as "fragment" or colloquially as
  * "hash".  You can specify an anchor for a result.</li>
  * </ul>
- * 
+ *
  * <p>
  * This result follows the same rules from {@link StrutsResultSupport}.
  * </p>
- * 
+ * <p>

Review comment:
       Looks like this `<p>` is not needed.

##########
File path: core/src/main/java/org/apache/struts2/result/PlainTextResult.java
##########
@@ -47,7 +47,7 @@
  *  response type (eg. Content-Type=text/plain; charset=UTF-8) and when reading
  *  using a Reader. Some example of charSet would be UTF-8, ISO-8859-1 etc.
  * </ul>
- *
+ * <p>

Review comment:
       Should this be `</p>` ?

##########
File path: core/src/main/java/org/apache/struts2/result/StreamResult.java
##########
@@ -64,10 +64,10 @@
  * of evaluating the expression will be used. If not set, then no charset will be set on
  * the header</li>
  * </ul>
- * 
+ *
  * <p>These parameters can also be set by exposing a similarly named getter method on your Action.  For example, you can
  * provide <code>getContentType()</code> to override that parameter for the current action.</p>
- *
+ * <p>

Review comment:
       Looks like this `<p>` is not needed.

##########
File path: core/src/main/java/org/apache/struts2/views/util/ContextUtil.java
##########
@@ -71,22 +71,23 @@
      * @param context stack's context
      * @return boolean
      */
-    public static boolean isUseAltSyntax(Map context) {
+    public static boolean isUseAltSyntax(Map<String, Object> context) {
         // We didn't make altSyntax static cause, if so, struts.configuration.xml.reload will not work
         // plus the Configuration implementation should cache the properties, which the framework's
         // configuration implementation does
-        return "true".equals(((Container)context.get(ActionContext.CONTAINER)).getInstance(String.class, StrutsConstants.STRUTS_TAG_ALTSYNTAX)) ||(
+        String tagAltSytnax = ActionContext.of(context).getContainer().getInstance(String.class, StrutsConstants.STRUTS_TAG_ALTSYNTAX);
+        return "true".equals(tagAltSytnax) ||(

Review comment:
       Maybe an extra space before the "(" for an easier read ?
   ```
   return "true".equals(tagAltSytnax) || (
   ```

##########
File path: core/src/test/java/org/apache/struts2/views/jsp/ActionTagTest.java
##########
@@ -199,7 +199,7 @@ public void testExecuteButResetReturnSameInvocation() throws Exception {
         ActionComponent component = (ActionComponent) tag.getComponent();
 
         tag.doEndTag();
-        assertTrue(oldInvocation == ActionContext.getContext().getActionInvocation());
+         assertSame(oldInvocation, ActionContext.getContext().getActionInvocation());

Review comment:
       Might be one extra space " " in front of `assertSame()`

##########
File path: plugins/rest/src/main/java/org/apache/struts2/rest/RestWorkflowInterceptor.java
##########
@@ -62,7 +61,7 @@
  * <!-- END SNIPPET: description -->
  *
  * <p><u>Interceptor parameters:</u></p>
- *
+ * <p>

Review comment:
       It looks like this `<p>` tag and the next two below may be unintended additions ?

##########
File path: core/src/main/resources/struts-default.xml
##########
@@ -68,7 +68,6 @@
     <constant name="struts.excludedPackageNames"
               value="
                 ognl.,
-                java.io.,

Review comment:
       Was this removal intentional ?  It might be safer to keep `java.io` in the list unless its presence broke something with the changes.




----------------------------------------------------------------
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