-
-
Notifications
You must be signed in to change notification settings - Fork 776
Custom error handling #927
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
Conversation
|
Seems like same as #900 |
| public interface IErrorHandler | ||
| { | ||
| Task<Exception> HandleErrorAsync(HttpRequestMessage message, HttpMethod httpMethod, HttpResponseMessage response, | ||
| RefitSettings refitSettings = null); |
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.
refitSettings shoudln't default to null as it's called by library code, which we control. refitSettings should never be null.
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.
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); |
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.
What happens if the error handler returns null?
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.
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); |
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.
Same, what happens if the error handler returns null?
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.
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; } |
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.
It is a breaking change.
Not sure it could be done.
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.
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?
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.
Ability to throw UnauthorizedAccessException was requested here:
#562
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.
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 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. |
|
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. |
|
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. |
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.DefaultErrorHandlerwith 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: