This is an automated email from the ASF dual-hosted git repository.
mthl pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
commit 7508917a0af0311694c37cf1ddd4231a0c8b3529
Author: Mathieu Lirzin <mathieu.lir...@nereide.fr>
AuthorDate: Sun Dec 29 16:29:01 2019 +0100
Improved: Remove ‘RequestHandler#ControllerConfig’ wrapper
(OFBIZ-11313)
‘RequestHandler#ControllerConfig’ was a wrapper around
‘ConfigXMLReader#ControllerConfig’ used as a convenience to gather
error handling when accessing configuration properties. Since the
later is now only throwing exceptions when instantiating the object,
‘RequestHandler#ControllerConfig’ is not useful anymore.
---
build.gradle | 2 +-
.../ofbiz/webapp/control/ConfigXMLReader.java | 11 +-
.../ofbiz/webapp/control/RequestHandler.java | 128 +++++----------------
.../webapp/event/ServiceMultiEventHandler.java | 7 +-
.../ofbiz/webapp/control/RequestHandlerTests.java | 7 +-
.../java/org/apache/ofbiz/widget/WidgetWorker.java | 9 +-
6 files changed, 45 insertions(+), 119 deletions(-)
diff --git a/build.gradle b/build.gradle
index 34bd3be..09ead78 100644
--- a/build.gradle
+++ b/build.gradle
@@ -285,7 +285,7 @@ checkstyle {
// the sum of errors that were present before introducing the
// ‘checkstyle’ tool present in the framework and in the official
// plugins.
- tasks.checkstyleMain.maxErrors = 37725
+ tasks.checkstyleMain.maxErrors = 37713
// Currently there are a lot of errors so we need to temporarily
// hide them to avoid polluting the terminal output.
showViolations = false
diff --git
a/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java
b/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java
index fddb48b..0802582 100644
---
a/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java
+++
b/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java
@@ -182,6 +182,9 @@ public class ConfigXMLReader {
}
public static class ControllerConfig {
+ private static final String DEFAULT_REDIRECT_STATUS_CODE =
+ UtilProperties.getPropertyValue("requestHandler",
"status-code", "302");
+
public URL url;
private String errorpage;
private String protectView;
@@ -299,8 +302,14 @@ public class ConfigXMLReader {
return getIncludes(ccfg -> ccfg.securityClass);
}
+ /**
+ * Provides the status code that should be used when redirecting an
HTTP client.
+ *
+ * @return an HTTP response status code.
+ */
public String getStatusCode() {
- return getIncludes(ccfg -> ccfg.statusCode);
+ String status = getIncludes(ccfg -> ccfg.statusCode);
+ return UtilValidate.isEmpty(status) ? DEFAULT_REDIRECT_STATUS_CODE
: status;
}
public Map<String, String> getViewHandlerMap() {
diff --git
a/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/RequestHandler.java
b/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/RequestHandler.java
index f7d8d08..965d0d1 100644
---
a/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/RequestHandler.java
+++
b/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/RequestHandler.java
@@ -55,13 +55,13 @@ import org.apache.ofbiz.base.util.UtilMisc;
import org.apache.ofbiz.base.util.UtilObject;
import org.apache.ofbiz.base.util.UtilProperties;
import org.apache.ofbiz.base.util.UtilValidate;
-import org.apache.ofbiz.base.util.collections.MultivaluedMapContext;
import org.apache.ofbiz.entity.Delegator;
import org.apache.ofbiz.entity.GenericEntityException;
import org.apache.ofbiz.entity.GenericValue;
import org.apache.ofbiz.entity.util.EntityQuery;
import org.apache.ofbiz.entity.util.EntityUtilProperties;
import org.apache.ofbiz.webapp.OfbizUrlBuilder;
+import org.apache.ofbiz.webapp.control.ConfigXMLReader.ControllerConfig;
import org.apache.ofbiz.webapp.control.ConfigXMLReader.RequestMap;
import org.apache.ofbiz.webapp.event.EventFactory;
import org.apache.ofbiz.webapp.event.EventHandler;
@@ -80,8 +80,6 @@ import org.apache.ofbiz.widget.model.ThemeFactory;
public class RequestHandler {
public static final String module = RequestHandler.class.getName();
- private final static String defaultStatusCodeString =
- UtilProperties.getPropertyValue("requestHandler", "status-code",
"302");
private final ViewFactory viewFactory;
private final EventFactory eventFactory;
private final URL controllerConfigURL;
@@ -89,66 +87,6 @@ public class RequestHandler {
private final boolean trackVisit;
private ControllerConfig ccfg;
- static class ControllerConfig {
- private final MultivaluedMapContext<String, RequestMap> requestMapMap;
- private final Map<String, ConfigXMLReader.ViewMap> viewMapMap;
- private String statusCodeString;
- private final String defaultRequest;
- private final Map<String, ConfigXMLReader.Event> firstVisitEventList;
- private final Map<String, ConfigXMLReader.Event> preprocessorEventList;
- private final Map<String, ConfigXMLReader.Event>
postprocessorEventList;
- private final String protectView;
-
- ControllerConfig(ConfigXMLReader.ControllerConfig ccfg) throws
WebAppConfigurationException {
- preprocessorEventList = ccfg.getPreprocessorEventList();
- postprocessorEventList = ccfg.getPostprocessorEventList();
- requestMapMap = ccfg.getRequestMapMultiMap();
- viewMapMap = ccfg.getViewMapMap();
- defaultRequest = ccfg.getDefaultRequest();
- firstVisitEventList = ccfg.getFirstVisitEventList();
- protectView = ccfg.getProtectView();
-
- String status = ccfg.getStatusCode();
- statusCodeString = UtilValidate.isEmpty(status) ?
defaultStatusCodeString : status;
- }
-
- public MultivaluedMapContext<String, RequestMap> getRequestMapMap() {
- return requestMapMap;
- }
-
- public Map<String, ConfigXMLReader.ViewMap> getViewMapMap() {
- return viewMapMap;
- }
-
- public String getStatusCodeString() {
- return statusCodeString;
- }
-
- public String getDefaultRequest() {
- return defaultRequest;
- }
-
- public void setStatusCodeString(String statusCodeString) {
- this.statusCodeString = statusCodeString;
- }
-
- public Map<String, ConfigXMLReader.Event> getFirstVisitEventList() {
- return firstVisitEventList;
- }
-
- public Map<String, ConfigXMLReader.Event> getPreprocessorEventList() {
- return preprocessorEventList;
- }
-
- public Map<String, ConfigXMLReader.Event> getPostprocessorEventList() {
- return postprocessorEventList;
- }
-
- public String getProtectView() {
- return protectView;
- }
- }
-
public static RequestHandler getRequestHandler(ServletContext
servletContext) {
RequestHandler rh = (RequestHandler)
servletContext.getAttribute("_REQUEST_HANDLER_");
if (rh == null) {
@@ -193,7 +131,7 @@ public class RequestHandler {
* @return a collection of request maps which might be empty
*/
static Collection<RequestMap> resolveURI(ControllerConfig ccfg,
HttpServletRequest req) {
- Map<String, List<RequestMap>> requestMapMap = ccfg.getRequestMapMap();
+ Map<String, List<RequestMap>> requestMapMap =
ccfg.getRequestMapMultiMap();
Collection<RequestMap> rmaps = resolveTemplateURI(requestMapMap, req);
if (rmaps.isEmpty()) {
Map<String, ConfigXMLReader.ViewMap> viewMapMap =
ccfg.getViewMapMap();
@@ -277,7 +215,7 @@ public class RequestHandler {
// Parse controller config.
try {
- ccfg = new ControllerConfig(getControllerConfig());
+ ccfg = ConfigXMLReader.getControllerConfig(controllerConfigURL);
} catch (WebAppConfigurationException e) {
Debug.logError(e, "Exception thrown while parsing controller.xml
file: ", module);
throw new RequestHandlerException(e);
@@ -342,7 +280,7 @@ public class RequestHandler {
// Check for chained request.
if (chain != null) {
String chainRequestUri = RequestHandler.getRequestUri(chain);
- requestMap = ccfg.getRequestMapMap().getFirst(chainRequestUri);
+ requestMap = ccfg.getRequestMapMap().get(chainRequestUri);
if (requestMap == null) {
throw new RequestHandlerException("Unknown chained request ["
+ chainRequestUri + "]; this request does not exist");
}
@@ -366,11 +304,11 @@ public class RequestHandler {
// Check to make sure we are allowed to access this request
directly. (Also checks if this request is defined.)
// If the request cannot be called, or is not defined, check and
see if there is a default-request we can process
if (!requestMap.securityDirectRequest) {
- if (ccfg.getDefaultRequest() == null ||
!ccfg.getRequestMapMap().getFirst(ccfg.getDefaultRequest()).securityDirectRequest)
{
+ if (ccfg.getDefaultRequest() == null ||
!ccfg.getRequestMapMap().get(ccfg.getDefaultRequest()).securityDirectRequest) {
// use the same message as if it was missing for security
reasons, ie so can't tell if it is missing or direct request is not allowed
throw new
RequestHandlerException(requestMissingErrorMessage);
} else {
- requestMap =
ccfg.getRequestMapMap().getFirst(ccfg.getDefaultRequest());
+ requestMap =
ccfg.getRequestMapMap().get(ccfg.getDefaultRequest());
}
}
// Check if we SHOULD be secure and are not.
@@ -412,7 +350,7 @@ public class RequestHandler {
String newUrl = RequestHandler.makeUrl(request, response,
urlBuf.toString());
if (newUrl.toUpperCase().startsWith("HTTPS")) {
// if we are supposed to be secure, redirect secure.
- callRedirect(newUrl, response, request,
ccfg.getStatusCodeString());
+ callRedirect(newUrl, response, request,
ccfg.getStatusCode());
return;
}
}
@@ -497,7 +435,7 @@ public class RequestHandler {
// Invoke the security handler
// catch exceptions and throw RequestHandlerException if failed.
if (Debug.verboseOn()) Debug.logVerbose("[RequestHandler]:
AuthRequired. Running security check. " + showSessionId(request), module);
- ConfigXMLReader.Event checkLoginEvent =
ccfg.getRequestMapMap().getFirst("checkLogin").event;
+ ConfigXMLReader.Event checkLoginEvent =
ccfg.getRequestMapMap().get("checkLogin").event;
String checkLoginReturnString = null;
try {
@@ -510,9 +448,9 @@ public class RequestHandler {
eventReturn = checkLoginReturnString;
// if the request is an ajax request we don't want to return
the default login check
if
(!"XMLHttpRequest".equals(request.getHeader("X-Requested-With"))) {
- requestMap =
ccfg.getRequestMapMap().getFirst("checkLogin");
+ requestMap = ccfg.getRequestMapMap().get("checkLogin");
} else {
- requestMap =
ccfg.getRequestMapMap().getFirst("ajaxCheckLogin");
+ requestMap = ccfg.getRequestMapMap().get("ajaxCheckLogin");
}
}
}
@@ -644,7 +582,7 @@ public class RequestHandler {
redirectTarget += "?" + queryString;
}
- callRedirect(makeLink(request, response, redirectTarget),
response, request, ccfg.getStatusCodeString());
+ callRedirect(makeLink(request, response, redirectTarget),
response, request, ccfg.getStatusCode());
return;
}
}
@@ -702,31 +640,32 @@ public class RequestHandler {
}
}
- String responseStatusCode = nextRequestResponse.statusCode;
- if(UtilValidate.isNotEmpty(responseStatusCode))
- ccfg.setStatusCodeString(responseStatusCode);
-
+ // The status code used to redirect the HTTP client.
+ String redirectSC =
UtilValidate.isNotEmpty(nextRequestResponse.statusCode)
+ ? nextRequestResponse.statusCode
+ : ccfg.getStatusCode();
+
if ("url".equals(nextRequestResponse.type)) {
if (Debug.verboseOn())
Debug.logVerbose("[RequestHandler.doRequest]: Response is a URL redirect." +
showSessionId(request), module);
- callRedirect(nextRequestResponse.value, response, request,
ccfg.getStatusCodeString());
+ callRedirect(nextRequestResponse.value, response, request,
redirectSC);
} else if ("url-redirect".equals(nextRequestResponse.type)) {
// check for a cross-application redirect
if (Debug.verboseOn())
Debug.logVerbose("[RequestHandler.doRequest]: Response is
a URL redirect with redirect parameters."
+ showSessionId(request), module);
callRedirect(nextRequestResponse.value +
this.makeQueryString(request, nextRequestResponse), response,
- request, ccfg.getStatusCodeString());
+ request, redirectSC);
} else if ("cross-redirect".equals(nextRequestResponse.type)) {
// check for a cross-application redirect
if (Debug.verboseOn())
Debug.logVerbose("[RequestHandler.doRequest]: Response is a Cross-Application
redirect." + showSessionId(request), module);
String url = nextRequestResponse.value.startsWith("/") ?
nextRequestResponse.value : "/" + nextRequestResponse.value;
- callRedirect(url + this.makeQueryString(request,
nextRequestResponse), response, request, ccfg.getStatusCodeString());
+ callRedirect(url + this.makeQueryString(request,
nextRequestResponse), response, request, redirectSC);
} else if ("request-redirect".equals(nextRequestResponse.type)) {
if (Debug.verboseOn())
Debug.logVerbose("[RequestHandler.doRequest]: Response is a Request redirect."
+ showSessionId(request), module);
- callRedirect(makeLinkWithQueryString(request, response, "/" +
nextRequestResponse.value, nextRequestResponse), response, request,
ccfg.getStatusCodeString());
+ callRedirect(makeLinkWithQueryString(request, response, "/" +
nextRequestResponse.value, nextRequestResponse), response, request, redirectSC);
} else if
("request-redirect-noparam".equals(nextRequestResponse.type)) {
if (Debug.verboseOn())
Debug.logVerbose("[RequestHandler.doRequest]: Response is a Request redirect
with no parameters." + showSessionId(request), module);
- callRedirect(makeLink(request, response,
nextRequestResponse.value), response, request, ccfg.getStatusCodeString());
+ callRedirect(makeLink(request, response,
nextRequestResponse.value), response, request, redirectSC);
} else if ("view".equals(nextRequestResponse.type)) {
if (Debug.verboseOn())
Debug.logVerbose("[RequestHandler.doRequest]: Response is a view." +
showSessionId(request), module);
@@ -1140,13 +1079,7 @@ public class RequestHandler {
String requestUri = RequestHandler.getRequestUri(url);
ConfigXMLReader.RequestMap requestMap = null;
if (requestUri != null) {
- try {
- requestMap =
getControllerConfig().getRequestMapMap().get(requestUri);
- } catch (WebAppConfigurationException e) {
- // If we can't read the controller.xml file, then there is no
point in continuing.
- Debug.logError(e, "Exception thrown while parsing
controller.xml file: ", module);
- return null;
- }
+ requestMap =
getControllerConfig().getRequestMapMap().get(requestUri);
}
boolean didFullSecure = false;
boolean didFullStandard = false;
@@ -1302,20 +1235,15 @@ public class RequestHandler {
if (uriString == null) {
uriString= "";
}
- try {
- Map<String, RequestMap> rmaps =
getControllerConfig().getRequestMapMap();
- RequestMap requestMap = rmaps.get(uriString);
+ Map<String, RequestMap> rmaps =
getControllerConfig().getRequestMapMap();
+ RequestMap requestMap = rmaps.get(uriString);
+ if (requestMap == null) {
+ requestMap = rmaps.get(getControllerConfig().getDefaultRequest());
if (requestMap == null) {
- requestMap =
rmaps.get(getControllerConfig().getDefaultRequest());
- if (requestMap == null) {
- return false;
- }
+ return false;
}
- return pred.test(requestMap);
- } catch (WebAppConfigurationException e) {
- Debug.logError(e, "Exception thrown while parsing controller.xml
file: ", module);
- return false;
}
+ return pred.test(requestMap);
}
/**
diff --git
a/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/ServiceMultiEventHandler.java
b/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/ServiceMultiEventHandler.java
index 55c8929..e768553 100644
---
a/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/ServiceMultiEventHandler.java
+++
b/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/ServiceMultiEventHandler.java
@@ -161,12 +161,7 @@ public class ServiceMultiEventHandler implements
EventHandler {
} catch (WebAppConfigurationException e) {
throw new EventHandlerException(e);
}
- boolean eventGlobalTransaction;
- try {
- eventGlobalTransaction =
controllerConfig.getRequestMapMap().get(requestUri).event.globalTransaction;
- } catch (WebAppConfigurationException e) {
- throw new EventHandlerException(e);
- }
+ boolean eventGlobalTransaction =
controllerConfig.getRequestMapMap().get(requestUri).event.globalTransaction;
// big try/finally to make sure commit or rollback are run
boolean beganTrans = false;
diff --git
a/framework/webapp/src/test/java/org/apache/ofbiz/webapp/control/RequestHandlerTests.java
b/framework/webapp/src/test/java/org/apache/ofbiz/webapp/control/RequestHandlerTests.java
index 0469d76..53d760d 100644
---
a/framework/webapp/src/test/java/org/apache/ofbiz/webapp/control/RequestHandlerTests.java
+++
b/framework/webapp/src/test/java/org/apache/ofbiz/webapp/control/RequestHandlerTests.java
@@ -40,6 +40,7 @@ import java.util.Map;
import javax.servlet.http.HttpServletRequest;
import org.apache.ofbiz.base.util.collections.MultivaluedMapContext;
+import org.apache.ofbiz.webapp.control.ConfigXMLReader.ControllerConfig;
import org.apache.ofbiz.webapp.control.ConfigXMLReader.RequestMap;
import org.apache.ofbiz.webapp.control.ConfigXMLReader.ViewMap;
import org.junit.Before;
@@ -52,15 +53,15 @@ public class RequestHandlerTests {
private Map<String, ViewMap> viewMaps;
private HttpServletRequest req;
private Element dummyElement;
- private RequestHandler.ControllerConfig ccfg;
+ private ControllerConfig ccfg;
@Before
public void setUp() {
- ccfg = mock(RequestHandler.ControllerConfig.class);
+ ccfg = mock(ControllerConfig.class);
reqMaps = new MultivaluedMapContext<>();
viewMaps = new HashMap<>();
when(ccfg.getDefaultRequest()).thenReturn(null);
- when(ccfg.getRequestMapMap()).thenReturn(reqMaps);
+ when(ccfg.getRequestMapMultiMap()).thenReturn(reqMaps);
when(ccfg.getViewMapMap()).thenReturn(viewMaps);
req = mock(HttpServletRequest.class);
dummyElement = mock(Element.class);
diff --git
a/framework/widget/src/main/java/org/apache/ofbiz/widget/WidgetWorker.java
b/framework/widget/src/main/java/org/apache/ofbiz/widget/WidgetWorker.java
index 3a1646d..2f043bb 100644
--- a/framework/widget/src/main/java/org/apache/ofbiz/widget/WidgetWorker.java
+++ b/framework/widget/src/main/java/org/apache/ofbiz/widget/WidgetWorker.java
@@ -27,7 +27,6 @@ import javax.servlet.ServletContext;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
-import org.apache.ofbiz.base.util.Debug;
import org.apache.ofbiz.base.util.UtilCodec;
import org.apache.ofbiz.base.util.UtilGenerics;
import org.apache.ofbiz.base.util.UtilHttp;
@@ -36,7 +35,6 @@ import org.apache.ofbiz.entity.Delegator;
import org.apache.ofbiz.service.LocalDispatcher;
import org.apache.ofbiz.webapp.control.ConfigXMLReader;
import org.apache.ofbiz.webapp.control.RequestHandler;
-import org.apache.ofbiz.webapp.control.WebAppConfigurationException;
import org.apache.ofbiz.webapp.taglib.ContentUrlTag;
import org.apache.ofbiz.widget.model.ModelForm;
import org.apache.ofbiz.widget.model.ModelFormField;
@@ -300,12 +298,7 @@ public final class WidgetWorker {
String requestUri = (target.indexOf('?') > -1) ?
target.substring(0, target.indexOf('?')) : target;
ServletContext servletContext =
request.getSession().getServletContext();
RequestHandler rh = (RequestHandler)
servletContext.getAttribute("_REQUEST_HANDLER_");
- ConfigXMLReader.RequestMap requestMap = null;
- try {
- requestMap =
rh.getControllerConfig().getRequestMapMap().get(requestUri);
- } catch (WebAppConfigurationException e) {
- Debug.logError(e, "Exception thrown while parsing
controller.xml file: ", module);
- }
+ ConfigXMLReader.RequestMap requestMap =
rh.getControllerConfig().getRequestMapMap().get(requestUri);
if (requestMap != null && requestMap.event != null) {
return "hidden-form";
}
