chore: Improve Exception Handling and Accessors to Response/Request #529
Conversation
…/ai-sdk-java into exceptions-with-httpresponse
| @@ -102,7 +101,7 @@ public static void send(@Nonnull final ResponseBodyEmitter emitter, @Nonnull fin | |||
| try { | |||
| emitter.send(chunk); | |||
| } catch (final IOException e) { | |||
| log.error(Arrays.toString(e.getStackTrace())); | |||
| log.error("Failed to send chunk: {}", e.getMessage(), e); | |||
There was a problem hiding this comment.
Minor drive-by change.
| @Getter(onMethod_ = @Beta, value = AccessLevel.PROTECTED) | ||
| @Setter(onMethod_ = @Beta, value = AccessLevel.PROTECTED) | ||
| ClientError clientError; | ||
| @Getter(onMethod_ = @Beta, value = AccessLevel.PUBLIC) |
There was a problem hiding this comment.
(Question/Suggestion)
Should getClientError() remain protected? As far as I see, ClientError is an internal wrapper for error response.
For users, there exist a getErrorResponse() in module specific exceptions like OpenAiClientException to shortcut access to the error response.
I feel like getErrorResponse() is convenient than getClientError() and would reduce confusion. Having both method public may be redundant?
There was a problem hiding this comment.
The following API is available
catch(OrchestrationClientException e) {
e.getMessage(); // default
e.getCause(); // default
e.getHttpResponse(); // ClassicHttpResponse
e.getHttpRequest(); // ClassicHttpRequest
e.getClientError().getMessage(); // convenient message
e.getErrorResponse(); // ErrorResponse (model class)
e.getErrorResponseStreaming(); // ErrorResponseStreaming (model class)
}The e.getClientError().getMessage() is the reason.
However I would also prefer if it wasn't split.
There was a problem hiding this comment.
The string from e.getMessage() contains e.getClientError().getMessage().
We don't need to expose e.getClientError().getMessage() and leave it as simply a method for error response type-agnostic retrieval of message.
No strong opinion.
There was a problem hiding this comment.
We don't need to expose
e.getClientError().getMessage()
I was not planning to challenge visibility of this method in this PR. 🤔
…esponse # Conflicts: # docs/release_notes.md # orchestration/src/main/java/com/sap/ai/sdk/orchestration/OrchestrationClientException.java # orchestration/src/main/java/com/sap/ai/sdk/orchestration/OrchestrationExceptionFactory.java # orchestration/src/main/java/com/sap/ai/sdk/orchestration/OrchestrationHttpExecutor.java
…to exceptions-with-httpresponse
Context
https://github.com/SAP/ai-sdk-java-backlog/issues/290
Since we removed logging of HTTP request/responses from our code, we want to ease troubleshooting in the future.
Feature scope:
pubicaccessorDefinition of Done
Aligned changes with the JavaScript SDKDocumentation updated