-
Notifications
You must be signed in to change notification settings - Fork 12
chore: Improve exception accessors #537
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
base: main
Are you sure you want to change the base?
Conversation
…/ai-sdk-java into exceptions-with-httpresponse
…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
…esponse2 # Conflicts: # orchestration/src/main/java/com/sap/ai/sdk/orchestration/OrchestrationChatResponse.java # orchestration/src/main/java/com/sap/ai/sdk/orchestration/OrchestrationClient.java # orchestration/src/main/java/com/sap/ai/sdk/orchestration/OrchestrationClientException.java # orchestration/src/main/java/com/sap/ai/sdk/orchestration/OrchestrationFilterException.java # orchestration/src/main/java/com/sap/ai/sdk/orchestration/OrchestrationHttpExecutor.java
…nto exceptions-with-httpresponse2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surely, the approach removes the need for having an additional getErrorResponseStreaming()
Here are my concerns. Correct me if I wrong.
- Approach doesn't scale well if new exceptions are introduced apart from filtering .
- This would have been better solved (in terms of #lines added) with the OpenRewrite recipe to add a parent interface for
ErrorResponse
andErrorResponseSreaming
classes.
Overall, though the solution is valid, I feel more comfortable with having additional getErrorResponseStreaming
.
The usage below, though not obvious, isn't unexpected.
try {
client.chatCompletion();
} catch (OrchestrationCLientException e) {
e.getErrorResponse();
}
try {
client.streamChatCompletion();
} catch (OrchestrationCLientException e) {
e.getErrorResponseStreaming();
}
We could also go about with adding documentation and improved javadoc for clarity.
Context
Follow-up from #529
I've noticed multiple "nullable" accessors in our exceptions.
Users wouldn't really know what data is available at design-time.
We can harden the exceptions a little for stricter API contract.
Motivation:
Before
After
Classes refactored:
Feature scope:
extends OrchestrationClientException.[Synchronous|Streaming] { @Nonnull Map<String, Object> getFilterDetails(); @Nullable AzureContentSafetyInput getAzureContentSafetyInput(); @Nullable LlamaGuard38b getLlamaGuard38b(); + @Nullable Integer getStatusCode(); }
Split classes
Definition of Done
Aligned changes with the JavaScript SDK