jamesfredley commented on code in PR #16516:
URL: https://github.com/apache/grails-core/pull/16516#discussion_r4220952546
##########
grails-controllers/src/main/groovy/grails/artefact/controller/support/ResponseRenderer.groovy:
##########
@@ -138,6 +138,7 @@ trait ResponseRenderer extends WebAttributes {
try {
response.writer.write(object.inspect())
+ response.flushBuffer()
Review Comment:
Those flushes are no longer the fix. render(text:) clears the render flag,
and the adapter returns no view after that call.
##########
grails-controllers/src/main/groovy/grails/artefact/controller/support/ResponseRenderer.groovy:
##########
@@ -409,6 +430,7 @@ trait ResponseRenderer extends WebAttributes {
input = IOUtils.openStream(new File(o.toString()))
}
SpringIOUtils.copy(input, response.getOutputStream())
+ response.flushBuffer()
Review Comment:
The extra flush path is gone with the same change. Body-writing render calls
clear the flag, and the adapter does not resolve a view afterward.
##########
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:
An error status by itself no longer drops the model. A redirect, or a
response that is already committed, can still fall through when the action
returns a Map. That case is noted 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(text:) now goes through render(). render(view:) still assigns
MODEL_AND_VIEW directly, so that part is still open.
##########
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) {
+ 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)
+ if (!webRequest.renderViewRequested) {
Review Comment:
This uses only the raw render flag. redirect() sets the redirect attribute
and a 3xx status without calling setRenderView(false), and a response can
already be committed without that call. An action that then returns a Map used
to produce no view, because isRenderView() was false. It now returns a
ModelAndView, so DispatcherServlet resolves the default view after the redirect
or the committed body. Keep returning the model for an error status when
render() was not called, and still return null when the redirect attribute is
set or the response is already committed.
##########
grails-web-url-mappings/src/test/groovy/org/grails/web/mapping/mvc/UrlMappingsHandlerMappingSpec.groovy:
##########
@@ -319,4 +403,50 @@ 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']
+ }
+
+ /**
+ * Simulates render(view: 'myView'): sets MODEL_AND_VIEW on the request
but does NOT set
+ * renderView=false. The adapter must return the ModelAndView so
DispatcherServlet resolves
+ * the named view. (#15819)
+ */
+ @Action
+ def renderView() {
+ request.setAttribute(
Review Comment:
This action still installs MODEL_AND_VIEW itself and returns null. The test
name says render(view:) is used, but render(view: 'myView') never runs, so the
assertion passes even if that call stops publishing the attribute. Call
render(view: 'myView') here, 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]
