jdaugherty commented on code in PR #16396:
URL: https://github.com/apache/grails-core/pull/16396#discussion_r4101133870
##########
grails-web-mvc/src/test/groovy/org/grails/web/errors/GrailsExceptionResolverSpec.groovy:
##########
@@ -546,4 +548,44 @@ class GrailsExceptionResolverSpec extends Specification {
then: 'the guard only suppresses re-entry, so both are forwarded'
forwards.size() == 2
}
+
+ void "resolveViewOrForward does not mask the original exception when a
plain ServletRequestAttributes is bound"() {
+ given: 'a plain ServletRequestAttributes (not a GrailsWebRequest) is
bound in RequestContextHolder'
+ def request = new MockHttpServletRequest('GET', '/fail')
+ RequestContextHolder.setRequestAttributes(new
ServletRequestAttributes(request, new MockHttpServletResponse()))
+
+ and: 'a UrlMappingInfo whose getControllerName() triggers a
closure-based name resolution'
+ def info = Mock(UrlMappingInfo)
+ info.getViewName() >> null
+ info.getControllerName() >> 'errors'
+ def urlMappings = Mock(UrlMappingsHolder)
+ urlMappings.match(_ as String) >> null
+ urlMappings.matchStatusCode(500, _ as Throwable) >> null
+ urlMappings.matchStatusCode(500) >> info
+
+ and: 'a resolver that records forwards without actually dispatching'
+ def forwards = []
+ def resolver = new GrailsExceptionResolver() {
+
+ @Override
+ protected void forwardRequest(UrlMappingInfo forwarded,
HttpServletRequest req,
Review 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.
##########
grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/AbstractUrlMappingInfo.java:
##########
@@ -125,7 +125,9 @@ else if (value instanceof RuntimeConstraintEvaluator) {
return evaluateCapturedName((RuntimeConstraintEvaluator) value);
}
else {
- GrailsWebRequest webRequest = (GrailsWebRequest)
RequestContextHolder.getRequestAttributes();
+ org.springframework.web.context.request.RequestAttributes attrs =
+ RequestContextHolder.getRequestAttributes();
+ GrailsWebRequest webRequest = attrs instanceof GrailsWebRequest ?
(GrailsWebRequest) attrs : null;
Review 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`.
##########
grails-web-url-mappings/src/test/groovy/org/grails/web/mapping/AbstractUrlMappingInfoSafeCastSpec.groovy:
##########
@@ -0,0 +1,150 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.grails.web.mapping
+
+import grails.core.DefaultGrailsApplication
+import grails.core.GrailsApplication
+import grails.web.mapping.UrlMapping
+import grails.web.mapping.UrlMappingInfo
+import org.grails.support.MockApplicationContext
+import org.springframework.mock.web.MockHttpServletRequest
+import org.springframework.mock.web.MockHttpServletResponse
+import org.springframework.web.context.request.RequestContextHolder
+import org.springframework.web.context.request.ServletRequestAttributes
+import spock.lang.Specification
+
+/**
+ * Verifies that {@link AbstractUrlMappingInfo} and {@link
DefaultUrlMappingInfo} do not throw a
+ * {@link ClassCastException} when {@link RequestContextHolder} holds a plain
+ * {@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}
+ * before the Grails filter had a chance to upgrade it to a {@code
GrailsWebRequest}. The unconditional
+ * cast that previously existed in {@code evaluateNameForValue} and {@code
getActionName} would then
+ * throw a {@code ClassCastException}, masking the original application
exception.
+ *
+ * @see <a href="https://github.com/apache/grails-core/issues/16129";>Issue
#16129</a>
+ */
+class AbstractUrlMappingInfoSafeCastSpec extends Specification {
+
+ def cleanup() {
+ RequestContextHolder.resetRequestAttributes()
+ }
+
+ private static UrlMapping closureActionMapping() {
+ MockApplicationContext ctx = new MockApplicationContext()
+ ctx.registerMockBean(GrailsApplication.APPLICATION_ID, new
DefaultGrailsApplication())
+ new DefaultUrlMappingEvaluator(ctx).evaluateMappings {
+ '/book'(controller: 'book', action: { request.method == 'GET' ?
'show' : 'save' })
+ }.first()
+ }
+
+ private static UrlMapping staticMapping() {
+ MockApplicationContext ctx = new MockApplicationContext()
+ ctx.registerMockBean(GrailsApplication.APPLICATION_ID, new
DefaultGrailsApplication())
+ new DefaultUrlMappingEvaluator(ctx).evaluateMappings {
+ '/book'(controller: 'book', action: 'show')
+ }.first()
+ }
+
+ void 'evaluateNameForValue does not throw ClassCastException when
RequestContextHolder holds a plain ServletRequestAttributes'() {
+ given: 'a plain ServletRequestAttributes (not a GrailsWebRequest) is
bound'
+ def request = new MockHttpServletRequest('GET', '/book')
+ RequestContextHolder.setRequestAttributes(new
ServletRequestAttributes(request, new MockHttpServletResponse()))
+
+ and: 'a mapping whose action is a closure (requires a GrailsWebRequest
to evaluate)'
+ UrlMapping mapping = closureActionMapping()
+ UrlMappingInfo info = mapping.match('/book')
+
+ when: 'action name is resolved while only a plain
ServletRequestAttributes is bound'
+ String actionName = info.actionName
+
+ then: 'no ClassCastException is thrown; the action gracefully returns
null'
+ noExceptionThrown()
+ actionName == null
+ }
+
+ void 'getActionName does not throw ClassCastException when
RequestContextHolder holds a plain ServletRequestAttributes'() {
Review 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 on
`8.0.x` it failed with an NPE (casting a null holder is fine).
- The static controller-name feature passes on `8.0.x`, because a `String`
name 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`).
##########
grails-web-mvc/src/test/groovy/org/grails/web/errors/GrailsExceptionResolverSpec.groovy:
##########
@@ -546,4 +548,44 @@ class GrailsExceptionResolverSpec extends Specification {
then: 'the guard only suppresses re-entry, so both are forwarded'
forwards.size() == 2
}
+
+ void "resolveViewOrForward does not mask the original exception when a
plain ServletRequestAttributes is bound"() {
+ given: 'a plain ServletRequestAttributes (not a GrailsWebRequest) is
bound in RequestContextHolder'
+ def request = new MockHttpServletRequest('GET', '/fail')
+ RequestContextHolder.setRequestAttributes(new
ServletRequestAttributes(request, new MockHttpServletResponse()))
+
+ and: 'a UrlMappingInfo whose getControllerName() triggers a
closure-based name resolution'
+ def info = Mock(UrlMappingInfo)
Review 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.
##########
grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/AbstractUrlMappingInfo.java:
##########
@@ -149,6 +154,9 @@ protected String evaluateNameForValue(Object value,
GrailsWebRequest webRequest)
name = result != null ? result.toString() : null;
}
else if (value instanceof Map) {
+ if (webRequest == null) {
Review 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.
##########
grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/AbstractUrlMappingInfo.java:
##########
@@ -141,6 +143,9 @@ protected String evaluateNameForValue(Object value,
GrailsWebRequest webRequest)
String name;
if (value instanceof Closure) {
+ if (webRequest == null) {
Review Comment:
Returning `null` here changes more than the cast:
- When nothing is bound, the closure used to run anyway, with a `null`
delegate. A closure that doesn't touch the request, such as `action: { 'show'
}`, resolved to `show` on `8.0.x` and resolves to `null` now.
- A controller name that comes back `null` makes `getControllerName()` throw
`UrlMappingException`. So a status mapping with a closure controller still
masks the original exception; the `ClassCastException` just becomes a
`UrlMappingException` (last row of the table on the `forwardRequest` comment).
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.
##########
grails-web-url-mappings/src/test/groovy/org/grails/web/mapping/AbstractUrlMappingInfoSafeCastSpec.groovy:
##########
@@ -0,0 +1,150 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.grails.web.mapping
+
+import grails.core.DefaultGrailsApplication
+import grails.core.GrailsApplication
+import grails.web.mapping.UrlMapping
+import grails.web.mapping.UrlMappingInfo
+import org.grails.support.MockApplicationContext
+import org.springframework.mock.web.MockHttpServletRequest
+import org.springframework.mock.web.MockHttpServletResponse
+import org.springframework.web.context.request.RequestContextHolder
+import org.springframework.web.context.request.ServletRequestAttributes
+import spock.lang.Specification
+
+/**
+ * Verifies that {@link AbstractUrlMappingInfo} and {@link
DefaultUrlMappingInfo} do not throw a
+ * {@link ClassCastException} when {@link RequestContextHolder} holds a plain
+ * {@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}
Review 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?
--
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]
