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

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

Closed
martincostello opened this issue Aug 23, 2024 · 10 comments · Fixed by #3076
Labels
Milestone

Comments

@martincostello
Copy link
Collaborator

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: #3018 (comment)

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.

public void Apply(OpenApiRequestBody requestBody, RequestBodyFilterContext context)
{
var parameterDescription =
context.BodyParameterDescription ??
context.FormParameterDescriptions.FirstOrDefault((p) => p is not null);
if (parameterDescription is null)
{
return;
}

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
Copy link

When will there be an update? @martincostello

@martincostello
Copy link
Collaborator Author

Whenever someone picks it up. Contributions welcome.

@jgarciadelanoceda
Copy link
Contributor

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
Copy link
Collaborator Author

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
Copy link
Contributor

jgarciadelanoceda commented Sep 3, 2024

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
Copy link
Contributor

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
Copy link
Collaborator Author

(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
Copy link
Collaborator Author

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
Copy link
Contributor

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

@martincostello martincostello added this to the v6.8.0 milestone Sep 23, 2024
@martincostello martincostello removed the help-wanted A change up for grabs for contributions from the community label Sep 23, 2024
@martincostello
Copy link
Collaborator Author

#3076

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants