Make error body available in FlurlHttpException.Message (opt-in?) for generic logging

[Will-at-FreedomDev]

Forgive me if there is a way to do this within the Flurl library already. If so, I'll gladly take your advice on a better solution :)

We have code that looks like this as it's pretty difficult to debug a failed request without reading the response:

    protected async Task<T> ExecuteRequest<T>(Func<Task<T>> executeRequest)
    {
        try
        {
            return await executeRequest();
        }
        catch (FlurlHttpException ex)
        {
            var errorResponse = string.Empty;

            try
            {
                errorResponse = await ex.GetResponseStringAsync();
            }
            catch { }

            if (string.IsNullOrEmpty(errorResponse))
            {
                throw;
            }
            else
            {
                throw new Exception(errorResponse, ex);
            }
        }
    }

It would be a great enhancement if the FlurlHttpException or even the initial request fluent API had a way to automatically enable this exception handling. Something like:

   protected async Task<T> ExecuteRequest<T>(Func<Task<T>> executeRequest)
    {
        try
        {
            return await executeRequest();
        }
        catch (FlurlHttpException ex)
        {
            await ex.ThrowResponseExceptionAsync();
        }
    }

Or via the fluent API:

return await url
    .WithResponseException()
    .GetAsync();

It may also need some features to allow deserializing the error response as well, but for my situation reading it as a string is all I need.

[tmenier]

I'm not sure I follow. Have you looked at using the OnError[Async] event handler?

https://flurl.dev/docs/configuration/#event-handlers

[Will-at-FreedomDev]

I definitely used the wrong language as Response is misleading... I'm talking about the response's body.

I've looked at it. From my understanding, it's just a global error handler for the same/similar code I snippeted above. I suggest adding a new Exception type that already has the response body assigned to it. Something like FlurlHttpResponseException.

I understand there is computational performance involved in reading the response, so I get why FlurlHttpException doesn't just have this available on the Exception itself. FlurlHttpException requires you to download it yourself as shown above. It's not as big of a deal with the OnError event handler as I can write the above exception handling once, but I still need to go and get the response, pass it into an exception type, rethrow the exception, etc.

I think newcomers to the library would benefit from this enhancement as well. Many times, I've seen code where the FlurlHttpException is bubbled up to a log but there is no real information there as the body is not read anywhere (hence my code above reads the body if it can and re-throws a new exception with the body as the new message).

It's not something critical by any means as people have been doing it the long-form way, but I'd like to make this the default error handler in a simple wrapping API method via Flurl if possible.

public FlurlHttpResponseException<T> : FlurlHttpException 
{
    public T Response { get; set; } // I'm not sure if exceptions can have generics - might just need to be `string`
}

I guess I'm just getting at the fact that I (maybe incorrectly) assume that people would generally always want the response's body to be included in the exception if Flurl had thrown one at all. Doing the extra work to go and get it and handle the exception better than the library feels like a feature miss?

[tmenier]

I guess I'm just getting at the fact that I (maybe incorrectly) assume that people would generally always want the response's body to be included in the exception

Not necessarily. Sometimes the HTTP status code is good enough. Flurl basically gives you 3 options:

1. ex.GetResponseStringAsync
2. ex.GetResponseJsonAsync<T> (slightly more efficient than getting a string first)
3. neither. skip the overhead entirely.

Keep in mind that options 1 & 2 are both one-liners in your catch block. I'm not seeing a real benefit of a new exception type, especially if you need to opt in with something like your WithResponseException.

[Will-at-FreedomDev]

No problem! I appreciate the time. We can have the error handler "download" (I'm actually not sure what the async is doing) the response body. We typically don't take additional action on exception, simply log them in case we need to investigate an API issue so I thought maybe we could bake that in a bit more elegantly but maybe it's a niche thing to do so.

[Will-at-FreedomDev]

I guess one follow up point, you say that 1 & 2 are one-liners, but they aren't really because we don't want to unintentionally throw a different exception in the handler due to calling 1/2. They can most definitely throw an exception because I recently was experiencing that. That's why my snippet has that wrapped in a try-catch to swallow it.

We can for sure make an extension method or some other utility to make it a one-liner though.

[tmenier]

I'm going to leave this open and think on it some more. I'm starting to see your point that if something up the stack is handling errors generically (such as by logging them), it's probably common that you'd want the body of an error response in the exception Message somehow. There may still be a better way to solve this via the event handler, but I'll come back to this at some point.