Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce duplication of ResponseHandlers #244

Merged
merged 22 commits into from
Dec 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions core/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

orchestration tests don't count towards core coverage, therefore I separated the values into individual modules so these bad limits on the core module don't impact the other modules

</properties>

<dependencies>
Expand Down Expand Up @@ -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>
Expand Down Expand Up @@ -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>
Expand Down
21 changes: 21 additions & 0 deletions core/src/main/java/com/sap/ai/sdk/core/commons/ClientError.java
Original file line number Diff line number Diff line change
@@ -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();
}
Original file line number Diff line number Diff line change
@@ -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 {}
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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
newtork marked this conversation as resolved.
Show resolved Hide resolved
@Slf4j
@RequiredArgsConstructor
class OrchestrationResponseHandler<T> implements HttpClientResponseHandler<T> {
public class ClientResponseHandler<T, E extends ClientException>
implements HttpClientResponseHandler<T> {
@Nonnull private final Class<T> responseType;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm marking this class as Beta, to discourage externals from using this class.

Maybe we can find a way to make this class not 100% dependent on Apache HTTP Client 5 API(?)

@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;
Expand All @@ -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;
Expand All @@ -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);
}
}
6 changes: 6 additions & 0 deletions foundation-models/openai/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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 {}
Loading
Loading