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

Make the RestMethodInfo available in the request options #1317

Merged
merged 5 commits into from
Apr 12, 2023

Conversation

0xced
Copy link
Contributor

@0xced 0xced commented Feb 15, 2022

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 0xced force-pushed the RequestOptions-RestMethodInfo branch from 63b5054 to a3c0a70 Compare February 15, 2022 20:03
@0xced 0xced mentioned this pull request Feb 15, 2022
2 tasks
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:

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

Inside the HTTP message handler:

```csharp
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:

```csharp
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 0xced force-pushed the RequestOptions-RestMethodInfo branch from a3c0a70 to 53ab900 Compare February 15, 2022 20:08
@Int32Overflow
Copy link
Contributor

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

james-s-tayler added a commit to james-s-tayler/refit that referenced this pull request May 26, 2022
@james-s-tayler
Copy link
Contributor

james-s-tayler commented May 27, 2022

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.

@gabrielmaldi
Copy link

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);
    }
}

@gokhanabatay
Copy link

@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.

@ChrisPulman ChrisPulman self-assigned this Apr 12, 2023
@glennawatson glennawatson merged commit c0af5c2 into reactiveui:main Apr 12, 2023
1 check passed
@github-actions
Copy link

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 github-actions bot locked as resolved and limited conversation to collaborators Apr 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants