struts-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From lukaszlen...@apache.org
Subject [struts] branch master updated: Forward port fix for WW-5024 to 2.6: - NOTE: If the PR is accepted please credit Robert Hollencamp (Github @rhollencamp) for this change as he found the issue, proposed a solution for 2.6 (master), and opened the JIRA. - Updated HttpParameters, ActionMappingParametersInterceptor to prevent multi-level Parameter wrapping from occuring. - Updated HttpParameters create(), build() and buildNoNestedWrapping() to replace redundant type operators with <> based on IDE hint. - Made HttpPara [...]
Date Mon, 01 Apr 2019 07:26:12 GMT
This is an automated email from the ASF dual-hosted git repository.

lukaszlenart pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/struts.git


The following commit(s) were added to refs/heads/master by this push:
     new 3e11e1d  Forward port fix for WW-5024 to 2.6: - NOTE: If the PR is accepted please
credit Robert Hollencamp (Github @rhollencamp) for this change         as he found the issue,
proposed a solution for 2.6 (master), and opened the JIRA. - Updated HttpParameters, ActionMappingParametersInterceptor
to prevent multi-level Parameter wrapping from occuring. - Updated HttpParameters create(),
build() and buildNoNestedWrapping() to replace redundant type operators with <> based
on IDE hi [...]
     new 91c6a6f  Merge pull request #346 from JCgH4164838Gh792C124B5/localS2_26x_B4
3e11e1d is described below

commit 3e11e1db4775a5f4f21d57f0361ea06ed06e1578
Author: JCgH4164838Gh792C124B5 <43964333+JCgH4164838Gh792C124B5@users.noreply.github.com>
AuthorDate: Sun Mar 31 23:44:40 2019 -0400

    Forward port fix for WW-5024 to 2.6:
    - NOTE: If the PR is accepted please credit Robert Hollencamp (Github @rhollencamp) for
this change
            as he found the issue, proposed a solution for 2.6 (master), and opened the JIRA.
    - Updated HttpParameters, ActionMappingParametersInterceptor to prevent multi-level Parameter
wrapping from occuring.
    - Updated HttpParameters create(), build() and buildNoNestedWrapping() to replace redundant
type operators with <> based on IDE hint.
    - Made HttpParameters parameters member final based on IDE hint.
    - Added a new ActionMappingParametersInterceptorTest to verify the fix.
---
 .../apache/struts2/dispatcher/HttpParameters.java  |  29 ++-
 .../ActionMappingParametersInterceptor.java        |  16 +-
 .../ActionMappingParametersInterceptorTest.java    | 199 +++++++++++++++++++++
 3 files changed, 233 insertions(+), 11 deletions(-)

diff --git a/core/src/main/java/org/apache/struts2/dispatcher/HttpParameters.java b/core/src/main/java/org/apache/struts2/dispatcher/HttpParameters.java
index 6bf35a9..d17b71c 100644
--- a/core/src/main/java/org/apache/struts2/dispatcher/HttpParameters.java
+++ b/core/src/main/java/org/apache/struts2/dispatcher/HttpParameters.java
@@ -33,7 +33,7 @@ import java.util.TreeSet;
 @SuppressWarnings("unchecked")
 public class HttpParameters implements Map<String, Parameter>, Cloneable {
 
-    private Map<String, Parameter> parameters;
+    final private Map<String, Parameter> parameters;
 
     private HttpParameters(Map<String, Parameter> parameters) {
         this.parameters = parameters;
@@ -44,7 +44,7 @@ public class HttpParameters implements Map<String, Parameter>, Cloneable
{
     }
 
     public static Builder create() {
-        return new Builder(new HashMap<String, Object>());
+        return new Builder(new HashMap<>());
     }
 
     public HttpParameters remove(Set<String> paramsToRemove) {
@@ -183,7 +183,7 @@ public class HttpParameters implements Map<String, Parameter>, Cloneable
{
 
         public HttpParameters build() {
             Map<String, Parameter> parameters = (parent == null)
-                    ? new HashMap<String, Parameter>()
+                    ? new HashMap<>()
                     : new HashMap<>(parent.parameters);
 
             for (Map.Entry<String, Object> entry : requestParameterMap.entrySet())
{
@@ -194,5 +194,28 @@ public class HttpParameters implements Map<String, Parameter>,
Cloneable {
 
             return new HttpParameters(parameters);
         }
+
+        /**
+        * Alternate Builder method which avoids wrapping any parameters that are already
+        * a {@link Parameter} element within another {@link Parameter} wrapper.
+        *
+        * @return 
+         */
+        public HttpParameters buildNoNestedWrapping() {
+            Map<String, Parameter> parameters = (parent == null)
+                    ? new HashMap<>()
+                    : new HashMap<>(parent.parameters);
+
+            for (Map.Entry<String, Object> entry : requestParameterMap.entrySet())
{
+                String name = entry.getKey();
+                Object value = entry.getValue();
+                Parameter parameterValue = (value instanceof Parameter)
+                        ? (Parameter) value
+                        : new Parameter.Request(name, value);
+                parameters.put(name, parameterValue);
+            }
+
+            return new HttpParameters(parameters);
+        }
     }
 }
diff --git a/core/src/main/java/org/apache/struts2/interceptor/ActionMappingParametersInterceptor.java
b/core/src/main/java/org/apache/struts2/interceptor/ActionMappingParametersInterceptor.java
index 967f748..2b04795 100644
--- a/core/src/main/java/org/apache/struts2/interceptor/ActionMappingParametersInterceptor.java
+++ b/core/src/main/java/org/apache/struts2/interceptor/ActionMappingParametersInterceptor.java
@@ -74,6 +74,8 @@ import java.util.Map;
 public class ActionMappingParametersInterceptor extends ParametersInterceptor {
 
     /**
+     * Get the parameter map from ActionMapping associated with the provided ActionContext.
+     * 
      * @param ac The action context
      * @return the parameters from the action mapping in the context.  If none found, returns
an empty map.
      */
@@ -81,27 +83,25 @@ public class ActionMappingParametersInterceptor extends ParametersInterceptor
{
     protected HttpParameters retrieveParameters(ActionContext ac) {
         ActionMapping mapping = (ActionMapping) ac.get(ServletActionContext.ACTION_MAPPING);
         if (mapping != null) {
-            return HttpParameters.create(mapping.getParams()).build();
+            return HttpParameters.create(mapping.getParams()).buildNoNestedWrapping();
         } else {
-            return HttpParameters.create().build();
+            return HttpParameters.create().buildNoNestedWrapping();
         }
     }
 
     /**
-     * Adds the parameters into context's ParameterMap
+     * Adds the parameters into the current ActionContext's parameter map.
+     *
+     * Note: The method should avoid re-wrapping values which are already of type Parameter.
      *
      * @param ac        The action context
      * @param newParams The parameter map to apply
-     *                  <p>
-     *                  In this class this is a no-op, since the parameters were fetched
from the same location.
-     *                  In subclasses both retrieveParameters() and addParametersToContext()
should be overridden.
-     *                  </p>
      */
     @Override
     protected void addParametersToContext(ActionContext ac, Map<String, ?> newParams)
{
         HttpParameters previousParams = ac.getParameters();
         HttpParameters.Builder combinedParams = HttpParameters.create().withParent(previousParams).withExtraParams(newParams);
 
-        ac.setParameters(combinedParams.build());
+        ac.setParameters(combinedParams.buildNoNestedWrapping());
     }
 }
diff --git a/core/src/test/java/org/apache/struts2/interceptor/ActionMappingParametersInterceptorTest.java
b/core/src/test/java/org/apache/struts2/interceptor/ActionMappingParametersInterceptorTest.java
new file mode 100644
index 0000000..5a102b0
--- /dev/null
+++ b/core/src/test/java/org/apache/struts2/interceptor/ActionMappingParametersInterceptorTest.java
@@ -0,0 +1,199 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *  http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.struts2.interceptor;
+
+import com.opensymphony.xwork2.ActionContext;
+import com.opensymphony.xwork2.XWorkTestCase;
+
+import org.apache.struts2.dispatcher.HttpParameters;
+import org.apache.struts2.ServletActionContext;
+import org.apache.struts2.dispatcher.Parameter;
+import org.apache.struts2.dispatcher.mapper.ActionMapping;
+
+import java.util.HashMap;
+import java.util.Map;
+
+
+/**
+ * Unit test for {@link ActionMappingParametersInterceptor}.
+ *
+ * Adapted from ParametersInterceptor (@author Jason Carreira)
+ */
+public class ActionMappingParametersInterceptorTest extends XWorkTestCase {
+
+    /**
+     * Confirm that ActionMappingParametersInterceptor processing methods avoid
+     * nested wrapping of parameters.
+     */
+    public void testParametersNoNestedWrapping() {
+        final ActionMappingParametersInterceptor ampi = createActionMappingParametersInterceptor();
+        final ActionContext ac = ActionContext.getContext();
+        final Map<String, Object> parameters = new HashMap<String, Object>()
{
+            {
+                put("fooKey", "fooValue");
+                put("barKey", "barValue");
+            }
+        };
+        final Map<String, Object> moreParameters = new HashMap<String, Object>()
{
+            {
+                put("fooKey2", "fooValue2");
+                put("barKey2", "barValue3");
+            }
+        };
+        final Map<String, Object> evenMoreParameters = new HashMap<String, Object>()
{
+            {
+                put("fooKey3", "fooValue3");
+                put("barKey3", "barValue3");
+            }
+        };
+        // Set up the initial ActionMapping with parameters in the context
+        final HttpParameters wrappedParameters = HttpParameters.create(parameters).build();
+        final ActionMapping actionMapping = new ActionMapping("testAction", "testNameSpace",
"testMethod", parameters);
+        ac.put(ServletActionContext.ACTION_MAPPING, actionMapping);
+
+        // Retrieve parameters and confirm both builder result equality and that the contained
Object
+        // values of each Parameter element are not of type Parameter (i.e. no double-wrapping).
+        // Note: ampi.retrieveParameters() only ever returns the parameters associated with
the ActionContext's
+        //   ActionMapping (not changed directly by ampi.addParametersToContext()).
+        final HttpParameters retrievedParameters = ampi.retrieveParameters(ac);
+        assertNotNull("retrievedParameters null ?", retrievedParameters);
+        // Note: Cannot perform equality test on HttpParameters directly as hashCode values
differ even
+        //   when the contents are equivalent.  Instead we compare size and content equality.
+        assertEquals("retrievedParameters size not equal to wrappedParameters size ?",
+                retrievedParameters.size(), wrappedParameters.size());
+
+        for (String parameterName : retrievedParameters.keySet() ) {
+            Parameter retrievedParameter = retrievedParameters.get(parameterName);
+            Parameter wrappedParameter = wrappedParameters.get(parameterName);
+            assertNotNull("retrievedParameter is null ?", retrievedParameter);
+            assertNotNull("wrappedParameter is null ?", wrappedParameter);
+            Object retrievedParameterValue = retrievedParameter.getObject();
+            Object wrappedParameterValue = wrappedParameter.getObject();
+            assertNotNull("retrievedParameterValue is null ?", retrievedParameterValue);
+            assertNotNull("wrappedParameterValue is null ?", wrappedParameterValue);
+            assertFalse("retrievedParameterValue is type Parameter ?", retrievedParameterValue
instanceof Parameter);
+            assertEquals("retrievedParameterValue not equal to wrappedParameterValue ?",
+                    retrievedParameterValue, wrappedParameterValue);
+        }
+
+        // Set up the initial ActionMapping with (artificially) already-wrapped parameters
in the context
+        final Map<String, Object> wrappedParametersMap = new HashMap<>(wrappedParameters.size());
+        for (String parameterName : wrappedParameters.keySet() ) {
+            wrappedParametersMap.put(parameterName, wrappedParameters.get(parameterName));
+        }
+        final ActionMapping actionMappingWrapped = new ActionMapping("testAction", "testNameSpace",
"testMethod", wrappedParametersMap);
+        ac.put(ServletActionContext.ACTION_MAPPING, actionMappingWrapped);
+
+        // Retrieve parameters and confirm that the contained Object values of each Parameter
+        // element are not of type Parameter (i.e. no double-wrapping).
+        // Note: ampi.retrieveParameters() only ever returns the parameters associated with
the ActionContext's
+        //   ActionMapping (not changed directly by ampi.addParametersToContext()).
+        final HttpParameters retrievedWrappedParameters = ampi.retrieveParameters(ac);
+        assertNotNull("retrievedWrappedParameters null ?", retrievedWrappedParameters);
+        // Note: Cannot perform equality test on HttpParameters directly as hashCode values
differ even
+        //   when the contents are equivalent.  Instead we compare size and content equality.
+        assertEquals("retrievedWrappedParameters size not equal to wrappedParametersMap size
?",
+                retrievedWrappedParameters.size(), wrappedParametersMap.size());
+
+        for (String parameterName : retrievedWrappedParameters.keySet() ) {
+            Parameter retrievedParameter = retrievedWrappedParameters.get(parameterName);
+            Object wrappedParameter = wrappedParametersMap.get(parameterName);
+            assertNotNull("retrievedParameter is null ?", retrievedParameter);
+            assertNotNull("wrappedParameter is null ?", wrappedParameter);
+            assertTrue("wrappedParameter is not a Parameter ?", wrappedParameter instanceof
Parameter);
+            Object retrievedParameterValue = retrievedParameter.getObject();
+            Object wrappedParameterValue = ((Parameter) wrappedParameter).getObject();
+            assertNotNull("retrievedParameterValue is null ?", retrievedParameterValue);
+            assertNotNull("wrappedParameterValue is null ?", wrappedParameterValue);
+            assertFalse("retrievedParameterValue is type Parameter ?", retrievedParameterValue
instanceof Parameter);
+            assertEquals("retrievedParameterValue not equal to wrappedParameterValue ?",
+                    retrievedParameterValue, wrappedParameterValue);
+        }
+
+        // Add parameters to the context
+        ampi.addParametersToContext(ac, parameters);
+
+        // Retrieve parameters and confirm the expected size and that the contained Object
+        // values of each Parameter element are not of type Parameter (i.e. no double-wrapping).
+        final HttpParameters retrievedACParameters = ac.getParameters();
+        assertNotNull("retrievedACParameters null ?", retrievedACParameters);
+        assertEquals("retrievedACParameters size not equal to parameters size ?",
+                retrievedACParameters.size(), parameters.size());
+
+        for (String parameterName : retrievedACParameters.keySet() ) {
+            Parameter retrievedACParameter = retrievedACParameters.get(parameterName);
+            assertNotNull("retrievedACParameter is null ?", retrievedACParameter);
+            Object parameterValue = retrievedACParameter.getObject();
+            assertNotNull("parameterValue is null ?", parameterValue);
+            assertFalse("parameterValue is of type Parameter ?", parameterValue instanceof
Parameter);
+        }
+
+        // Add additional parameters to the context
+        ampi.addParametersToContext(ac, moreParameters);
+
+        // Retrieve parameters and confirm the expected size and that the contained Object
+        // values of each Parameter element are not of type Parameter (i.e. no double-wrapping).
+        final HttpParameters retrievedACMoreParameters = ac.getParameters();
+        assertNotNull("retrievedACMoreParameters null ?", retrievedACMoreParameters);
+        assertEquals("retrievedACMoreParameters size not equal to combined size (parameters
+ moreParameters) ?",
+                retrievedACMoreParameters.size(), parameters.size() + moreParameters.size());
+
+        for (String parameterName : retrievedACMoreParameters.keySet() ) {
+            Parameter retrievedACMoreParameter = retrievedACMoreParameters.get(parameterName);
+            assertNotNull("retrievedACMoreParameter is null ?", retrievedACMoreParameter);
+            Object parameterValue = retrievedACMoreParameter.getObject();
+            assertNotNull("parameterValue (more) is null ?", parameterValue);
+            assertFalse("parameterValue (more) is of type Parameter ?", parameterValue instanceof
Parameter);
+        }
+
+        // Build some "already wrapped" parameters and attempt to add them to the context
+        final HttpParameters evenMoreWrappedParameters = HttpParameters.create(evenMoreParameters).build();
+        assertNotNull("evenMoreWrappedParameters null ?", evenMoreWrappedParameters);
+        assertEquals("evenMoreWrappedParameters size not equal to evenMoreParameters size
?",
+                evenMoreWrappedParameters.size(), evenMoreParameters.size());
+        ampi.addParametersToContext(ac, evenMoreWrappedParameters);
+
+        // Retrieve parameters and confirm the expected size and that the contained Object
+        // values of each Parameter element are not of type Parameter (i.e. no double-wrapping).
+        final HttpParameters retrievedACEvenMoreParameters = ac.getParameters();
+        assertNotNull("retrievedACEvenMoreParameters null ?", retrievedACEvenMoreParameters);
+        assertEquals("retrievedACEvenMoreParameters size not equal to combined size (parameters
+ moreParameters + evenMoreParameters) ?",
+                retrievedACEvenMoreParameters.size(), parameters.size() + moreParameters.size()
+ evenMoreParameters.size());
+
+        for (String parameterName : retrievedACEvenMoreParameters.keySet() ) {
+            Parameter retrievedACEvenMoreParameter = retrievedACEvenMoreParameters.get(parameterName);
+            assertNotNull("retrievedACEvenMoreParameter is null ?", retrievedACEvenMoreParameter);
+            Object parameterValue = retrievedACEvenMoreParameter.getObject();
+            assertNotNull("parameterValue (even more) is null ?", parameterValue);
+            assertFalse("parameterValue (even more) is of type Parameter ?", parameterValue
instanceof Parameter);
+        }
+    }
+
+    /**
+     * Create and configure an ActionMappingParametersInterceptor instance
+     * 
+     * @return 
+     */
+    private ActionMappingParametersInterceptor createActionMappingParametersInterceptor()
{
+        ActionMappingParametersInterceptor ampi = new ActionMappingParametersInterceptor();
+        container.inject(ampi);
+        return ampi;
+    }
+
+}


Mime
View raw message