Add the possibility to set Properties in HttpRequestMessage

[tranb3r]

Hi, Flurl is almost perfect, except I need to pass some Properties to the HttpRequestMessage, and Flurl.Http does not support it yet. I think the best way to do it would be to add a "WithProperty" extension, similar to the existing "WithHeader" extension.

I've already already implemented it, so if you agree I can create a PR with my changes. Thanks tranb3r

[tmenier]

Can you explain the specific use case?

[tranb3r]

Using HttpRequestMessage.Properties is a way to pass data to an http DelegatingHandler, on a per-request basis. Several examples:

• log some contextual data, that is not already part of the request's query, content, headers or cookies. For example, a userId.
• override the default httpclient timeout, on a per-request basis. More details here : https://thomaslevesque.com/tag/httpclient/
• have a per-request error handling policy

[tranb3r]

So, what do you think ? The code is here if you want to take a look before accepting a PR.

[tmenier]

Sorry for the delay. I'm on the fence about whether this is a common enough scenario to get first-class treatment. I would likely accept it if there was no easy hook to get at the HttpRequestMessage, thus allowing you to build an extension method if it's a common scenario for you. And there is - via FlurlRequest.Settings.BeforeCall. You can even get at this fluently inline:

await "https://api.com"
    .ConfigureRequest(config => config
        .BeforeCall = call => call.Request.Properties["foo"] = "bar")
    .GetAsync();

With that, writing an extension method would be fairly trivial.

[tranb3r]

You're absolutely right, it's easier to do with FlurlRequest.Settings.BeforeCall. Would you accept a PR for adding such an extension to Flurl, or is it better if I keep it in my own project?

[tranb3r]

New code with pure extension is here if you want to take a look.

[tmenier]

I'm suggesting keeping the extension method in your own project for now. I don't dislike the idea conceptually but I want to think it over in the context of other feature requests in the backlog, looking out for feature bloat, etc. In short, I need some time to think on it, but an extension method that can be created out of what's available today will unblock you for now.

[tranb3r]

Adding this extension to Flurl would be a guarantee that it is properly tested and that it will continue to work with future versions. But I understand your point, and I'm fine with keeping it in my own project for now. Thanks a lot.

[tmenier]

Please know that this issue is far from dead! I will first point out that I know not everyone is going to read the contribution guidelines and it's not an "absolute", but the one guideline I do wish people would follow more often is to please just open an issue first so we can discuss it, before you spend time coding it. I think it just makes sense to be on the same page that it's a good fit for the library first.

That said, I'm warming up to the idea in general. But I'd want to do it in a way that's robust and consistent with other features. For example, in addition to WithPropery, it would probably make sense to have a WithProperties that takes an object (typically anonymous) and parses its properties into key/value pairs.

But before you code that ;), I might want to take it even a little farther. It could be a feature primarily aimed at providing "context" data to Flurl's event handlers, and the fact the it's backed by HttpRequestMessage.Properties (and can therefore be accessed in a message handler) would be more of a side-benefit. In this case I don't know if we'd even need to be married to the word "Properties". I'm not sure if I like that, maybe "Context" or just "Data" would be better.

Thoughts?

One other thing to mention is I'm in the early stages of 3.0 planning and I'm probably going to put out a survey of some sort to get feedback on what new features people want most. I think this is a good idea to add.

[tmenier]

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

[tranb3r]

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

Thank you! Done.

[tranb3r]

But before you code that ;)

Actually, I've read the guidelines before opening this issue... I really needed a solution, this is why I coded the extension, and I'm very happy using it right now in my project, so it's not waisted time ;)

Gathering feedback is a very good idea and I totally trust you on doing what's best for Flurl.

[tmenier]

Thought of another possible use case for this. It's been asked before how to dynamically change the proxy server used per call. The short answer is you can't, indirectly because HttpClient doesn't support that, so you'd need to create a new client. It would be nice to be able to somehow use FlurlClient to create and cache instances per proxy server address, but currently that object only supports deriving a cache key from the URL. If some per-request properties/settings dictionary were also passed to it, this would be possible.

More of a note to self but I'm liking this idea better all the time. :)

[unleashed-jamest]

Plus one for this feature!

One of my main use cases is declaring success/error messages as part of the client and have DelegatingHandlers that are responsible for logging certain types of errors and success messages in a user facing log they can see, reporting certain other types of errors back to the client directly and logging diagnostic error messages to a non-user facing log. I don't want the calling code peppered with try catches and lots of custom logic to do all the logging, so I declare it as part of the client itself currently by adding HTTP headers like X-User-Facing-Error: "Customer {id} does not exist" and then pulling them out in a series of DelegatingHandlers each responsible for different types of logging. It's a nasty hack to have to use HTTP headers, but it works and in general I'm really liking the programming model, but Properties is where this stuff really belongs.

In general being able to plumb a consistent Context object through your entire pipeline of DelegatingHandlers is a huge win. You can collect performance metrics and counters of each part of the pipeline and report on it at the end. You can't exactly stuff a complex object into HTTP headers, but with access to Properties on a per-request basis you could do neat stuff.

[kayhustle]

Was this ever implemented? I could use this as well. Currently I'm using a BeforeCall handler that caches the request in a dictionary but would be much easier if it were on the request itself.

[james-s-tayler]

@kayhustle I just got a PR for it merged into Refit https://github.com/reactiveui/refit/pull/1001 at least. RestEase is another library that supports it too. I opened an issue on Refit repo at same time I commented on this repo, as It's actually a pretty important feature if you want to fully support Polly especially with Polly.Context etc.

[kayhustle]

@kayhustle I just got a PR for it merged into Refit reactiveui/refit#1001 at least. RestEase is another library that supports it too. I opened an issue on Refit repo at same time I commented on this repo, as It's actually a pretty important feature if you want to fully support Polly especially with Polly.Context etc.

Most definitely. Nice! Refit is pretty cool as well.

[torrentula]

I know its been a while, but I just came across this issue trying to implement the Polly policy pipeline in my ASP.NET Core project. I really like the idea of using a "WithProperties" extension method to achieve my needs for this explicit used case, pointed out by the Polly project as a good practice. Please check this part of the documentation regarding the HttpClientFactory and settings up policies for common resilience scenarios: https://github.com/App-vNext/Polly/wiki/Polly-and-HttpClientFactory#using-addtransienthttperrorpolicy

This works just fine, but if you need any dependency like a ILogger or so, you have a problem, cause you can only get it from the Polly.Context the policy offers you. There is an extension method made by the Microsoft team that is able to set the context, but since it relies on a HttpRequestMessage and its already named Properties, there is no easy way to achieve whats described here with Flurl: https://github.com/App-vNext/Polly/wiki/Polly-and-HttpClientFactory#configuring-httpclientfactory-policies-to-use-an-iloggert-from-the-call-site

In my opinion a really elegant way to utilize the Polly.Context to get the required ILogger instance. Of course this could be also be very useful to get a correlation ID or any other information that is carried by an OperationContext. I did not find any way set the Polly.Context using Flurl, and I will now try to utilize tranb3e's proposed extension to get it working without getting back to the old HttpClient and its HttpRequestMessage object, cause I really like Flurl.

[torrentula]

Sorry for the delay. I'm on the fence about whether this is a common enough scenario to get first-class treatment. I would likely accept it if there was no easy hook to get at the HttpRequestMessage, thus allowing you to build an extension method if it's a common scenario for you. And there is - via FlurlRequest.Settings.BeforeCall. You can even get at this fluently inline:

await "https://api.com"
    .ConfigureRequest(config => config
        .BeforeCall = call => call.Request.Properties["foo"] = "bar")
    .GetAsync();

With that, writing an extension method would be fairly trivial.

This did work for me, thanks for pointing it out, and I created a corresponding extension:

public static class FlurlRequestExtensions
{
      public static IFlurlRequest SetPolicyExecutionContext(this IFlurlRequest request, Context context)
      {
            return request.ConfigureRequest(config => config.BeforeCall = call => call.HttpRequestMessage.SetPolicyExecutionContext(context));
      }
}

The following NuGet package besides Polly is required of course: Microsoft.Extensions.Http.Polly.

[tmenier]

Dusting this one off and giving it some love (finally!) for 4.0. There's another issue (#639) that could really benefit from having a simpler way to access the HttpRequestMessage just before it's sent. I'm fiddling with the idea of a chainable ConfigureRequestMessage method that I'd say would be slightly less pretty than WithProperties but more pretty than handling BeforeCall as proposed here.

For anyone actually still following this issue, feel free to weigh in. 😄

[tranb3r]

I still think WithProperties makes sense (alongside WithCookie, WithHeader...). I'm not a huge fan of ConfigureRequestMessage, I'd prefer if Flurl could take care of the low-level stuff internally. But... you already know what I think, since I've created this issue more than 4 years ago ;)

[kayhustle]

I agree with adding WithProperties. I think it's a very elegant solution. Glad to see this one finally getting the love it deserves some 4 years later :)

[tmenier]

I will reconsider. #639 was easy to solve in a more direct way using the Settings objects, and I'll consider a more direct solution for this one as well. In the mean time, BeforeCall is still the work-around for modifying HttpRequestMessage directly.