dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.43k stars 10.01k forks source link

Automatically create OpenApiParameters when using BindAsync in Minimal API #51218

Open Misiu opened 1 year ago

Misiu commented 1 year ago

Is there an existing issue for this?

Is your feature request related to a problem? Please describe the problem.

I'm trying to migrate my controllers-based project to .NET 7 minimal API. In the old project, I have multiple custom value providers (FromClaim, FromHeaderPart, etc).

In minimal API we can have custom model binding using BindAsync, but when using this approach we don't get any params in SwaggerUI.

To display parameters we must add this code:

.WithOpenApi(operation =>
{
    operation.Parameters.Add(new OpenApiParameter
    {
        Name = "sortBy",
        Schema = new OpenApiSchema
        {
            Type = "string",
            Default = new OpenApiString("")
        },
        Description = "Sort by",
        Required = false,
        In = ParameterLocation.Query
    });

    operation.Parameters.Add(new OpenApiParameter
    {
        Name = "sortDir",
        Schema = new OpenApiSchema
        {
            Type = "string",
            Default = new OpenApiString("asc"),
            Enum = new List<IOpenApiAny>
            {
                new OpenApiString("asc"),
                new OpenApiString("desc")
            }
        },
        Description = "Sort direction",
        Required = true,
        In = ParameterLocation.Query
    });

    operation.Parameters.Add(new OpenApiParameter
    {
        Name = "page",
        Schema = new OpenApiSchema
        {
            Type = "integer",
            Default = new OpenApiInteger(1)
        },
        Description = "Page number",
        Required = true,
        In = ParameterLocation.Query
    });

    return operation;

});

Ideally, this should be done automatically based on attributes

Describe the solution you'd like

Ideally, we should be able to annotate properties that are binding from known places with attributes. So the above WithOpenApi could be omitted and a valid list of parameters would be created automatically. We could use attributes from Microsoft.AspNetCore.Mvc or create new ones if needed.

public class MyCustomParameters
{
    [FromQuery]
    public string? SortBy { get; init; }

    [FromQuery]
    [Required]
    public SortDirection Sort { get; init; }

    [FromQuery]
    [Required]
    public int CurrentPage { get; init; } = 1;

    public static ValueTask<MyCustomParameters?> BindAsync(HttpContext context, ParameterInfo parameter)
    {
        const string sortByKey = "sortBy";
        const string sortDirectionKey = "sortDir";
        const string currentPageKey = "page";

        Enum.TryParse<SortDirection>(context.Request.Query[sortDirectionKey],ignoreCase: true, out var sortDirection);
        int.TryParse(context.Request.Query[currentPageKey], out var page);
        page = page == 0 ? 1 : page;

        var result = new MyCustomParameters
        {
            SortBy = context.Request.Query[sortByKey],
            Sort = sortDirection,
            CurrentPage = page
        };

        return ValueTask.FromResult<MyCustomParameters?>(result);
    }

    public enum SortDirection
    {
        Asc,
        Desc
    }
}

Additional context

My main idea is to remove repetitive code that must be added for each endpoint that is using custom binding.

Misiu commented 1 year ago

My current approach involves a custom OperationFilter which supports FromQuery params and has major limitations, but maybe it will help someone having a similar problem.

public class BindAsyncOperationFilter : IOperationFilter
{
    public void Apply(OpenApiOperation operation, OperationFilterContext context)
    {
        var models = context.MethodInfo.GetParameters()
            .Where(p => p.ParameterType.IsClass)
            .ToList();

        if (models.Count != 1)
        {
            return;
        }

        var model = models.First();

        //check if the model has a public static BindAsync method
        var bindAsyncMethod = model.ParameterType.GetMethod("BindAsync", BindingFlags.Public | BindingFlags.Static);
        if (bindAsyncMethod == null)
        {
            return;
        }

        //get all public properties from the model that have `FromQuery` attribute
        var fromQueryProperties = model.ParameterType.GetProperties(BindingFlags.Public | BindingFlags.Instance)
            .Where(p => p.GetCustomAttribute<FromQueryAttribute>() != null)
            .ToList();

        foreach (var property in fromQueryProperties)
        {
            //get property type
            var propertyType = property.PropertyType;
            //check if the property is nullable
            if (propertyType.IsGenericType && propertyType.GetGenericTypeDefinition() == typeof(Nullable<>))
            {
                propertyType = propertyType.GetGenericArguments()[0];
            }

            var schema = new OpenApiSchema
            {
                Type = propertyType == typeof(int) ? "integer" : "string",
            };

            if (propertyType.IsEnum)
            {
                var enumValues = Enum.GetValues(propertyType)
                    .Cast<object>()
                    .Select(v => v.ToString()?.ToLowerInvariant())
                    .ToList();

                schema.Enum = new List<IOpenApiAny>(enumValues.Select(v => new OpenApiString(v)));
            }

            var name = property.Name;
            //check if the property has JsonPropertyName attribute
            var jsonPropertyName = property.GetCustomAttribute<JsonPropertyNameAttribute>();
            if (jsonPropertyName != null)
            {
                name = jsonPropertyName.Name;
            }

            //check if the property has Required attribute
            var isRequired = property.GetCustomAttribute<RequiredAttribute>() != null;

            operation.Parameters.Add(new OpenApiParameter
            {
                Name = name,
                Schema = schema,
                Required = isRequired,
                In = ParameterLocation.Query
            });
        }
    }
}

If there are better ways of doing this, or maybe something built in, please let me know.

captainsafia commented 1 year ago

@Misiu Thanks for reporting this issue and brainstorming some solutions!

As you mentioned, it's challenging to provide OpenAPI annotations for BindAsync types because, unlike other types, they are a block box to the framework with regard to where they source values from.

The proposed solutions works, but when I squint at it, it's difficult to tell the difference between using the source attributes in a BindAsync type or using AsParameters to create a surrogate parameter. I worry about blurring the boundary between the two by adding support for this feature.

Another solution might be to provide a way for a BindAsync type to describe its annotations to OpenAPI.

Misiu commented 1 year ago

@captainsafia Thank you for the reply. The model that I added in my initial post was just am example. In the app that I'm rewriting, I read values from multiple places - query, body, headers, and claims principal. I parse values (for example I pass two IDs separated by a comma in the header). AsParameters is great for simple types, but when you need something more complex the only option, for now, is BindAsync.

The provided filter was just an example, but I'm already working on adding support for other attributes, so when my command is taking values from standard places I want SwaggerUI to show those parameters.

The next missing thing in BindAsync approach is no way to return info about invalid parameters, For now, we can return ValueTask.FromResult<PagingData?>(result) or return ValueTask.FromResult<PagingData?>(null);, when doing the second we get this error:

Microsoft.AspNetCore.Http.BadHttpRequestException: Required parameter "PagingData model" was not provided from PagingData.BindAsync(HttpContext).

Ideally, we should be able to return custom errors per property and finally return 400 or 422. Maybe there is some built-in way, if yes, then please let me know.

captainsafia commented 1 year ago

In the app that I'm rewriting, I read values from multiple places - query, body, headers, and claims principal.

Indeed. Some of these sources map to concepts that exist in OpenAPI (query, body) but a claims principal doesn't have a source in the same sense.

The provided filter was just an example, but I'm already working on adding support for other attributes, so when my command is taking values from standard places I want SwaggerUI to show those parameters.

Can you provide examples of these other attributes?

All in all, I'm incline to think that the solution might be providing a syntax on BindAsync for a type to be able to declare where it's parameters are sourced from.

The next missing thing in BindAsync approach is no way to return info about invalid parameters, For now, we can return ValueTask.FromResult<PagingData?>(result) or return ValueTask.FromResult<PagingData?>(null);, when doing the second we get this error:

Interesting -- can you file a new issue for this?

Misiu commented 1 year ago

Can you provide examples of these other attributes?

I was thinking about standard From* attributes, ref: https://learn.microsoft.com/en-us/aspnet/core/mvc/models/model-binding?view=aspnetcore-7.0#sources The idea is to add those attributes to properties that are retrieved from the standard places (places that can be added as OpenApiParameter).

This is a simple model I have in my old app:

public sealed class PasswordSignIn : ICommand<SignInResultDto>
{
    [FromHeader(Name = "TenantId")]
    [NotDefault]
    [Required]
    public Guid TenantId { get; init; }

    [Required]
    [Email]
    public string UserName { get; init; } = null!;

    [Required]
    public string Password { get; init; } = null!;

    public string? FcmToken { get; init; }

    [FromHeader(Name = "DeviceId")]
    [NotDefault]
    [Required]
    public Guid DeviceId { get; init; }

    [FromZoneIdHeader(ZoneIdHeaderParts.ZoneId)]
    [NotDefault]
    [Required]
    public Guid ZoneId { get; init; }
}

This shows well in Swagger (in old all), but when using the minimal API with the BindAsync approach I'm unable to generate a valid Swagger (without adding extra code as shown in my initial message), so the endpoint can't be called via SwaggerUI.

I need to use BindAsync, because I mix body (UserName, Password, FcmToken), with header (TenantId, DeviceId, ZoneId) and sometimes read values from claims.

Interesting -- can you file a new issue for this?

🙂 https://github.com/dotnet/aspnetcore/issues/51318

captainsafia commented 11 months ago

@Misiu Sorry for taking a while to get back to you here!

I'm warming up to the idea of using [FromX] attributes to define the source of properties in a BindAsync-able type. My previous stance on this was a little too puritan, I believe that the attributes should only be applied on properties/parameters where the binding was handled by the framework. I didn't feel comfortable extending these to be generic markers to identifying the source of an attribute, particularly because they have a legacy in MVC.

I think I'm more comfortable with treating them as marker attributes now but want to explore the scenario further.

But admittedly, this feature would probably rank pretty low on the priority scale, unfortunately. At the moment, the operation filter does seem like the most capable approach.

I'm backlogging this but we can revisit as we plan out the OpenAPI area some more.

ghost commented 11 months ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

Misiu commented 1 month ago

@captainsafia maybe this can be added to .NET 10 Planning Milestone?

Talento90 commented 4 weeks ago

My current approach involves a custom OperationFilter which supports FromQuery params and has major limitations, but maybe it will help someone having a similar problem.

public class BindAsyncOperationFilter : IOperationFilter
{
    public void Apply(OpenApiOperation operation, OperationFilterContext context)
    {
        var models = context.MethodInfo.GetParameters()
            .Where(p => p.ParameterType.IsClass)
            .ToList();

        if (models.Count != 1)
        {
            return;
        }

        var model = models.First();

        //check if the model has a public static BindAsync method
        var bindAsyncMethod = model.ParameterType.GetMethod("BindAsync", BindingFlags.Public | BindingFlags.Static);
        if (bindAsyncMethod == null)
        {
            return;
        }

        //get all public properties from the model that have `FromQuery` attribute
        var fromQueryProperties = model.ParameterType.GetProperties(BindingFlags.Public | BindingFlags.Instance)
            .Where(p => p.GetCustomAttribute<FromQueryAttribute>() != null)
            .ToList();

        foreach (var property in fromQueryProperties)
        {
            //get property type
            var propertyType = property.PropertyType;
            //check if the property is nullable
            if (propertyType.IsGenericType && propertyType.GetGenericTypeDefinition() == typeof(Nullable<>))
            {
                propertyType = propertyType.GetGenericArguments()[0];
            }

            var schema = new OpenApiSchema
            {
                Type = propertyType == typeof(int) ? "integer" : "string",
            };

            if (propertyType.IsEnum)
            {
                var enumValues = Enum.GetValues(propertyType)
                    .Cast<object>()
                    .Select(v => v.ToString()?.ToLowerInvariant())
                    .ToList();

                schema.Enum = new List<IOpenApiAny>(enumValues.Select(v => new OpenApiString(v)));
            }

            var name = property.Name;
            //check if the property has JsonPropertyName attribute
            var jsonPropertyName = property.GetCustomAttribute<JsonPropertyNameAttribute>();
            if (jsonPropertyName != null)
            {
                name = jsonPropertyName.Name;
            }

            //check if the property has Required attribute
            var isRequired = property.GetCustomAttribute<RequiredAttribute>() != null;

            operation.Parameters.Add(new OpenApiParameter
            {
                Name = name,
                Schema = schema,
                Required = isRequired,
                In = ParameterLocation.Query
            });
        }
    }
}

If there are better ways of doing this, or maybe something built in, please let me know.

Thank you this is quite handy 👍