You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@grails.apache.org by "jdaugherty (via GitHub)" <gi...@apache.org> on 2026/10/03 04:11:26 UTC

[PR] Keep mock response output lazy after reset [grails-core]

jdaugherty opened a new pull request, #16490:
URL: https://github.com/apache/grails-core/pull/16490

   ## Summary
   
   Keep `GrailsMockHttpServletResponse.reset()` from selecting a character writer before the next response chooses its output mode.
   
   The reset implementation currently calls `getWriter()` to replace the bound `GrailsWebRequest`'s cached output. Spring 7 enforces the servlet restriction against using both a writer and an output stream, so a subsequent binary or CSV download fails with `IllegalStateException: getWriter() has already been called`, even when the test reset the response before invoking the controller.
   
   Clear the cached web-request output with `setOut(null)` instead. `GrailsWebRequest.getOut()` already resolves the current response's writer lazily when text output is actually requested.
   
   ## Regression coverage
   
   Add tests with a bound `GrailsWebRequest` for:
   
   - Binary output after resetting a fresh response.
   - Binary output after resetting a response previously used for text.
   - Text output after resetting a response previously used for binary data.
   - Replacing a cached web-request writer without writing to the previous writer.
   
   This was diagnosed in a real controller integration test: the query and CSV generation succeeded, but the mock response had already acquired a writer during `reset()` before the download reached its output-stream write.
   
   ## Verification
   
   - `:grails-test-core:codeStyle` and `validateRepositoryConventions` passed.
   - `:grails-test-suite-uber:testAll`: **638 passed, 4 skipped, 0 failures** across the main and isolated test tasks.
   - `git diff --check` passed.
   - Full-repository test verification is left to CI.
   


-- 
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: notifications-unsubscribe@grails.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Keep mock response output lazy after reset [grails-core]

Posted by "jdaugherty (via GitHub)" <gi...@apache.org>.
jdaugherty commented on code in PR #16490:
URL: https://github.com/apache/grails-core/pull/16490#discussion_r4174499084


##########
grails-test-suite-uber/src/test/groovy/org/grails/plugins/testing/GrailsMockHttpServletResponseTests.groovy:
##########
@@ -19,15 +19,89 @@
 package org.grails.plugins.testing
 
 import org.grails.plugins.testing.GrailsMockHttpServletResponse
+import org.grails.web.servlet.mvc.GrailsWebRequest
 import org.junit.jupiter.api.Test
+import org.springframework.web.context.request.RequestAttributes
+import org.springframework.web.context.request.RequestContextHolder
 
+import static org.junit.jupiter.api.Assertions.assertArrayEquals
 import static org.junit.jupiter.api.Assertions.assertEquals
 import static org.junit.jupiter.api.Assertions.assertFalse
 
 /**
  * Test case for {@link org.grails.plugins.testing.GrailsMockHttpServletResponse}.
  */
 class GrailsMockHttpServletResponseTests {
+    @Test
+    void testResetDoesNotSelectAWriterBeforeBinaryOutput() {
+        withBoundWebRequest { response, webRequest ->
+            response.reset()
+
+            byte[] data = [0, 1, 127, -1] as byte[]
+            response.outputStream.write(data)
+
+            assertArrayEquals data, response.contentAsByteArray
+        }
+    }
+
+    @Test
+    void testResetAllowsBinaryOutputAfterText() {
+        withBoundWebRequest { response, webRequest ->
+            webRequest.out.write('previous text')
+            response.flushBuffer()
+
+            response.reset()
+            byte[] data = [0, 1, 127, -1] as byte[]
+            response.outputStream.write(data)
+
+            assertArrayEquals data, response.contentAsByteArray
+        }
+    }
+
+    @Test
+    void testResetAllowsTextOutputAfterBinary() {

Review Comment:
   Added `testResetUsesCharacterEncodingChosenAfterReset` in 8bd0204061. It writes text first so a writer exists, resets, sets UTF-8 through `response.characterEncoding`, writes `café €` through `webRequest.out`, and asserts the UTF-8 bytes. With the previous `reset()` it fails (6 bytes instead of 9) because the writer opened during reset was already set to ISO-8859-1.



-- 
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: notifications-unsubscribe@grails.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Keep mock response output lazy after reset [grails-core]

Posted by "jdaugherty (via GitHub)" <gi...@apache.org>.
jdaugherty commented on PR #16490:
URL: https://github.com/apache/grails-core/pull/16490#issuecomment-5965412752

   Noticed this when testing downloads in my application on 8.0.0


-- 
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: notifications-unsubscribe@grails.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Keep mock response output lazy after reset [grails-core]

Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #16490:
URL: https://github.com/apache/grails-core/pull/16490#issuecomment-5972276532

   <!-- Bito Reply -->
   The current test suite for `GrailsMockHttpServletResponse` verifies that `reset()` correctly clears the `webRequest` output, allowing for subsequent selection of either a writer or an output stream. To address the concern regarding character encoding, you can add a test case that sets the character encoding to UTF-8 after `reset()` and verifies that non-ASCII text is correctly handled.
   
   Here is an example of how you could implement this test:
   
   ```groovy
       @Test
       void testResetAllowsUtf8OutputAfterReset() {
           withBoundWebRequest { response, webRequest ->
               response.reset()
               response.characterEncoding = 'UTF-8'
               String text = '€'
               webRequest.out.write(text)
               webRequest.out.flush()
   
               assertArrayEquals text.getBytes('UTF-8'), response.contentAsByteArray
           }
       }
   ```
   
   This test ensures that the lazily opened writer respects the character encoding set after the `reset()` call.
   
   **grails-test-suite-uber/src/test/groovy/org/grails/plugins/testing/GrailsMockHttpServletResponseTests.groovy**
   ```
   @Test
       void testResetAllowsUtf8OutputAfterReset() {
           withBoundWebRequest { response, webRequest ->
               response.reset()
               response.characterEncoding = 'UTF-8'
               String text = '€'
               webRequest.out.write(text)
               webRequest.out.flush()
   
               assertArrayEquals text.getBytes('UTF-8'), response.contentAsByteArray
           }
       }
   ```


-- 
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: notifications-unsubscribe@grails.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Keep mock response output lazy after reset [grails-core]

Posted by "jamesfredley (via GitHub)" <gi...@apache.org>.
jamesfredley commented on code in PR #16490:
URL: https://github.com/apache/grails-core/pull/16490#discussion_r4174363341


##########
grails-test-suite-uber/src/test/groovy/org/grails/plugins/testing/GrailsMockHttpServletResponseTests.groovy:
##########
@@ -19,15 +19,89 @@
 package org.grails.plugins.testing
 
 import org.grails.plugins.testing.GrailsMockHttpServletResponse
+import org.grails.web.servlet.mvc.GrailsWebRequest
 import org.junit.jupiter.api.Test
+import org.springframework.web.context.request.RequestAttributes
+import org.springframework.web.context.request.RequestContextHolder
 
+import static org.junit.jupiter.api.Assertions.assertArrayEquals
 import static org.junit.jupiter.api.Assertions.assertEquals
 import static org.junit.jupiter.api.Assertions.assertFalse
 
 /**
  * Test case for {@link org.grails.plugins.testing.GrailsMockHttpServletResponse}.
  */
 class GrailsMockHttpServletResponseTests {
+    @Test
+    void testResetDoesNotSelectAWriterBeforeBinaryOutput() {
+        withBoundWebRequest { response, webRequest ->
+            response.reset()
+
+            byte[] data = [0, 1, 127, -1] as byte[]
+            response.outputStream.write(data)
+
+            assertArrayEquals data, response.contentAsByteArray
+        }
+    }
+
+    @Test
+    void testResetAllowsBinaryOutputAfterText() {
+        withBoundWebRequest { response, webRequest ->
+            webRequest.out.write('previous text')
+            response.flushBuffer()
+
+            response.reset()
+            byte[] data = [0, 1, 127, -1] as byte[]
+            response.outputStream.write(data)
+
+            assertArrayEquals data, response.contentAsByteArray
+        }
+    }
+
+    @Test
+    void testResetAllowsTextOutputAfterBinary() {

Review Comment:
   The post-reset text assertions use only ASCII (`new text`). They do not show that a charset chosen after `reset()` still reaches the lazily opened writer, rather than a writer that already captured the default encoding.
   
   Non-blocking follow-up: reset, set UTF-8 through the public response API, write non-ASCII text, and assert the UTF-8 bytes. The writer/stream selection bug itself is already covered.



-- 
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: notifications-unsubscribe@grails.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Keep mock response output lazy after reset [grails-core]

Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #16490:
URL: https://github.com/apache/grails-core/pull/16490#issuecomment-5966122684

   ## ✅ All tests passed ✅
   
   🏷️ Commit: f220260869a176898f7f488a1f0ef9b0cca5ffe7
   ▶️ Tests:  107715 executed
   ⚪️ Checks: 87/87 completed
   
   ---
   _Learn more about TestLens at [testlens.app/docs](https://testlens.app/docs/features/pr-comment/)._
   


-- 
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: notifications-unsubscribe@grails.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Keep mock response output lazy after reset [grails-core]

Posted by "jdaugherty (via GitHub)" <gi...@apache.org>.
jdaugherty merged PR #16490:
URL: https://github.com/apache/grails-core/pull/16490


-- 
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: notifications-unsubscribe@grails.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Keep mock response output lazy after reset [grails-core]

Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #16490:
URL: https://github.com/apache/grails-core/pull/16490#issuecomment-5965693264

   ## [Codecov](https://app.codecov.io/gh/apache/grails-core/pull/16490?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   :x: Patch coverage is `0%` with `1 line` in your changes missing coverage. Please review.
   :white_check_mark: Project coverage is 59.2854%. Comparing base ([`80a1ff5`](https://app.codecov.io/gh/apache/grails-core/commit/80a1ff5637600e6e5fb9c34d983e9a734584c6b3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`f220260`](https://app.codecov.io/gh/apache/grails-core/commit/f220260869a176898f7f488a1f0ef9b0cca5ffe7?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   
   | [Files with missing lines](https://app.codecov.io/gh/apache/grails-core/pull/16490?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
   |---|---|---|
   | [...sting/AbstractGrailsMockHttpServletResponse.groovy](https://app.codecov.io/gh/apache/grails-core/pull/16490?src=pr&el=tree&filepath=grails-test-core%2Fsrc%2Fmain%2Fgroovy%2Forg%2Fgrails%2Fplugins%2Ftesting%2FAbstractGrailsMockHttpServletResponse.groovy&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z3JhaWxzLXRlc3QtY29yZS9zcmMvbWFpbi9ncm9vdnkvb3JnL2dyYWlscy9wbHVnaW5zL3Rlc3RpbmcvQWJzdHJhY3RHcmFpbHNNb2NrSHR0cFNlcnZsZXRSZXNwb25zZS5ncm9vdnk=) | 0.0000% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/grails-core/pull/16490?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   
   <details><summary>Additional details and impacted files</summary>
   
   
   
   [![Impacted file tree graph](https://app.codecov.io/gh/apache/grails-core/pull/16490/graphs/tree.svg?width=650&height=150&src=pr&token=qJrryhpk7u&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)](https://app.codecov.io/gh/apache/grails-core/pull/16490?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   ```diff
   @@                Coverage Diff                 @@
   ##                8.0.x     #16490        +/-   ##
   ==================================================
   + Coverage     59.2827%   59.2854%   +0.0028%     
   + Complexity      24770      24769         -1     
   ==================================================
     Files            2170       2170                
     Lines          108121     108121                
     Branches        19709      19709                
   ==================================================
   + Hits            64097      64100         +3     
   + Misses          35147      35143         -4     
   - Partials         8877       8878         +1     
   ```
   
   | [Files with missing lines](https://app.codecov.io/gh/apache/grails-core/pull/16490?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [...sting/AbstractGrailsMockHttpServletResponse.groovy](https://app.codecov.io/gh/apache/grails-core/pull/16490?src=pr&el=tree&filepath=grails-test-core%2Fsrc%2Fmain%2Fgroovy%2Forg%2Fgrails%2Fplugins%2Ftesting%2FAbstractGrailsMockHttpServletResponse.groovy&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z3JhaWxzLXRlc3QtY29yZS9zcmMvbWFpbi9ncm9vdnkvb3JnL2dyYWlscy9wbHVnaW5zL3Rlc3RpbmcvQWJzdHJhY3RHcmFpbHNNb2NrSHR0cFNlcnZsZXRSZXNwb25zZS5ncm9vdnk=) | `12.0000% <0.0000%> (ø)` | |
   
   ... and [3 files with indirect coverage changes](https://app.codecov.io/gh/apache/grails-core/pull/16490/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   </details>
   <details><summary> :rocket: New features to boost your workflow: </summary>
   
   - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
   - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
   </details>


-- 
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: notifications-unsubscribe@grails.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org