From 4890ab89b59485841ff285c76c87e3732aee6427 Mon Sep 17 00:00:00 2001 From: Eric Deandrea Date: Tue, 29 Sep 2026 15:25:21 -0400 Subject: [PATCH] fix(client): log a non-JSON response body as it is With both logResponses() and prettyPrint() enabled, logging a response that is not JSON, such as an error page from a gateway in front of docling-serve, failed with a raw Jackson exception before the status code was checked, so the status code and body were lost. readValue now throws JsonReadException, a library-neutral exception that is not a DoclingServeClientException, for text that is not valid JSON or does not match the requested type. logResponse catches only that and logs the body unchanged; any other failure still propagates. References #709 Signed-off-by: Eric Deandrea --- .gitignore | 2 + .../serve/client/DoclingServeClient.java | 19 +++- .../client/DoclingServeJackson2Client.java | 2 +- .../client/DoclingServeJackson3Client.java | 8 +- .../serve/client/JsonReadException.java | 26 +++++ .../client/operations/HttpOperations.java | 22 ++-- .../AbstractDoclingServeClientTests.java | 107 ++++++++++++++++++ .../DoclingServeJackson2ClientTests.java | 24 ++++ .../DoclingServeJackson3ClientTests.java | 25 ++++ docs/src/doc/docs/whats-new.md | 1 + 10 files changed, 221 insertions(+), 15 deletions(-) create mode 100644 docling-serve/docling-serve-client/src/main/java/ai/docling/serve/client/JsonReadException.java diff --git a/.gitignore b/.gitignore index 03f1fb3d..301a9330 100644 --- a/.gitignore +++ b/.gitignore @@ -15,3 +15,5 @@ build bin/ !**/src/main/**/bin/ !**/src/test/**/bin/ + +.explyt/ diff --git a/docling-serve/docling-serve-client/src/main/java/ai/docling/serve/client/DoclingServeClient.java b/docling-serve/docling-serve-client/src/main/java/ai/docling/serve/client/DoclingServeClient.java index faffaa2a..e1d6cb71 100644 --- a/docling-serve/docling-serve-client/src/main/java/ai/docling/serve/client/DoclingServeClient.java +++ b/docling-serve/docling-serve-client/src/main/java/ai/docling/serve/client/DoclingServeClient.java @@ -174,7 +174,10 @@ public DoclingServeApiConfig config() { * @param valueType the {@link Class} of the target type; must not be {@code null} * @param the type of the object to be deserialized * @return an instance of {@code T} deserialized from the provided JSON - * @throws RuntimeException if the JSON parsing fails + * @throws JsonReadException if {@code json} is not valid JSON, or does not match {@code valueType}. Implementations + * must throw it, with the failure of their JSON library as its cause, for these failures + * only: any other failure, such as one of a custom deserializer, must propagate as it is, + * so that it is not mistaken for a text that simply cannot be read */ protected abstract T readValue(String json, Class valueType); @@ -233,12 +236,24 @@ protected void logResponse(HttpResponse response, Optional respo ); responseBody - .map(body -> this.config.prettyPrint() ? writeValueAsString(readValue(body, Object.class)) : body) + .map(body -> this.config.prettyPrint() ? prettyPrintForLog(body) : body) .ifPresent(body -> stringBuilder.append(" BODY:\n%s".formatted(body))); LOG.info(stringBuilder.toString()); } } + // A response is not always JSON (e.g. an error page from a gateway in front of docling-serve), and logging + // it must never replace the outcome of the request, so a body that is not JSON is logged as it is. + private String prettyPrintForLog(String body) { + try { + return writeValueAsString(readValue(body, Object.class)); + } + catch (JsonReadException e) { + LOG.debug("The response body is not JSON, so it is logged as it is", e); + return body; + } + } + protected T execute(HttpRequest request, Class expectedValueType) { if (this.config.logRequests()) { logRequest(request); diff --git a/docling-serve/docling-serve-client/src/main/java/ai/docling/serve/client/DoclingServeJackson2Client.java b/docling-serve/docling-serve-client/src/main/java/ai/docling/serve/client/DoclingServeJackson2Client.java index 14d140c0..db34bb4a 100644 --- a/docling-serve/docling-serve-client/src/main/java/ai/docling/serve/client/DoclingServeJackson2Client.java +++ b/docling-serve/docling-serve-client/src/main/java/ai/docling/serve/client/DoclingServeJackson2Client.java @@ -53,7 +53,7 @@ protected T readValue(String json, Class valueType) { return this.jsonMapper.readValue(json, valueType); } catch (JsonProcessingException e) { - throw new RuntimeException(e); + throw new JsonReadException(e); } } diff --git a/docling-serve/docling-serve-client/src/main/java/ai/docling/serve/client/DoclingServeJackson3Client.java b/docling-serve/docling-serve-client/src/main/java/ai/docling/serve/client/DoclingServeJackson3Client.java index f64ee57a..2fd33295 100644 --- a/docling-serve/docling-serve-client/src/main/java/ai/docling/serve/client/DoclingServeJackson3Client.java +++ b/docling-serve/docling-serve-client/src/main/java/ai/docling/serve/client/DoclingServeJackson3Client.java @@ -2,6 +2,7 @@ import static ai.docling.serve.api.util.ValidationUtils.ensureNotNull; +import tools.jackson.core.JacksonException; import tools.jackson.databind.json.JsonMapper; /** @@ -41,7 +42,12 @@ public static Builder builder() { @Override protected T readValue(String json, Class valueType) { - return this.jsonMapper.readValue(json, valueType); + try { + return this.jsonMapper.readValue(json, valueType); + } + catch (JacksonException e) { + throw new JsonReadException(e); + } } @Override diff --git a/docling-serve/docling-serve-client/src/main/java/ai/docling/serve/client/JsonReadException.java b/docling-serve/docling-serve-client/src/main/java/ai/docling/serve/client/JsonReadException.java new file mode 100644 index 00000000..d7fceb7c --- /dev/null +++ b/docling-serve/docling-serve-client/src/main/java/ai/docling/serve/client/JsonReadException.java @@ -0,0 +1,26 @@ +package ai.docling.serve.client; + +/** + * Exception thrown when a text cannot be read as the JSON value that was expected, either because it is not + * valid JSON at all (for example an error page from a gateway or proxy in front of Docling Serve), or because + * it is JSON of another shape. + * + *

It is what {@link DoclingServeClient#readValue(String, Class)} reports, whichever JSON library the client + * uses, so that code shared by every client can tell such a failure from any other one, such as a failure of a + * custom deserializer, without depending on a JSON library. The failure of the JSON library is its + * {@linkplain #getCause() cause}. + * + *

It is not a {@link DoclingServeClientException}, which reports the outcome of an HTTP exchange: reading a + * text as JSON has nothing to say about a status code or a response body. + */ +public class JsonReadException extends RuntimeException { + /** + * Constructs a new {@code JsonReadException} for the failure of a JSON library. + * + * @param cause the failure of the JSON library to read the text. The message of this exception is its + * {@link Throwable#toString() string representation}, which starts with the name of its class + */ + public JsonReadException(Throwable cause) { + super(cause); + } +} diff --git a/docling-serve/docling-serve-client/src/main/java/ai/docling/serve/client/operations/HttpOperations.java b/docling-serve/docling-serve-client/src/main/java/ai/docling/serve/client/operations/HttpOperations.java index 2250b641..bc9c215a 100644 --- a/docling-serve/docling-serve-client/src/main/java/ai/docling/serve/client/operations/HttpOperations.java +++ b/docling-serve/docling-serve-client/src/main/java/ai/docling/serve/client/operations/HttpOperations.java @@ -31,10 +31,10 @@ public abstract class HttpOperations { /** * Executes an HTTP GET request using the details specified in the provided {@code RequestContext}. * - * @param the type of the request payload - * @param the type of the response object + * @param the type of the request payload + * @param the type of the response object * @param requestContext the context containing details such as the URI, request payload, - * and expected response type of the GET operation + * and expected response type of the GET operation * @return an instance of the response type {@code O}, which represents the deserialized response data */ protected abstract O executeGet(RequestContext requestContext); @@ -42,9 +42,9 @@ public abstract class HttpOperations { /** * Executes an HTTP GET request using the details specified in the provided {@code RequestContext}. * - * @param the type of the request payload + * @param the type of the request payload * @param requestContext the context containing details such as the URI, request payload, - * and expected response type of the GET operation + * and expected response type of the GET operation * @return an instance of the {@link StreamResponse}, which represents the response. */ protected abstract StreamResponse executeGetWithStreamResponse(RequestContext requestContext); @@ -54,10 +54,10 @@ public abstract class HttpOperations { * This method is designed to be implemented by subclasses and facilitates sending POST requests * with a specified request payload and receiving a deserialized response. * - * @param the type of the request payload - * @param the type of the response object + * @param the type of the request payload + * @param the type of the response object * @param requestContext the context containing details such as the URI, request payload, and - * expected response type of the POST operation + * expected response type of the POST operation * @return an instance of the response type {@code O}, which represents the deserialized response data */ protected abstract O executePost(RequestContext requestContext); @@ -67,9 +67,9 @@ public abstract class HttpOperations { * This method is designed to be implemented by subclasses and facilitates sending POST requests * with a specified request payload and receiving a stream response. * - * @param the type of the request payload + * @param the type of the request payload * @param requestContext the context containing details such as the URI, request payload, and - * expected response type of the POST operation + * expected response type of the POST operation * @return an instance of the {@link StreamResponse}, which represents the response. */ protected abstract StreamResponse executePostWithStreamResponse(RequestContext requestContext); @@ -81,7 +81,7 @@ public abstract class HttpOperations { * @param valueType the {@link Class} of the target type; must not be {@code null} * @param the type of the object to be deserialized * @return an instance of {@code T} deserialized from the provided JSON - * @throws RuntimeException if the JSON parsing fails + * @throws ai.docling.serve.client.JsonReadException if {@code json} is not valid JSON, or does not match {@code valueType} */ protected abstract T readValue(String json, Class valueType); } diff --git a/docling-serve/docling-serve-client/src/test/java/ai/docling/serve/client/AbstractDoclingServeClientTests.java b/docling-serve/docling-serve-client/src/test/java/ai/docling/serve/client/AbstractDoclingServeClientTests.java index b5b690a1..2fb6740f 100644 --- a/docling-serve/docling-serve-client/src/test/java/ai/docling/serve/client/AbstractDoclingServeClientTests.java +++ b/docling-serve/docling-serve-client/src/test/java/ai/docling/serve/client/AbstractDoclingServeClientTests.java @@ -1,5 +1,6 @@ package ai.docling.serve.client; +import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; import static com.github.tomakehurst.wiremock.client.WireMock.equalTo; import static com.github.tomakehurst.wiremock.client.WireMock.equalToJson; import static com.github.tomakehurst.wiremock.client.WireMock.get; @@ -44,6 +45,7 @@ import org.assertj.core.api.InstanceOfAssertFactories; import org.jspecify.annotations.Nullable; +import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtensionContext; @@ -151,6 +153,16 @@ public void testFailed(ExtensionContext context, @Nullable Throwable cause) { protected abstract DoclingServeApi getDoclingClient(boolean requiresAuth, boolean useWiremock); + /** + * A client that logs and pretty-prints responses, and whose JSON mapper throws an {@link IllegalStateException} + * with the message {@code boom} when it deserializes an {@link Object}, which is what logging a response does + * to pretty-print it. It is a failure that is not caused by the text being something other than JSON. + * + *

The failure is registered for {@link Object} only because Jackson does not consult a deserializer + * registered for a type that has a builder, such as {@link HealthCheckResponse}, over the builder. + */ + protected abstract DoclingServeApi getDoclingClientWithFailingDeserializer(); + protected DoclingServeApi getDoclingClient(boolean requiresAuth) { return getDoclingClient(requiresAuth, false); } @@ -163,6 +175,10 @@ private T readValue(String json, Class valueType) { return ((DoclingServeClient) getDoclingClient()).readValue(json, valueType); } + private T readValue(DoclingServeApi client, String json, Class valueType) { + return ((DoclingServeClient) client).readValue(json, valueType); + } + private String writeValueAsString(T value) { return ((DoclingServeClient) getDoclingClient()).writeValueAsString(value); } @@ -448,6 +464,97 @@ void shouldSuccessfullyCallHealthEndpoint() { } } + // The clients used by these tests log responses and pretty-print them, which is when a response that is not + // JSON, such as an error page from a gateway in front of docling-serve, used to fail while it was being logged + @Nested + class NonJsonResponseTests { + @AfterEach + void resetStubs() { + getWiremockServer().resetAll(); + } + + @Test + void htmlErrorBodyKeepsStatusAndBody() { + stubHealth(502, "text/html", "Bad Gateway"); + + assertThatThrownBy(() -> getDoclingClient(false, true).health()) + .asInstanceOf(InstanceOfAssertFactories.type(DoclingServeClientException.class)) + .returns(502, DoclingServeClientException::getStatusCode) + .returns("Bad Gateway", DoclingServeClientException::getResponseBody); + } + + @Test + void plainTextErrorBodyKeepsStatusAndBody() { + stubHealth(500, "text/plain", "Internal Server Error"); + + assertThatThrownBy(() -> getDoclingClient(false, true).health()) + .asInstanceOf(InstanceOfAssertFactories.type(DoclingServeClientException.class)) + .returns(500, DoclingServeClientException::getStatusCode) + .returns("Internal Server Error", DoclingServeClientException::getResponseBody); + } + + @Test + void jsonErrorBodyKeepsStatusAndBody() { + stubHealth(500, "application/json", "{\"detail\":\"Something went wrong\"}"); + + assertThatThrownBy(() -> getDoclingClient(false, true).health()) + .asInstanceOf(InstanceOfAssertFactories.type(DoclingServeClientException.class)) + .returns(500, DoclingServeClientException::getStatusCode) + .returns("{\"detail\":\"Something went wrong\"}", DoclingServeClientException::getResponseBody); + } + + @Test + void readValueReadsJson() { + assertThat(readValue(getDoclingClient(), "{\"status\": \"ok\"}", HealthCheckResponse.class)) + .extracting(HealthCheckResponse::getStatus) + .isEqualTo("ok"); + } + + @Test + void readValueOfTextThatIsNotJsonThrowsJsonReadException() { + assertThatThrownBy(() -> readValue(getDoclingClient(), "Bad Gateway", Object.class)) + .isExactlyInstanceOf(JsonReadException.class) + .isNotInstanceOf(DoclingServeClientException.class) + .hasCauseInstanceOf(Exception.class); + } + + @Test + void readValueOfJsonOfAnotherShapeThrowsJsonReadException() { + assertThatThrownBy(() -> readValue(getDoclingClient(), "[1, 2]", HealthCheckResponse.class)) + .isExactlyInstanceOf(JsonReadException.class) + .hasCauseInstanceOf(Exception.class); + } + + @Test + void failureThatIsNotAParseFailureIsNotHiddenWhileLoggingAResponse() { + stubHealth(200, "application/json", "{\"status\": \"ok\"}"); + + assertThatThrownBy(() -> getDoclingClientWithFailingDeserializer().health()) + .isExactlyInstanceOf(IllegalStateException.class) + .hasMessage("boom"); + } + + @Test + void readValueLetsAFailureThatIsNotAParseFailurePropagate() { + assertThatThrownBy(() -> readValue(getDoclingClientWithFailingDeserializer(), "{\"status\": \"ok\"}", Object.class)) + .isExactlyInstanceOf(IllegalStateException.class) + .hasMessage("boom"); + } + + + private void stubHealth(int status, String contentType, String body) { + getWiremockServer().stubFor( + get(urlPathEqualTo("/health")) + .willReturn( + aResponse() + .withStatus(status) + .withHeader("Content-Type", contentType) + .withBody(body) + ) + ); + } + } + @Nested class ConvertTests { static void assertConvertSingleHttpSourceWithDefaultTarget(ConvertDocumentResponse response) { diff --git a/docling-serve/docling-serve-client/src/test/java/ai/docling/serve/client/DoclingServeJackson2ClientTests.java b/docling-serve/docling-serve-client/src/test/java/ai/docling/serve/client/DoclingServeJackson2ClientTests.java index 7f61ab91..e85be414 100644 --- a/docling-serve/docling-serve-client/src/test/java/ai/docling/serve/client/DoclingServeJackson2ClientTests.java +++ b/docling-serve/docling-serve-client/src/test/java/ai/docling/serve/client/DoclingServeJackson2ClientTests.java @@ -5,6 +5,11 @@ import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; +import com.fasterxml.jackson.core.JsonParser; +import com.fasterxml.jackson.databind.DeserializationContext; +import com.fasterxml.jackson.databind.JsonDeserializer; +import com.fasterxml.jackson.databind.json.JsonMapper; +import com.fasterxml.jackson.databind.module.SimpleModule; import com.github.tomakehurst.wiremock.WireMockServer; import ai.docling.serve.api.DoclingServeApi; @@ -50,4 +55,23 @@ protected DoclingServeApi getDoclingClient(boolean requiresAuth, boolean useWire return useWiremock ? wiremockDoclingClient : doclingClient; } + + @Override + protected DoclingServeApi getDoclingClientWithFailingDeserializer() { + var failingDeserializers = new SimpleModule().addDeserializer(Object.class, new FailingDeserializer<>()); + + return DoclingServeJackson2Client.builder() + .baseUrl(wireMockServer.baseUrl()) + .logResponses() + .prettyPrint() + .jsonParser(JsonMapper.builder().addModule(failingDeserializers)) + .build(); + } + + static class FailingDeserializer extends JsonDeserializer { + @Override + public T deserialize(JsonParser parser, DeserializationContext context) { + throw new IllegalStateException("boom"); + } + } } diff --git a/docling-serve/docling-serve-client/src/test/java/ai/docling/serve/client/DoclingServeJackson3ClientTests.java b/docling-serve/docling-serve-client/src/test/java/ai/docling/serve/client/DoclingServeJackson3ClientTests.java index dc8aa98b..40c022b7 100644 --- a/docling-serve/docling-serve-client/src/test/java/ai/docling/serve/client/DoclingServeJackson3ClientTests.java +++ b/docling-serve/docling-serve-client/src/test/java/ai/docling/serve/client/DoclingServeJackson3ClientTests.java @@ -2,6 +2,12 @@ import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.options; +import tools.jackson.core.JsonParser; +import tools.jackson.databind.DeserializationContext; +import tools.jackson.databind.ValueDeserializer; +import tools.jackson.databind.json.JsonMapper; +import tools.jackson.databind.module.SimpleModule; + import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; @@ -50,4 +56,23 @@ protected DoclingServeApi getDoclingClient(boolean requiresAuth, boolean useWire return useWiremock ? wiremockDoclingClient : doclingClient; } + + @Override + protected DoclingServeApi getDoclingClientWithFailingDeserializer() { + var failingDeserializers = new SimpleModule().addDeserializer(Object.class, new FailingDeserializer<>()); + + return DoclingServeJackson3Client.builder() + .baseUrl(wireMockServer.baseUrl()) + .logResponses() + .prettyPrint() + .jsonParser(JsonMapper.builder().addModule(failingDeserializers)) + .build(); + } + + static class FailingDeserializer extends ValueDeserializer { + @Override + public T deserialize(JsonParser parser, DeserializationContext context) { + throw new IllegalStateException("boom"); + } + } } diff --git a/docs/src/doc/docs/whats-new.md b/docs/src/doc/docs/whats-new.md index 7e87d493..93f2f498 100644 --- a/docs/src/doc/docs/whats-new.md +++ b/docs/src/doc/docs/whats-new.md @@ -31,6 +31,7 @@ Docling Java {{ gradle.project_version }} includes important breaking changes, a * **`toBuilder()` of the reference client keeps the timeouts and the redirect policy** — Copying a `DoclingServeJackson2Client` or `DoclingServeJackson3Client` with `toBuilder()` used to reset `connectTimeout` and `readTimeout` to their defaults, and stop following redirects. The copy now keeps them. Other `HttpClient` settings, such as a proxy or an SSL context, are still not copied. * **The builders of the reference client validate values when they are set** — Setting an invalid value on the builder of `DoclingServeJackson2Client` or `DoclingServeJackson3Client`, such as `connectTimeout(Duration.ZERO)` or `httpClientBuilder(null)`, now throws an `IllegalArgumentException` from the setter instead of from `build()`. The builders also gain `config(DoclingServeApiConfig)`, which applies a whole configuration at once, and `config()` on the reference client now reports only the options that were explicitly set. * **Custom `Executor` for async operations** — The async methods (`convertSourceAsync`, `convertSourceBatchAsync`, `convertFilesAsync`, `chunkSourceWith*ChunkerAsync`, ...) used to run on `CompletableFuture`'s default executor (usually `ForkJoinPool.commonPool()`). A new `asyncExecutor(Executor)` builder method lets you run them (task submission, status polling and result retrieval) on your own executor instead, e.g. a virtual-thread executor or one managed by your framework. When not set, the behavior is unchanged. The client never shuts the executor down. +* **Logging a response that is not JSON no longer hides the error** — With both `logResponses()` and `prettyPrint()` enabled, a response body that is not JSON, such as an error page from a gateway or proxy in front of `docling-serve`, made the client fail with a raw Jackson exception while it was logging the response, so the status code and body were lost. The body is now logged as it is, and the request reports the usual `DoclingServeClientException`. The `readValue(String, Class)` method that subclasses of `DoclingServeClient` implement must now throw the new `JsonReadException`, with the failure of the JSON library as its cause, when the text is not valid JSON or does not match the expected type. ### 0.6.6