ApiExplorer expanding IFormFile into multiple ParameterDescriptions

[domaindrivendev]

This behavior was added by #2502. As far as I can tell it expands complex types into multiple ParameterDescriptions unless the parameter has a BindingSource of "body". While the expansion is really convenient for documenting complex types with a BindingSource of "query" or "form", it makes it very tricky for ApiExplorer consumers (e.g. Swashbuckle.AspNetCore) to document parameters of type IFormFile. In this case it would be ideal to have a single parameter of type IFormFile.

In fact, this is a problem for other "special" parameter types as well. For example, CancellationToken. A tool like Swashbuckle needs to omit this parameter from API docs (e.g. Swagger). But, rather than omitting ParameterDescriptions with a type CancellationToken it needs to omit all params that are likely "part of" an expanded CancellationToken. Today, it uses the following code for this:

public static bool IsPartOfCancellationToken(this ApiParameterDescription parameterDescription)
{
    if (parameterDescription.Source != BindingSource.ModelBinding) return false;

    var name = parameterDescription.Name;
    return name == "CanBeCanceled"
        || name == "IsCancellationRequested"
        || name.StartsWith("WaitHandle.");
    }
}

Not sure if I've explained this well. Let me know if it makes sense

[rynowak]

This makes sense. We should look at the swagger/API Explorer results for t all of the types we provide special model binding support for "out of the box" and make sure it's doing something appropriate.

[Eilon]

@jbagga let's make sure we understand the scope of the problem, e.g. which types are problematic, how to handle them, etc.

[domaindrivendev]

From Swashbuckle's perspective - it's just the two "special" types above that are causing problems.

For me, ApiExplorer and its abstractions are closer to the C# / MVC layer than the HTTP layer. So one could argue that it shouldn't expand out the properties at all, instead putting that responsibility on the tools that do operate purely at the HTTP layer of abstraction e.g. Swashbuckle. Not saying that's right just throwing it out there for philosophical discussion :)

[rynowak]

For cancellation token we already have a way to say this parameter is "not user input". We should double check the other types that aren't really going to bind user input but can appear in a parameter list like IFormCollection.

For IFormFile and other variations there are some significant semantics around these, specifically how we match names in the form to the parameter name. We don't have a BindingSource that represents form files, and I'm not sure yet if that would be the right solution. I will have to think on it.

[domaindrivendev]

@rynowak - I was hoping BindingSource.IsFromRequest would be "false" for CancellationToken but that's not currently the case. Is there something else I can look at so I don't have to do the hacky prop name matching above.

As you guys think about a solution here, I just want to call out my wishlist:

• don't expand out API params for CancellationToken and IFormFile so they can be easily identified in the param collection
• rather than having to look at the type, it would be ideal for SB to leverage some other metadata to identify these "special" param types.
• in addition to body-bound params, populate ApiDescription.SupportedResponseFormats for standard form-bound and IFormFile params so SB can assign the Swagger consumes field accordingly.

[jbagga]

@domaindrivendev Let us know if the changes in #5750 do not resolve the issue

[domaindrivendev]

@jbagga, @rynowak - thanks for this, I just have two comments to make ...

1. I really appreciate you guys making this change and so far as I can tell it looks great. Regarding incorporating into Swashbuckle what would your recommendation be? When do you expect this to be released? At that point I could have SB take a minimum dependency on that version. My only worry here is that I'll be forcing consumers to upgrade Core.

2. Another SB issue just cropped up - Swagger Generation not respecting overrided schema for type (via MapType<T>) domaindrivendev/Swashbuckle.AspNetCore#309 where the root cause is the expanding out of "path" parameters (BindingSource.Path). At this point, I'm getting the feeling that preemptively expanding all-but-body parameters, while convenient for the "query" case, is going to continue causing problems for other cases.

[domaindrivendev]

I guess at a minimum, I can't think of any good reason to expand out "path" parameters at all.

[rynowak]

I really appreciate you guys making this change and so far as I can tell it looks great. Regarding incorporating into Swashbuckle what would your recommendation be? When do you expect this to be released? At that point I could have SB take a minimum dependency on that version. My only worry here is that I'll be forcing consumers to upgrade Core.

This will be part of 2.0.0 - the best date estimate I can give is Q3 2017. If you want this change right away, there's nothing fundamental about it, just implement IBindingMetadataProvider and set the BindingSource on these two types to something other than the default.

Another SB issue just cropped up - domaindrivendev/Swashbuckle.AspNetCore#309 where the root cause is the expanding out of "path" parameters (BindingSource.Path). At this point, I'm getting the feeling that preemptively expanding all-but-body parameters, while convenient for the "query" case, is going to continue causing problems for other cases.

I guess at a minimum, I can't think of any good reason to expand out "path" parameters at all.

I'm going to open another issue where we can track that feedback. The reason why we expand non-body parameters is that it represents something fundamental about how model binding works. I think there's a disconnect in the system where implementing a custom model binder doesn't prevent us from expanding the parameter.