From 0f6b2331ce360f03ad16aafcd3bb69ad52a265f8 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Tue, 21 Apr 2015 15:37:16 -0700 Subject: [PATCH] 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. --- .../DefaultApiDescriptionProvider.cs | 86 +++---------------- .../DefaultApiDescriptionProviderTest.cs | 83 +++++++++++++----- .../ApiExplorerTest.cs | 24 ++++-- 3 files changed, 91 insertions(+), 102 deletions(-) diff --git a/src/Microsoft.AspNet.Mvc.ApiExplorer/DefaultApiDescriptionProvider.cs b/src/Microsoft.AspNet.Mvc.ApiExplorer/DefaultApiDescriptionProvider.cs index a6cd7a7ea2..f0ee860ef9 100644 --- a/src/Microsoft.AspNet.Mvc.ApiExplorer/DefaultApiDescriptionProvider.cs +++ b/src/Microsoft.AspNet.Mvc.ApiExplorer/DefaultApiDescriptionProvider.cs @@ -325,8 +325,8 @@ namespace Microsoft.AspNet.Mvc.ApiExplorer if (responseFormatMetadataProvider != null) { var supportedTypes = responseFormatMetadataProvider.GetSupportedContentTypes( - declaredType, - runtimeType, + declaredType, + runtimeType, contentType); if (supportedTypes != null) @@ -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)) - { - // 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)); - } + Visit(context, source, containerName: string.Empty); } /// /// Visits a node in a model, and attempts to create for any - /// model properties where we can definitely compute an answer. + /// model properties that are leaf nodes (see comments). /// /// The metadata for the model. /// The from the ambient context. /// The current name prefix (to prepend to property names). - /// - /// true if the set of objects were created for the model. - /// false if no objects were created for the model. - /// - /// - /// 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. - /// - private bool Visit( + private void 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 true; + return; } var modelMetadata = bindingContext.ModelMetadata; @@ -554,25 +536,11 @@ namespace Microsoft.AspNet.Mvc.ApiExplorer !modelMetadata.IsComplexType || !modelMetadata.Properties.Any()) { - if (source == null || source == ambientSource) - { - // 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; - } + Context.Results.Add(CreateResult(bindingContext, source ?? ambientSource, containerName)); + return; } // 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: // @@ -592,8 +560,6 @@ namespace Microsoft.AspNet.Mvc.ApiExplorer // Order - source: Body // - var unboundProperties = new HashSet(); - // 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)) - { - unboundProperties.Add(propertyContext); - } + Visit(propertyContext, source ?? ambientSource, newContainerName); } 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( diff --git a/test/Microsoft.AspNet.Mvc.ApiExplorer.Test/DefaultApiDescriptionProviderTest.cs b/test/Microsoft.AspNet.Mvc.ApiExplorer.Test/DefaultApiDescriptionProviderTest.cs index bad01946d3..4a915c387f 100644 --- a/test/Microsoft.AspNet.Mvc.ApiExplorer.Test/DefaultApiDescriptionProviderTest.cs +++ b/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); - Assert.Equal("product", parameter.Name); + var parameters = description.ParameterDescriptions; + 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); - Assert.Equal("product", parameter.Name); + var parameters = description.ParameterDescriptions; + 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); - Assert.Equal("product", parameter.Name); + var parameters = description.ParameterDescriptions; + 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"); - Assert.Same(BindingSource.Query, product.Source); - Assert.Equal(typeof(OrderProductDTO), product.Type); + var productId = Assert.Single(description.ParameterDescriptions, p => p.Name == "Product.Id"); + Assert.Same(BindingSource.Query, productId.Source); + 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); - Assert.Same(BindingSource.ModelBinding, c.Source); - Assert.Equal("c", c.Name); - Assert.Equal(typeof(HasCollection_Complex), c.Type); + var items = Assert.Single(description.ParameterDescriptions); + Assert.Same(BindingSource.ModelBinding, items.Source); + Assert.Equal("Items", items.Name); + 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); - Assert.Same(BindingSource.Query, r.Source); - Assert.Equal("r", r.Name); - Assert.Equal(typeof(RedundentMetadata), r.Type); + var parameters = description.ParameterDescriptions; + Assert.Equal(2, parameters.Count); + + 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] diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/ApiExplorerTest.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/ApiExplorerTest.cs index 3660a1c3ac..be1958ee11 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/ApiExplorerTest.cs +++ b/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"); - Assert.Equal(BindingSource.ModelBinding.Id, product.Source); - Assert.Equal(typeof(ApiExplorerWebSite.Product).FullName, product.Type); + var id = Assert.Single(parameters, p => p.Name == "Id"); + Assert.Equal(BindingSource.ModelBinding.Id, id.Source); + 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"); - Assert.Equal(BindingSource.Form.Id, product.Source); - Assert.Equal(typeof(ApiExplorerWebSite.Product).FullName, product.Type); + var productId = Assert.Single(parameters, p => p.Name == "Details.Product.Id"); + Assert.Equal(BindingSource.Form.Id, productId.Source); + 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);