diff --git a/grails-doc/src/en/guide/theWebLayer/urlmappings/mappingToResponseCodes.adoc b/grails-doc/src/en/guide/theWebLayer/urlmappings/mappingToResponseCodes.adoc index 1c779a454af..75481f95318 100644 --- a/grails-doc/src/en/guide/theWebLayer/urlmappings/mappingToResponseCodes.adoc +++ b/grails-doc/src/en/guide/theWebLayer/urlmappings/mappingToResponseCodes.adoc @@ -85,4 +85,6 @@ class ErrorsController { } ---- +NOTE: A problem reaching the error handler does not replace the exception being handled. If the mapping for a status code cannot be resolved for a request, or an error-handling controller cannot be forwarded to because the request did not pass through the Grails filters - a Spring `MockMvc` test that leaves out the application's filters, for example - the problem is logged and the `/error` view renders the original exception instead. + WARNING: If your error-handling controller action throws an exception as well, you'll end up with a `StackOverflowException`. diff --git a/grails-web-mvc/src/main/groovy/org/grails/web/errors/GrailsExceptionResolver.java b/grails-web-mvc/src/main/groovy/org/grails/web/errors/GrailsExceptionResolver.java index 649d4bd0372..9e5c87e3962 100644 --- a/grails-web-mvc/src/main/groovy/org/grails/web/errors/GrailsExceptionResolver.java +++ b/grails-web-mvc/src/main/groovy/org/grails/web/errors/GrailsExceptionResolver.java @@ -63,6 +63,7 @@ import org.grails.exceptions.reporting.StackTraceFilterer; import org.grails.web.mapping.DefaultUrlMappingInfo; import org.grails.web.mapping.UrlMappingUtils; +import org.grails.web.servlet.mvc.GrailsWebRequest; import org.grails.web.servlet.mvc.exceptions.GrailsMVCException; import org.grails.web.util.GrailsApplicationAttributes; import org.grails.web.util.WebUtils; @@ -204,29 +205,47 @@ Map extractRequestParamsWithUrlMappingHolder(UrlMappingsHolder urlMappings, Http protected ModelAndView resolveViewOrForward(Exception ex, UrlMappingsHolder urlMappings, HttpServletRequest request, HttpServletResponse response, ModelAndView mv) { - UrlMappingInfo info = matchStatusCode(ex, urlMappings); - - if (info != null) { - Map params = extractRequestParamsWithUrlMappingHolder(urlMappings, request); - if (params != null && !params.isEmpty()) { - Map infoParams = info.getParameters(); - if (infoParams != null) { - params.putAll(info.getParameters()); + UrlMappingInfo info; + boolean mapsToView; + boolean mapsToController; + try { + info = matchStatusCode(ex, urlMappings); + if (info != null) { + Map params = extractRequestParamsWithUrlMappingHolder(urlMappings, request); + if (params != null && !params.isEmpty()) { + Map infoParams = info.getParameters(); + if (infoParams != null) { + params.putAll(info.getParameters()); + } + info = new DefaultUrlMappingInfo(info, params, grailsApplication); } - info = new DefaultUrlMappingInfo(info, params, grailsApplication); } + mapsToView = info != null && info.getViewName() != null; + mapsToController = !mapsToView && info != null && info.getControllerName() != null; + } + catch (RuntimeException e) { + // An error handler that cannot be resolved for this request - a mapping that computes its controller + // from request state Grails did not set up, for example - must not replace the exception being + // resolved, so the default error view renders that exception instead + LOG.error("Unable to resolve the error handler mapped for [{}]: {}", request.getRequestURI(), e.getMessage(), e); + return mv; } try { - if (info != null && info.getViewName() != null) { + if (mapsToView) { resolveView(request, info, mv); } - else if (info != null && info.getControllerName() != null) { + else if (mapsToController) { if (isErrorHandlerForwardInProgress(request)) { LOG.error("The error handler for this request failed as well; not forwarding to it again"); return mv; } String uri = determineUri(request); + if (GrailsWebRequest.lookup(request) == null) { + LOG.warn("Not forwarding [{}] to the error handler it maps to, because the request has no " + + "GrailsWebRequest to dispatch it with; rendering the default error view instead", uri); + return mv; + } if (!response.isCommitted()) { if (response instanceof GrailsResponseMutator) { // prevent further mutation of the request since an error page needs rendered instead @@ -273,7 +292,7 @@ protected boolean isErrorHandlerForwardInProgress(HttpServletRequest request) { protected void forwardRequest(UrlMappingInfo info, HttpServletRequest request, HttpServletResponse response, ModelAndView mv, String uri) throws ServletException, IOException { - info.configure(WebUtils.retrieveGrailsWebRequest()); + info.configure(GrailsWebRequest.lookup(request)); String forwardUrl = UrlMappingUtils.forwardRequestForUrlMappingInfo( request, response, info, mv.getModel(), true); LOG.debug("Matched URI [{}] to URL mapping [{}], forwarding to [{}] with response [{}]", diff --git a/grails-web-mvc/src/test/groovy/org/grails/web/errors/GrailsExceptionResolverSpec.groovy b/grails-web-mvc/src/test/groovy/org/grails/web/errors/GrailsExceptionResolverSpec.groovy index 15f6dd8bfdc..7fe2756f4f2 100644 --- a/grails-web-mvc/src/test/groovy/org/grails/web/errors/GrailsExceptionResolverSpec.groovy +++ b/grails-web-mvc/src/test/groovy/org/grails/web/errors/GrailsExceptionResolverSpec.groovy @@ -19,6 +19,7 @@ package org.grails.web.errors import grails.config.Config +import grails.core.DefaultGrailsApplication import grails.core.GrailsApplication import grails.web.mapping.UrlMappingInfo import grails.web.mapping.UrlMappingsHolder @@ -27,17 +28,29 @@ import org.apache.grails.core.testing.support.LogCapture import org.grails.exceptions.reporting.DefaultStackTraceFilterer import org.apache.grails.core.GrailsBootstrapRegistryInitializer import org.grails.exceptions.reporting.StackTraceFilterer +import org.grails.web.mapping.DefaultUrlMappingEvaluator +import org.grails.web.mapping.DefaultUrlMappingsHolder +import org.grails.web.mapping.mvc.GrailsControllerUrlMappings +import org.grails.web.servlet.mvc.GrailsWebRequest +import org.grails.web.servlet.view.CompositeViewResolver +import org.grails.web.util.WebUtils as GrailsWebUtils import org.springframework.beans.factory.BeanNotOfRequiredTypeException import org.springframework.beans.factory.NoSuchBeanDefinitionException import org.springframework.context.ApplicationContext +import org.springframework.context.support.StaticApplicationContext import org.springframework.mock.web.MockHttpServletRequest import org.springframework.mock.web.MockHttpServletResponse import org.springframework.mock.web.MockServletContext +import org.springframework.web.context.request.RequestContextHolder +import org.springframework.web.context.request.ServletRequestAttributes import org.springframework.web.context.request.async.AsyncRequestTimeoutException import org.springframework.web.util.WebUtils import org.springframework.web.context.WebApplicationContext import org.springframework.web.context.support.StaticWebApplicationContext import org.springframework.web.servlet.ModelAndView +import org.springframework.web.servlet.ViewResolver +import org.springframework.web.servlet.view.InternalResourceView +import spock.lang.Issue import spock.lang.Specification import jakarta.servlet.http.HttpServletRequest @@ -45,6 +58,19 @@ import jakarta.servlet.http.HttpServletResponse class GrailsExceptionResolverSpec extends Specification { + private static final String GRAILS = 'a GrailsWebRequest' + private static final String PLAIN_OVER_GRAILS = 'plain attributes over a stored GrailsWebRequest' + private static final String PLAIN = 'plain attributes' + private static final String NOTHING = 'nothing' + + private final MockServletContext servletContext = new MockServletContext() + private StaticWebApplicationContext webContext + + def cleanup() { + RequestContextHolder.resetRequestAttributes() + webContext?.close() + } + void 'async timeouts resolve as 503 without an error stack trace'() { given: def resolverLog = new LogCapture(GrailsExceptionResolver) @@ -502,6 +528,7 @@ class GrailsExceptionResolverSpec extends Specification { } def request = new MockHttpServletRequest('POST', '/upload/upload') def response = new MockHttpServletResponse() + bind(GRAILS, request, response) when: def result = resolver.resolveViewOrForward(new RuntimeException('boom'), urlMappings, request, response, @@ -536,6 +563,7 @@ class GrailsExceptionResolverSpec extends Specification { } def request = new MockHttpServletRequest('POST', '/upload/upload') def response = new MockHttpServletResponse() + bind(GRAILS, request, response) when: 'two errors are resolved in sequence, as an include and its enclosing request would' resolver.resolveViewOrForward(new RuntimeException('boom'), urlMappings, request, response, @@ -546,4 +574,124 @@ class GrailsExceptionResolverSpec extends Specification { then: 'the guard only suppresses re-entry, so both are forwarded' forwards.size() == 2 } + + @Issue('https://github.com/apache/grails-core/issues/16129') + void "an error handler mapped to #handler is forwarded to with #attributes bound"() { + given: + def resolver = resolverFor(mappings, true) + def request = new MockHttpServletRequest(servletContext, 'GET', '/fail') + def response = new MockHttpServletResponse() + bind(attributes, request, response) + def original = new IllegalStateException('original') + + when: + def result = resolver.resolveException(request, response, null, original) + + then: 'the error handler is dispatched with the exception being resolved' + result.empty + response.forwardedUrl == '/errors/serverError' + (request.getAttribute(GrailsExceptionResolver.EXCEPTION_ATTRIBUTE) as Throwable).cause.is(original) + + where: + handler | attributes | mappings + 'a controller' | GRAILS | { '500'(controller: 'errors', action: 'serverError') } + 'a controller' | PLAIN_OVER_GRAILS | { '500'(controller: 'errors', action: 'serverError') } + 'a controller closure' | GRAILS | { '500'(controller: { 'errors' }, action: 'serverError') } + 'a controller closure' | PLAIN_OVER_GRAILS | { '500'(controller: { 'errors' }, action: 'serverError') } + 'an HTTP method map' | PLAIN_OVER_GRAILS | { '500'(controller: 'errors', action: [GET: 'serverError']) } + } + + @Issue('https://github.com/apache/grails-core/issues/16129') + void "the default error view renders the exception when #handler cannot be forwarded to with #attributes bound"() { + given: + def resolver = resolverFor(mappings, controllerMappings) + def request = new MockHttpServletRequest(servletContext, 'GET', '/fail') + def response = new MockHttpServletResponse() + bind(attributes, request, response) + def original = new IllegalStateException('original') + + when: + def result = resolver.resolveException(request, response, null, original) + + then: 'the exception being resolved is not replaced by the failure to reach its error handler' + result.viewName == '/error' + response.forwardedUrl == null + (result.model[GrailsExceptionResolver.EXCEPTION_ATTRIBUTE] as Throwable).cause.is(original) + + where: + handler | attributes | controllerMappings | mappings + 'a controller' | PLAIN | true | { '500'(controller: 'errors', action: 'serverError') } + 'a controller' | NOTHING | true | { '500'(controller: 'errors', action: 'serverError') } + 'a controller closure' | PLAIN | true | { '500'(controller: { 'errors' }, action: 'serverError') } + 'a controller closure' | PLAIN | false | { '500'(controller: { 'errors' }, action: 'serverError') } + 'a controller closure' | NOTHING | true | { '500'(controller: { 'errors' }, action: 'serverError') } + 'an HTTP method map' | PLAIN | true | { '500'(controller: 'errors', action: [GET: 'serverError']) } + 'a controller closure that fails' | GRAILS | true | { '500'(controller: { throw new IllegalStateException('broken mapping') }) } + } + + @Issue('https://github.com/apache/grails-core/issues/16129') + void "an error handler mapped to a view renders with #attributes bound"() { + given: + def resolver = resolverFor({ '500'(view: '/serverError') }, true) + def request = new MockHttpServletRequest(servletContext, 'GET', '/fail') + def response = new MockHttpServletResponse() + bind(attributes, request, response) + def original = new IllegalStateException('original') + + when: + def result = resolver.resolveException(request, response, null, original) + + then: + (result.view as InternalResourceView).url == '/serverError' + (result.model[GrailsExceptionResolver.EXCEPTION_ATTRIBUTE] as Throwable).cause.is(original) + + where: + attributes << [GRAILS, PLAIN_OVER_GRAILS, PLAIN, NOTHING] + } + + private GrailsExceptionResolver resolverFor(Closure mappings, boolean controllerMappings) { + def grailsApplication = new DefaultGrailsApplication().tap { + initialise() + } + def evaluatorContext = new StaticApplicationContext() + evaluatorContext.beanFactory.registerSingleton(GrailsApplication.APPLICATION_ID, grailsApplication) + evaluatorContext.refresh() + UrlMappingsHolder urlMappings = new DefaultUrlMappingsHolder( + new DefaultUrlMappingEvaluator(evaluatorContext).evaluateMappings(mappings)) + if (controllerMappings) { + urlMappings = new GrailsControllerUrlMappings(grailsApplication, urlMappings) + } + ViewResolver viewResolver = { String name, Locale locale -> new InternalResourceView(name) } as ViewResolver + + webContext = new StaticWebApplicationContext() + webContext.beanFactory.registerSingleton(GrailsApplication.APPLICATION_ID, grailsApplication) + webContext.beanFactory.registerSingleton(UrlMappingsHolder.BEAN_ID, urlMappings) + webContext.beanFactory.registerSingleton(CompositeViewResolver.BEAN_NAME, + new CompositeViewResolver(viewResolvers: [viewResolver])) + webContext.refresh() + servletContext.setAttribute(WebApplicationContext.ROOT_WEB_APPLICATION_CONTEXT_ATTRIBUTE, webContext) + + def resolver = new GrailsExceptionResolver() + resolver.servletContext = servletContext + resolver.grailsApplication = grailsApplication + resolver.exceptionMappings = ['java.lang.Exception': '/error'] as Properties + resolver + } + + private void bind(String attributes, MockHttpServletRequest request, MockHttpServletResponse response) { + switch (attributes) { + case GRAILS: + GrailsWebUtils.storeGrailsWebRequest(new GrailsWebRequest(request, response, servletContext)) + break + case PLAIN_OVER_GRAILS: + GrailsWebUtils.storeGrailsWebRequest(new GrailsWebRequest(request, response, servletContext)) + RequestContextHolder.setRequestAttributes(new ServletRequestAttributes(request, response)) + break + case PLAIN: + RequestContextHolder.setRequestAttributes(new ServletRequestAttributes(request, response)) + break + default: + RequestContextHolder.resetRequestAttributes() + } + } } diff --git a/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/AbstractUrlMappingInfo.java b/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/AbstractUrlMappingInfo.java index 1d46432d381..54f9c24295f 100644 --- a/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/AbstractUrlMappingInfo.java +++ b/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/AbstractUrlMappingInfo.java @@ -28,7 +28,10 @@ import groovy.lang.Closure; +import jakarta.servlet.http.HttpServletRequest; + import org.springframework.web.context.request.RequestContextHolder; +import org.springframework.web.context.request.ServletRequestAttributes; import org.springframework.web.util.UriUtils; import grails.util.GrailsStringUtils; @@ -125,11 +128,24 @@ else if (value instanceof RuntimeConstraintEvaluator) { return evaluateCapturedName((RuntimeConstraintEvaluator) value); } else { - GrailsWebRequest webRequest = (GrailsWebRequest) RequestContextHolder.getRequestAttributes(); - return evaluateNameForValue(value, webRequest); + return evaluateNameForValue(value, UrlMappingUtils.lookupWebRequest()); } } + /** + * Resolves a controller, action, namespace, view or id name held by this instance. A closure is called with + * the given request as its delegate, and a map of HTTP methods to names is keyed by the method of the request. + * + *
Without a {@code webRequest} but with request attributes bound - a request dispatched by a + * {@code DispatcherServlet} other than the Grails one, without {@code GrailsWebRequestFilter} - a closure is + * not called and resolves to null, since there is no Grails request state for it to read, and a map is keyed + * by the method of the bound request. With no request attributes bound at all, a closure is called without a + * delegate.
+ * + * @param value The name held by this instance + * @param webRequest The current request, or null if there is none + * @return The name, or null if it cannot be resolved + */ protected String evaluateNameForValue(Object value, GrailsWebRequest webRequest) { if (value == null) { return null; @@ -141,6 +157,9 @@ protected String evaluateNameForValue(Object value, GrailsWebRequest webRequest) String name; if (value instanceof Closure) { + if (webRequest == null && RequestContextHolder.getRequestAttributes() != null) { + return null; + } Closure callable = (Closure) value; final Closure cloned = (Closure) callable.clone(); cloned.setDelegate(webRequest); @@ -149,8 +168,12 @@ protected String evaluateNameForValue(Object value, GrailsWebRequest webRequest) name = result != null ? result.toString() : null; } else if (value instanceof Map) { + HttpServletRequest request = webRequest != null ? webRequest.getRequest() : currentRequest(); + if (request == null) { + return null; + } Map httpMethods = (Map) value; - name = (String) httpMethods.get(HiddenHttpMethod.effectiveMethod(webRequest.getRequest())); + name = (String) httpMethods.get(HiddenHttpMethod.effectiveMethod(request)); } else { name = value.toString(); @@ -158,6 +181,13 @@ else if (value instanceof Map) { return name != null ? name.trim() : null; } + private static HttpServletRequest currentRequest() { + if (RequestContextHolder.getRequestAttributes() instanceof ServletRequestAttributes attributes) { + return attributes.getRequest(); + } + return null; + } + /** * Resolves a name the mapping captured from the URI - the {@code $controller} token of * {@code "/$controller/$action?"}, for example - from this instance's own parameters, so that the diff --git a/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/DefaultUrlCreator.java b/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/DefaultUrlCreator.java index 78dad3e0f01..47cabf6db46 100644 --- a/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/DefaultUrlCreator.java +++ b/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/DefaultUrlCreator.java @@ -26,8 +26,6 @@ import java.util.Iterator; import java.util.Map; -import org.springframework.web.context.request.RequestContextHolder; - import grails.core.GrailsControllerClass; import grails.util.GrailsStringUtils; import grails.util.GrailsWebUtil; @@ -61,7 +59,7 @@ public DefaultUrlCreator(String controller, String action) { public String createURL(Map parameterValues, String encoding) { if (parameterValues == null) parameterValues = Collections.emptyMap(); - GrailsWebRequest webRequest = (GrailsWebRequest) RequestContextHolder.getRequestAttributes(); + GrailsWebRequest webRequest = UrlMappingUtils.lookupWebRequest(); return createURLWithWebRequest(parameterValues, webRequest, true); } @@ -86,8 +84,9 @@ private String createURLWithWebRequest(Map parameterValues, GrailsWebRequest web } FastStringWriter actualUriBuf = new FastStringWriter(); - if (includeContextPath) { - actualUriBuf.append(requestStateLookupStrategy.getContextPath()); + String contextPath = includeContextPath ? requestStateLookupStrategy.getContextPath() : null; + if (contextPath != null) { + actualUriBuf.append(contextPath); } if (actionName != null) { if (actionName.indexOf(SLASH) > -1) { @@ -127,7 +126,7 @@ public String createURL(String controller, String action, String namespace, Stri } private String createURLInternal(String controller, String action, Map