jamesfredley commented on code in PR #16516:
URL: https://github.com/apache/grails-core/pull/16516#discussion_r4179009844
##########
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:
`response.flushBuffer()` does not stop the exception in #15819.
Spring Framework 7.0.9 `DispatcherServlet.processDispatchResult` renders
whenever the handler returns a non-null, uncleared `ModelAndView`. It does not
consult `response.isCommitted()`. `render()` then throws `ServletException:
Could not resolve view with name '...'` if that name does not resolve
(`DispatcherServlet` around the `if (mv != null && !mv.wasCleared())` check,
and the `Could not resolve view with name` throw in `render()`).
On this branch the skip signal is already `GrailsWebRequest.renderView`.
`UrlMappingsInfoHandlerAdapter` builds the default action view only when the
action result is null and `webRequest.renderView` is true (that property calls
`isRenderView()`). These branches already set the flag to false, so a null
return does not reach view resolution. The adapter ignores the flag when the
action returns a `Map`, and when `GrailsApplicationAttributes.MODEL_AND_VIEW`
is already set (`UrlMappingsInfoHandlerAdapter` lines 164-180). `render('ok');
return [foo: 'bar']` still selects the action view after this flush.
Committing here also has a cost. `GrailsExceptionResolver` only forwards to
the error handler when the response is still uncommitted, and later status or
header changes cannot take effect. `IncludeResponseWrapper.flushBuffer()` is a
no-op, so this call does not commit an included response either.
Please add a DispatcherServlet-level regression for the reported failure,
and make the handler adapter honor response-rendering suppression before it
builds an implicit view. Do not treat `flushBuffer()` as the signal that skips
view resolution.
##########
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:
This is the last new flush in `render(Map)`, but the same method still has a
content-writing branch that sets `renderView = false` and does not commit.
Immediately below, the `else if (!statusSet)` branch (lines 447-460) does:
```groovy
webRequest.renderView = false
// JSONElement: renderWritable(...) with no flushBuffer() after it
// otherwise:
response.writer.write(argMap.inspect())
```
`render([a: 1])` therefore stays uncommitted, unlike `render(someObject)` at
line 141. The existing `RenderMethodTests` already exercise this public Map
form, and the new spec does not check it.
If the policy is that every content-writing `renderView = false` branch
commits, this branch needs the same `flushBuffer()` and
`ControllerExecutionException` wrapping, plus a public-API test for `render([a:
1])` that checks both the body and `isCommitted()`.
--
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]
