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

`FromFormAttribute.Name` not respected by `ApiParameterDescription.Name` #43815

Closed ascott18 closed 2 years ago

ascott18 commented 2 years ago

Is there an existing issue for this?

Describe the bug

For a parameter with [FromForm(Name = "SomeName")], the provided Name is not included in the ApiParameterDescription.Name produced for that parameter.

Expected Behavior

I would expect the circled parameters in the screenshot below (for the code in the repro steps) to be named childItem.Id and childItem.Name, since this is the name they must be (or other valid nested naming scheme like childItem[Id]) in the form data POST body in order to be bound. image

Steps To Reproduce

Look at the ApiDescription for the following action:

[Route("test")]
public class MyController : Controller
{
    [HttpPost]
    public virtual IActionResult AddChildItem(
        [FromForm(Name = "id")] int id,
        [FromForm(Name = "childItem")] Item childItem
    )
    {
        return Ok(new
        {
            id,
            ChildId = childItem.Id
        });
    }

    public class Item
    {
        public int Id { get; set; }
        public string Name { get; set; }
    }
}

Exceptions (if any)

No response

.NET Version

6.0.303

Anything else?

This is breaking Swashbuckle - I have also reported this at https://github.com/domaindrivendev/Swashbuckle.AspNetCore/issues/2495, because I'm not sure if its intentional and consumers like Swashbuckle are expected to compensate for it themselves, or if it is expected that aspnetcore be the one accounting for this behavior.

captainsafia commented 2 years ago

During triage, I had assumed this bug was related to the fact that we were originally not respecting the Name property on FromX attributes applied to a parameter. After looking at the issue description further, it looks like this is actually related to the way the ApiParameterDescription.Name is generated for parameters that have DTOs. In particular, the ApiExplorer is set up to ignore the parameter name to account for scenarios where composite model binding is used:

public class Account
{
  public string UserId { get; set; }
  [FromBody]
  public User UserToAdd { get; set; }
}

In the case above, the desired goal is to ignore the parameter name/DTO name altogether and use the property names for ApiParameterDesscription.Name -- this is what causes the particular bug here.

Now, as mentioned, the dynamics of FromForm attributes are a little different so we could probably add some logic to handle the concatenation for form parameters to support constructing the parameter name correctly.

Edit: On second thought, this behavior seems to have been codified in the ApiDescriptionProvider for a long time, as evidence by this test and this test.

It looks like this behavior was introduced as part of this commit almost 8 years. The commit adds support for expanding composite DTOs as mentioned in the paragraph above.

So, I'm a little iffy about making a change here given the fact that this behavior appears to have been in place for 8+ years. It very well might've been the intent here for property names to be captured regardless of the binding source and the expectation is one consumers to use the raw data exposed by ApiParameterDescription to compose the correct name for their intents and purposes. If we were to take this approach, it would be on the consumer to concatenate ApiParameterDescription.ParameterDescriptor.Name and ApiParameterDescription.Name to form the appropriate form now.

Now, one thing I'd like to understand is how this bug interacts with the model binding rules for complex type documented here. Particularly, I don't think the model binding logic implemented by the description provider should produce descriptions that cannot actually be processed by model binding.

Edit 2: Using the /test endpoint above, it looks like both ChildItem.Name and childItem.Name are bound correctly to the form parameters so it's reasonable for Swashbuckle to use ApiParameterDescription.ModelMetadata.PropertyName.

Alright, after going back and forth on whether we should do a tactical patch in framework or make a patch upstream in SB, I've decided that patching in SB might be the best option for now. If it becomes clear that other consumers of ApiExplorer are running into problems with this, then we can address in the framework.

captainsafia commented 2 years ago

@ascott18 In the meantime, you can use this workaround to correct the naming using a RequestBodyFilter in SB.

builder.Services.AddSwaggerGen(c => {
    c.RequestBodyFilter<FormNameFilter>();
});

...

public class FormNameFilter : IRequestBodyFilter
{
    public void Apply(OpenApiRequestBody body, RequestBodyFilterContext context)
    {
        foreach (var param in context.FormParameterDescriptions)
        {
            if (param.ModelMetadata.PropertyName is not null)
            {
                var props = body.Content["multipart/form-data"].Schema.Properties;
                if (props.ContainsKey(param.Name))
                {
                    var element = props[param.Name];
                    props.Remove(param.Name);
                    props[$"{param.ParameterDescriptor.Name}.{param.Name}"] = element;
                }
            }

        }
    }
}
captainsafia commented 2 years ago

Marking this as answered for now and using the other issue to track a fix in Swashbuckle.

ascott18 commented 2 years ago

@captainsafia Thanks so much for the investigation and workaround!