Skip to content

Conversation

@yamaoto
Copy link

@yamaoto yamaoto commented May 19, 2020

What kind of change does this PR introduce?
feature

What is the current behavior?

What is the new behavior?
Provide the ability to independently manage errors instead of the current set of ApiException and ValidationApiException.

How doies it works?
I provided IErrorHandler that presented in RefitSettings and RestMethodInfo, by default it set to DefaultErrorHandler with current logic.

RequestBuilderImplementation for now is calling RestMethodInfo.IErrorHandler.HandleErrorAsync instead of ApiException.Create.

What might this PR break?
There is nothing to break. I provided Refit.DefaultErrorHandler with current exception management logic that used by default.

Please check if the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)

  • Docs have been added / updated (for bug fixes / features)

Other information:

@dnfclas
Copy link

dnfclas commented May 19, 2020

CLA assistant check
All CLA requirements met.

@Dreamescaper
Copy link
Contributor

Seems like same as #900

public interface IErrorHandler
{
Task<Exception> HandleErrorAsync(HttpRequestMessage message, HttpMethod httpMethod, HttpResponseMessage response,
RefitSettings refitSettings = null);
Copy link
Member

Choose a reason for hiding this comment

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

refitSettings shoudln't default to null as it's called by library code, which we control. refitSettings should never be null.

Copy link
Author

Choose a reason for hiding this comment

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

This is due to the fact that in RestMethodInfo constructor it may be null https://github.com/reactiveui/refit/pull/927/files#diff-12d38083c42706a6ad12f6b11083cea6R43.
Should i do refitSettings not null in IErrorHandler and RestMethodInfo? This will require changing tests in Refit.Tests\RequestBuilder.cs

{
disposeResponse = false; // caller has to dispose
e = await ApiException.Create(rq, restMethod.HttpMethod, resp, restMethod.RefitSettings).ConfigureAwait(false);
e = await restMethod.ErrorHandler.HandleErrorAsync(rq, restMethod.HttpMethod, resp, restMethod.RefitSettings).ConfigureAwait(false);
Copy link
Member

Choose a reason for hiding this comment

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

What happens if the error handler returns null?

Copy link
Author

Choose a reason for hiding this comment

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

Now it will go the way of deserializing the response and the Refit code will return default for this type.

It is possible to alter on when the user implementation considers that this is not an error scenario, they can return null and the refit code can deserialize the response.

For example:
Normal response

{
    "result": true,
    "data" : { ... }
}

Error response

{
    "result": false,
    "error": "error description"
}

and model for this

public class ApiMethodResponse
{
    public bool Result { get; set; }
    public string Error { get; set; }
    public SomeDataModel Data { get; set; }
}

if (!resp.IsSuccessStatusCode)
{
throw await ApiException.Create(rq, restMethod.HttpMethod, resp, settings).ConfigureAwait(false);
throw await restMethod.ErrorHandler.HandleErrorAsync(rq, restMethod.HttpMethod, resp, settings).ConfigureAwait(false);
Copy link
Member

Choose a reason for hiding this comment

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

Same, what happens if the error handler returns null?

Copy link
Author

Choose a reason for hiding this comment

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

this will now throw a NullReferenceException
it can be redone with

- throw await restMethod.ErrorHandler.HandleErrorAsync(rq, restMethod.HttpMethod, resp, settings).ConfigureAwait(false);
+ var exception = await restMethod.ErrorHandler.HandleErrorAsync(rq, restMethod.HttpMethod, resp, settings).ConfigureAwait(false);
+ if (exception != null)
+ 
+    throw exception;
+ }

and go according to the scenario described above

HttpStatusCode StatusCode { get; }
Version Version { get; }
ApiException Error { get; }
Exception Error { get; }
Copy link
Contributor

Choose a reason for hiding this comment

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

It is a breaking change.
Not sure it could be done.

Copy link
Member

Choose a reason for hiding this comment

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

Good point. We could make it so that the custom exception has to derive from ApiException. That would allow people to have custom exception types.

It's not clear to me what other exceptions people are looking to throw here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ability to throw UnauthorizedAccessException was requested here:
#562

Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure that exception should not be thrown in this context. That's for things wrapping Windows APIs

https://docs.microsoft.com/en-us/dotnet/api/system.unauthorizedaccessexception?view=netcore-3.1

@clairernovotny clairernovotny changed the base branch from master to main November 13, 2020 13:50
@anaisbetts
Copy link
Member

@clairernovotny has sold me on this idea, great work! Though I agree with her that we should not change ApiException => Exception, because this means that nearly everyone will have to cast this Exception to be able to get the details of the HTTP response out.

@clairernovotny
Copy link
Member

We also now have #900 that was merged -- that enables logic to happen to create custom exceptions. If there are additional updates to that, we'd be happy to consider them.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants