jdaugherty opened a new pull request, #16422:
URL: https://github.com/apache/grails-core/pull/16422
## Summary
Backports #16396 (merged to `8.0.x` as `5e35c2c101`) to `7.0.x`. Fixes
#16129 on `7.0.x`.
A `DispatcherServlet` other than the Grails one (the one MockMvc runs, for
example) binds a plain `ServletRequestAttributes` over the `GrailsWebRequest`
that `GrailsWebRequestFilter` bound. The unconditional `(GrailsWebRequest)`
casts in the URL mapping layer then threw a `ClassCastException`.
`GrailsExceptionResolver` reported that exception in place of the one it was
resolving.
- URL mapping names, reverse mappings and the default URL creator now find
the `GrailsWebRequest` the filter stored on the request when the bound
attributes are plain (`UrlMappingUtils.lookupWebRequest()`)
- Without a `GrailsWebRequest`, 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`. An error handler that cannot be resolved
is logged, and the `/error` view renders the original exception instead of the
failure
- The default URL creator no longer prefixes URLs with `null` when there is
no context path
- A `NOTE` in `mappingToResponseCodes.adoc` describes the fallback
## Why this is not a cherry-pick
`git cherry-pick` of the two #16396 commits conflicts in
`GrailsExceptionResolver`, `GrailsExceptionResolverSpec`,
`AbstractUrlMappingInfo` and `DefaultUrlMappingInfo`, because `8.0.x` has moved
on in all four. This PR applies the net #16396 diff and adapts it to `7.0.x`:
- `DefaultUrlMappingInfo.getActionName` keeps `7.0.x`'s deprecated
`_action_` dispatch-parameter handling (`checkDispatchAction`), which `8.0.x`
removed. It now uses the recovered `GrailsWebRequest` instead of casting
- The HTTP-method map keeps `7.0.x`'s `getCurrentRequest().getMethod()`.
`HiddenHttpMethod` is `8.0.x`-only
- `GrailsExceptionResolver` logs through commons-logging on `7.0.x`, so the
new log calls use concatenation instead of `{}` placeholders. `8.0.x`'s guard
against re-forwarding to a failing error handler isn't on `7.0.x`, so the new
checks sit in the `7.0.x` method as it is
- `GrailsExceptionResolverSpec` starts from the `7.0.x` spec, plus the three
#16129 features and their helpers from #16396
## One addition `8.0.x` did not need
On `7.0.x`, `RegexUrlMapping.createRuntimeConstraintEvaluator` builds the
closures that resolve the names a mapping captures from the URI (`$controller`,
`$action`, ...). Each closure casts
`RequestContextHolder.currentRequestAttributes()` to `GrailsWebRequest` itself.
`8.0.x` replaced them with `RuntimeConstraintEvaluator`, which reads the
mapping's own captured params, so #16396 never reached this cast.
A probe confirmed the gap on `7.0.x`: with plain attributes bound over a
stored `GrailsWebRequest`, `"/$controller/$action?/$id?"` throws the #16129
`ClassCastException` from `getControllerName()`/`getActionName()`.
`AbstractGrailsControllerUrlMappings.match` evaluates `controllerName`. So any
app with the default `$controller` mapping would still hit it in
`GrailsExceptionResolver`'s request-parameter extraction, even with a static
`"500"(controller: ...)` handler. The closures now use
`UrlMappingUtils.lookupWebRequest()` like the other sites.
Side effect: with no request attributes bound at all, a name captured from
the URI resolves to null (so `getControllerName()` throws the usual
`UrlMappingException`, "Unable to establish controller name"), where it
previously threw `IllegalStateException` ("No thread-bound request found").
## Test plan
- [x] `./gradlew :grails-web-url-mappings:test` - 162 tests, 0 failures
(`DO_NOT_CACHE_TESTS=1`)
- [x] `./gradlew :grails-web-mvc:test` - 20 tests, 0 failures
- [x] `./gradlew :grails-test-suite-web:test` - 339 tests, 0 failures, 1
skipped
- [x] `./gradlew :grails-web-url-mappings:codeStyle
:grails-web-mvc:codeStyle` - clean
- [x] New `7.0.x` tests: `$controller`/`$action`/`$id` resolution under each
binding state, the `_action_` dispatch parameter under each binding state, and
error-handler forwarding next to a `"/$controller/$action?"` mapping
- [x] Mutation check: with every main-source change reverted, 13 of 20
`GrailsExceptionResolverSpec` and 17 of 28
`UrlMappingInfoRequestAttributesSpec` tests fail. With only the
`RegexUrlMapping` closure fix reverted, 3 fail
--
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]
