kusalk commented on code in PR #831:
URL: https://github.com/apache/struts/pull/831#discussion_r1441161370
##########
core/src/main/java/org/apache/struts2/dispatcher/Parameter.java:
##########
@@ -58,7 +58,7 @@ public String getName() {
@Override
public String getValue() {
String[] values = toStringArray();
- return (values != null && values.length > 0) ? values[0] : null;
Review Comment:
`#toStringArray` never returns a null value
##########
core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java:
##########
@@ -132,138 +133,149 @@ static private int countOGNLCharacters(String s) {
@Override
public String doIntercept(ActionInvocation invocation) throws Exception {
Review Comment:
Used early returns to reduce nesting and improve readability
##########
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);
Review Comment:
Redundant in interfaces
##########
core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java:
##########
@@ -132,138 +133,149 @@ static private int countOGNLCharacters(String s) {
@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) {
Review Comment:
Extracted a bunch of helper methods to make this method much more readable
##########
core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java:
##########
@@ -338,18 +344,17 @@ protected boolean acceptableName(String name) {
return false;
}
boolean accepted = isWithinLengthLimit(name) && !isExcluded(name) &&
isAccepted(name);
- if (devMode && accepted) { // notify only when in devMode
Review Comment:
Cleaned out comments for behaviours which are fairly obvious from code
##########
core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java:
##########
@@ -287,13 +302,11 @@ protected boolean isAcceptableParameter(String name,
Object action) {
* @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());
Review Comment:
`&=` doesn't short-circuit
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
