Skip to content

BindAsync Surrogate Method in Minimal APIs #50672

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

Open
commonsensesoftware opened this issue Sep 13, 2023 · 1 comment
Open

BindAsync Surrogate Method in Minimal APIs #50672

commonsensesoftware opened this issue Sep 13, 2023 · 1 comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc

Comments

@commonsensesoftware
Copy link

Background and Motivation

Minimal APIs currently support a number of mechanisms for parameter binding, but are limited to a static TryParse or BindAsync method for custom binding.

Some types of parameter binding, such as features, cannot be achieved without resorting to IHttpContextAccessor. This is both clunky and a forceful hand by platform extenders for consuming developers. TryParse is insufficient because the process can be more involved than simple parsing and BindAsync is impractical for types that do not have affinity to HTTP and cannot implement IBindableFromHttpContext<TSelf>.

Consider the following scenario for a Minimal API in ASP.NET API Versioning:

var app = builder.Build();
var orders = app.NewVersionedApi();
var v1 = orders.MapGroup("/orders").HasApiVersion(1.0);

var.MapGet("/{id:int}", (int id, ApiVersion version) => new Order() { Id = id, Version = version.ToString() });

For the ApiVersion parameter to be bound (currently), the following DI workaround is required:

builder.Services.AddHttpContextAccessor();
builder.Services.AddTransient(sp => sp.GetRequiredService<IHttpContextAccessor>().HttpContext?.GetRequestedApiVersion()!);

While this is just one scenario, there are many others like it. This is particularly true for edge cases that continue to crop up in the ever expanding items outlined in the internal EndpointParameterSource enumeration.

Proposed API

The proposed API would expand upon the capabilities of the BindAsync conventions and rules by allowing a surrogate method. These surrogates would be defined and consumed through Options. A surrogate BindAsync method would have the following rules:

  • Must be a static method (to avoid closure issues)
  • Must use the same supported signatures as the BindAsync convention
  • Must use the same supported IBindableFromHttpContext<TSelf>.BindAsync signature
  • May allow an alternate to an existing BindAsync implementation

The options might be defined as follows:

public sealed class BindAsyncOptions : IReadOnlyDictionary<Type, MethodInfo>
{
    private Dictionary<Type, MethodInfo>? map;

    public void Add<T>(Func<HttpContext, ValueTask<T?>> bindAsync) =>
        Add(typeof(T), bindAsync.Method);

    public void Add<T>(Func<HttpContext, ParameterInfo, ValueTask<T?>> bindAsync) =>
        Add(typeof(T), bindAsync.Method);

    private void Add(Type type, MethodInfo method)
    {
        if (!method.IsStatic)
        {
            throw new ArgumentException("The specified method must be static.");
        }

        map ??= new();
        map.Add(type, method);
    }

    MethodInfo IReadOnlyDictionary<Type, MethodInfo>.this[Type key] =>
        map is null ? throw new KeyNotFoundException() : map[key];

    IEnumerable<Type> IReadOnlyDictionary<Type, MethodInfo>.Keys =>
        map is null ? Enumerable.Empty<Type>() : map.Keys;

    IEnumerable<MethodInfo> IReadOnlyDictionary<Type, MethodInfo>.Values =>
        map is null ? Enumerable.Empty<MethodInfo>() : map.Values;

    public int Count => map is null ? 0 : map.Count;

    public bool ContainsKey(Type key) => map is not null && map.ContainsKey(key);

    IEnumerator<KeyValuePair<Type, MethodInfo>> IEnumerable<KeyValuePair<Type, MethodInfo>>.GetEnumerator() =>
        map is null ? Enumerable.Empty<KeyValuePair<Type, MethodInfo>>().GetEnumerator() : map.GetEnumerator();

    public bool TryGetValue(Type key, [MaybeNullWhen(false)] out MethodInfo value)
    {
        if (map is null)
        {
            value = default;
            return false;
        }

        return map.TryGetValue(key, out value);
    }

    IEnumerator System.Collections.IEnumerable.GetEnumerator() =>
        map is null ? Enumerable.Empty<KeyValuePair<Type, MethodInfo>>().GetEnumerator() : map.GetEnumerator();
}

To leverage the configuration, the internal ParameterBindingMethodCache would have to be changed. This class is source-shared across multiple libraries so the change should only use types known to all implementations (which may otherwise look strange).

HasBindAsyncMethod needs a new overload that can resolve the surrogate mapping dictionary:

+    [RequiresUnreferencedCode("Performs reflection on type hierarchy. This cannot be statically analyzed.")]
+    [RequiresDynamicCode("Performs reflection on type hierarchy. This cannot be statically analyzed.")]
+    public bool HasBindAsyncMethod(
+        ParameterInfo parameter,
+        IServiceProvider serviceProvider,
+        Func<IServiceProvider, IReadOnlyDictionary<Type, MethodInfo>?> resolve) =>
+        FindBindAsyncMethod(parameter, serviceProvider, resolve).Expression is not null;

A new GetIBindableSurrogate method would be added to allow resolving BindAsync mapped to a specific type.

+    private static MethodInfo? GetIBindableSurrogate(
+        Type type,
+        IServiceProvider? serviceProvider,
+        Func<IServiceProvider, IReadOnlyDictionary<Type, MethodInfo>?>? resolve)
+    {
+        if (serviceProvider is not null &&
+            resolve is not null &&
+            resolve(serviceProvider) is { } map &&
+            map.TryGetValue(type, out var method))
+        {
+            return method;
+        }
+
+        return null;
+    }

The FindBindAsyncMethod

var methodInfo = GetIBindableFromHttpContextMethod(nonNullableParameterType);

would be updated to allow:

+    var methodInfo = GetIBindableSurrogate(nonNullableParameterType, serviceProvider, resolve) ??
+                     GetIBindableFromHttpContextMethod(nonNullableParameterType);

The remaining rules and processing for BindAsync would remain unchanged and just work.

RequestDelegateFactory would subsequently be updated as follows:

+    private static IReadOnlyDictionary<Type, MethodInfo> ResolveBindAsyncSurrogates(IServiceProvider serviceProvider) =>
+        serviceProvider.GetService<IOptions<BindAsyncOptions>>()?.Value);

and then:

else if (ParameterBindingMethodCache.HasBindAsyncMethod(parameter))

becomes:

else if (ParameterBindingMethodCache.HasBindAsyncMethod(
         parameter,
+        factoryContext.ServiceProvider,
+        ResolveBindAsyncSurrogates))

and:

var bindAsyncMethod = ParameterBindingMethodCache.FindBindAsyncMethod(parameter);

becomes:

    var bindAsyncMethod = ParameterBindingMethodCache.FindBindAsyncMethod(
            parameter,
+           factoryContext.ServiceProvider,
+           ResolveBindAsyncSurrogates);

Usage Examples

This will now allow developers and platform extenders to define arbitrary BindAsync methods mapped to a specific type.

In the original example, instead of using IHttpContextAccessor, API Versioning could now register:

private static ValueTask<ApiVersion?> BindApiVersionAsync(HttpContext context) =>
    context.ApiVersioningFeature().RequestedApiVersion;

services.Configure((BindAsyncOptions options) => options.Add(BindApiVersionAsync));

This approach would work for any other feature, type, and so on that doesn't have TryParse (or is unsuitable) and cannot implement IBindableFromHttpContext<TSelf>. Consumers of such mappings are none the wiser and do not implicitly have to take a dependency on IHttpContextAccessor (which should be avoided).

Alternative Designs

  • BindAsyncOptions does not have to be IReadOnlyDictionary<Type, MethodInfo>; however,
    • most of the capabilities are dictionary-like
    • the instance would likely to need to expose or project into another dictionary instance
  • BindAsyncOptions does not have to supersede existing implementations
  • The surrogate method must be static, which means it must be a static function or local function
    • static lambdas are not guaranteed to be static functions by the compiler
    • It might be worth changing or loosening the rules to allow something like static (context) => ValueTask.FromResult(context.ApiVersioningFeature().RequestedApiVersion)

Open Questions

  • Should BindAsyncOptions.Add replace an existing registration or are multiple registrations an error?
  • Is source generation still possible for surrogate methods?
  • It is not entirely clear how IServiceProvider is resolved from RequestDelegateFactoryOptions

Assuming it ties to the root container, resolving the options just works and the following unit test passes:

[Fact]
public async Task CanExecuteRequestDelegateWithBindAsyncSurrogate()
{
    // Arrange
    IResult actionWithBindAsyncSurrogate(FeatureValue value) => Results.Extensions.TestResult(value.IsTest.ToString());

    var httpContext = CreateHttpContext();
    var responseBodyStream = new MemoryStream();

    httpContext.Response.Body = responseBodyStream;
    httpContext.Features.Set(new TestFeature(new() { IsTest = true }));

    var services = new ServiceCollection();

    services.Configure((BindAsyncOptions options) => options.Add(BindAsync));

    var factoryResult = RequestDelegateFactory.Create(
        actionWithBindAsyncSurrogate,
        new RequestDelegateFactoryOptions() { ServiceProvider = services.BuildServiceProvider() });

    var requestDelegate = factoryResult.RequestDelegate;

    // Act
    await requestDelegate(httpContext);

    // Assert
    Assert.Equal(200, httpContext.Response.StatusCode);
    Assert.Equal(@"""Hello True. This is from an extension method.""", Encoding.UTF8.GetString(responseBodyStream.ToArray()));

    static ValueTask<FeatureValue?> BindAsync(HttpContext h, ParameterInfo c) =>
        ValueTask.FromResult(h.Features.Get<TestFeature>()?.Value);
}

Risks

  • BindAsync implementations might collide (but there can still only be one)
  • BindAsync processing might be unclear
@commonsensesoftware commonsensesoftware added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Sep 13, 2023
@ghost ghost added the old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Sep 13, 2023
@commonsensesoftware
Copy link
Author

This is duplicate request of:

A key difference is this implementation variant could potentially address the Source Generation limitation.

@captainsafia captainsafia added area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Sep 19, 2023
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc
Projects
None yet
Development

No branches or pull requests

3 participants