feat: [Orchestration] Filtering details and additional convenience on exception #497

rpanackal · 2025-07-14T16:01:39Z

Context

AI/ai-sdk-java-backlog#253.

Previously, we didn't propagate the deserialized error response along with any thrown exception. This also meant our users have no access to diagnostic details returned by orchestration service (also applicable for openai).

The PR aims to rectify the above problem and especiallyy provide convenience for input and output filtering exceptions.

Here are two user issues created requesting the same convenience. They have both suggested some api. Maybe you can also look at this to have a better picture on what convenience may be added

#491
#468

Feature scope:

Better separation of module specific error handling with ClientExceptionFactory
Add getClientError that returns module specific error objects wrapping deserialized server response.
Add OrchestrationInputFilteringException and OrchestrationOutputFilteringException to distinguish filtering exception types with catch blocks.
Both filtering exception types contain Map<String, Object> getFilterDetails() to inspect the filtering details available.
Add getStatusCode to OrchestrationClientException to get the http status code
Extend unit testing for ClientResponseHandler
Add unit testing for ClientStreamingHandler
Improve Unit testing of input and output filtering
E2E test refactored to demo correct usage

Definition of Done

Functionality scope stated & covered
Tests cover the scope above
Error handling created / updated & covered by the tests above
~~Aligned changes with the JavaScript SDK~~
Documentation updated
Release notes updated

…on-handling-factory # Conflicts: # orchestration/src/main/java/com/sap/ai/sdk/orchestration/OrchestrationClient.java

…on-handling-factory

- remove crafty object mapping in response handler

newtork · 2025-07-17T13:29:52Z

core/src/main/java/com/sap/ai/sdk/core/common/ClientException.java

+   */
+  @Nullable
+  @Getter(onMethod_ = @Beta)
+  public ClientError clientError;


(MInor)

Please make it (package) private.
Please add Setter, maybe?

newtork · 2025-07-17T13:31:08Z

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

+  @Nonnull final Class<T> successType;
+
+  /** The HTTP error response type */
+  @Nonnull protected final Class<R> errorType;


(Question)

Why protected?

newtork · 2025-07-17T13:32:08Z

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

@@ -48,8 +50,8 @@ public class ClientResponseHandler<T, E extends ClientException>
   */
  @Beta
  @Nonnull
-  public ClientResponseHandler<T, E> objectMapper(@Nonnull final ObjectMapper jackson) {
-    objectMapper = jackson;
+  public ClientResponseHandler<T, R, E> objectMapper(@Nonnull final ObjectMapper jackson) {


(Question)

Is the fluent API reasonable here? Would the code look cleaner if this was a regular void setter?

newtork · 2025-07-17T13:35:24Z

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

    if (response.getCode() >= 300) {
-      buildExceptionAndThrow(response);
+      throw buildException(response);


(Minor)

In Java the "expensive" part of exceptions is the implicit buildup of current stacktrace. That happens at exception instantiation, not when throwing. That also means this will look surprising that the stacktrace points at "new" and not at "throw" (here). Therefore, this line has a code smell.

newtork · 2025-07-17T13:36:25Z

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

    final HttpEntity responseEntity = response.getEntity();
    if (responseEntity == null) {
-      throw exceptionConstructor.apply("Response was empty.", null);
+      throw exceptionFactory.create("Response was empty.", null);


(Minor)

I know this is semi-related but can we change the message to "HTTP response is empty"?

newtork · 2025-07-17T13:40:55Z

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

-      exception.addSuppressed(maybeContent.getCause());
-      throw exception;
+      baseException.addSuppressed(maybeContent.getCause());
+      return baseException;
    }
    val content = maybeContent.get();
    if (content.isBlank()) {


(Major) content may be null.

newtork · 2025-07-17T13:46:12Z

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

-            null);
-    val entity = response.getEntity();
+  @Nonnull
+  protected E buildException(@Nonnull final ClassicHttpResponse httpResponse) {


(Comment)

I was thinking, If we moved this whole method and related logic into ClientExceptionFactory then we could get rid of generic argument R extends ClientError in this class :/ But this would risk further duplicate code though. It was just a thought

newtork · 2025-07-17T13:50:44Z

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

+   */
+  @Beta
+  @Nullable
+  public Integer getStatusCode() {


(Comment)

We can leave it in for now. But we will likely extend the exception class with optional HttpResponse field reference. Once we have that, then this method will appear very confusing and we may have to change it.

It's beta anyway.

newtork · 2025-07-17T13:53:28Z

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

+  }
+
+  /** Exception thrown output filtering in orchestration when finish reason is content filter */
+  public static class OrchestrationOutputFilterException extends OrchestrationFilterException {


(Minor/Discussion)

I would rename it to "Output". Such that class reference becomes
OrchestrationFilterException.Output

newtork · 2025-07-17T13:56:22Z

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

+public class OrchestrationFilterException extends OrchestrationClientException {
+
+  /** Details about the filter that caused the exception. */
+  @Getter @Nonnull protected Map<String, Object> filterDetails;


(Minor)

IntelliJ is correctly warning me: This field cannot be "Nonnull" when default constructor of this class sets it to "null". I think there are multiple ways to address that.

Easiest (and obvious?) way is probably initializing it with empty map.

newtork · 2025-07-17T14:02:53Z

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

+        .flatMap(moduleResults -> Optional.ofNullable(moduleResults.getInputFiltering()))
+        .flatMap(inputFiltering -> Optional.ofNullable(inputFiltering.getData()))
+        .filter(Map.class::isInstance)
+        .map(map -> (Map<String, Object>) map)


(Minor)

Technically this is an unsafe cast. We could suppress the warning. (do you see the warning?). Since JSON only assumes dictionaries/maps with String as key-type, this could be considered safe.

Casting to Map<String, Map<String,Object>> would be dangerous. I think.

CharlesDuboisSAP · 2025-07-18T09:26:03Z

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

-    throw exceptionConstructor.apply(message, baseException);
+    final R clientError = maybeClientError.get();
+    val extendErrorMessage = "%s: %s".formatted(baseErrorMessage, clientError.getMessage());
+    return exceptionFactory.fromClientError(extendErrorMessage, clientError);


@newtork this return doesn't reuse the baseException, it creates a new exception. The forgotten baseException might have a performance cost, plus get picked up by logging frameworks

Roshin Rajan Panackal added 10 commits July 8, 2025 15:20

Input Filtering resolved

e863592

Input Filtering and Output filtering resolved

c16677f

Refactor - from enum to individual types of filtering exception

842d53c

Refactor - from enum to individual types of filtering exception

4a1a026

Exception factory introduced

69087b6

Merge branch 'refs/heads/main' into feat/orchestration/filter-excepti…

6e48dab

…on-handling-factory # Conflicts: # orchestration/src/main/java/com/sap/ai/sdk/orchestration/OrchestrationClient.java

Finish merge with main

e6d371c

Update tests and add better javadocs

485482f

Merge branch 'refs/heads/main' into feat/orchestration/filter-excepti…

55e1ee5

…on-handling-factory

Updating error messages

f6f3528

- remove crafty object mapping in response handler

rpanackal self-assigned this Jul 14, 2025

rpanackal added the please-review Request to review a pull-request label Jul 14, 2025

newtork reviewed Jul 17, 2025

View reviewed changes

CharlesDuboisSAP reviewed Jul 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: [Orchestration] Filtering details and additional convenience on exception #497

feat: [Orchestration] Filtering details and additional convenience on exception #497

Uh oh!

rpanackal commented Jul 14, 2025 •

edited

Loading

Uh oh!

newtork Jul 17, 2025

Uh oh!

newtork Jul 17, 2025

Uh oh!

newtork Jul 17, 2025

Uh oh!

newtork Jul 17, 2025

Uh oh!

newtork Jul 17, 2025

Uh oh!

newtork Jul 17, 2025

Uh oh!

newtork Jul 17, 2025

Uh oh!

newtork Jul 17, 2025

Uh oh!

newtork Jul 17, 2025

Uh oh!

newtork Jul 17, 2025

Uh oh!

newtork Jul 17, 2025

Uh oh!

newtork Jul 17, 2025

Uh oh!

CharlesDuboisSAP Jul 18, 2025 •

edited

Loading

Uh oh!

Uh oh!

feat: [Orchestration] Filtering details and additional convenience on exception #497

Are you sure you want to change the base?

feat: [Orchestration] Filtering details and additional convenience on exception #497

Uh oh!

Conversation

rpanackal commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Feature scope:

Definition of Done

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CharlesDuboisSAP Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rpanackal commented Jul 14, 2025 •

edited

Loading

CharlesDuboisSAP Jul 18, 2025 •

edited

Loading