Fix #2378 - Fully expand models in ApiExplorer

This change dramatically simplifies the parameter discovery logic in
DefaultApiDescriptionProvider. Instead of surfacing POCO objects as
parameters, we now fully expand every model.

The rationale is that we want to show every key/value that can be set by
the user and not force consumers of ApiDescription to do that themselves.

Tests are cleaned up to match the new behavior.

src/Microsoft.AspNet.Mvc.ApiExplorer/DefaultApiDescriptionProvider.cs

@@ -496,34 +496,17 @@ namespace Microsoft.AspNet.Mvc.ApiExplorer
                 //
                 // The default is ModelBinding (aka all default value providers)
                 var source = BindingSource.ModelBinding;
-                if (!Visit(context, source, containerName: string.Empty))
+                Visit(context, source, containerName: string.Empty);
-                {
-                    // If we get here, then it means we didn't find a match for any of the model. This means that it's
-                    // likely 'model-bound' in the traditional MVC sense (formdata + query string + route data) and
-                    // doesn't use any IBinderMetadata.
-                    // 
-                    // Add a single 'default' parameter description for the model.
-                    Context.Results.Add(CreateResult(context, source, containerName: string.Empty));
-                }
             }
 
             /// <summary>
             /// Visits a node in a model, and attempts to create <see cref="ApiParameterDescription"/> for any
-            /// model properties where we can definitely compute an answer. 
+            /// model properties that are leaf nodes (see comments).
             /// </summary>
             /// <param name="modelMetadata">The metadata for the model.</param>
             /// <param name="ambientSource">The <see cref="BindingSource"/> from the ambient context.</param>
             /// <param name="containerName">The current name prefix (to prepend to property names).</param>
-            /// <returns>
+            private void Visit(
-            /// <c>true</c> if the set of <see cref="ApiParameterDescription"/> objects were created for the model.
-            /// <c>false</c> if no <see cref="ApiParameterDescription"/> objects were created for the model.
-            /// </returns>
-            /// <remarks>
-            /// Its the reponsibility of this method to create a parameter description for ALL of the current model
-            /// or NONE of it. If a parameter description is created for ANY sub-properties of the model, then a parameter
-            /// description will be created for ALL of them.
-            /// </remarks>
-            private bool Visit(
                 ApiParameterDescriptionContext bindingContext,
                 BindingSource ambientSource,
                 string containerName)
@@ -534,8 +517,7 @@ namespace Microsoft.AspNet.Mvc.ApiExplorer
                     // We have a definite answer for this model. This is a greedy source like
                     // [FromBody] so there's no need to consider properties.
                     Context.Results.Add(CreateResult(bindingContext, source, containerName));
-
+                    return;
-                    return true;
                 }
 
                 var modelMetadata = bindingContext.ModelMetadata;
@@ -554,26 +536,12 @@ namespace Microsoft.AspNet.Mvc.ApiExplorer
                     !modelMetadata.IsComplexType ||
                     !modelMetadata.Properties.Any())
                 {
-                    if (source == null || source == ambientSource)
+                    Context.Results.Add(CreateResult(bindingContext, source ?? ambientSource, containerName));
-                    {
+                    return;
-                        // If it's a leaf node, and we have no new source then we don't know how to bind this.
-                        // Return without creating any parameters, so that this can be included in the parent model.
-                        return false;
-                    }
-                    else
-                    {
-                        // We found a new source, and this model has no properties. This is probabaly
-                        // a simple type with an attribute like [FromQuery].
-                        Context.Results.Add(CreateResult(bindingContext, source, containerName));
-                        return true;
-                    }
                 }
 
                 // This will come from composite model binding - so investigate what's going on with each property.
                 //
-                // Basically once we find something that we know how to bind, we want to treat all properties at that
-                // level (and higher levels) as separate parameters. 
-                //
                 // Ex:
                 //
                 //      public IActionResult PlaceOrder(OrderDTO order) {...}
@@ -592,8 +560,6 @@ namespace Microsoft.AspNet.Mvc.ApiExplorer
                 //  Order - source: Body
                 //
 
-                var unboundProperties = new HashSet<ApiParameterDescriptionContext>();
-
                 // We don't want to append the **parameter** name when building a model name.
                 var newContainerName = containerName;
                 if (modelMetadata.ContainerType != null)
@@ -609,45 +575,17 @@ namespace Microsoft.AspNet.Mvc.ApiExplorer
                         propertyMetadata,
                         bindingInfo: null,
                         propertyName: null);
+
                     if (Visited.Add(key))
                     {
-                        if (!Visit(propertyContext, source ?? ambientSource, newContainerName))
+                        Visit(propertyContext, source ?? ambientSource, newContainerName);
-                        {
-                            unboundProperties.Add(propertyContext);
-                        }
                     }
                     else
                     {
-                        unboundProperties.Add(propertyContext);
+                        // This is cycle, so just add a result rather than traversing.
+                        Context.Results.Add(CreateResult(propertyContext, source ?? ambientSource, newContainerName));
                     }
                 }
-
-                if (unboundProperties.Count == modelMetadata.Properties.Count)
-                {
-                    if (source == null || source == ambientSource)
-                    {
-                        // No properties were bound and we didn't find a new source, let the caller handle it.
-                        return false;
-                    }
-                    else
-                    {
-                        // We found a new source, and didn't create a result for any of the properties yet,
-                        // so create a result for the current object.
-                        Context.Results.Add(CreateResult(bindingContext, source, containerName));
-                        return true;
-                    }
-                }
-                else
-                {
-                    // This model was only partially bound, so create a result for all the other properties
-                    foreach (var property in unboundProperties)
-                    {
-                        // Create a 'default' description for each property
-                        Context.Results.Add(CreateResult(property, source ?? ambientSource, newContainerName));
-                    }
-
-                    return true;
-                }
             }
 
             private ApiParameterDescription CreateResult(

test/Microsoft.AspNet.Mvc.ApiExplorer.Test/DefaultApiDescriptionProviderTest.cs

@@ -543,9 +543,20 @@ namespace Microsoft.AspNet.Mvc.Description
             // Assert
             var description = Assert.Single(descriptions);
 
-            var parameter = Assert.Single(description.ParameterDescriptions);
+            var parameters = description.ParameterDescriptions;
-            Assert.Equal("product", parameter.Name);
+            Assert.Equal(3, parameters.Count);
+
+            var parameter = Assert.Single(parameters, p => p.Name == "ProductId");
             Assert.Same(BindingSource.ModelBinding, parameter.Source);
+            Assert.Equal(typeof(int), parameter.Type);
+
+            parameter = Assert.Single(parameters, p => p.Name == "Name");
+            Assert.Same(BindingSource.ModelBinding, parameter.Source);
+            Assert.Equal(typeof(string), parameter.Type);
+
+            parameter = Assert.Single(parameters, p => p.Name == "Description");
+            Assert.Same(BindingSource.ModelBinding, parameter.Source);
+            Assert.Equal(typeof(string), parameter.Type);
         }
 
         [Fact]
@@ -611,9 +622,20 @@ namespace Microsoft.AspNet.Mvc.Description
             // Assert
             var description = Assert.Single(descriptions);
 
-            var parameter = Assert.Single(description.ParameterDescriptions);
+            var parameters = description.ParameterDescriptions;
-            Assert.Equal("product", parameter.Name);
+            Assert.Equal(3, parameters.Count);
+
+            var parameter = Assert.Single(parameters, p => p.Name == "ProductId");
             Assert.Same(BindingSource.Form, parameter.Source);
+            Assert.Equal(typeof(int), parameter.Type);
+
+            parameter = Assert.Single(parameters, p => p.Name == "Name");
+            Assert.Same(BindingSource.Form, parameter.Source);
+            Assert.Equal(typeof(string), parameter.Type);
+
+            parameter = Assert.Single(parameters, p => p.Name == "Description");
+            Assert.Same(BindingSource.Form, parameter.Source);
+            Assert.Equal(typeof(string), parameter.Type);
         }
 
         [Fact]
@@ -677,9 +699,20 @@ namespace Microsoft.AspNet.Mvc.Description
             // Assert
             var description = Assert.Single(descriptions);
 
-            var parameter = Assert.Single(description.ParameterDescriptions);
+            var parameters = description.ParameterDescriptions;
-            Assert.Equal("product", parameter.Name);
+            Assert.Equal(3, parameters.Count);
+
+            var parameter = Assert.Single(parameters, p => p.Name == "ProductId");
             Assert.Same(BindingSource.ModelBinding, parameter.Source);
+            Assert.Equal(typeof(int), parameter.Type);
+
+            parameter = Assert.Single(parameters, p => p.Name == "Name");
+            Assert.Same(BindingSource.ModelBinding, parameter.Source);
+            Assert.Equal(typeof(string), parameter.Type);
+
+            parameter = Assert.Single(parameters, p => p.Name == "Description");
+            Assert.Same(BindingSource.ModelBinding, parameter.Source);
+            Assert.Equal(typeof(string), parameter.Type);
         }
 
         [Fact]
@@ -789,7 +822,7 @@ namespace Microsoft.AspNet.Mvc.Description
 
             // Assert
             var description = Assert.Single(descriptions);
-            Assert.Equal(3, description.ParameterDescriptions.Count);
+            Assert.Equal(4, description.ParameterDescriptions.Count);
 
             var id = Assert.Single(description.ParameterDescriptions, p => p.Name == "Id");
             Assert.Same(BindingSource.Path, id.Source);
@@ -799,9 +832,13 @@ namespace Microsoft.AspNet.Mvc.Description
             Assert.Same(BindingSource.Query, quantity.Source);
             Assert.Equal(typeof(int), quantity.Type);
 
-            var product = Assert.Single(description.ParameterDescriptions, p => p.Name == "Product");
+            var productId = Assert.Single(description.ParameterDescriptions, p => p.Name == "Product.Id");
-            Assert.Same(BindingSource.Query, product.Source);
+            Assert.Same(BindingSource.Query, productId.Source);
-            Assert.Equal(typeof(OrderProductDTO), product.Type);
+            Assert.Equal(typeof(int), productId.Type);
+
+            var productPrice = Assert.Single(description.ParameterDescriptions, p => p.Name == "Product.Price");
+            Assert.Same(BindingSource.Query, productPrice.Source);
+            Assert.Equal(typeof(decimal), productPrice.Type);
         }
 
         [Fact]
@@ -819,7 +856,7 @@ namespace Microsoft.AspNet.Mvc.Description
 
             var c = Assert.Single(description.ParameterDescriptions);
             Assert.Same(BindingSource.Query, c.Source);
-            Assert.Equal("C.C", c.Name);
+            Assert.Equal("C.C.C.C", c.Name);
             Assert.Equal(typeof(Cycle1), c.Type);
         }
 
@@ -856,14 +893,14 @@ namespace Microsoft.AspNet.Mvc.Description
             // Assert
             var description = Assert.Single(descriptions);
 
-            var c = Assert.Single(description.ParameterDescriptions);
+            var items = Assert.Single(description.ParameterDescriptions);
-            Assert.Same(BindingSource.ModelBinding, c.Source);
+            Assert.Same(BindingSource.ModelBinding, items.Source);
-            Assert.Equal("c", c.Name);
+            Assert.Equal("Items", items.Name);
-            Assert.Equal(typeof(HasCollection_Complex), c.Type);
+            Assert.Equal(typeof(Child[]), items.Type);
         }
 
         [Fact]
-        public void GetApiDescription_ParameterDescription_RedundentMetadataMergedWithParent()
+        public void GetApiDescription_ParameterDescription_RedundentMetadata_NotMergedWithParent()
         {
             // Arrange
             var action = CreateActionDescriptor(nameof(AcceptsRedundentMetadata));
@@ -875,10 +912,16 @@ namespace Microsoft.AspNet.Mvc.Description
             // Assert
             var description = Assert.Single(descriptions);
 
-            var r = Assert.Single(description.ParameterDescriptions);
+            var parameters = description.ParameterDescriptions;
-            Assert.Same(BindingSource.Query, r.Source);
+            Assert.Equal(2, parameters.Count);
-            Assert.Equal("r", r.Name);
+
-            Assert.Equal(typeof(RedundentMetadata), r.Type);
+            var id = Assert.Single(parameters, p => p.Name == "Id");
+            Assert.Same(BindingSource.Query, id.Source);
+            Assert.Equal(typeof(int), id.Type);
+
+            var name = Assert.Single(parameters, p => p.Name == "Name");
+            Assert.Same(BindingSource.Query, name.Source);
+            Assert.Equal(typeof(string), name.Type);
         }
 
         [Fact]

test/Microsoft.AspNet.Mvc.FunctionalTests/ApiExplorerTest.cs

@@ -744,11 +744,15 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests
             var description = Assert.Single(result);
             var parameters = description.ParameterDescriptions;
 
-            Assert.Equal(1, parameters.Count);
+            Assert.Equal(2, parameters.Count);
 
-            var product = Assert.Single(parameters, p => p.Name == "product");
+            var id = Assert.Single(parameters, p => p.Name == "Id");
-            Assert.Equal(BindingSource.ModelBinding.Id, product.Source);
+            Assert.Equal(BindingSource.ModelBinding.Id, id.Source);
-            Assert.Equal(typeof(ApiExplorerWebSite.Product).FullName, product.Type);
+            Assert.Equal(typeof(int).FullName, id.Type);
+
+            var name = Assert.Single(parameters, p => p.Name == "Name");
+            Assert.Equal(BindingSource.ModelBinding.Id, name.Source);
+            Assert.Equal(typeof(string).FullName, name.Type);
         }
 
         [Fact]
@@ -796,7 +800,7 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests
             var description = Assert.Single(result);
             var parameters = description.ParameterDescriptions;
 
-            Assert.Equal(6, parameters.Count);
+            Assert.Equal(7, parameters.Count);
 
             var customerId = Assert.Single(parameters, p => p.Name == "CustomerId");
             Assert.Equal(BindingSource.Query.Id, customerId.Source);
@@ -810,9 +814,13 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests
             Assert.Equal(BindingSource.Form.Id, quantity.Source);
             Assert.Equal(typeof(int).FullName, quantity.Type);
 
-            var product = Assert.Single(parameters, p => p.Name == "Details.Product");
+            var productId = Assert.Single(parameters, p => p.Name == "Details.Product.Id");
-            Assert.Equal(BindingSource.Form.Id, product.Source);
+            Assert.Equal(BindingSource.Form.Id, productId.Source);
-            Assert.Equal(typeof(ApiExplorerWebSite.Product).FullName, product.Type);
+            Assert.Equal(typeof(int).FullName, productId.Type);
+
+            var productName = Assert.Single(parameters, p => p.Name == "Details.Product.Name");
+            Assert.Equal(BindingSource.Form.Id, productName.Source);
+            Assert.Equal(typeof(string).FullName, productName.Type);
 
             var shippingInstructions = Assert.Single(parameters, p => p.Name == "Comments.ShippingInstructions");
             Assert.Equal(BindingSource.Query.Id, shippingInstructions.Source);