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

[BUG] When response deserialization fails, Content is null in ApiException #1384

Closed
KarolStosik opened this issue Jul 19, 2022 · 7 comments · Fixed by #1712
Closed

[BUG] When response deserialization fails, Content is null in ApiException #1384

KarolStosik opened this issue Jul 19, 2022 · 7 comments · Fixed by #1712
Labels

Comments

@KarolStosik
Copy link

KarolStosik commented Jul 19, 2022

Describe the bug

When Refit can't deserialize response from api, ApiException is thrown. Unfortunately, Content property is always null, but it should be populated with the response content, that can't be deserialized.

Steps To Reproduce

Reproduction with simple XUnit test (api can be arbitrary, here I've used first public api I've found):

using System.Threading.Tasks;
using Refit;
using Xunit;

namespace RefitReproduction
{
    public class RefitNullContentTest
    {
        [Fact]
        public async Task ShouldHaveContentInException()
        {
            var api = RestService.For<IApi>("https://api.spacexdata.com");
            var exception = await Assert.ThrowsAsync<ApiException>(() => api.GetLaunches());
            Assert.NotNull(exception.Content); // fails!
        }
    }

    public interface IApi
    {
        [Get("/v3/launches")]
        public Task<double> GetLaunches(); // any type not matching api response
    }
}

Expected behavior

When deserialization fails, Content property should be populated with response content.

Environment

  • OS: Windows
  • Device: PC
  • .NET version: 6.0
  • Refit version: 6.3.2
@KarolStosik
Copy link
Author

I've found the source of bug:

ApiException.cs, line 132:

            try
            {
                exception.ContentHeaders = response.Content.Headers;
                var content = await response.Content.ReadAsStringAsync().ConfigureAwait(false);
                exception.Content = content;

                if (response.Content.Headers?.ContentType?.MediaType?.Equals("application/problem+json") ?? false)
                {
                    exception = ValidationApiException.Create(exception);                    
                }

                response.Content.Dispose();
            }
            catch
            {
                // NB: We're already handling an exception at this point, 
                // so we want to make sure we don't throw another one 
                // that hides the real error.
            }

response.Content.ReadAsStringAsync() throws System.InvalidOperationException: 'The stream was already consumed. It cannot be read again.'

Anyone knows how to fix it?

@XVCoder
Copy link

XVCoder commented Aug 20, 2023

So, any solutions now?

@thomasb-gr
Copy link

Hi, were you able to figure out a solution to this problem?

@KSwift87
Copy link

KSwift87 commented Mar 18, 2024

I just ran into this myself -- I found a stopgap solution in the interim while Dwolla gets around to resolving this which unfortunately judging by the timestamps on comments on this thread could be awhile. 😞 I just manually deserialize the RawContent property into the expected type myself. The below example is utilizing Newtonsoft.Json for the deserialization and Dwolla.Client v6.0.0:

// Omitted setting up uri and headers for brevity.
var result = await client.GetAsync<BalanceResponse>(uri, headers);

// Omitted response error checking for brevity.
return JsonConvert.DeserializeObject<BalanceResponse>(result.RawContent);

@LichP
Copy link
Contributor

LichP commented May 30, 2024

I think I have a solution to this: if we force the HttpContent to load into a buffer before doing anything with it, the exception handling path is able to re-read the content later on:

diff --git a/Refit/RequestBuilderImplementation.cs b/Refit/RequestBuilderImplementation.cs
index 20092b5..2ed7c62 100644
--- a/Refit/RequestBuilderImplementation.cs
+++ b/Refit/RequestBuilderImplementation.cs
@@ -337,6 +337,7 @@ object itemValue
                         .SendAsync(rq, HttpCompletionOption.ResponseHeadersRead, ct)
                         .ConfigureAwait(false);
                     content = resp.Content ?? new StringContent(string.Empty);
+                    await content.LoadIntoBufferAsync().ConfigureAwait(false);
                     Exception? e = null;
                     disposeResponse = restMethod.ShouldDisposeResponse;

I've tried this out on the project I'm currently working on, and I'm now getting the content coming through in the ApiException resulting from a System.Text.Json.JsonException thrown during response content deserialization. I'll put this into a PR tomorrow.

@LichP
Copy link
Contributor

LichP commented May 31, 2024

Just to note I've moved the LoadIntoBufferAsync to just before the deserialization in the PR to avoid any potential problems for endpoint methods expecting to handle the underlying stream directly (if an app is expecting a 1TB video from an endpoint we probably don't want to try loading the whole thing into a buffer!)

@ChrisPulman ChrisPulman mentioned this issue Jun 9, 2024
2 tasks
Copy link

github-actions bot commented Jul 5, 2024

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants