domaindrivendev / Swashbuckle.AspNetCore

Swagger tools for documenting API's built on ASP.NET Core
MIT License
5.22k stars 1.3k forks source link

[Bug]: Operation parameter with [FromForm] should not be used to describe a request body if more than one such parameter #3038

Open martincostello opened 4 weeks ago

martincostello commented 4 weeks ago

Describe the bug

As part of #3020 to fix #3018, a change was made to fix XML comments not being used to describe the request body for a form.

On reflection, this fix wasn't correct as if there are multiple parameter the first parameter is used to describe the request body in its entirety: https://github.com/domaindrivendev/Swashbuckle.AspNetCore/issues/3018#issuecomment-2306506421

Expected behavior

The request body should only be documented from the XML documentation for a [FromForm]-annotated argument if there is exactly one such parameter.

https://github.com/domaindrivendev/Swashbuckle.AspNetCore/blob/7a7230dd54be847ac82982010e00497d9fe5f2cf/src/Swashbuckle.AspNetCore.SwaggerGen/XmlComments/XmlCommentsRequestBodyFilter.cs#L17-L26

Actual behavior

The request body is documented from the XML documentation from the first of multiple arguments with [FromForm].

Steps to reproduce

Render the OpenAPI document for an application including the following controller:

using Microsoft.AspNetCore.Mvc;

namespace FromFormXmlIssue;

[ApiController]
[Route("[controller]")]
public class ReproController : ControllerBase
{
    [HttpGet]
    public int[] Get([FromForm] string First, [FromForm] string Second)
    {
        return [42];
    }
}

Exception(s) (if any)

No response

Swashbuckle.AspNetCore version

6.7.1

.NET Version

8.0.8

Anything else?

No response

qcjxberin commented 3 weeks ago

When will there be an update? @martincostello

martincostello commented 3 weeks ago

Whenever someone picks it up. Contributions welcome.

jgarciadelanoceda commented 2 weeks ago

I am thinking in picking it up. But I think that if there are more than one property and the schema has the properties then we could asign the description to the Property instead of the whole OpenApiRequestBody. What do you think?

With this code you will see what I am referring to:

public sealed class BodyFilter : IRequestBodyFilter
{
    public void Apply(OpenApiRequestBody requestBody, RequestBodyFilterContext context)
    {
        if (context.FormParameterDescriptions?.Any() ?? false)
        {
            foreach (var (_, mediaType) in requestBody.Content)
            {
                if (mediaType?.Schema?.Properties?.Any() ?? false)
                {
                    foreach (var (nameForProperty, schemaForProperty) in mediaType.Schema.Properties)
                    {
                        schemaForProperty.Description = $"{nameForProperty} Description";
                    }
                }
            }
        }
    }
}
martincostello commented 2 weeks ago

To be honest I'm not sure - the lines are getting blurred a bit between the C# parameters and the OpenAPI parameters, which is what probably what confused me in the first place with the incorrect behaviour.

jgarciadelanoceda commented 2 weeks ago

I am still studying this issue @qcjxberin for now I have a WorkAround that is to put those 3 properties inside a class with the documentation of the Properties. At first I thought that the XmlSchemaFilter was missing looking for ParameterInfo (Because this class is the one that is documenting the properties inside a class, but I am having problems with the IntegrationTests.

The problem I have is that I end up duplicating so much in the OpenApi document :( however all the info inserted is right:

image But I also got the documentation from parameters :(

jgarciadelanoceda commented 1 week ago

I do not have clear what to do. Because for me it's a Parameter as if it was a QueryParameter. But as this goes in the Body of the request the only Filter that it takes it's the SchemaFilter(This filter does not look for method parameters). The AnnotationsSchemaFilter looks for parameters whereas the XmlCommentsSchemaFilter does not.. If I create a PR to look for Parameters then it duplicates descriptions for both Query and Request Body requests.

For sure that the description of the body of a FromForm couldn't be set from part of the parameters when there are more than one but I am not sure what to do

martincostello commented 1 week ago

(I haven't forgotten about this, it's in my todo pile, but I need to sit and think about it properly to give an answer about how to proceed)

martincostello commented 1 day ago

For the purposes of this issue, I think the intention is exactly as described originally - refactor the code to not use FirstOrDefault(), but instead only use the value if there is exactly 1 non-null form parameter description.

That doesn't actually make things behave as the OP wants with respect to documentation for form parameters, but it corrects the duplication of the descriptions from the original attempt to fix it.

jgarciadelanoceda commented 1 day ago

Yeah the original Bug #3018 is just a duplicate of #2062. I have to study what commit made the parameter of a FromForm to do not show in the documentation.. Once I have time I analyze it