refit icon indicating copy to clipboard operation
refit copied to clipboard

Make the `RestMethodInfo` available in the request options

Open 0xced opened this issue 3 years ago • 3 comments

While having the interface type available is nice, it might not be enough if using reflection on the called method is desired. Providing the RestMethodInfo opens new possibilities to introspect the called method.

Currently, to workaround this missing information, I'm using a methodName parameter decorated with a [Property] attribute and a default value to make it possible to introspect the return type of the called method.

At the interface method:

Task<ApiResponse<User>> GetUserAsync(string id, [Property] string methodName = nameof(GetUserAsync));

Inside the HTTP message handler:

request.Options.TryGetValue(new HttpRequestOptionsKey<Type>(HttpRequestMessageOptions.InterfaceType), out var refitInterfaceType)
request.Options.TryGetValue(new HttpRequestOptionsKey<string>("methodName"), out var methodName)
var methodReturnType = refitInterfaceType.GetMethod(methodName).ReturnType;

With the new RestMethodInfo, it becomes possible to access the method without having to pollute the interface definition:

request.Options.TryGetValue(HttpRequestMessageOptions.RestMethodInfoKey, out var restMethodInfo);
var methodReturnType = restMethodInfo.MethodInfo.ReturnType;

Also, the new HttpRequestMessageOptions.InterfaceTypeKey and HttpRequestMessageOptions.RestMethodInfoKey (available on .NET 5 onwards) make it easier to access the request options.

0xced avatar Feb 15 '22 19:02 0xced

Please can someone review this pull request? @clairernovotny @hamidrezakp @dannyBies

Int32Overflow avatar May 06 '22 10:05 Int32Overflow

I really want to see the MethodInfo available in the HttpRequestMessage.Options, but I think exposing the entire RestMethodInfo leaks too many internal implementation details to clients that if clients rely on then coupling to those implementation details means less flexibility for Refit to make changes in the future as the surface of what's potentially a breaking change becomes quite large.

I've submitted a PR for this prior that was rejected for being too fancy, but part of my motivation for that particular (flexible) solution I came up with was not wanting to have the MethodInfo included into the HttpRequestMessage.Options by default as object graphs in the reflection namespace tend to be very heavy and by including it by default we could introduce a somewhat nasty gotcha or very fun production incident upon upgrade of Refit for anyone (rightly or wrongly!) using Serilog to log the destructured object graph of HttpRequestMessage.Options. I desperately want it to be available as I think it's the last feature that Refit is really missing to make it complete, but strongly prefer it be available on an opt-in basis.

Previously the author/maintainers have expressed a desire to simply include the method name as a string, but from my point of view once you have an overloaded method on your interface then all bets are off and I would expect that to cause bugs and support issues somewhere down the line. Passing the MethodInfo makes it a rock-solid guarantee.

I might submit a PR for that particular implementation as seeing renewed interest in it has sparked my desire to see this particular feature get across the line in some format.

james-s-tayler avatar May 27 '22 12:05 james-s-tayler

I would love to have this to be able to override HttpClient's timeout on a per-operation basis:

services
    .AddRefitClient()
    .ConfigureHttpClient(httpClient => httpClient.Timeout = TimeSpan.FromSeconds(30));
public class HttpMessageHandler : DelegatingHandler
{
    protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
    {
        var timeoutSeconds = 15; // ToDo: read configuration for operation identified by request.Options[HttpRequestMessageOptions.RestMethodInfoKey]
        using var timeoutTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(timeoutSeconds));
        return await base.SendAsync(request, timeoutTokenSource.Token);
    }
}

gabrielmaldi avatar Aug 05 '22 23:08 gabrielmaldi

@james-s-tayler when dou you plan to handle this pr there are lots of demands on this subject passing method info to delegating handler. Its needed most cases; passing timeout, getting some kind of caching behaviour or return type per operation.

gokhanabatay avatar Jan 15 '23 01:01 gokhanabatay

This pull request 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[bot] avatar Apr 27 '23 00:04 github-actions[bot]