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

Use exception handling for responses #196

Conversation

kikaragyozov
Copy link
Contributor

@kikaragyozov kikaragyozov commented Dec 2, 2023

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 new HttpClient extension 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.

@@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

What is the advantage of changing this?

Copy link
Contributor Author

@kikaragyozov kikaragyozov Dec 2, 2023

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

lets make sure we separate our classing into separate files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I completely agree. My intention was to quickly show you what I've done so far, as explained in my initial post.

Copy link
Member

@StephenHodgson StephenHodgson left a comment

Choose a reason for hiding this comment

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

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.

@kikaragyozov
Copy link
Contributor Author

kikaragyozov commented Dec 2, 2023

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 BaseResponse. Those properties though were never being populated to begin with. If we break a consumer or anyone currently using them, it might be for the best. There's no way for them to work if the consumer is expecting to use them through BaseResponse, as we've decided to throw on 4XX which is where 429 falls under. We could also not remove them, i.e., leave them as duplicates. Then no breaking changes would happen.

I'll pause on any new development for now until we're sure we want to have this feature implemented in some way.

@StephenHodgson StephenHodgson linked an issue Dec 2, 2023 that may be closed by this pull request
@StephenHodgson
Copy link
Member

closing this since it is out of sync

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Dedicated exceptions for specific events
2 participants