[
https://issues.apache.org/jira/browse/WW-5065?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17091015#comment-17091015
]
ASF GitHub Bot commented on WW-5065:
------------------------------------
JCgH4164838Gh792C124B5 commented on a change in pull request #405:
URL: https://github.com/apache/struts/pull/405#discussion_r414181934
##########
File path:
core/src/main/java/com/opensymphony/xwork2/config/impl/AbstractMatcher.java
##########
@@ -36,6 +36,9 @@
* @since 2.1
*/
public abstract class AbstractMatcher<E> implements Serializable {
+
+ private static final Logger LOG =
LogManager.getLogger(AbstractMatcher.class);
Review comment:
This looks like it introduces a second private `Logger` reference ?
There is already:
```
private static final Logger log =
LogManager.getLogger(AbstractMatcher.class);
```
Maybe the intention was to rename `log` to `LOG` and update all the logger
calls ?
##########
File path: core/src/main/java/org/apache/struts2/StrutsConstants.java
##########
@@ -341,4 +341,7 @@
public static final String STRUTS_DISALLOW_PROXY_MEMBER_ACCESS =
"struts.disallowProxyMemberAccess";
public static final String STRUTS_OGNL_AUTO_GROWTH_COLLECTION_LIMIT =
"struts.ognl.autoGrowthCollectionLimit";
+
+ /** See {@link
com.opensymphony.xwork2.config.impl.AbstractMatcher#appendNamedParameters */
+ public static final String STRUTS_MATCHER_APPEND_NAMED_PARAMETERS = "";
Review comment:
Looks like the constant is currently `""` (empty string). Was that
intentional, or should it be changed to:
```
public static final String STRUTS_MATCHER_APPEND_NAMED_PARAMETERS =
"struts.matcher.appendNamedParameters";
```
which was the name Lukasz favoured in PR #400.
##########
File path:
core/src/main/java/com/opensymphony/xwork2/config/impl/AbstractMatcher.java
##########
@@ -50,10 +53,23 @@
* <p> The compiled patterns and their associated target objects </p>
*/
List<Mapping<E>> compiledPatterns = new ArrayList<>();
- ;
+
+ /**
+ * This flag controls if passed named params should be appended
+ * to the map in {@link #replaceParameters(Map, Map)}
+ * and will be accessible in {@link
com.opensymphony.xwork2.config.entities.ResultConfig}.
+ * If set to false, the named parameters won't be appended.
+ *
+ * This behaviour is controlled by {@link
org.apache.struts2.StrutsConstants#STRUTS_MATCHER_APPEND_NAMED_PARAMETERS}
+ *
+ * @since 2.5.23
+ * See WW-5065
+ */
+ private final boolean appendNamedParameters;
- public AbstractMatcher(PatternMatcher<?> helper) {
+ public AbstractMatcher(PatternMatcher<?> helper, boolean
appendNamedParameters) {
Review comment:
Because the original method
```
public AbstractMatcher(PatternMatcher<?> helper)
```
is public, people may have extended this class, which would then break their
code unexpectedly with this change. Maybe it would be a good idea to add keep
the old method signature present with something like:
```
public AbstractMatcher(PatternMatcher<?> helper) {
this(helper, true); // Set boolean to whatever default setting is
decided on
}
```
That should preserve backward-compatbility.
----------------------------------------------------------------
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:
us...@infra.apache.org
> AbstractMatcher adds values to the map passed into replaceParameters
> --------------------------------------------------------------------
>
> Key: WW-5065
> URL: https://issues.apache.org/jira/browse/WW-5065
> Project: Struts 2
> Issue Type: Bug
> Affects Versions: 2.5.22
> Reporter: Alex Kaiser
> Priority: Minor
> Fix For: 2.5.23, 2.6
>
>
> There is a bug with the AbstractMatcher#replaceParameters method in
> struts/core/src/main/java/com/opensymphony/xwork2/config/impl/AbstractMatcher.java
> (currently lines 153-170). As the function currently works it will return a
> map that has more keys than the "orig" map that is passed into it. For
> example, assume that I have the following config defined in my struts.xml
> file:
> {code:java}
> <package name="test" namespace="/test">
> <action name="{paramOne}/{paramTwo} class="org.MyActionClass"
> method="execute">
> <result name="success" type="stream">
> <param name="inputName">random</param>
> </result>
> </action>
> </package>{code}
> If you send a request to "/test/uno/dos", this will trigger code in
> ActionConfigMatcher (lines 95-103) that will construct the ResultConfig
> objects to be used later on. At one point you are going to be making a call
> to AbstractMatcher#replaceParameters with something that looks like the
> following:
> orig:
> "inputName" -> "random"
> vars:
> "0" -> "uno/dos"
> "paramOne" -> "uno"
> "1" -> "uno"
> "paramTwo" -> "dos"
> "2" -> "dos"
> The result of this will be a map that looks like:
> "inputName" -> "random"
> "paramOne" -> "uno"
> "paramTwo" -> "dos"
> The bug is that "paramOne" and "paramTwo" should not be in the return map.
> For the most part this but won't cause any problems but it will cause some
> performance problems in certain situations when trying to set these
> parameters on the results objects that aren't expecting them.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
