You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@struts.apache.org by ku...@apache.org on 2024/01/03 09:55:03 UTC

(struts) branch WW-5352-parameter-annotation-2 created (now dba0c94aa)

This is an automated email from the ASF dual-hosted git repository.

kusal pushed a change to branch WW-5352-parameter-annotation-2
in repository https://gitbox.apache.org/repos/asf/struts.git


      at dba0c94aa WW-5352 Refactor ParametersInterceptor

This branch includes the following new commits:

     new dba0c94aa WW-5352 Refactor ParametersInterceptor

The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails.  The revisions
listed as "add" were already present in the repository and have only
been added to this reference.



(struts) 01/01: WW-5352 Refactor ParametersInterceptor

Posted by ku...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

kusal pushed a commit to branch WW-5352-parameter-annotation-2
in repository https://gitbox.apache.org/repos/asf/struts.git

commit dba0c94aa28d5b71137f2c94e4c62bdfb6b79230
Author: Kusal Kithul-Godage <gi...@kusal.io>
AuthorDate: Wed Jan 3 20:54:55 2024 +1100

    WW-5352 Refactor ParametersInterceptor
---
 .../xwork2/security/AcceptedPatternsChecker.java   |  12 +-
 .../xwork2/security/ExcludedPatternsChecker.java   |  12 +-
 .../java/org/apache/struts2/StrutsConstants.java   |   2 +
 .../org/apache/struts2/dispatcher/Parameter.java   |   8 +-
 .../ActionMappingParametersInterceptor.java        |   6 +-
 .../parameter/ParametersInterceptor.java           | 247 +++++++++++----------
 6 files changed, 148 insertions(+), 139 deletions(-)

diff --git a/core/src/main/java/com/opensymphony/xwork2/security/AcceptedPatternsChecker.java b/core/src/main/java/com/opensymphony/xwork2/security/AcceptedPatternsChecker.java
index f5a329459..4af56ff9e 100644
--- a/core/src/main/java/com/opensymphony/xwork2/security/AcceptedPatternsChecker.java
+++ b/core/src/main/java/com/opensymphony/xwork2/security/AcceptedPatternsChecker.java
@@ -32,37 +32,37 @@ public interface AcceptedPatternsChecker {
      * @param value to check
      * @return object containing result of matched pattern and pattern itself
      */
-    public IsAccepted isAccepted(String value);
+    IsAccepted isAccepted(String value);
 
     /**
      * Sets excluded patterns during runtime
      *
      * @param commaDelimitedPatterns comma delimited string with patterns
      */
-    public void setAcceptedPatterns(String commaDelimitedPatterns);
+    void setAcceptedPatterns(String commaDelimitedPatterns);
 
     /**
      * Set excluded patterns during runtime
      *
      * @param patterns array of additional excluded patterns
      */
-    public void setAcceptedPatterns(String[] patterns);
+    void setAcceptedPatterns(String[] patterns);
 
     /**
      * Sets excluded patterns during runtime
      *
      * @param patterns set of additional patterns
      */
-    public void setAcceptedPatterns(Set<String> patterns);
+    void setAcceptedPatterns(Set<String> patterns);
 
     /**
      * Allow access list of all defined excluded patterns
      *
      * @return set of excluded patterns
      */
-    public Set<Pattern> getAcceptedPatterns();
+    Set<Pattern> getAcceptedPatterns();
 
-    public final static class IsAccepted {
+    final class IsAccepted {
 
         private final boolean accepted;
         private final String acceptedPattern;
diff --git a/core/src/main/java/com/opensymphony/xwork2/security/ExcludedPatternsChecker.java b/core/src/main/java/com/opensymphony/xwork2/security/ExcludedPatternsChecker.java
index 6fa54d0d4..086c75d0b 100644
--- a/core/src/main/java/com/opensymphony/xwork2/security/ExcludedPatternsChecker.java
+++ b/core/src/main/java/com/opensymphony/xwork2/security/ExcludedPatternsChecker.java
@@ -32,37 +32,37 @@ public interface ExcludedPatternsChecker {
      * @param value to check
      * @return object containing result of matched pattern and pattern itself
      */
-    public IsExcluded isExcluded(String value);
+    IsExcluded isExcluded(String value);
 
     /**
      * Sets excluded patterns during runtime
      *
      * @param commaDelimitedPatterns comma delimited string with patterns
      */
-    public void setExcludedPatterns(String commaDelimitedPatterns);
+    void setExcludedPatterns(String commaDelimitedPatterns);
 
     /**
      * Sets excluded patterns during runtime
      *
      * @param patterns array of additional excluded patterns
      */
-    public void setExcludedPatterns(String[] patterns);
+    void setExcludedPatterns(String[] patterns);
 
     /**
      * Sets excluded patterns during runtime
      *
      * @param patterns set of additional patterns
      */
-    public void setExcludedPatterns(Set<String> patterns);
+    void setExcludedPatterns(Set<String> patterns);
 
     /**
      * Allow access list of all defined excluded patterns
      *
      * @return set of excluded patterns
      */
-    public Set<Pattern> getExcludedPatterns();
+    Set<Pattern> getExcludedPatterns();
 
-    public final static class IsExcluded {
+    final class IsExcluded {
 
         private final boolean excluded;
         private final String excludedPattern;
diff --git a/core/src/main/java/org/apache/struts2/StrutsConstants.java b/core/src/main/java/org/apache/struts2/StrutsConstants.java
index caddee16d..5feebd0c3 100644
--- a/core/src/main/java/org/apache/struts2/StrutsConstants.java
+++ b/core/src/main/java/org/apache/struts2/StrutsConstants.java
@@ -463,6 +463,8 @@ public final class StrutsConstants {
     public static final String STRUTS_ADDITIONAL_EXCLUDED_PATTERNS = "struts.additional.excludedPatterns";
     public static final String STRUTS_ADDITIONAL_ACCEPTED_PATTERNS = "struts.additional.acceptedPatterns";
 
+    public static final String STRUTS_PARAMETERS_REQUIRE_ANNOTATIONS = "struts.parameters.requireAnnotations";
+
     public static final String STRUTS_CONTENT_TYPE_MATCHER = "struts.contentTypeMatcher";
 
     public static final String STRUTS_SMI_METHOD_REGEX = "struts.strictMethodInvocation.methodRegex";
diff --git a/core/src/main/java/org/apache/struts2/dispatcher/Parameter.java b/core/src/main/java/org/apache/struts2/dispatcher/Parameter.java
index edae6c3a1..06aa6783f 100644
--- a/core/src/main/java/org/apache/struts2/dispatcher/Parameter.java
+++ b/core/src/main/java/org/apache/struts2/dispatcher/Parameter.java
@@ -18,12 +18,12 @@
  */
 package org.apache.struts2.dispatcher;
 
-import java.util.Objects;
-
 import org.apache.commons.text.StringEscapeUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 
+import java.util.Objects;
+
 public interface Parameter {
 
     String getName();
@@ -58,7 +58,7 @@ public interface Parameter {
         @Override
         public String getValue() {
             String[] values = toStringArray();
-            return (values != null && values.length > 0) ? values[0] : null;
+            return values.length > 0 ? values[0] : null;
         }
 
         private String[] toStringArray() {
@@ -124,7 +124,7 @@ public interface Parameter {
 
     class Empty implements Parameter {
 
-        private String name;
+        private final String name;
 
         public Empty(String name) {
             this.name = name;
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 ac9d41708..ecb1f7f9f 100644
--- a/core/src/main/java/org/apache/struts2/interceptor/ActionMappingParametersInterceptor.java
+++ b/core/src/main/java/org/apache/struts2/interceptor/ActionMappingParametersInterceptor.java
@@ -76,12 +76,12 @@ public class ActionMappingParametersInterceptor extends ParametersInterceptor {
     /**
      * Get the parameter map from ActionMapping associated with the provided ActionContext.
      *
-     * @param ac The action context
+     * @param actionContext The action context
      * @return the parameters from the action mapping in the context.  If none found, returns an empty map.
      */
     @Override
-    protected HttpParameters retrieveParameters(ActionContext ac) {
-        ActionMapping mapping = ac.getActionMapping();
+    protected HttpParameters retrieveParameters(ActionContext actionContext) {
+        ActionMapping mapping = actionContext.getActionMapping();
         if (mapping != null) {
             return HttpParameters.create(mapping.getParams()).buildNoNestedWrapping();
         } else {
diff --git a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java
index c55f97a6a..d1750b5f9 100644
--- a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java
+++ b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java
@@ -43,7 +43,6 @@ import org.apache.struts2.dispatcher.HttpParameters;
 import org.apache.struts2.dispatcher.Parameter;
 
 import java.util.Collection;
-import java.util.Collections;
 import java.util.Comparator;
 import java.util.HashSet;
 import java.util.Map;
@@ -51,6 +50,8 @@ import java.util.Set;
 import java.util.TreeMap;
 import java.util.regex.Pattern;
 
+import static java.util.Collections.unmodifiableSet;
+import static java.util.stream.Collectors.joining;
 import static org.apache.commons.lang3.StringUtils.normalizeSpace;
 
 /**
@@ -75,6 +76,7 @@ public class ParametersInterceptor extends MethodFilterInterceptor {
     private AcceptedPatternsChecker acceptedPatterns;
     private Set<Pattern> excludedValuePatterns = null;
     private Set<Pattern> acceptedValuePatterns = null;
+    private boolean requireAnnotations = false;
 
     @Inject
     public void setValueStackFactory(ValueStackFactory valueStackFactory) {
@@ -132,138 +134,149 @@ public class ParametersInterceptor extends MethodFilterInterceptor {
     @Override
     public String doIntercept(ActionInvocation invocation) throws Exception {
         Object action = invocation.getAction();
-        if (!(action instanceof NoParameters)) {
-            ActionContext ac = invocation.getInvocationContext();
-            HttpParameters parameters = retrieveParameters(ac);
+        if (action instanceof NoParameters) {
+            return invocation.invoke();
+        }
 
-            if (LOG.isDebugEnabled()) {
-                LOG.debug("Setting params {}", normalizeSpace(getParameterLogMap(parameters)));
-            }
+        ActionContext actionContext = invocation.getInvocationContext();
+        HttpParameters parameters = retrieveParameters(actionContext);
 
-            if (parameters != null) {
-                Map<String, Object> contextMap = ac.getContextMap();
-                try {
-                    ReflectionContextState.setCreatingNullObjects(contextMap, true);
-                    ReflectionContextState.setDenyMethodExecution(contextMap, true);
-                    ReflectionContextState.setReportingConversionErrors(contextMap, true);
-
-                    ValueStack stack = ac.getValueStack();
-                    setParameters(action, stack, parameters);
-                } finally {
-                    ReflectionContextState.setCreatingNullObjects(contextMap, false);
-                    ReflectionContextState.setDenyMethodExecution(contextMap, false);
-                    ReflectionContextState.setReportingConversionErrors(contextMap, false);
-                }
-            }
+        if (parameters == null) {
+            return invocation.invoke();
         }
+
+        if (LOG.isDebugEnabled()) {
+            LOG.debug("Setting params {}", normalizeSpace(getParameterLogMap(parameters)));
+        }
+
+        Map<String, Object> contextMap = actionContext.getContextMap();
+        batchSetReflectionContextState(contextMap, true);
+        try {
+            setParameters(action, actionContext.getValueStack(), parameters);
+        } finally {
+            batchSetReflectionContextState(contextMap, false);
+        }
+
         return invocation.invoke();
     }
 
     /**
      * Gets the parameter map to apply from wherever appropriate
      *
-     * @param ac The action context
+     * @param actionContext The action context
      * @return The parameter map to apply
      */
-    protected HttpParameters retrieveParameters(ActionContext ac) {
-        return ac.getParameters();
+    protected HttpParameters retrieveParameters(ActionContext actionContext) {
+        return actionContext.getParameters();
     }
 
 
     /**
      * Adds the parameters into context's ParameterMap
+     * <p>
+     * In this class this is a no-op, since the parameters were fetched from the same location. In subclasses both this
+     * and {@link #retrieveParameters} should be overridden.
      *
      * @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>
      */
     protected void addParametersToContext(ActionContext ac, Map<String, ?> newParams) {
     }
 
     protected void setParameters(final Object action, ValueStack stack, HttpParameters parameters) {
-        HttpParameters params;
-        Map<String, Parameter> acceptableParameters;
-        if (ordered) {
-            params = HttpParameters.create().withComparator(getOrderedComparator()).withParent(parameters).build();
-            acceptableParameters = new TreeMap<>(getOrderedComparator());
-        } else {
-            params = HttpParameters.create().withParent(parameters).build();
-            acceptableParameters = new TreeMap<>();
-        }
+        Map<String, Parameter> acceptableParameters = toAcceptableParameters(parameters, action);
 
-        for (Map.Entry<String, Parameter> entry : params.entrySet()) {
-            String parameterName = entry.getKey();
-            boolean isAcceptableParameter = isAcceptableParameter(parameterName, action);
-            isAcceptableParameter &= isAcceptableParameterValue(entry.getValue(), action);
+        ValueStack newStack = toNewStack(stack);
+        batchSetReflectionContextState(newStack.getContext(), true);
+        setMemberAccessProperties(newStack);
 
-            if (isAcceptableParameter) {
-                acceptableParameters.put(parameterName, entry.getValue());
-            }
+        setParametersOnStack(newStack, acceptableParameters);
+
+        if (newStack instanceof ClearableValueStack) {
+            stack.getActionContext().withConversionErrors(newStack.getActionContext().getConversionErrors());
         }
 
+        addParametersToContext(ActionContext.getContext(), acceptableParameters);
+    }
+
+    protected void batchSetReflectionContextState(Map<String, Object> context, boolean value) {
+        ReflectionContextState.setCreatingNullObjects(context, value);
+        ReflectionContextState.setDenyMethodExecution(context, value);
+        ReflectionContextState.setReportingConversionErrors(context, value);
+    }
+
+    protected ValueStack toNewStack(ValueStack stack) {
         ValueStack newStack = valueStackFactory.createValueStack(stack);
-        boolean clearableStack = newStack instanceof ClearableValueStack;
-        if (clearableStack) {
-            //if the stack's context can be cleared, do that to prevent OGNL
-            //from having access to objects in the stack, see XW-641
+        if (newStack instanceof ClearableValueStack) {
             ((ClearableValueStack) newStack).clearContextValues();
-            Map<String, Object> context = newStack.getContext();
-            ReflectionContextState.setCreatingNullObjects(context, true);
-            ReflectionContextState.setDenyMethodExecution(context, true);
-            ReflectionContextState.setReportingConversionErrors(context, true);
-
-            //keep locale from original context
             newStack.getActionContext().withLocale(stack.getActionContext().getLocale()).withValueStack(stack);
         }
+        return newStack;
+    }
+
+    protected void setMemberAccessProperties(ValueStack stack) {
+        if (!(stack instanceof MemberAccessValueStack)) {
+            return;
+        }
+        ((MemberAccessValueStack) stack).useAcceptProperties(acceptedPatterns.getAcceptedPatterns());
+        ((MemberAccessValueStack) stack).useExcludeProperties(excludedPatterns.getExcludedPatterns());
+    }
+
+    protected Map<String, Parameter> toAcceptableParameters(HttpParameters parameters, Object action) {
+        HttpParameters newParams = initNewHttpParameters(parameters);
+        Map<String, Parameter> acceptableParameters = initParameterMap();
+
+        for (Map.Entry<String, Parameter> entry : newParams.entrySet()) {
+            String parameterName = entry.getKey();
+            Parameter parameterValue = entry.getValue();
+            if (isAcceptableParameter(parameterName, action) && isAcceptableParameterValue(parameterValue, action)) {
+                acceptableParameters.put(parameterName, parameterValue);
+            }
+        }
+        return acceptableParameters;
+    }
 
-        boolean memberAccessStack = newStack instanceof MemberAccessValueStack;
-        if (memberAccessStack) {
-            //block or allow access to properties
-            //see WW-2761 for more details
-            MemberAccessValueStack accessValueStack = (MemberAccessValueStack) newStack;
-            accessValueStack.useAcceptProperties(acceptedPatterns.getAcceptedPatterns());
-            accessValueStack.useExcludeProperties(excludedPatterns.getExcludedPatterns());
+    protected Map<String, Parameter> initParameterMap() {
+        if (ordered) {
+            return new TreeMap<>(getOrderedComparator());
+        } else {
+            return new TreeMap<>();
         }
+    }
 
-        for (Map.Entry<String, Parameter> entry : acceptableParameters.entrySet()) {
-            String name = entry.getKey();
-            Parameter value = entry.getValue();
+    protected HttpParameters initNewHttpParameters(HttpParameters parameters) {
+        if (ordered) {
+            return HttpParameters.create().withComparator(getOrderedComparator()).withParent(parameters).build();
+        } else {
+            return HttpParameters.create().withParent(parameters).build();
+        }
+    }
+
+    protected void setParametersOnStack(ValueStack stack, Map<String, Parameter> parameters) {
+        for (Map.Entry<String, Parameter> entry : parameters.entrySet()) {
             try {
-                newStack.setParameter(name, value.getObject());
+                stack.setParameter(entry.getKey(), entry.getValue().getObject());
             } catch (RuntimeException e) {
                 if (devMode) {
-                    notifyDeveloperParameterException(action, name, e.getMessage());
+                    notifyDeveloperParameterException(stack.peek(), entry.getKey(), e.getMessage());
                 }
             }
         }
-
-        if (clearableStack) {
-            stack.getActionContext().withConversionErrors(newStack.getActionContext().getConversionErrors());
-        }
-
-        addParametersToContext(ActionContext.getContext(), acceptableParameters);
     }
 
     protected void notifyDeveloperParameterException(Object action, String property, String message) {
-        String developerNotification = "Unexpected Exception caught setting '" + property + "' on '" + action.getClass() + ": " + message;
+        String logMsg = "Unexpected Exception caught setting '" + property + "' on '" + action.getClass() + ": " + message;
         if (action instanceof TextProvider) {
             TextProvider tp = (TextProvider) action;
-            developerNotification = tp.getText("devmode.notification",
-                "Developer Notification:\n{0}",
-                new String[]{developerNotification}
-            );
+            logMsg = tp.getText("devmode.notification", "Developer Notification:\n{0}", new String[]{logMsg});
         }
-
-        LOG.error(developerNotification);
+        LOG.error(logMsg);
 
         if (action instanceof ValidationAware) {
-            // see https://issues.apache.org/jira/browse/WW-4066
-            Collection<String> messages = ((ValidationAware) action).getActionMessages();
+            ValidationAware validationAware = (ValidationAware) action;
+            Collection<String> messages = validationAware.getActionMessages();
             messages.add(message);
-            ((ValidationAware) action).setActionMessages(messages);
+            validationAware.setActionMessages(messages);
         }
     }
 
@@ -277,6 +290,11 @@ public class ParametersInterceptor extends MethodFilterInterceptor {
     protected boolean isAcceptableParameter(String name, Object action) {
         ParameterNameAware parameterNameAware = (action instanceof ParameterNameAware) ? (ParameterNameAware) action : null;
         return acceptableName(name) && (parameterNameAware == null || parameterNameAware.acceptableParameterName(name));
+        return acceptableName(name) && isAcceptableParameterNameAware(name, action);
+    }
+
+    protected boolean isAcceptableParameterNameAware(String name, Object action) {
+        return (!(action instanceof ParameterNameAware) || ((ParameterNameAware) action).acceptableParameterName(name));
     }
 
     /**
@@ -287,13 +305,9 @@ public class ParametersInterceptor extends MethodFilterInterceptor {
      * @return true if parameter is accepted
      */
     protected boolean isAcceptableParameterValue(Parameter param, Object action) {
-        ParameterValueAware parameterValueAware = (action instanceof ParameterValueAware) ? (ParameterValueAware) action : null;
-        boolean acceptableParamValue = (parameterValueAware == null || parameterValueAware.acceptableParameterValue(param.getValue()));
-        if (hasParamValuesToExclude() || hasParamValuesToAccept()) {
-            // Additional validations to process
-            acceptableParamValue &= acceptableValue(param.getName(), param.getValue());
-        }
-        return acceptableParamValue;
+        return (!(action instanceof ParameterValueAware)
+                || ((ParameterValueAware) action).acceptableParameterValue(param.getValue()))
+                && acceptableValue(param.getName(), param.getValue());
     }
 
     /**
@@ -311,16 +325,9 @@ public class ParametersInterceptor extends MethodFilterInterceptor {
         if (parameters == null) {
             return "NONE";
         }
-
-        StringBuilder logEntry = new StringBuilder();
-        for (Map.Entry<String, Parameter> entry : parameters.entrySet()) {
-            logEntry.append(entry.getKey());
-            logEntry.append(" => ");
-            logEntry.append(entry.getValue().getValue());
-            logEntry.append(" ");
-        }
-
-        return logEntry.toString();
+        return parameters.entrySet().stream()
+                .map(entry -> String.format("%s => %s ", entry.getKey(), entry.getValue().getValue()))
+                .collect(joining());
     }
 
     /**
@@ -338,18 +345,17 @@ public class ParametersInterceptor extends MethodFilterInterceptor {
             return false;
         }
         boolean accepted = isWithinLengthLimit(name) && !isExcluded(name) && isAccepted(name);
-        if (devMode && accepted) { // notify only when in devMode
+        if (devMode && accepted) {
             LOG.debug("Parameter [{}] was accepted and will be appended to action!", name);
         }
         return accepted;
     }
 
     private boolean isIgnoredDMI(String name) {
-        if (dmiEnabled) {
-            return DMI_IGNORED_PATTERN.matcher(name).matches();
-        } else {
+        if (!dmiEnabled) {
             return false;
         }
+        return DMI_IGNORED_PATTERN.matcher(name).matches();
     }
 
     /**
@@ -363,7 +369,7 @@ public class ParametersInterceptor extends MethodFilterInterceptor {
      * @return true if accepted
      */
     protected boolean acceptableValue(String name, String value) {
-        boolean accepted = (value == null || value.isEmpty() || (!isParamValueExcluded(value) && isParamValueAccepted(value)));
+        boolean accepted = value == null || value.isEmpty() || (!isParamValueExcluded(value) && isParamValueAccepted(value));
         if (!accepted) {
             String message = "Value [{}] of parameter [{}] was not accepted and will be dropped!";
             if (devMode) {
@@ -378,7 +384,7 @@ public class ParametersInterceptor extends MethodFilterInterceptor {
     protected boolean isWithinLengthLimit(String name) {
         boolean matchLength = name.length() <= paramNameMaxLength;
         if (!matchLength) {
-            if (devMode) { // warn only when in devMode
+            if (devMode) {
                 LOG.warn("Parameter [{}] is too long, allowed length is [{}]. Use Interceptor Parameter Overriding " +
                         "to override the limit, see more at\n" +
                         "https://struts.apache.org/core-developers/interceptors.html#interceptor-parameter-overriding",
@@ -392,22 +398,23 @@ public class ParametersInterceptor extends MethodFilterInterceptor {
 
     protected boolean isAccepted(String paramName) {
         AcceptedPatternsChecker.IsAccepted result = acceptedPatterns.isAccepted(paramName);
-        if (result.isAccepted()) {
-            return true;
-        } else if (devMode) { // warn only when in devMode
-            LOG.warn("Parameter [{}] didn't match accepted pattern [{}]! See Accepted / Excluded patterns at\n" +
-                    "https://struts.apache.org/security/#accepted--excluded-patterns",
-                paramName, result.getAcceptedPattern());
-        } else {
-            LOG.debug("Parameter [{}] didn't match accepted pattern [{}]!", paramName, result.getAcceptedPattern());
+        if (!result.isAccepted()) {
+            if (devMode) {
+                LOG.warn("Parameter [{}] didn't match accepted pattern [{}]! See Accepted / Excluded patterns at\n" +
+                                "https://struts.apache.org/security/#accepted--excluded-patterns",
+                        paramName, result.getAcceptedPattern());
+            } else {
+                LOG.debug("Parameter [{}] didn't match accepted pattern [{}]!", paramName, result.getAcceptedPattern());
+            }
+            return false;
         }
-        return false;
+        return true;
     }
 
     protected boolean isExcluded(String paramName) {
         ExcludedPatternsChecker.IsExcluded result = excludedPatterns.isExcluded(paramName);
         if (result.isExcluded()) {
-            if (devMode) { // warn only when in devMode
+            if (devMode) {
                 LOG.warn("Parameter [{}] matches excluded pattern [{}]! See Accepted / Excluded patterns at\n" +
                         "https://struts.apache.org/security/#accepted--excluded-patterns",
                     paramName, result.getExcludedPattern());
@@ -460,11 +467,11 @@ public class ParametersInterceptor extends MethodFilterInterceptor {
     }
 
     private boolean hasParamValuesToExclude() {
-        return excludedValuePatterns != null && excludedValuePatterns.size() > 0;
+        return excludedValuePatterns != null && !excludedValuePatterns.isEmpty();
     }
 
     private boolean hasParamValuesToAccept() {
-        return acceptedValuePatterns != null && acceptedValuePatterns.size() > 0;
+        return acceptedValuePatterns != null && !acceptedValuePatterns.isEmpty();
     }
 
     /**
@@ -530,7 +537,7 @@ public class ParametersInterceptor extends MethodFilterInterceptor {
                 acceptedValuePatterns.add(Pattern.compile(pattern, Pattern.CASE_INSENSITIVE));
             }
         } finally {
-            acceptedValuePatterns = Collections.unmodifiableSet(acceptedValuePatterns);
+            acceptedValuePatterns = unmodifiableSet(acceptedValuePatterns);
         }
     }
 
@@ -555,7 +562,7 @@ public class ParametersInterceptor extends MethodFilterInterceptor {
                 excludedValuePatterns.add(Pattern.compile(pattern, Pattern.CASE_INSENSITIVE));
             }
         } finally {
-            excludedValuePatterns = Collections.unmodifiableSet(excludedValuePatterns);
+            excludedValuePatterns = unmodifiableSet(excludedValuePatterns);
         }
     }
 }