feature: Allow decoration of interface methods to be used by DelegatingHandlers

[davidkvc]

Problem:

I want to be able to make decisions in DelegatingHandler based on the client
interface method being executed.

Specifically I want to apply retry logic to some requests. I would like to be able
to have one DelegatingHandler type for that and be able to somehow mark
interface methods where retry should be allowed. Deciding only based on request
method is not enough.

Describe the solution you'd like

One possible solution is to add another property to HttpRequestMessage.Properties/Options
that would contain MethodInfo of the method. Then I could define custom attribute that can
mark methods where retry is enabled and my DelegatingHandler could look for that attribute.

Another solution could be to add a new attribute like ConstantProperty that could be used
on method. It would work like existing PropertyAttribute with the difference that the value
of the property would be specified in constructor and would be constant. Again, I could look
for that property in the DelegatingHandler

Describe alternatives you've considered

I don't really see how could this be done today in a clean way. The only way to pass additional
information to DelegatingHandler is through PropertyAttribute but that requires adding
method parameter which is not right. Caller shouldn't have to pass another value to each
method to manage how the client works internally.

Additional context

#1150 seems like similar request

[james-s-tayler]

Hmm... I currently solve exactly this problem today using https://github.com/App-vNext/Polly which in behind the scenes actually leverages DelegatingHandlers to do its magic.

I have some endpoints in a 3rd party API I need to integrate with that have different needs with respect to when/what to retry on, and I simply grouped stuff with the same needs into one Refit client, and grouped stuff with a different set of needs into a different Refit client, and then applied the appropriate Polly policies to each Refit client separately. That worked pretty well.

As for adding a new attribute, the additional complexity and effort required to add a new attribute is kind of high in conjunction with that not really being a very intuitive programming model, so I wouldn't think that's the right way to solve it.

Adding MethodInfo into HttpRequestMessage.Properties/Options is potentially a better solution, as the InterfaceName is already always added there, but I would be a little bit wary of including it that entire object. To be honest I'm not even really a fan of including the interface name by default, as I think it would be better if Refit didn't have any strong opinion about what winds up in HttpRequestMessage.Properties by default as there could well be other DelegatingHandlers that we have no knowledge of that could behave in unexpected ways if we populate data into that willy nilly. For example in my case I use a DelegatingHandler to log the request/response payloads to Serilog. This includes logging the contents of HttpRequestMessage.Properties as that's where Polly stores the PollyContext which can be used for example to populate the number of retries a request has made etc. It'd be fairly nasty if Refit suddenly had the opinion that the entire object graph of MethodInfo should also be serialized into my logs. I mean, I could obviously work around that, but it's still not super nice and could very well introduce subtle and annoying bugs for consumers of the library.

I do think it's an interesting use case and warrants more thought on a design that enables the kinds of extension that you want to do, as I think fundamentally that's quite worthwhile. A couple of thoughts come to mind. First off, I would only really want to see something appearing in HttpRequestMessage.Properties on a purely opt-in basis. With the right design I think that can be accomplished. Second would be doing it in a way that opened up a whole class of extensions that provides a very low friction way to annotate a method with your own custom attribute and then have access to that information inside your delegating handler.

What springs to mind is something more along the lines of adding a base class attribute that you subclass and annotate your method with and then when we build the request in Refit we could inspect the custom attributes and dump those particular ones into HttpRequestMessage.Properties using a particular key like "RefitCustomAttributes" or something like this. I almost think we don't even need a base class. Just get the custom attributes and if it's not one that is part of the Refit code base, simply populate it into HttpRequestMessage.Properties with the key being the name of custom attribute. Or something like that. I think that could be quite a neat way to be able to declaratively pass through information to DelegatingHandlers without adding too much complexity or giving Refit too strong an opinion on what winds up in HttpRequestMessage.Properties.

GitHub

App-vNext/Polly

Polly is a .NET resilience and transient-fault-handling library that allows developers to express policies such as Retry, Circuit Breaker, Timeout, Bulkhead Isolation, and Fallback in a fluent and ...

[james-s-tayler]

Alternatively, as a thought... similar to how we have an ExceptionFactoryProvider that allows you to splice in custom behavior regarding what happens in certain error scenarios, it could be incredibly powerful to allow a PropertyFactoryProvider which allows simply hands you the MethodInfo and you hand it back a IDictionary<string, object> that you want to wind up in HttpRequestMessage.Properties! Then anything is possible.

[james-s-tayler]

Polly is a pretty flexible library, and it has support for IHttpClientFactory which applies DelegatingHandlers constructed by Polly with the policies you have configured to an HttpClient. From memory it has some degree of customization as to allow only running policies when certain predicates return true. Off the top of my head I can't remember what data you have access to during the evaluation of the predicates. I think it's the HttpResponseMessage and/or a particular type of exception etc. If you had access to the HttpRequestMessage.Properties you and could leverage that, that'd allow you to decide whether to execute or not based of anything you've managed to ferry into that object which would be quite powerful. Might be worth checking what's available there and if you could leverage it at present to solve this, or if Polly could be extended to allow that.

Either way I'm pretty keen on the PropertyFactoryProvider idea as I think that's a real enabler of many use cases we can't anticipate and if you can splice in behavior you need to solve a problem and unblock yourself that's a plus in my book.

[davidkvc]

I like the idea of PropertyFactoryProvider as well. I am sure it will enable a lot of new features. Thank you for looking into this.

[lostincomputer]

I second that allowing users to extend Refit would be great. We also has the requirement of passing constant values to DelegatingHandler and is the biggest reason why we aren't using Refit.

[james-s-tayler]

@lostincomputer working on a PR right now. Almost finished.

[james-s-tayler]

@dejvid-smth / @lostincomputer PR for this is here #1180

Would be excellent to see this merged. More or less you would just have to do this:

var settings = new RefitSettings
{
    PropertyProviders = PropertyProviderFactory
        .WithPropertyProviders()
        .RefitTargetInterfaceTypePropertyProvider()
        .MethodInfoPropertyProvider()
        .CustomAttributePropertyProvider()
        .PropertyProvider(new MyCustomPropertyProvider())
        .Build()
};

Game changer.