From commits-return-18355-archive-asf-public=cust-asf.ponee.io@struts.apache.org Mon Apr 1 07:26:15 2019 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by mx-eu-01.ponee.io (Postfix) with SMTP id 3C1D8180621 for ; Mon, 1 Apr 2019 09:26:14 +0200 (CEST) Received: (qmail 53843 invoked by uid 500); 1 Apr 2019 07:26:13 -0000 Mailing-List: contact commits-help@struts.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@struts.apache.org Delivered-To: mailing list commits@struts.apache.org Received: (qmail 53834 invoked by uid 99); 1 Apr 2019 07:26:13 -0000 Received: from ec2-52-202-80-70.compute-1.amazonaws.com (HELO gitbox.apache.org) (52.202.80.70) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 01 Apr 2019 07:26:13 +0000 Received: by gitbox.apache.org (ASF Mail Server at gitbox.apache.org, from userid 33) id 68D5885C38; Mon, 1 Apr 2019 07:26:12 +0000 (UTC) Date: Mon, 01 Apr 2019 07:26:12 +0000 To: "commits@struts.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 [...] MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Message-ID: <155410357215.14213.9355679893083703872@gitbox.apache.org> From: lukaszlenart@apache.org X-Git-Host: gitbox.apache.org X-Git-Repo: struts X-Git-Refname: refs/heads/master X-Git-Reftype: branch X-Git-Oldrev: f251d1c737c5f2ee2e645a35da8615bbd1245700 X-Git-Newrev: 3e11e1db4775a5f4f21d57f0361ea06ed06e1578 X-Git-Rev: 3e11e1db4775a5f4f21d57f0361ea06ed06e1578 X-Git-NotificationType: ref_changed_plus_diff X-Git-Multimail-Version: 1.5.dev Auto-Submitted: auto-generated 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, Cloneable { - private Map parameters; + final private Map parameters; private HttpParameters(Map parameters) { this.parameters = parameters; @@ -44,7 +44,7 @@ public class HttpParameters implements Map, Cloneable { } public static Builder create() { - return new Builder(new HashMap()); + return new Builder(new HashMap<>()); } public HttpParameters remove(Set paramsToRemove) { @@ -183,7 +183,7 @@ public class HttpParameters implements Map, Cloneable { public HttpParameters build() { Map parameters = (parent == null) - ? new HashMap() + ? new HashMap<>() : new HashMap<>(parent.parameters); for (Map.Entry entry : requestParameterMap.entrySet()) { @@ -194,5 +194,28 @@ public class HttpParameters implements Map, 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 parameters = (parent == null) + ? new HashMap<>() + : new HashMap<>(parent.parameters); + + for (Map.Entry 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 - *

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

*/ @Override protected void addParametersToContext(ActionContext ac, Map 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 parameters = new HashMap() { + { + put("fooKey", "fooValue"); + put("barKey", "barValue"); + } + }; + final Map moreParameters = new HashMap() { + { + put("fooKey2", "fooValue2"); + put("barKey2", "barValue3"); + } + }; + final Map evenMoreParameters = new HashMap() { + { + 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 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; + } + +}