Conversation
…th GrailsWebRequest cast failure.
…uest A DispatcherServlet other than the Grails one - the one MockMvc runs, for example - binds a plain ServletRequestAttributes over the GrailsWebRequest that GrailsWebRequestFilter bound. Guarding the casts alone left GrailsExceptionResolver forwarding to a controller error handler with a null GrailsWebRequest, so the original exception was still replaced. - URL mapping names, reverse mappings and the default URL creator find the GrailsWebRequest the filter stored on the request when the bound attributes are plain - Without one, a name closure is not called and resolves to null, and a map of HTTP methods is keyed by the method of the bound request. With no attributes bound at all, a closure is still called without a delegate - GrailsExceptionResolver forwards to a controller error handler only when the request has a GrailsWebRequest, and an error handler that cannot be resolved no longer replaces the exception being resolved: it is logged and the /error view renders the original exception - The default URL creator no longer prefixes URLs with "null" when there is no context path Fixes apache#16129
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
Hi @ruthst00 & thank you picking this up. I reviewed this PR with an LLM and have noted it's feedback below for others to review. I believe all of these issues are addressed in my latest push:
Guarding the cast is the right direction, but as it stands only a view-mapped status code is fixed. A controller-mapped one still loses the original exception, now to an NPE instead of a ClassCastException. Details are inline. I checked this in a local checkout by running resolveException with real URL mappings, and by reverting each production hunk against the new specs.
Beyond the inline comments:
- The same unchecked
(GrailsWebRequest) RequestContextHolder.getRequestAttributes()cast is still inRegexUrlMapping.createURLInternaland twice inDefaultUrlCreator. Those run when the error view renders a link under the same plain attributes. Since the issue asks that this path stop assuming aGrailsWebRequest, it makes sense to route them through the same lookup in this PR.
| def resolver = new GrailsExceptionResolver() { | ||
|
|
||
| @Override | ||
| protected void forwardRequest(UrlMappingInfo forwarded, HttpServletRequest req, |
There was a problem hiding this comment.
Overriding forwardRequest hides the part of this bug the PR doesn't fix yet (commenting here because GrailsExceptionResolver isn't in the diff). The real forwardRequest calls info.configure(WebUtils.retrieveGrailsWebRequest()), and retrieveGrailsWebRequest() returns null when the bound attributes aren't a GrailsWebRequest. populateParamsForMapping(null) then throws an NPE, which resolveViewOrForward wraps in a GrailsRuntimeException. So the original exception is still lost.
I ran resolveException against real mappings built by DefaultUrlMappingEvaluator, with a plain ServletRequestAttributes bound:
| status mapping | 8.0.x |
this PR |
|---|---|---|
"500"(view: '/error') |
ClassCastException |
resolves (view set, status 500) |
"500"(controller: 'errors', action: 'serverError') |
ClassCastException |
GrailsRuntimeException → NPE in populateParamsForMapping |
"500"(controller: { 'errors' }, action: 'serverError') |
ClassCastException |
GrailsRuntimeException → UrlMappingException |
Controller-mapped error handlers are common, so the resolver needs a fix as well. For the MockMvc case in the issue, GrailsWebRequestFilter has already stored the GrailsWebRequest on the request before the dispatcher rebinds plain attributes over it. So info.configure(GrailsWebRequest.lookup(request)) finds it; with just that change, the controller mapping forwards to /errors/serverError. When there's no GrailsWebRequest at all (the issue's direct resolveException reproduction), the forward can't be configured. Returning mv without forwarding would let the original exception reach the default error view. UrlMappingUtils.forwardRequestForUrlMappingInfo also dereferences GrailsWebRequest.lookup(request) without a null check, so that path needs the same guard.
| RequestContextHolder.setRequestAttributes(new ServletRequestAttributes(request, new MockHttpServletResponse())) | ||
|
|
||
| and: 'a UrlMappingInfo whose getControllerName() triggers a closure-based name resolution' | ||
| def info = Mock(UrlMappingInfo) |
There was a problem hiding this comment.
This test passes with both production changes reverted (I put AbstractUrlMappingInfo and DefaultUrlMappingInfo back to 8.0.x and it stays green). UrlMappingInfo and UrlMappingsHolder are mocks, so nothing reads RequestContextHolder, and getControllerName() is stubbed to return 'errors'. The closure-based name resolution described in the and: label never happens.
Could this build real mappings with DefaultUrlMappingEvaluator and DefaultUrlMappingsHolder, and go through resolveException (the entry point from the issue) without overriding forwardRequest? Cover both a view mapping and a controller mapping, and assert that the returned ModelAndView's exception entry wraps the original exception. That version fails today for the controller mapping (see the comment on forwardRequest) and will pin the fix once it's in.
| GrailsWebRequest webRequest = (GrailsWebRequest) RequestContextHolder.getRequestAttributes(); | ||
| org.springframework.web.context.request.RequestAttributes attrs = | ||
| RequestContextHolder.getRequestAttributes(); | ||
| GrailsWebRequest webRequest = attrs instanceof GrailsWebRequest ? (GrailsWebRequest) attrs : null; |
There was a problem hiding this comment.
This is the frame from the issue's stack trace (getNamespace() → evaluateNameForValue(Object)), but no test fails if it's reverted. With only this hunk put back to the unconditional cast, all of AbstractUrlMappingInfoSafeCastSpec and GrailsExceptionResolverSpec still pass. The specs go through either getActionName(), which calls the two-arg overload directly, or a String controller name, which returns before the cast. A spec calling getNamespace() / getViewName() / getId() on a mapping with no namespace, while plain attributes are bound, would cover it.
Also, GrailsWebRequest.lookup() already does exactly this instanceof check. Use it, or the helper suggested on the closure branch below, here and in DefaultUrlMappingInfo.getActionName() instead of two inline copies with a fully-qualified RequestAttributes.
|
|
||
| String name; | ||
| if (value instanceof Closure) { | ||
| if (webRequest == null) { |
There was a problem hiding this comment.
Returning null here changes more than the cast:
- When nothing is bound, the closure used to run anyway, with a
nulldelegate. A closure that doesn't touch the request, such asaction: { 'show' }, resolved toshowon8.0.xand resolves tonullnow. - A controller name that comes back
nullmakesgetControllerName()throwUrlMappingException. So a status mapping with a closure controller still masks the original exception; theClassCastExceptionjust becomes aUrlMappingException(last row of the table on theforwardRequestcomment).
In the MockMvc case from the issue the GrailsWebRequest still exists: GrailsWebRequestFilter stored it on the request before the dispatcher bound plain attributes over it. When the bound attributes are a ServletRequestAttributes, GrailsWebRequest.lookup(((ServletRequestAttributes) attrs).getRequest()) recovers it, and the closure keeps evaluating against the real request. null would then only be returned when no GrailsWebRequest exists anywhere. A private helper for that lookup would also replace the inline copies in both files.
| name = result != null ? result.toString() : null; | ||
| } | ||
| else if (value instanceof Map) { | ||
| if (webRequest == null) { |
There was a problem hiding this comment.
Selecting by HTTP method doesn't need a GrailsWebRequest: HiddenHttpMethod.effectiveMethod takes the HttpServletRequest, which a plain ServletRequestAttributes already has. As written, action: [GET: 'show', POST: 'save'] quietly resolves to null whenever the bound attributes aren't Grails'. Taking the request from the bound ServletRequestAttributes would keep this branch working. No spec covers the Map branch yet.
| actionName == null | ||
| } | ||
|
|
||
| void 'getActionName does not throw ClassCastException when RequestContextHolder holds a plain ServletRequestAttributes'() { |
There was a problem hiding this comment.
This feature is identical to the one above: same mapping, same bound attributes, same info.actionName call. The one above is named for evaluateNameForValue(Object) but never reaches it. Also in this spec:
- The empty-holder feature below is named for
ClassCastException, but on8.0.xit failed with an NPE (casting a null holder is fine). - The static controller-name feature passes on
8.0.x, because aStringname returns before the cast.
A data-driven feature would cover the one-arg path this spec is missing. Run it over the public getters (namespace, controllerName, actionName, viewName, id), crossed with the holder state (GrailsWebRequest, plain ServletRequestAttributes, nothing bound) and the value shape (String, closure, method Map).
| * {@link ServletRequestAttributes} instead of a {@link org.grails.web.servlet.mvc.GrailsWebRequest}. | ||
| * | ||
| * <p>This scenario arises when {@link org.grails.web.errors.GrailsExceptionResolver} resolves an | ||
| * exception: Spring's {@code DispatcherServlet} may have bound a {@code ServletRequestAttributes} |
There was a problem hiding this comment.
This isn't quite how the plain attributes get there. In a Grails app, GrailsDispatcherServlet.buildRequestAttributes replaces a plain ServletRequestAttributes with a GrailsWebRequest, so exception resolution under Grails' own dispatcher always sees one. The plain attributes come either from a non-Grails dispatcher rebinding over the GrailsWebRequest that GrailsWebRequestFilter set (MockMvc's TestDispatcherServlet, as in the issue), or from code calling the resolver directly. Could the Javadoc, and the root-cause section of the PR description, say that?
|
@matrei @codeconsole can one of you review this too please? I believe I've fixed all of the issues I found with it. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #16396 +/- ##
==================================================
+ Coverage 57.4998% 57.6932% +0.1934%
- Complexity 22590 22749 +159
==================================================
Files 2129 2133 +4
Lines 103863 104248 +385
Branches 18651 18701 +50
==================================================
+ Hits 59721 60144 +423
+ Misses 35871 35808 -63
- Partials 8271 8296 +25
🚀 New features to boost your workflow:
|
✅ All tests passed ✅🏷️ Commit: 912be07 Learn more about TestLens at testlens.app/docs. |
This branch has not been deployed
2 participants
…th GrailsWebRequest cast failure.
Description
Root Cause
Two unconditional casts to
GrailsWebRequestin the URL mapping layer would throwClassCastExceptionwhen Spring'sRequestContextHolderheld a plainServletRequestAttributes(not aGrailsWebRequest). This happens during exception resolution when the Grails filter hasn't had a chance to upgrade the request attributes. TheClassCastExceptionthen replaced the original application exception inGrailsExceptionResolver, masking the real error.Changes Made
grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/AbstractUrlMappingInfo.javaevaluateNameForValue(Object value): replaced the unconditional(GrailsWebRequest) RequestContextHolder.getRequestAttributes()cast with a safeinstanceofcheck, passingnullto the overload when the attributes are not aGrailsWebRequest.evaluateNameForValue(Object value, GrailsWebRequest webRequest): added null-guards for theClosureandMapbranches so they returnnullgracefully whenwebRequestisnull(rather than NPE-ing).grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/DefaultUrlMappingInfo.javagetActionName(): replaced the unconditional(GrailsWebRequest)cast with the same safeinstanceofpattern.New test:
grails-web-url-mappings/src/test/groovy/org/grails/web/mapping/AbstractUrlMappingInfoSafeCastSpec.groovyServletRequestAttributesbound, closure-action mapping with no attributes bound, and static string names always resolving correctly regardless ofRequestContextHolderstate.Updated test:
grails-web-mvc/src/test/groovy/org/grails/web/errors/GrailsExceptionResolverSpec.groovyresolveViewOrForward does not mask the original exception when a plain ServletRequestAttributes is bound— directly reproduces the issue scenario and verifies the original exception is not masked.Fixes #16129
Generated with Claude Sonnet 4.6 via Cline API Provider inside IntelliJ 2025.3.3
Contributor Checklist
Please review the following checklist before submitting your pull request. Pull requests that do not meet these requirements may be closed without review.
Issue and Scope
7.0.x): Bug fixes only. No new features or API changes.7.1.x): New features are welcome, but breaking existing APIs must be avoided.8.0.x): Reserved for major changes. Breaking API changes are permitted.Code Quality
./gradlew build --rerun-tasks../gradlew codeStyleand resolved any violations. See Code Style for details.Licensing and Attribution
Documentation