Merge pull request #1254 from rainboyan

* pr/1254: Refactor GrailsExceptionResolver

- Introduce `GrailsUrlMappingsExceptionResolver` to make grace-web-mvc decoupled from grace-web-url-mappings
- `GrailsExceptionResolver` should not reuse cached controller attribute to find the given error path
- Cleanup `GrailsWrappedRuntimeException`

Closes gh-1254

Author: rainboyan

grace-plugin-controllers/build.gradle

@@ -8,6 +8,7 @@ dependencies {
     api project(":grace-plugin-validation")
     api project(":grace-web-gsp")
     api project(":grace-web-mvc")
+    api project(":grace-web-url-mappings")
     api project(":grace-util")
 
     compileOnly libs.jakarta.servlet

grace-plugin-controllers/src/main/groovy/org/grails/plugins/web/controllers/ControllersPluginConfiguration.java

@@ -51,6 +51,7 @@
 import org.grails.web.errors.GrailsExceptionResolver;
 import org.grails.web.filters.HiddenHttpMethodFilter;
 import org.grails.web.filters.OrderedHiddenHttpMethodFilter;
+import org.grails.web.mapping.mvc.GrailsUrlMappingsExceptionResolver;
 import org.grails.web.servlet.mvc.GrailsDispatcherServlet;
 import org.grails.web.servlet.mvc.GrailsWebRequestFilter;
 import org.grails.web.servlet.mvc.ParameterCreationListener;
@@ -101,7 +102,7 @@ public StackTraceFilterer stackTraceFilterer(ObjectProvider<GrailsApplication> g
     @Bean
     public GrailsExceptionResolver exceptionHandler(ObjectProvider<GrailsApplication> grailsApplicationProvider,
             ObjectProvider<StackTraceFilterer> stackTraceFiltererObjectProvider) {
-        GrailsExceptionResolver exceptionResolver = new GrailsExceptionResolver();
+        GrailsUrlMappingsExceptionResolver exceptionResolver = new GrailsUrlMappingsExceptionResolver();
         exceptionResolver.setGrailsApplication(grailsApplicationProvider.getIfAvailable());
         exceptionResolver.setStackTraceFilterer(stackTraceFiltererObjectProvider.getIfAvailable());
 

grace-test-suite-uber/src/test/groovy/org/grails/web/errors/GrailsExceptionResolverTests.groovy

@@ -1,43 +1,34 @@
 package org.grails.web.errors
 
-import grails.core.DefaultGrailsApplication
-import grails.core.GrailsApplication
-import grails.util.Environment
-import grails.util.GrailsWebMockUtil
-import grails.web.CamelCaseUrlConverter
-import grails.web.UrlConverter
-import grails.web.mapping.UrlMappingsHolder
-import org.grails.config.PropertySourcesConfig
-import org.grails.exceptions.reporting.DefaultStackTraceFilterer
-import org.grails.plugins.testing.GrailsMockHttpServletRequest
-import org.grails.plugins.testing.GrailsMockHttpServletResponse
-import org.grails.support.MockApplicationContext
-import org.grails.web.mapping.DefaultUrlMappingEvaluator
-import org.grails.web.mapping.DefaultUrlMappingsHolder
-import org.grails.web.servlet.view.CompositeViewResolver
 import org.junit.jupiter.api.AfterEach
 import org.junit.jupiter.api.BeforeEach
 import org.junit.jupiter.api.Test
 import org.springframework.mock.web.MockHttpServletRequest
-import org.springframework.mock.web.MockHttpServletResponse
 import org.springframework.mock.web.MockServletContext
-import org.springframework.web.context.WebApplicationContext
 import org.springframework.web.context.request.RequestContextHolder
-import org.springframework.web.multipart.support.StandardServletMultipartResolver
 import org.springframework.web.servlet.View
 import org.springframework.web.servlet.ViewResolver
 import org.springframework.web.servlet.view.InternalResourceView
 
-import static org.junit.jupiter.api.Assertions.*
+import grails.core.DefaultGrailsApplication
+import grails.core.GrailsApplication
+import grails.util.Environment
+import grails.web.CamelCaseUrlConverter
+import grails.web.UrlConverter
+
+import org.grails.config.PropertySourcesConfig
+import org.grails.exceptions.reporting.DefaultStackTraceFilterer
+import org.grails.support.MockApplicationContext
+
+import static org.junit.jupiter.api.Assertions.assertEquals
+import static org.junit.jupiter.api.Assertions.assertThrows
 
 /**
  * Test case for {@link org.grails.web.errors.GrailsExceptionResolver}.
  */
 class GrailsExceptionResolverTests {
 
     private application = new DefaultGrailsApplication()
-    private resolver = new GrailsExceptionResolver()
-    private mockContext = new MockServletContext()
     private mockCtx = new MockApplicationContext()
 
     @AfterEach
@@ -49,8 +40,8 @@ class GrailsExceptionResolverTests {
     void setUp() throws Exception {
         mockCtx.registerMockBean(GrailsApplication.APPLICATION_ID, new DefaultGrailsApplication())
         def mainContext = new MockApplicationContext();
-        mainContext.registerMockBean(UrlConverter.BEAN_NAME, new CamelCaseUrlConverter());
-        application.mainContext =  mainContext
+        mainContext.registerMockBean(UrlConverter.BEAN_NAME, new CamelCaseUrlConverter())
+        application.mainContext = mainContext
     }
 
     @Test
@@ -70,101 +61,6 @@ class GrailsExceptionResolverTests {
         }
     }
 
-    @Test
-    void testResolveExceptionToView() {
-        def mappings = new DefaultUrlMappingEvaluator(mockCtx).evaluateMappings {
-            "500"(view:"myView")
-        }
-
-        def urlMappingsHolder = new DefaultUrlMappingsHolder(mappings)
-        def webRequest = GrailsWebMockUtil.bindMockWebRequest(mockCtx,
-                new GrailsMockHttpServletRequest(), new GrailsMockHttpServletResponse())
-
-        mockCtx.registerMockBean UrlMappingsHolder.BEAN_ID, urlMappingsHolder
-        ViewResolver viewResolver = new DummyViewResolver()
-        mockCtx.registerMockBean "viewResolver", viewResolver
-        mockCtx.registerMockBean 'grailsApplication', application
-        mockCtx.registerMockBean CompositeViewResolver.BEAN_NAME, new CompositeViewResolver(viewResolvers: [viewResolver])
-        mockContext.setAttribute WebApplicationContext.ROOT_WEB_APPLICATION_CONTEXT_ATTRIBUTE, mockCtx
-
-        resolver.servletContext = mockContext
-        resolver.exceptionMappings = ['java.lang.Exception': '/error'] as Properties
-        resolver.grailsApplication = application
-        resolver.stackFilterer = new DefaultStackTraceFilterer()
-
-        def ex = new Exception()
-        def request = webRequest.currentRequest
-        def response = webRequest.currentResponse
-        def handler = new Object()
-        def modelAndView = resolver.resolveException(request, response, handler, ex)
-
-        assertNotNull modelAndView, "should have returned a ModelAndView"
-        assertEquals "/myView", modelAndView.view.url
-    }
-
-    @Test
-    void testResolveExceptionToController() {
-        def mappings = new DefaultUrlMappingEvaluator(mockCtx).evaluateMappings {
-            "500"(controller:"foo", action:"bar")
-        }
-
-        def urlMappingsHolder = new DefaultUrlMappingsHolder(mappings)
-        def webRequest = GrailsWebMockUtil.bindMockWebRequest()
-
-        mockCtx.registerMockBean UrlMappingsHolder.BEAN_ID, urlMappingsHolder
-        mockCtx.registerMockBean "viewResolver", new DummyViewResolver()
-        mockCtx.registerMockBean GrailsApplication.APPLICATION_ID, application
-        mockCtx.registerMockBean "multipartResolver", new StandardServletMultipartResolver()
-        mockContext.setAttribute WebApplicationContext.ROOT_WEB_APPLICATION_CONTEXT_ATTRIBUTE, mockCtx
-
-        resolver.servletContext = mockContext
-        resolver.exceptionMappings = ['java.lang.Exception': '/error'] as Properties
-        resolver.grailsApplication = application
-        resolver.stackFilterer = new DefaultStackTraceFilterer()
-
-        def ex = new Exception()
-        def request = webRequest.currentRequest
-        MockHttpServletResponse response = webRequest.currentResponse
-        def handler = new Object()
-        def modelAndView = resolver.resolveException(request, response, handler, ex)
-
-        assertNotNull modelAndView, "should have returned a ModelAndView"
-        assertTrue modelAndView.empty
-
-        assertEquals "/foo/bar",response.getForwardedUrl()
-    }
-
-    @Test
-    void testResolveExceptionToControllerWhenResponseCommitted() {
-        def mappings = new DefaultUrlMappingEvaluator(mockCtx).evaluateMappings {
-            "500"(controller:"foo", action:"bar")
-        }
-
-        def urlMappingsHolder = new DefaultUrlMappingsHolder(mappings)
-        def webRequest = GrailsWebMockUtil.bindMockWebRequest()
-
-        mockCtx.registerMockBean UrlMappingsHolder.BEAN_ID, urlMappingsHolder
-        mockCtx.registerMockBean "viewResolver", new DummyViewResolver()
-        mockCtx.registerMockBean GrailsApplication.APPLICATION_ID, application
-        mockCtx.registerMockBean "multipartResolver", new StandardServletMultipartResolver()
-        mockContext.setAttribute WebApplicationContext.ROOT_WEB_APPLICATION_CONTEXT_ATTRIBUTE, mockCtx
-
-        resolver.servletContext = mockContext
-        resolver.exceptionMappings = ['java.lang.Exception': '/error'] as Properties
-        resolver.grailsApplication = application
-        resolver.stackFilterer = new DefaultStackTraceFilterer()
-
-        def ex = new Exception()
-        def request = webRequest.currentRequest
-        MockHttpServletResponse response = webRequest.currentResponse
-        def handler = new Object()
-        response.setCommitted(true)
-        def modelAndView = resolver.resolveException(request, response, handler, ex)
-
-        assertNotNull modelAndView, "should have returned a ModelAndView"
-        assertFalse modelAndView.empty
-    }
-
     @Test
     void testLogRequestWithException() {
         def config = new ConfigSlurper().parse('''
@@ -179,7 +75,7 @@ grails.exceptionresolver.params.exclude = ['jennysPhoneNumber']
         request.addParameter "jennysPhoneNumber", "8675309"
 
         System.setProperty(Environment.KEY, Environment.DEVELOPMENT.name)
-        def resolver = new GrailsExceptionResolver(grailsApplication:new DefaultGrailsApplication(config:new PropertySourcesConfig().merge(config)))
+        def resolver = new GrailsExceptionResolver(grailsApplication: new DefaultGrailsApplication(config: new PropertySourcesConfig().merge(config)))
         resolver.stackFilterer = new DefaultStackTraceFilterer()
         def msg = resolver.getRequestLogMessage(new RuntimeException("bad things happened"), request)
 
@@ -210,7 +106,7 @@ grails.exceptionresolver.params.exclude = ['jennysPhoneNumber']
         request.addParameter "jennysPhoneNumber", "8675309"
 
         System.setProperty(Environment.KEY, Environment.DEVELOPMENT.name)
-        def resolver = new GrailsExceptionResolver(grailsApplication:new DefaultGrailsApplication(config:new PropertySourcesConfig().merge(config)))
+        def resolver = new GrailsExceptionResolver(grailsApplication: new DefaultGrailsApplication(config: new PropertySourcesConfig().merge(config)))
         resolver.stackFilterer = new DefaultStackTraceFilterer()
         def msg = resolver.getRequestLogMessage(request)
 
@@ -251,19 +147,19 @@ Method: GET
 Filtered stacktrace:'''.replaceAll('[\n\r]', '')
 
             System.setProperty(Environment.KEY, Environment.DEVELOPMENT.name)
-            def resolver = new GrailsExceptionResolver(grailsApplication:application)
+            def resolver = new GrailsExceptionResolver(grailsApplication: application)
             resolver.stackFilterer = new DefaultStackTraceFilterer()
             def msg = resolver.getRequestLogMessage(request)
             assertEquals msgWithParameters, msg.replaceAll('[\n\r]', '')
 
             System.setProperty(Environment.KEY, Environment.PRODUCTION.name)
-            resolver = new GrailsExceptionResolver(grailsApplication:application)
+            resolver = new GrailsExceptionResolver(grailsApplication: application)
             resolver.stackFilterer = new DefaultStackTraceFilterer()
             msg = resolver.getRequestLogMessage(request)
             assertEquals msgWithoutParameters, msg.replaceAll('[\n\r]', '')
 
             System.setProperty(Environment.KEY, Environment.TEST.name)
-            resolver = new GrailsExceptionResolver(grailsApplication:application)
+            resolver = new GrailsExceptionResolver(grailsApplication: application)
             resolver.stackFilterer = new DefaultStackTraceFilterer()
             msg = resolver.getRequestLogMessage(request)
             assertEquals msgWithoutParameters, msg.replaceAll('[\n\r]', '')
@@ -273,19 +169,19 @@ grails.exceptionresolver.logRequestParameters = false
 ''')
 
             System.setProperty(Environment.KEY, Environment.DEVELOPMENT.name)
-            resolver = new GrailsExceptionResolver(grailsApplication:new DefaultGrailsApplication(config:new PropertySourcesConfig().merge(config)))
+            resolver = new GrailsExceptionResolver(grailsApplication: new DefaultGrailsApplication(config: new PropertySourcesConfig().merge(config)))
             resolver.stackFilterer = new DefaultStackTraceFilterer()
             msg = resolver.getRequestLogMessage(request)
             assertEquals msgWithoutParameters, msg.replaceAll('[\n\r]', '')
 
             System.setProperty(Environment.KEY, Environment.PRODUCTION.name)
-            resolver = new GrailsExceptionResolver(grailsApplication:new DefaultGrailsApplication(config:new PropertySourcesConfig().merge(config)))
+            resolver = new GrailsExceptionResolver(grailsApplication: new DefaultGrailsApplication(config: new PropertySourcesConfig().merge(config)))
             resolver.stackFilterer = new DefaultStackTraceFilterer()
             msg = resolver.getRequestLogMessage(request)
             assertEquals msgWithoutParameters, msg.replaceAll('[\n\r]', '')
 
             System.setProperty(Environment.KEY, Environment.TEST.name)
-            resolver = new GrailsExceptionResolver(grailsApplication:new DefaultGrailsApplication(config:new PropertySourcesConfig().merge(config)))
+            resolver = new GrailsExceptionResolver(grailsApplication: new DefaultGrailsApplication(config: new PropertySourcesConfig().merge(config)))
             resolver.stackFilterer = new DefaultStackTraceFilterer()
             msg = resolver.getRequestLogMessage(request)
             assertEquals msgWithoutParameters, msg.replaceAll('[\n\r]', '')
@@ -295,30 +191,34 @@ grails.exceptionresolver.logRequestParameters = true
 ''')
 
             System.setProperty(Environment.KEY, Environment.DEVELOPMENT.name)
-            resolver = new GrailsExceptionResolver(grailsApplication:new DefaultGrailsApplication(config:new PropertySourcesConfig().merge(config)))
+            resolver = new GrailsExceptionResolver(grailsApplication: new DefaultGrailsApplication(config: new PropertySourcesConfig().merge(config)))
             resolver.stackFilterer = new DefaultStackTraceFilterer()
             msg = resolver.getRequestLogMessage(request)
             assertEquals msgWithParameters, msg.replaceAll('[\n\r]', '')
 
             System.setProperty(Environment.KEY, Environment.PRODUCTION.name)
-            resolver = new GrailsExceptionResolver(grailsApplication:new DefaultGrailsApplication(config:new PropertySourcesConfig().merge(config)))
+            resolver = new GrailsExceptionResolver(grailsApplication: new DefaultGrailsApplication(config: new PropertySourcesConfig().merge(config)))
             resolver.stackFilterer = new DefaultStackTraceFilterer()
             msg = resolver.getRequestLogMessage(request)
             assertEquals msgWithParameters, msg.replaceAll('[\n\r]', '')
 
             System.setProperty(Environment.KEY, Environment.TEST.name)
-            resolver = new GrailsExceptionResolver(grailsApplication:new DefaultGrailsApplication(config:new PropertySourcesConfig().merge(config)))
+            resolver = new GrailsExceptionResolver(grailsApplication: new DefaultGrailsApplication(config: new PropertySourcesConfig().merge(config)))
             resolver.stackFilterer = new DefaultStackTraceFilterer()
             msg = resolver.getRequestLogMessage(request)
             assertEquals msgWithParameters, msg.replaceAll('[\n\r]', '')
-        } finally {
+        }
+        finally {
             System.setProperty(Environment.KEY, oldEnvName)
         }
     }
 }
 
 class DummyViewResolver implements ViewResolver {
+
+    @Override
     View resolveViewName(String viewName, Locale locale) {
         new InternalResourceView(viewName)
     }
+
 }

grace-web-mvc/build.gradle

@@ -1,7 +1,6 @@
 dependencies {
     api project(":grace-gsp")
     api project(":grace-web")
-    api project(":grace-web-url-mappings")
 
     compileOnlyApi libs.jakarta.servlet