fix(client): detect streamed ClickHouse exceptions#331
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #331 +/- ##
==========================================
+ Coverage 94.33% 94.57% +0.23%
==========================================
Files 42 42
Lines 742 774 +32
==========================================
+ Hits 700 732 +32
Misses 42 42 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR improves error handling by detecting ClickHouse “streamed exception” blocks that can appear inside HTTP 200 response bodies, ensuring sync/async clients raise ServerError before attempting to parse output.
Changes:
- Extend
ServerErrorparsing and addServerError::bodyContainsStreamedException()for streamed exception detection. - Add streamed-exception detection to both
PsrClickHouseClientandPsrClickHouseAsyncClientfor HTTP 200 responses (with opt-out forselectStream). - Add regression tests covering streamed exception detection and parsing for sync/async clients.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Exception/ServerErrorTest.php | Adds parsing/detection coverage for streamed exception bodies. |
| tests/Client/PsrClickHouseClientStreamedExceptionTest.php | Adds sync/async client regression tests for streamed exception handling and selectStream behavior. |
| src/Exception/ServerError.php | Improves error code parsing to match “Code:” not only at start and adds streamed exception detection helper. |
| src/Client/PsrClickHouseClient.php | Adds streamed exception detection for HTTP 200 responses (disabled for streaming select). |
| src/Client/PsrClickHouseAsyncClient.php | Adds streamed exception detection for HTTP 200 responses in async flow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $bodyContent = $response->getBody()->__toString(); | ||
| if ( | ||
| ServerError::bodyContainsStreamedException( | ||
| $bodyContent, | ||
| $response->getHeaderLine('X-ClickHouse-Exception-Tag'), | ||
| ) | ||
| ) { | ||
| throw ServerError::fromResponse($response); | ||
| } | ||
|
|
||
| return $response; |
There was a problem hiding this comment.
Addressed in 804ee0d. The inspected body is now captured into a fresh string-backed response body before returning or throwing, so non-seekable streams are not lost after detection. Added regression coverage using a non-seekable read-once stream.
| $bodyContent = $response->getBody()->__toString(); | ||
| if ( | ||
| ServerError::bodyContainsStreamedException( | ||
| $bodyContent, | ||
| $response->getHeaderLine('X-ClickHouse-Exception-Tag'), | ||
| ) | ||
| ) { | ||
| throw ServerError::fromResponse($response); | ||
| } |
There was a problem hiding this comment.
Addressed in 804ee0d. Async detection now also replaces the consumed response body with a fresh stream before reusing the ResponseInterface, with non-seekable stream coverage in the async regression test.
| fwrite($body, $bodyContent); | ||
| rewind($body); |
| fwrite($body, $bodyContent); | ||
| rewind($body); |
Summary
Notes