Reduce duplication of ResponseHandlers (#244)

* Orchestration streaming first version * Added unit tests * Formatting * Added documentation * Added tests * Release notes * Applied Alex's review comments * Reduce duplication of ResponseHandlers * OpenAiStreamingHandler * fix spotbugs * Apply suggestions from code review * Formatting * Add Beta annotation to new public API * Minor code reduction * Formatting * Minor change --------- Co-authored-by: SAP Cloud SDK Bot <cloudsdk@sap.com> Co-authored-by: Alexander Dümont <22489773+newtork@users.noreply.github.com> Co-authored-by: Alexander Dümont <alexander_duemont@web.de>
SAP · Dec 23, 2024 · e173db5 · e173db5
1 parent b183e81
commit e173db5
Show file tree

Hide file tree

Showing 17 changed files with 211 additions and 211 deletions.
diff --git a/core/pom.xml b/core/pom.xml
@@ -30,6 +30,12 @@
   </developers>
   <properties>
     <project.rootdir>${project.basedir}/../</project.rootdir>
+    <coverage.complexity>60%</coverage.complexity>
+    <coverage.line>69%</coverage.line>
+    <coverage.instruction>68%</coverage.instruction>
+    <coverage.branch>52%</coverage.branch>
+    <coverage.method>73%</coverage.method>
+    <coverage.class>87%</coverage.class>
   </properties>
 
   <dependencies>
@@ -66,6 +72,10 @@
       <groupId>org.apache.httpcomponents.client5</groupId>
       <artifactId>httpclient5</artifactId>
     </dependency>
+    <dependency>
+      <groupId>org.apache.httpcomponents.core5</groupId>
+      <artifactId>httpcore5</artifactId>
+    </dependency>
     <dependency>
       <groupId>com.google.code.findbugs</groupId>
       <artifactId>jsr305</artifactId>
@@ -129,11 +139,6 @@
       <artifactId>wiremock</artifactId>
       <scope>test</scope>
     </dependency>
-    <dependency>
-      <groupId>org.apache.httpcomponents.core5</groupId>
-      <artifactId>httpcore5</artifactId>
-      <scope>test</scope>
-    </dependency>
     <dependency>
       <groupId>org.assertj</groupId>
       <artifactId>assertj-core</artifactId>

diff --git a/core/src/main/java/com/sap/ai/sdk/core/commons/ClientError.java b/core/src/main/java/com/sap/ai/sdk/core/commons/ClientError.java
@@ -0,0 +1,21 @@
+package com.sap.ai.sdk.core.commons;
+
+import com.google.common.annotations.Beta;
+import javax.annotation.Nullable;
+
+/**
+ * Generic class that contains a JSON error response.
+ *
+ * @since 1.1.0
+ */
+@Beta
+@FunctionalInterface
+public interface ClientError {
+  /**
+   * Get the error message.
+   *
+   * @return The error message
+   */
+  @Nullable
+  String getMessage();
+}
diff --git a/core/src/main/java/com/sap/ai/sdk/core/commons/ClientException.java b/core/src/main/java/com/sap/ai/sdk/core/commons/ClientException.java
@@ -0,0 +1,13 @@
+package com.sap.ai.sdk.core.commons;
+
+import com.google.common.annotations.Beta;
+import lombok.experimental.StandardException;
+
+/**
+ * Generic exception for errors occurring when using AI SDK clients.
+ *
+ * @since 1.1.0
+ */
+@Beta
+@StandardException
+public class ClientException extends RuntimeException {}
diff --git a/...tration/OrchestrationResponseHandler.java → ...k/core/commons/ClientResponseHandler.java b/...tration/OrchestrationResponseHandler.java → ...k/core/commons/ClientResponseHandler.java
@@ -1,12 +1,15 @@
-package com.sap.ai.sdk.orchestration;
+package com.sap.ai.sdk.core.commons;
 
-import static com.sap.ai.sdk.orchestration.OrchestrationClient.JACKSON;
+import static com.sap.ai.sdk.core.JacksonConfiguration.getDefaultObjectMapper;
 
 import com.fasterxml.jackson.core.JsonProcessingException;
-import com.sap.ai.sdk.orchestration.model.ErrorResponse;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.annotations.Beta;
 import io.vavr.control.Try;
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
+import java.util.Objects;
+import java.util.function.BiFunction;
 import javax.annotation.Nonnull;
 import lombok.RequiredArgsConstructor;
 import lombok.extern.slf4j.Slf4j;
@@ -18,28 +21,80 @@
 import org.apache.hc.core5.http.io.HttpClientResponseHandler;
 import org.apache.hc.core5.http.io.entity.EntityUtils;
 
+/**
+ * Parse incoming JSON responses and handles any errors. For internal use only.
+ *
+ * @param <T> The type of the response.
+ * @param <E> The type of the exception to throw.
+ * @since 1.1.0
+ */
+@Beta
 @Slf4j
 @RequiredArgsConstructor
-class OrchestrationResponseHandler<T> implements HttpClientResponseHandler<T> {
+public class ClientResponseHandler<T, E extends ClientException>
+    implements HttpClientResponseHandler<T> {
   @Nonnull private final Class<T> responseType;
+  @Nonnull private final Class<? extends ClientError> errorType;
+  @Nonnull private final BiFunction<String, Throwable, E> exceptionType;
 
+  /** The parses for JSON responses, will be private once we can remove mixins */
+  @Nonnull public ObjectMapper JACKSON = getDefaultObjectMapper();
+
+  /**
+   * Processes a {@link ClassicHttpResponse} and returns some value corresponding to that response.
+   *
+   * @param response The response to process
+   * @return A model class instantiated from the response
+   * @throws E in case of a problem or the connection was aborted
+   */
+  @Nonnull
+  @Override
+  public T handleResponse(@Nonnull final ClassicHttpResponse response) throws E {
+    if (response.getCode() >= 300) {
+      buildExceptionAndThrow(response);
+    }
+    return parseResponse(response);
+  }
+
+  // The InputStream of the HTTP entity is closed by EntityUtils.toString
+  @SuppressWarnings("PMD.CloseResource")
   @Nonnull
-  private static String getContent(@Nonnull final HttpEntity entity) {
+  private T parseResponse(@Nonnull final ClassicHttpResponse response) throws E {
+    final HttpEntity responseEntity = response.getEntity();
+    if (responseEntity == null) {
+      throw exceptionType.apply("Response was empty.", null);
+    }
+    val content = getContent(responseEntity);
+    log.debug("Parsing response from JSON response: {}", content);
+    try {
+      return JACKSON.readValue(content, responseType);
+    } catch (final JsonProcessingException e) {
+      log.error("Failed to parse the following response: {}", content);
+      throw exceptionType.apply("Failed to parse response", e);
+    }
+  }
+
+  @Nonnull
+  private String getContent(@Nonnull final HttpEntity entity) {
     try {
       return EntityUtils.toString(entity, StandardCharsets.UTF_8);
     } catch (IOException | ParseException e) {
-      throw new OrchestrationClientException("Failed to read response content.", e);
+      throw exceptionType.apply("Failed to read response content.", e);
     }
   }
 
-  // The InputStream of the HTTP entity is closed by EntityUtils.toString
+  /**
+   * Parse the error response and throw an exception.
+   *
+   * @param response The response to process
+   */
   @SuppressWarnings("PMD.CloseResource")
-  static void buildExceptionAndThrow(@Nonnull final ClassicHttpResponse response)
-      throws OrchestrationClientException {
+  public void buildExceptionAndThrow(@Nonnull final ClassicHttpResponse response) throws E {
     val exception =
-        new OrchestrationClientException(
-            "Request to orchestration service failed with status %s %s"
-                .formatted(response.getCode(), response.getReasonPhrase()));
+        exceptionType.apply(
+            "Request failed with status %s %s"
+                .formatted(response.getCode(), response.getReasonPhrase()),
+            null);
     val entity = response.getEntity();
     if (entity == null) {
       throw exception;
@@ -54,9 +109,7 @@ static void buildExceptionAndThrow(@Nonnull final ClassicHttpResponse response)
       throw exception;
     }
 
-    log.error(
-        "The orchestration service responded with an HTTP error and the following content: {}",
-        content);
+    log.error("The service responded with an HTTP error and the following content: {}", content);
     val contentType = ContentType.parse(entity.getContentType());
     if (!ContentType.APPLICATION_JSON.isSameMimeType(contentType)) {
       throw exception;
@@ -68,59 +121,19 @@ static void buildExceptionAndThrow(@Nonnull final ClassicHttpResponse response)
   /**
    * Parse the error response and throw an exception.
    *
-   * @param errorResponse the error response, most likely a JSON of {@link ErrorResponse}.
+   * @param errorResponse the error response, most likely a unique JSON class.
    * @param baseException a base exception to add the error message to.
    */
-  static void parseErrorAndThrow(
-      @Nonnull final String errorResponse,
-      @Nonnull final OrchestrationClientException baseException)
-      throws OrchestrationClientException {
-    val maybeError = Try.of(() -> JACKSON.readValue(errorResponse, ErrorResponse.class));
+  public void parseErrorAndThrow(
+      @Nonnull final String errorResponse, @Nonnull final E baseException) throws E {
+    val maybeError = Try.of(() -> JACKSON.readValue(errorResponse, errorType));
     if (maybeError.isFailure()) {
       baseException.addSuppressed(maybeError.getCause());
       throw baseException;
     }
 
-    throw new OrchestrationClientException(
-        "%s and error message: '%s'"
-            .formatted(baseException.getMessage(), maybeError.get().getMessage()));
-  }
-
-  /**
-   * Processes a {@link ClassicHttpResponse} and returns some value corresponding to that response.
-   *
-   * @param response The response to process
-   * @return A model class instantiated from the response
-   * @throws OrchestrationClientException in case of a problem or the connection was aborted
-   */
-  @Override
-  public T handleResponse(@Nonnull final ClassicHttpResponse response)
-      throws OrchestrationClientException {
-    if (response.getCode() >= 300) {
-      buildExceptionAndThrow(response);
-    }
-    val result = parseResponse(response);
-    log.debug("Received the following response from orchestration service: {}", result);
-    return result;
-  }
-
-  // The InputStream of the HTTP entity is closed by EntityUtils.toString
-  @SuppressWarnings("PMD.CloseResource")
-  @Nonnull
-  private T parseResponse(@Nonnull final ClassicHttpResponse response)
-      throws OrchestrationClientException {
-    final HttpEntity responseEntity = response.getEntity();
-    if (responseEntity == null) {
-      throw new OrchestrationClientException("Response from Orchestration service was empty.");
-    }
-    val content = getContent(responseEntity);
-    log.debug("Parsing response from JSON response: {}", content);
-    try {
-      return JACKSON.readValue(content, responseType);
-    } catch (final JsonProcessingException e) {
-      log.error("Failed to parse the following response from orchestration service: {}", content);
-      throw new OrchestrationClientException(
-          "Failed to parse response from orchestration service", e);
-    }
+    val error = Objects.requireNonNullElse(maybeError.get().getMessage(), "");
+    val message = "%s and error message: '%s'".formatted(baseException.getMessage(), error);
+    throw exceptionType.apply(message, baseException);
   }
 }
diff --git a/foundation-models/openai/pom.xml b/foundation-models/openai/pom.xml
@@ -33,6 +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.method>83%</coverage.method>
+    <coverage.class>85%</coverage.class>
   </properties>
   <dependencies>
     <dependency>

diff --git a/...tion-models/openai/src/main/java/com/sap/ai/sdk/foundationmodels/openai/OpenAiClient.java b/...tion-models/openai/src/main/java/com/sap/ai/sdk/foundationmodels/openai/OpenAiClient.java
@@ -7,13 +7,15 @@
 import com.google.common.annotations.Beta;
 import com.sap.ai.sdk.core.AiCoreService;
 import com.sap.ai.sdk.core.DeploymentResolutionException;
+import com.sap.ai.sdk.core.commons.ClientResponseHandler;
 import com.sap.ai.sdk.foundationmodels.openai.model.OpenAiChatCompletionDelta;
 import com.sap.ai.sdk.foundationmodels.openai.model.OpenAiChatCompletionOutput;
 import com.sap.ai.sdk.foundationmodels.openai.model.OpenAiChatCompletionParameters;
 import com.sap.ai.sdk.foundationmodels.openai.model.OpenAiChatMessage.OpenAiChatSystemMessage;
 import com.sap.ai.sdk.foundationmodels.openai.model.OpenAiChatMessage.OpenAiChatUserMessage;
 import com.sap.ai.sdk.foundationmodels.openai.model.OpenAiEmbeddingOutput;
 import com.sap.ai.sdk.foundationmodels.openai.model.OpenAiEmbeddingParameters;
+import com.sap.ai.sdk.foundationmodels.openai.model.OpenAiError;
 import com.sap.ai.sdk.foundationmodels.openai.model.StreamedDelta;
 import com.sap.cloud.sdk.cloudplatform.connectivity.ApacheHttpClient5Accessor;
 import com.sap.cloud.sdk.cloudplatform.connectivity.DefaultHttpDestination;
@@ -283,7 +285,9 @@ private <T> T executeRequest(
       final BasicClassicHttpRequest request, @Nonnull final Class<T> responseType) {
     try {
       final var client = ApacheHttpClient5Accessor.getHttpClient(destination);
-      return client.execute(request, new OpenAiResponseHandler<>(responseType));
+      return client.execute(
+          request,
+          new ClientResponseHandler<>(responseType, OpenAiError.class, OpenAiClientException::new));
     } catch (final IOException e) {
       throw new OpenAiClientException("Request to OpenAI model failed", e);
     }

diff --git a/...ls/openai/src/main/java/com/sap/ai/sdk/foundationmodels/openai/OpenAiClientException.java b/...ls/openai/src/main/java/com/sap/ai/sdk/foundationmodels/openai/OpenAiClientException.java
@@ -1,7 +1,8 @@
 package com.sap.ai.sdk.foundationmodels.openai;
 
+import com.sap.ai.sdk.core.commons.ClientException;
 import lombok.experimental.StandardException;
 
 /** Generic exception for errors occurring when using OpenAI foundation models. */
 @StandardException
-public class OpenAiClientException extends RuntimeException {}
+public class OpenAiClientException extends ClientException {}