Summary
When logResponses() and prettyPrint() are both enabled, DoclingServeClient.logResponse(...) tries to parse every response body as JSON so it can pretty-print it. If the body is not JSON (for example an HTML error page from a gateway or proxy in front of docling-serve), that parse throws, and the exception replaces the real error. The caller gets a raw Jackson exception instead of a DoclingServeClientException, and the HTTP status code and response body are lost.
Reproduction
Tested on main (174a56e) with a WireMock stub for GET /health, on both DoclingServeJackson2Client and DoclingServeJackson3Client, with logResponses() enabled:
| Status |
Content-Type |
Body |
prettyPrint() |
Result |
| 502 |
text/html |
<html>Bad Gateway</html> |
off |
DoclingServeClientException (status 502, body kept) |
| 502 |
text/html |
<html>Bad Gateway</html> |
on |
Jackson 2: RuntimeException wrapping JsonParseException; Jackson 3: StreamReadException |
| 500 |
text/plain |
Internal Server Error |
on |
same as above |
| 422 |
text/html |
<html>x</html> |
on |
same as above |
So this is not specific to 422: any non-JSON response body fails this way when both options are on.
Cause
logResponse runs from getResponse before the status code is checked:
responseBody
.map(body -> this.config.prettyPrint() ? writeValueAsString(readValue(body, Object.class)) : body)readValue(body, Object.class) throws for a body that is not valid JSON, and nothing catches it.
Suggested fix
Make the pretty-printing step best-effort: if the body cannot be parsed and re-serialized, log the raw body instead. A broad catch (RuntimeException) is appropriate here. It is only formatting for a log line, so it has no reason to tell parse failures from other failures, and the base class cannot name the Jackson exception types anyway (only one Jackson version may be on the classpath).
Logging must never change the outcome of the request.
Tests
The shared clients in AbstractDoclingServeClientTests are built with .logRequests().logResponses().prettyPrint(), so a regression test belongs there and will run against both Jackson backends: a WireMock stub returning 502 with an HTML body, asserting a DoclingServeClientException with status 502 and the original body.
Related
Found while reviewing #704, which adds a fallback to DoclingServeClientException for a 422 whose body is not a validation error. That fallback is bypassed for clients with both options enabled, because this code throws first.
Summary
When
logResponses()andprettyPrint()are both enabled,DoclingServeClient.logResponse(...)tries to parse every response body as JSON so it can pretty-print it. If the body is not JSON (for example an HTML error page from a gateway or proxy in front of docling-serve), that parse throws, and the exception replaces the real error. The caller gets a raw Jackson exception instead of aDoclingServeClientException, and the HTTP status code and response body are lost.Reproduction
Tested on
main(174a56e) with a WireMock stub forGET /health, on bothDoclingServeJackson2ClientandDoclingServeJackson3Client, withlogResponses()enabled:prettyPrint()text/html<html>Bad Gateway</html>DoclingServeClientException(status 502, body kept)text/html<html>Bad Gateway</html>RuntimeExceptionwrappingJsonParseException; Jackson 3:StreamReadExceptiontext/plainInternal Server Errortext/html<html>x</html>So this is not specific to 422: any non-JSON response body fails this way when both options are on.
Cause
logResponseruns fromgetResponsebefore the status code is checked:readValue(body, Object.class)throws for a body that is not valid JSON, and nothing catches it.Suggested fix
Make the pretty-printing step best-effort: if the body cannot be parsed and re-serialized, log the raw body instead. A broad
catch (RuntimeException)is appropriate here. It is only formatting for a log line, so it has no reason to tell parse failures from other failures, and the base class cannot name the Jackson exception types anyway (only one Jackson version may be on the classpath).Logging must never change the outcome of the request.
Tests
The shared clients in
AbstractDoclingServeClientTestsare built with.logRequests().logResponses().prettyPrint(), so a regression test belongs there and will run against both Jackson backends: a WireMock stub returning502with an HTML body, asserting aDoclingServeClientExceptionwith status502and the original body.Related
Found while reviewing #704, which adds a fallback to
DoclingServeClientExceptionfor a422whose body is not a validation error. That fallback is bypassed for clients with both options enabled, because this code throws first.