fix(client): log a non-JSON response body as it is

.gitignore

@@ -15,3 +15,5 @@ build
 bin/
 !**/src/main/**/bin/
 !**/src/test/**/bin/
+
+.explyt/

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 <T>       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> T readValue(String json, Class<T> valueType);
 
@@ -233,12 +236,24 @@ protected void logResponse(HttpResponse<String> response, Optional<String> 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> T execute(HttpRequest request, Class<T> expectedValueType) {
     if (this.config.logRequests()) {
       logRequest(request);

docling-serve/docling-serve-client/src/main/java/ai/docling/serve/client/DoclingServeJackson2Client.java

@@ -53,7 +53,7 @@ protected <T> T readValue(String json, Class<T> valueType) {
       return this.jsonMapper.readValue(json, valueType);
     }
     catch (JsonProcessingException e) {
-      throw new RuntimeException(e);
+      throw new JsonReadException(e);
     }
   }
 

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> T readValue(String json, Class<T> valueType) {
-    return this.jsonMapper.readValue(json, valueType);
+    try {
+      return this.jsonMapper.readValue(json, valueType);
+    }
+    catch (JacksonException e) {
+      throw new JsonReadException(e);
+    }
   }
 
   @Override

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.
+ *
+ * <p>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}.
+ *
+ * <p>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);
+  }
+}

docling-serve/docling-serve-client/src/main/java/ai/docling/serve/client/operations/HttpOperations.java

@@ -31,20 +31,20 @@ public abstract class HttpOperations {
   /**
    * Executes an HTTP GET request using the details specified in the provided {@code RequestContext}.
    *
-   * @param <I> the type of the request payload
-   * @param <O> the type of the response object
+   * @param <I>            the type of the request payload
+   * @param <O>            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 <I, O> O executeGet(RequestContext<I, O> requestContext);
 
   /**
    * Executes an HTTP GET request using the details specified in the provided {@code RequestContext}.
    *
-   * @param <I> the type of the request payload
+   * @param <I>            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 <I> StreamResponse executeGetWithStreamResponse(RequestContext<I, StreamResponse> 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 <I> the type of the request payload
-   * @param <O> the type of the response object
+   * @param <I>            the type of the request payload
+   * @param <O>            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 <I, O> O executePost(RequestContext<I, O> 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 <I> the type of the request payload
+   * @param <I>            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 <I> StreamResponse executePostWithStreamResponse(RequestContext<I, StreamResponse> requestContext);
@@ -81,7 +81,7 @@ public abstract class HttpOperations {
    * @param valueType the {@link Class} of the target type; must not be {@code null}
    * @param <T>       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> T readValue(String json, Class<T> valueType);
 }

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.
+   *
+   * <p>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> T readValue(String json, Class<T> valueType) {
     return ((DoclingServeClient) getDoclingClient()).readValue(json, valueType);
   }
 
+  private <T> T readValue(DoclingServeApi client, String json, Class<T> valueType) {
+    return ((DoclingServeClient) client).readValue(json, valueType);
+  }
+
   private <T> 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", "<html>Bad Gateway</html>");
+
+      assertThatThrownBy(() -> getDoclingClient(false, true).health())
+          .asInstanceOf(InstanceOfAssertFactories.type(DoclingServeClientException.class))
+          .returns(502, DoclingServeClientException::getStatusCode)
+          .returns("<html>Bad Gateway</html>", 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(), "<html>Bad Gateway</html>", 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) {

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;
@@ -28,8 +33,8 @@
         .baseUrl(doclingContainer.getApiUrl())
         .build();
 
     authDoclingClient = doclingClient.toBuilder().apiKey("key").build();
     wiremockDoclingClient = doclingClient.toBuilder().baseUrl(wireMockServer.baseUrl()).build();
   }
 
   @AfterAll
@@ -50,4 +55,23 @@
 
     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<T> extends JsonDeserializer<T> {
+    @Override
+    public T deserialize(JsonParser parser, DeserializationContext context) {
+      throw new IllegalStateException("boom");
+    }
+  }
 }

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;
 
@@ -28,8 +34,8 @@
         .baseUrl(doclingContainer.getApiUrl())
         .build();
 
     authDoclingClient = doclingClient.toBuilder().apiKey("key").build();
     wiremockDoclingClient = doclingClient.toBuilder().baseUrl(wireMockServer.baseUrl()).build();
   }
 
   @AfterAll
@@ -50,4 +56,23 @@
 
     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<T> extends ValueDeserializer<T> {
+    @Override
+    public T deserialize(JsonParser parser, DeserializationContext context) {
+      throw new IllegalStateException("boom");
+    }
+  }
 }

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