jamesfredley commented on code in PR #16516:
URL: https://github.com/apache/grails-core/pull/16516#discussion_r4222936862
##########
grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/mvc/UrlMappingsInfoHandlerAdapter.groovy:
##########
@@ -161,11 +161,20 @@ class UrlMappingsInfoHandlerAdapter implements
HandlerAdapter, ApplicationContex
}
}
+ // render(view:) sets MODEL_AND_VIEW on the request and does
not set renderView=false,
+ // so this path is always intentional view resolution — honour
it unconditionally.
def modelAndView =
request.getAttribute(GrailsApplicationAttributes.MODEL_AND_VIEW)
if (modelAndView instanceof ModelAndView) {
return (ModelAndView) modelAndView
}
- else if (result instanceof Map) {
+ // All other render() variants (template, text, JSON, file,
closure, object) set
+ // webRequest.renderView = false. If that flag is clear the
response has already been
+ // handled; returning a ModelAndView here would cause
DispatcherServlet to attempt
+ // view resolution and throw "Could not resolve view". (#15819)
+ if (!webRequest.renderView) {
Review Comment:
A redirect, and a response that is already committed, now return no view. An
included action whose outer response is already committed is still dropped.
That case is on the new check.
##########
grails-web-url-mappings/src/test/groovy/org/grails/web/mapping/mvc/UrlMappingsHandlerMappingSpec.groovy:
##########
@@ -319,4 +377,44 @@ class FooController {
def notFound() {
RequestContextHolder.currentRequestAttributes().response.writer <<
"Not Found"
}
+
+ /**
+ * Simulates render(text: 'hello') or render(template: '_partial'): sets
renderView=false,
+ * writes content, returns null. The adapter must return null so
DispatcherServlet does not
+ * attempt view resolution. (#15819)
+ */
+ @Action
+ def renderText() {
+ def webRequest = RequestContextHolder.currentRequestAttributes()
+ webRequest.renderView = false
+ webRequest.response.writer.write('hello')
+ null
+ }
+
+ /**
+ * Simulates an action that calls render(text:) but also returns a Map —
the bug scenario
+ * from #15819 where the adapter previously ignored renderView=false when
result instanceof Map.
+ */
+ @Action
+ def renderTextWithMap() {
+ def webRequest = RequestContextHolder.currentRequestAttributes()
Review Comment:
render(view:) now calls render(view:). The new redirect row still sets the
redirect attribute itself instead of calling redirect().
##########
grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/mvc/UrlMappingsInfoHandlerAdapter.groovy:
##########
@@ -161,11 +161,27 @@ class UrlMappingsInfoHandlerAdapter implements
HandlerAdapter, ApplicationContex
}
}
+ // render(view:) sets MODEL_AND_VIEW on the request and does
not set renderView=false,
+ // so this path is always intentional view resolution — honour
it unconditionally.
def modelAndView =
request.getAttribute(GrailsApplicationAttributes.MODEL_AND_VIEW)
if (modelAndView instanceof ModelAndView) {
return (ModelAndView) modelAndView
}
- else if (result instanceof Map) {
+ if (result instanceof Map) {
+ // All render() variants except render(view:) set
webRequest.renderView = false.
+ // Check the raw flag (not the composite isRenderView(),
which also returns false
+ // for error status, committed response, or redirect) so
that only an explicit
+ // render() call suppresses view resolution. (#15819)
+ //
+ // redirect() sets REDIRECT_ISSUED on the request and a
3xx status without calling
+ // setRenderView(false), so renderViewRequested stays
true. Similarly, the response
+ // may already be committed (e.g. the body was written
directly) without that flag
+ // being cleared. In both cases there is nothing left for
DispatcherServlet to do.
+ if (!webRequest.renderViewRequested
+ ||
request.getAttribute(GrailsApplicationAttributes.REDIRECT_ISSUED) != null
+ || response.committed) {
Review Comment:
response.committed is already true when a servlet include runs after the
outer response has been flushed. The include can still append output, and an
included action that returns a Map used to get a view. This check now returns
null, so that include writes nothing. Skip the committed check for an include
dispatch, and cover an already-committed outer response.
##########
grails-web-url-mappings/src/test/groovy/org/grails/web/mapping/mvc/UrlMappingsHandlerMappingSpec.groovy:
##########
@@ -319,4 +405,73 @@ class FooController {
def notFound() {
RequestContextHolder.currentRequestAttributes().response.writer <<
"Not Found"
}
+
+ /**
+ * Calls render(text:), which sets renderView=false and writes the body.
The adapter must
+ * return null so DispatcherServlet does not attempt view resolution.
(#15819)
+ */
+ @Action
+ def renderText() {
+ render(text: 'hello')
+ null
+ }
+
+ /**
+ * Calls render(text:) and also returns a Map — the exact bug scenario
from #15819 where the
+ * adapter previously ignored renderView=false when result instanceof Map.
+ */
+ @Action
+ def renderTextWithMap() {
+ render(text: 'hello')
+ [foo: 'bar']
+ }
+
+ /**
+ * Calls redirect() (which sets REDIRECT_ISSUED on the request and a 3xx
status without calling
+ * setRenderView(false)) and also returns a Map. The adapter must return
null — there is nothing
+ * left for DispatcherServlet to do after a redirect. (#15819)
+ */
+ @Action
+ def redirectWithMap() {
+ request.setAttribute(
Review Comment:
This does not call redirect(). It sets REDIRECT_ISSUED and status 302
itself, so the row stays green even if redirect() stops setting that attribute.
Call redirect() and assert the status and Location header, the way renderText()
calls render(text:).
--
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]
