Use exception handling for responses #196
Conversation
| @@ -21,7 +22,7 @@ public sealed class AssistantsEndpoint : BaseEndPoint | |||
| /// <returns><see cref="ListResponse{Assistant}"/></returns> | |||
| public async Task<ListResponse<AssistantResponse>> ListAssistantsAsync(ListQuery query = null, CancellationToken cancellationToken = default) | |||
| { | |||
| var response = await client.Client.GetAsync(GetUrl(queryParameters: query), cancellationToken).ConfigureAwait(false); | |||
| var response = await client.Client.GetResponseAsync(GetUrl(queryParameters: query), HttpMethod.Get, cancellationToken: cancellationToken); | |||
There was a problem hiding this comment.
What is the advantage of changing this?
There was a problem hiding this comment.
GetResponseAsync is basically the wrapper/method that handles the whole new feature.
Every single GetAsync/GetPost/SendAsync/etc needs to go through GetResponseAsync for proper error handling.
It's supposed to represent the following method in the python library, where they've chosen to represent internal http exceptions as user-friendly ones, as defined by their own library.
The method handles both 2 internal exceptions, as well as successfully received responses with status codes 4XX-5XX.
This is how our own version looks for now.
ThrowApiExceptionIfUnsuccessfulAsync is called from within which handles the rest of the logic for 4XX-5XX status codes.
I see why they did this - the consumer shouldn't be dealing with exceptions at such a low level, though in the .NET world perhaps the OpenAITimeoutException is unnecessary as anyone dealing with CancellationTokens would expect to receive a TaskCanceledException or an exception deriving from OperationCanceledException.
If we decide to throw those 2 exceptions in the bin, the need for GetResponseAsync is unnecessary. Instead, we can simply add a call to ThrowApiExceptionIfUnsuccessfulAsync after every single method for sending an HTTP request.
| public string? Param { get; } = param; | ||
| } | ||
|
|
||
| public class ApiErrorResponse |
There was a problem hiding this comment.
lets make sure we separate our classing into separate files
There was a problem hiding this comment.
I completely agree. My intention was to quickly show you what I've done so far, as explained in my initial post.
There was a problem hiding this comment.
While the changes look ok on first glance, I am apprehensive about making breaking changes, unless we are focusing on problems that directly impact consumers.
I also keep this library highly aligned with my unity package and the changes need to translate there as well, which means no nullability and compatible calls and handling.
We can change exceptions to derive from the one we initially were throwing before implementing this feature and keep its structure. That way no breaking changes would occur, except for the fact we removed a bunch of properties from I'll pause on any new development for now until we're sure we want to have this feature implemented in some way. |
|
closing this since it is out of sync |
This design is heavily inspired by the original python library.
I've done my best to make it as close to 1 to 1 as possible. I've not troubled myself with handling retries, which the original library does. I've also not handled what they've done by setting a default timeout of 10 minutes on requests without user-defined cancellation. That I believe is outside the scope of this feature.
Please bear in mind that this is not the final version. For starters, I need to separate my classes outside a sillily named
Generated.cs, as well as call the newHttpClientextension methods where appropriate.I quickly wanted to put what I've built up till now with the community before diving deeper, hence why this is a draft and why it's committed in a rather "poor" shape. My goal here is rather to show you the changes I've made, and discuss if they're okay, or if I've taken a very wrong route.
Tagging @StephenHodgson for attention.
Related to #193.