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

FlurlResponse: a better way to deserialize a response body after inspecting other properties of the response #354

Closed
tmenier opened this issue Jul 27, 2018 · 28 comments

Comments

@tmenier
Copy link
Owner

tmenier commented Jul 27, 2018

Problem:

Flurl.Http has a set of methods that let you fluently specify how to receive/deserialize response bodies. Example:

var result = await url.PostJsonAsync(data).ReceiveJson<T>();

But sometimes you want to inspect other parts of the response before you deserialize it. The advice has been to do this:

HttpResponseMessage response = await url.PostJsonAsync(data);
// do something with the response, then deserialize it yourself

The problem is you can't use ReceiveJson for the for the "deserialize it yourself" part because it's an extension method on Task<HttpResponseMessage> rather than on HttpResponseMessage.

Solution:

Flurl.Http 3.0 introduces a new object, FlurlResponse. It wraps HttpResponseMessage and provides familiar methods for getting/deserializing the response body, after the call has completed. It also includes properties that provide uniformity with FlurlRequest, such as a simplified Headers collection that makes no distinction between response-level and content-level headers.

var resp = await url.PostJsonAsync(data);
var headerVal = resp.Headers.FirstOrDefault("my-header");
var result = resp.GetJsonAsync<T>();

Here is the full interface (subject to change, will update here when it's finalized):

public interface IFlurlResponse : IDisposable
{
    HttpResponseMessage ResponseMessage { get; }

    IReadOnlyNameValueList<string> Headers { get; }
    IReadOnlyList<FlurlCookie> Cookies { get; }
    int StatusCode { get; }

    Task<T> GetJsonAsync<T>();
    Task<dynamic> GetJsonAsync();
    Task<IList<dynamic>> GetJsonListAsync();
    Task<string> GetStringAsync();
    Task<Stream> GetStreamAsync();
    Task<byte[]> GetBytesAsync();
}

This will be a breaking change in that methods that currently return a "raw" HttpResponseMessage (SendAsync, GetAsync, etc.) will return a FlurlResponse instead. (GetJson, ReceiveJson, etc will continue to work the same.) It will make working with headers easier and more consistent with FlurlRequest, and will allow calling Receive* in a separate step.

Note that one of the most common reasons you might want to handle a response differently is to handle error responses, since they likely take a different shape than successful responses. For this I still recommend the try/catch pattern as a clean and elegant solution. But there are other cases where a 2-step approach could be needed, such as checking a response header.

@alexsorokoletov
Copy link

+1 to this one. Our scenario is to get some headers from the response itself as the API we use provides some additional information along the response body.

@tmenier tmenier removed the next up label Nov 18, 2018
@tmenier tmenier removed this from the Flurl.Http 2.4.1 milestone Jan 19, 2019
@tmenier tmenier added this to the Flurl.Http 3.0 milestone Jan 26, 2019
@tmenier
Copy link
Owner Author

tmenier commented Jan 26, 2019

3.0 will tentatively introduce a FlurlResponse object, which will be the perfect place to address this.

@tmenier tmenier added 3.0 and removed reviewed labels Mar 24, 2019
@tmenier
Copy link
Owner Author

tmenier commented Apr 6, 2019

I'm actively gathering feedback to help prioritize issues for 3.0. If this one is important to you, please vote for it here!

@tmenier tmenier changed the title ReceiveXXX-equivalent extensions on HttpResponseMessage FlurlResponse: a better way to deserialize a response body after inspecting other properties of the response Apr 26, 2019
@Jestar342
Copy link

Does this even require an entire new class?

If there were just overloads that didn't take a Task<HttpResponseMessage> but just the HttpResponseMessage this would be resolved without any breaking change, and without disturbing the "lightweight touch" nature of Flurl.

Creating a FlurlResponse is a big step in the wrong direction for this otherwise great library.

@tmenier
Copy link
Owner Author

tmenier commented Sep 13, 2019

@Jestar342 Thanks for the feedback, I take it seriously because maintaining the "lightweight touch" is a huge priority for the library. (I've declined many feature requests on that premise.)

I am still of the opinion though that the FlurlResponse idea is a natural fit. It adds uniformity with FlurlRequest and little else. I don't think it'll make anything feel "heavy", and implementation-wise it certainly won't add significant overhead. Flurl has always been about lightweight wrappers around objects from the HttpClient stack that are a little nicer to work with, and HttpResponseMessage never got any love.

The best example of what it'll improve is working with headers. I (and others) find HttpRequestMessage.Headers and HttpResponseMessage.Headers a little clunky to work with, and FlurlRequest attempts to solve that by exposing them as a simple and intuitive string dictionary. FlurlResponse will just do the same on the response side.

I'm willing to keep the conversation going though. If you have any more details on why you think this is a step in the wrong direction, I'm interested in hearing them.

tmenier added a commit that referenced this issue Nov 9, 2019
@tmenier
Copy link
Owner Author

tmenier commented Nov 9, 2019

This has been released in the first 3.0 prerelease.

https://www.nuget.org/packages/Flurl.Http/3.0.0-pre1

Important note: FlurlResponse.Cookies collection is NOT YET WORKING. That will implemented soon.

@imtrobin
Copy link

Why make Call.Response.StatusCode int and not HttpStatusCode directly? I see you do a int cast anyway.

@Ivutar
Copy link

Ivutar commented Mar 4, 2020

We can use static method Task.FromResult

var response = await "some url".GetAsync(); // <-- get headers, cookies and etc
var result = await Task.FromResult(response).ReceiveString(); // <-- get response content

@tmenier
Copy link
Owner Author

tmenier commented Mar 4, 2020

@imtrobin I decided to standardize on ints for status codes because many developers (myself included) prefer they not be abstracted away in an enum. APIs are documented in terms of numeric status codes. I'd rather see 401 in code than HttpStatusCode.Unauthorized because I know exactly what it is, or at the very least I can easily match it up with the API documentation (or swagger spec or cURL or Postman or whatever I'm looking at for reference). Yes, you can easily just cast it to an int, but Flurl is all about saving keystrokes. And it's opinionated. :)

@tmenier
Copy link
Owner Author

tmenier commented Mar 4, 2020

@Ivutar Yeah, that's a good way to do it now, but it's a little ugly.

@imtrobin
Copy link

imtrobin commented Mar 5, 2020

Then why not make a overload Call.Response.HttpStatusCode for those who want the old behavior? For error, I can use reflection to output the error in an understandable stringrather than looking up[ what is error 405, 406.

(Also this has the problem with Response being possibly null, as mentioned in other issue)

@tmenier
Copy link
Owner Author

tmenier commented Mar 5, 2020

@imtrobin If you prefer the enum you can use Call.HttpResponseMessage.StatusCode.

(To guard against a null response, Call.HttpResponseMessage?.StatusCode .)

@imtrobin
Copy link

imtrobin commented Mar 8, 2020

Thanks but I think we need more than that e.g something like
int error = Call?HttpResponseMessage?.StatusCode ?? 0 ; // or some default value

Which is a lot more typing than the old way (I'm not in front of coding PC, might be wrong name)
Exception ex;
ex.StatusCode

@tmenier
Copy link
Owner Author

tmenier commented Mar 8, 2020

@imtrobin This might be a better conversation for #488. But I will say that status code was never directly on the exception object. It was on Call, but that felt redundant with having it on Call.Response without much gain. And it never had a default - it was always nullable.

I'll re-examine what's available directly on FlurlException though. I do want to make it as easy and noise-free as possible to get at the things most commonly needed in an error-handling context.

@snekbaev
Copy link

snekbaev commented Mar 8, 2020

@tmenier not specific to this thread: given there's an ongoing v3.0 refactoring and I will need to upgrade at some point, would really appreciate if Flurl API docs (if that's not the case currently) would mention whether certain properties can be null and if yes - when :)

@tmenier
Copy link
Owner Author

tmenier commented Mar 22, 2020

@snekbaev I'm not going to promise you that I'll explicitly document everything that could be null. I tend to think it's pretty intuitive in most cases. For example, Call.Response represents an HTTP response, and if one was never received, it's reasonable to assume that it'll be null. (A "default" response wouldn't accurately represent that situation, in my opinion.) But if there is a response, its StatusCode will never be null because by definition it's a required part of a response, not mention it's a non-nullable int in the library.

Are there specific cases you find confusing?

@snekbaev
Copy link

@tmenier following reply is not Flurl specific: was thinking about cases when it could be null, for example, if exception happens and depending on the exception type or even for the same type certain properties may or may not be populated. Say, same exception is thrown when serialization fails and when actual http call fails. In the former case .Response will be null. Documenting this helps to avoid gotcha's in the try-catch logic later, because one may assume Response is there because during dev/testing he never got a serialization related exception... and then it happens, now there's a null-ref on top of that :)

@tmenier tmenier mentioned this issue Mar 29, 2020
@jimmcslim
Copy link

Would FlurlResponse better support RFC 7807 Problem Details? I've added that as a separate feature request at any rate: #528

@tmenier
Copy link
Owner Author

tmenier commented Aug 8, 2020

Headers and Cookies collection types have changed for prerelease 4. See #541.

@gitfool
Copy link
Contributor

gitfool commented Sep 12, 2020

FWIW, re documenting null flows, that could be also covered by Flurl eventually using c# 8 nullable reference types.

@tmenier tmenier removed this from the Flurl.Http 3.0 milestone Oct 1, 2020
@tmenier tmenier closed this as completed Nov 1, 2020
@HolisticDeveloper
Copy link

I think the link to the try/catch pattern in the first post should be https://flurl.dev/docs/error-handling/.

@tmenier
Copy link
Owner Author

tmenier commented Jan 8, 2021

@HolisticDeveloper Thanks, fixed.

@stellarpower
Copy link

I don't know if this is better put as a new issue, or added into this one (LMK!) - what you mention in the initial report sounds quite similar to my issue, but I believe it may be separate.

I have been hitting recently some quite hard-to-trace issues, which I assume are because ReceiveJson is an extension method - I'm not a .net developer so please excuse my oversight, I may be way off and it's another 2 AM moment.

As mentioned, it seems I can:
var r = request.PostJsonAsync(); var j = await r.ReceiveJson<MyType>();
or:
var r = await request.PostJsonAsync(); var j = await r.GetJsonAsync<MyType>();
but I can't:
var r = await request.PostJsonAsync(); var j = await r.ReceiveJson<MyType>();

My calls are spread out a bit so it's not so overt as above - safe ot say I never want to hear the words "async" or "await" again as long as I live! The issue I have had is that the last one is in fact compiling, whilst IFlurlResponse doesn't support it, and so this becomes a runtime exception. I don't know if this is the fault of setup on my end - some things seem to have become dynamically typed for reasons I don't understand, so that may be the cause, but if using the extension methods (or whatever it is they are) means one would expect the third call to fail only at runtime, then that's been quite a nuisance for me - it's been about 90 minutes trying to chase some failure to deserealise that works in another piece of code, so it would definitely be preferable if it could be caught at compilation!!

Thanks!

@tmenier
Copy link
Owner Author

tmenier commented Oct 11, 2021

@stellarpower your 3rd call shouldn't even compile because ReceiveJson is an extension method of Task<FlurlResponse>, not FlurlResponse. Also, closed issues are not a good place for programming questions. If you have further questions about this, please ask on Stack Overflow.

@stellarpower
Copy link

Thanks for getting back to me. It wasn't so much a question, but as I'm not a .net developer, I can't say with confidence what I think I'm seeing. But you have hit the upshot of what I observed, IIRC. I should have had a compile-time error for the third call, but I didn't. Instead it built and then threw an exception that was tricky to pin down, because the runtime type didn't support that extension method.

@rcdailey
Copy link

One use case this doesn't seem to handle is the ability to deserialize based on typeof(T). There are cases where the type of the object to deserialize is only known at runtime.

@tmenier
Copy link
Owner Author

tmenier commented Sep 23, 2023

@rcdailey I don't believe that's an exceedingly common scenario, but the simple work-around is to use GetStringAsync or GetStreamAsync along with JsonSerializer.Deserialize.

@rcdailey
Copy link

I thought about that but I don't think there's a way to pass through the serializer options.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests