Support returning api response in case of deserialization exception #1167
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…return type is ApiResponse
…andle deserializations directly, and non ApiResponse to throw
…erialization_exception
|
Also @clairernovotny I noticed while putting this together that ApiResponse doesn't seem to support ValidationApiException either... I didn't finish going all the way down the rabbit hole of why but it seems related to it being a subclass of ApiException. I would somewhat prefer a design whereby the details that ValidationApiException exposes live as an inner exception inside of ApiException as having two top-level exceptions here means more try/catch and IMO violates the principle of least surprise. Thoughts? |
…esponse<T>> and Task<T>
…n_exception' of https://github.com/james-s-tayler/refit into support_returning_ApiResponse_in_case_of_deserialization_exception
2 tasks
…erialization_exception
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What kind of change does this PR introduce?
This is a bug fix for #1160 and #1017 - ApiResponse wasn't trapping exceptions that occurred while attempting to deserialize the response, which violated the expectations of the ApiResponse programming model.
What is the current behavior?
The current behavior is that an ApiResponse will throw a Refit.ApiException if an exception is thrown while attempting to deserialize the response.
What is the new behavior?
The deserialization exception is properly wrapped by a Refit.ApiException and returned in the ApiResponse
What might this PR break?
I've added tests for this and ensured the existing tests for the non ApiResponse case still pass. If callers knew about the current behavior and had explicit try/catch blocks those will no longer be invoked anymore. Though, all callers will almost certainly have existing exception handling logic for the ApiResponse which should pick up the deserialization exceptions now too. I, for one, didn't realize the library behaved as it does at present and expected every and all exception to come back via ApiResponse.Error.
Please check if the PR fulfills these requirements