feature: support returning ApiResponse in case of deserialization exception

[toanxyz]

Is your feature request related to a problem? Please describe.

We're evaluating the usage of Refit in our project. One of our concerns is how to have general exception handling when using Refit. Refit will not throw the exception in the case of ApiResponse, this is good. However, in the case of the Deserialization process, Refit throws the exception directly to the application (refer here) so it could make us have try/catch everywhere or we have to create a wrapper class that handles the exception and can lead to many boilerplate or unnecessary code.

Describe the solution you'd like

I would like to have something similar in another method that will check the response is ApiResponse, if yes we can create that instead of throwing the exception.

Describe alternatives you've considered

N/A

Describe suggestions on how to achieve the feature

N/A

Additional context

N/A

[james-s-tayler]

Looking at the code I see exactly the problem, and you're right.

  if (typeof(T) != typeof(HttpResponseMessage))
                {
                    e = await settings.ExceptionFactory(resp).ConfigureAwait(false);
                }

                
                if (restMethod.IsApiResponse)
                {
                    // Only attempt to deserialize content if no error present for backward-compatibility
                    var body = e == null
                        ? await DeserializeContentAsync<TBody>(resp, content, ct).ConfigureAwait(false)
                        : default;

                    return ApiResponse.Create<T, TBody>(resp, body, settings, e as ApiException);
                }
                else if (e != null)
                {
                    disposeResponse = false; // caller has to dispose
                    throw e;
                }
                else
                    return await DeserializeContentAsync<T>(resp, content, ct).ConfigureAwait(false);

Both of these call DeserializeContentAsync and that method wraps a try/catch around the deserialization attempt and even creates an ApiException in it's catch block that it then simply just always rethrows, which is not ideal.

Probably what we can do is something like this:

   if (restMethod.IsApiResponse)
                {
                    // Only attempt to deserialize content if no error present for backward-compatibility
                    var body = default(TBody);
                    try
                    {
                        body = e == null
                        ? await DeserializeContentAsync<TBody>(resp, content, ct).ConfigureAwait(false)
                        : default;
                    }
                    catch(ApiException ex)
                    {
                        e = ex;
                    }

                    return ApiResponse.Create<T, TBody>(resp, body, settings, e as ApiException);
                }

[toanxyz]

@james-s-tayler I would like to have your opinion on another use case that we're having now, in our old logic, we have the try-catch block to always return a Result class. When we use Refit, there is ApiResponse. However, it would be great if we can have a response creation logic that can be customized. So instead of ApiResponse.Create<T, TBody>(resp, body, settings, e as ApiException);, it could be from the settings like settings.ResultFactory<T, TBody>(resp, body, settings, e as ApiException);

By doing this way, it allows us to customize our response instead of ApiResponse. And I think it won't introduce any breaking changes, just an added feature like the ExceptionFactory we have in the RefitSettings.

[james-s-tayler]

@toanxyz I'm planning to submit a PR for #1156 to add a PropertyFactoryProvider, while I'm in that area of the code I could have a look and evaluate that.

I noticed too that #1017 has similar requirements to this issue you've opened also. I think that should be enough for me to put together a PR to fix this and give much better handling of serialization exceptions.

[james-s-tayler]

@toanxyz got a PR for this now.

Had a look at the feasibility of ResultFactory, and to be honest I think it may be possible. The code makes use of generics all the way down, such that it's theoretically possible, but it's got some hardcoded logic to test if return type is ApiResponse and in that case returns ApiResponse.Create().

I can imagine adding a ResultFactory and in RestMethodInfo.cs checking if settings.ResultFactory != null, then just set a boolean to true that tells it to return the result via the ResultFactory instead, similar to how ApiResponse behaves.

Could then even shift the current two existing behaviors into their own ResultFactory implementations. DefaultResultFactory, and ApiResponseResultFactory etc.

The only part I'm not sure about and might throw a spanner on the works is what happens if your ResultFactory implementation throws an exception? In the case of ApiResponse the behavior is now well defined, because we know when/where to put the exception. But in the case ResultFactory maybe there's no way to trap and surface those exceptions... which means try/catch everywhere! And that's not what we want...

Kinda curious though, in what way do you want to customise the result? What does ApiResponse not have that you need?

[toanxyz]

@james-s-tayler Looked at the PR, it's awesome.

I was thinking about this and I realize the fix of deserialization is maybe enough for our use case, the ResultFactory may not really be needed when we can still leverage the ApiResponse.

However, I want to share with you more about our context.

We're planning to migrate our system to use Refit as our main HTTP caller for the service communication, we have thousand places that need to be migrated, so the concern to migrate but still keep the same behavior is one of our top priorities.

In our project, we have a class Result that is responsible for the API result, different result types will inherit from this Result base class, the purpose is somehow similar to Refit ApiResponse. The difference comes with how we handle errors (we treated everything as a ResultError class, even the .NET Exception will be converted to ResultError). There are several types of error:

1. Error happens at the caller (For example the network is down so the client can't even make the call to the endpoint, or deserialization exception when parsing the response, etc.)
2. Error happens at the service: in this case, the service will return a response that is inherited from the Result as well, so it will contain Errors property containing error messages.
3. Others (unexpected/uncontrolled): like service server down such as 503, etc.

Because we always return the Result object to the caller so the code will look the same when handling the response, and look like this:

• To check the Result is good: myResult.IsValid
• To get the errors: myResult.Errors

After the fix of deserialization exception in this #1167, without the ResultFactory, our code will look like this:

• To check the Result is good (which is ApiResponse): myResult.IsSuccessStatusCode && myResult.Content?.IsValid
• To get the full errors: myResult.Error?.ConverToResultError() && myResult.Content?.Errors
  -> I think we can create extension methods to the ApiResponse class to beautify the process of checking the Result and getting the error so it will look like: myResult.IsValid() and myResult.GetErrors()

The rest of the work is migrating the existing code to use the new Refit pattern. So from this point of view, I think we can still use ApiReponse to satisfy our requirement without introducing a new ResultFactory.

[james-s-tayler]

Thanks for sharing that. It makes a lot of sense. I'm doing something similar myself using Refit to replace SDKs of 3rd party APIs we integrate with, in order to get consistent behavior and I also build my own result object I return, but it's a little different because it aggregates the results of several calls and not just a single call, but I can definitely see the appeal of the ability to customize it.

[james-s-tayler]

#1167 has been merged now @toanxyz , so future releases won't suffer from this problem. Thanks for raising this.

[github-actions]

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