Review comments

Author: CharlesDuboisSAP

core/src/main/java/com/sap/ai/sdk/core/commons/ClientResponseHandler.java

@@ -33,12 +33,12 @@
 @RequiredArgsConstructor
 public class ClientResponseHandler<T, E extends ClientException>
     implements HttpClientResponseHandler<T> {
-  @Nonnull private final Class<T> responseType;
+  @Nonnull final Class<T> responseType;
   @Nonnull private final Class<? extends ClientError> errorType;
-  @Nonnull private final BiFunction<String, Throwable, E> exceptionType;
+  @Nonnull final BiFunction<String, Throwable, E> exceptionType;
 
   /** The parses for JSON responses, will be private once we can remove mixins */
-  @Nonnull private ObjectMapper JACKSON = getDefaultObjectMapper();
+  @Nonnull ObjectMapper objectMapper = getDefaultObjectMapper();
 
   /**
    * Set the {@link ObjectMapper} to use for parsing JSON responses.
@@ -48,7 +48,7 @@ public class ClientResponseHandler<T, E extends ClientException>
   @Beta
   @Nonnull
   public ClientResponseHandler<T, E> objectMapper(@Nonnull final ObjectMapper jackson) {
-    JACKSON = jackson;
+    objectMapper = jackson;
     return this;
   }
 
@@ -79,7 +79,7 @@ private T parseResponse(@Nonnull final ClassicHttpResponse response) throws E {
     val content = getContent(responseEntity);
     log.debug("Parsing response from JSON response: {}", content);
     try {
-      return JACKSON.readValue(content, responseType);
+      return objectMapper.readValue(content, responseType);
     } catch (final JsonProcessingException e) {
       log.error("Failed to parse the following response: {}", content);
       throw exceptionType.apply("Failed to parse response", e);
@@ -138,7 +138,7 @@ public void buildExceptionAndThrow(@Nonnull final ClassicHttpResponse response)
    */
   public void parseErrorAndThrow(
       @Nonnull final String errorResponse, @Nonnull final E baseException) throws E {
-    val maybeError = Try.of(() -> JACKSON.readValue(errorResponse, errorType));
+    val maybeError = Try.of(() -> objectMapper.readValue(errorResponse, errorType));
     if (maybeError.isFailure()) {
       baseException.addSuppressed(maybeError.getCause());
       throw baseException;

core/src/main/java/com/sap/ai/sdk/core/commons/ClientStreamingHandler.java

@@ -1,7 +1,5 @@
 package com.sap.ai.sdk.core.commons;
 
-import static com.sap.ai.sdk.core.JacksonConfiguration.getDefaultObjectMapper;
-
 import com.fasterxml.jackson.databind.ObjectMapper;
 import java.io.IOException;
 import java.util.function.BiFunction;
@@ -18,14 +16,8 @@
  * @since 1.1.0
  */
 @Slf4j
-public class ClientStreamingHandler<D extends StreamedDelta, E extends ClientException> {
-
-  @Nonnull private final Class<D> deltaType;
-  @Nonnull private final ClientResponseHandler<? extends ClientError, E> errorHandler;
-  @Nonnull private final BiFunction<String, Throwable, E> exceptionType;
-
-  /** The parses for JSON responses, will be private once we can remove mixins */
-  @Nonnull private ObjectMapper JACKSON = getDefaultObjectMapper();
+public class ClientStreamingHandler<D extends StreamedDelta, E extends ClientException>
+    extends ClientResponseHandler<D, E> {
 
   /**
    * Set the {@link ObjectMapper} to use for parsing JSON responses.
@@ -34,8 +26,7 @@ public class ClientStreamingHandler<D extends StreamedDelta, E extends ClientExc
    */
   @Nonnull
   public ClientStreamingHandler<D, E> objectMapper(@Nonnull final ObjectMapper jackson) {
-    JACKSON = jackson;
-    errorHandler.objectMapper(jackson);
+    super.objectMapper(jackson);
     return this;
   }
 
@@ -50,9 +41,7 @@ public ClientStreamingHandler(
       @Nonnull final Class<D> deltaType,
       @Nonnull final Class<? extends ClientError> errorType,
       @Nonnull final BiFunction<String, Throwable, E> exceptionType) {
-    this.deltaType = deltaType;
-    this.exceptionType = exceptionType;
-    this.errorHandler = new ClientResponseHandler<>(errorType, errorType, exceptionType);
+    super(deltaType, errorType, exceptionType);
   }
 
   /**
@@ -65,9 +54,9 @@ public ClientStreamingHandler(
    */
   @SuppressWarnings("PMD.CloseResource") // Stream is closed automatically when consumed
   @Nonnull
-  public Stream<D> handleResponse(@Nonnull final ClassicHttpResponse response) throws E {
+  public Stream<D> handleStreamingResponse(@Nonnull final ClassicHttpResponse response) throws E {
     if (response.getCode() >= 300) {
-      errorHandler.buildExceptionAndThrow(response);
+      super.buildExceptionAndThrow(response);
     }
     return IterableStreamConverter.lines(response.getEntity(), exceptionType)
         // half of the lines are empty newlines, the last line is "data: [DONE]"
@@ -76,14 +65,14 @@ public Stream<D> handleResponse(@Nonnull final ClassicHttpResponse response) thr
             line -> {
               if (!line.startsWith("data: ")) {
                 final String msg = "Failed to parse response";
-                errorHandler.parseErrorAndThrow(line, exceptionType.apply(msg, null));
+                super.parseErrorAndThrow(line, exceptionType.apply(msg, null));
               }
             })
         .map(
             line -> {
               final String data = line.substring(5); // remove "data: "
               try {
-                return JACKSON.readValue(data, deltaType);
+                return objectMapper.readValue(data, responseType);
               } catch (final IOException e) { // exception message e gets lost
                 log.error("Failed to parse the following response: {}", line);
                 throw exceptionType.apply("Failed to parse delta message: " + line, e);

foundation-models/openai/pom.xml

@@ -33,12 +33,12 @@
   </developers>
   <properties>
     <project.rootdir>${project.basedir}/../../</project.rootdir>
-    <coverage.complexity>70%</coverage.complexity>
-    <coverage.line>76%</coverage.line>
-    <coverage.instruction>75%</coverage.instruction>
-    <coverage.branch>66%</coverage.branch>
+    <coverage.complexity>71%</coverage.complexity>
+    <coverage.line>80%</coverage.line>
+    <coverage.instruction>76%</coverage.instruction>
+    <coverage.branch>69%</coverage.branch>
     <coverage.method>83%</coverage.method>
-    <coverage.class>85%</coverage.class>
+    <coverage.class>84%</coverage.class>
   </properties>
   <dependencies>
     <dependency>

foundation-models/openai/src/main/java/com/sap/ai/sdk/foundationmodels/openai/OpenAiClient.java

@@ -300,7 +300,7 @@ private <D extends StreamedDelta> Stream<D> streamRequest(
     try {
       final var client = ApacheHttpClient5Accessor.getHttpClient(destination);
       return new ClientStreamingHandler<>(deltaType, OpenAiError.class, OpenAiClientException::new)
-          .handleResponse(client.executeOpen(null, request, null));
+          .handleStreamingResponse(client.executeOpen(null, request, null));
     } catch (final IOException e) {
       throw new OpenAiClientException("Request to OpenAI model failed", e);
     }

orchestration/pom.xml

@@ -32,10 +32,10 @@
   <properties>
     <project.rootdir>${project.basedir}/../</project.rootdir>
     <coverage.complexity>70%</coverage.complexity>
-    <coverage.line>86%</coverage.line>
-    <coverage.instruction>86%</coverage.instruction>
-    <coverage.branch>67%</coverage.branch>
-    <coverage.method>85%</coverage.method>
+    <coverage.line>87%</coverage.line>
+    <coverage.instruction>88%</coverage.instruction>
+    <coverage.branch>65%</coverage.branch>
+    <coverage.method>65%</coverage.method>
     <coverage.class>100%</coverage.class>
   </properties>
 

orchestration/src/main/java/com/sap/ai/sdk/orchestration/OrchestrationClient.java

@@ -288,7 +288,7 @@ private <D extends StreamedDelta> Stream<D> streamRequest(
       return new ClientStreamingHandler<>(
               deltaType, OrchestrationError.class, OrchestrationClientException::new)
           .objectMapper(JACKSON)
-          .handleResponse(client.executeOpen(null, request, null));
+          .handleStreamingResponse(client.executeOpen(null, request, null));
     } catch (final IOException e) {
       throw new OrchestrationClientException("Request to the Orchestration service failed", e);
     }