Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Cleanup InferParameterBindingInfoConvention #8665

Merged
merged 2 commits into from Oct 31, 2018
Merged

Cleanup InferParameterBindingInfoConvention #8665

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d98c27f
Cleanup InferParameterBindingInfoConvention
pranavkm Oct 29, 2018
e3c444e
Changes per PR comments
pranavkm Oct 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelMetadata.cs
View file
Open in desktop
Expand Up @@ -349,8 +349,9 @@ public virtual ModelMetadata ContainerMetadata
/// Gets a value indicating whether <see cref="ModelType"/> is a complex type.
/// </summary>
/// <remarks>
/// A complex type is defined as a <see cref="Type"/> which has a
/// <see cref="TypeConverter"/> that can convert from <see cref="string"/>.
/// A complex type is defined as a <see cref="Type"/> without a <see cref="TypeConverter"/> that can convert
/// from <see cref="string"/>. Most POCO and <see cref="IEnumerable"/> types are therefore complex. Most, if
/// not all, BCL value types are simple types.
/// </remarks>
public bool IsComplexType { get; private set; }

Expand Down
40 changes: 40 additions & 0 deletions src/Microsoft.AspNetCore.Mvc.Core/ApiBehaviorOptions.cs
View file
Open in desktop
Expand Up @@ -17,6 +17,7 @@ public class ApiBehaviorOptions : IEnumerable<ICompatibilitySwitch>
{
private readonly CompatibilitySwitch<bool> _suppressMapClientErrors;
private readonly CompatibilitySwitch<bool> _suppressUseValidationProblemDetailsForInvalidModelStateResponses;
private readonly CompatibilitySwitch<bool> _allowInferringBindingSourceForCollectionTypesAsFromQuery;
private readonly ICompatibilitySwitch[] _switches;

private Func<ActionContext, IActionResult> _invalidModelStateResponseFactory;
Expand All @@ -28,10 +29,12 @@ public ApiBehaviorOptions()
{
_suppressMapClientErrors = new CompatibilitySwitch<bool>(nameof(SuppressMapClientErrors));
_suppressUseValidationProblemDetailsForInvalidModelStateResponses = new CompatibilitySwitch<bool>(nameof(SuppressUseValidationProblemDetailsForInvalidModelStateResponses));
_allowInferringBindingSourceForCollectionTypesAsFromQuery = new CompatibilitySwitch<bool>(nameof(AllowInferringBindingSourceForCollectionTypesAsFromQuery));
_switches = new[]
{
_suppressMapClientErrors,
_suppressUseValidationProblemDetailsForInvalidModelStateResponses,
_allowInferringBindingSourceForCollectionTypesAsFromQuery
};
}

Expand Down Expand Up @@ -151,6 +154,43 @@ public bool SuppressUseValidationProblemDetailsForInvalidModelStateResponses
set => _suppressUseValidationProblemDetailsForInvalidModelStateResponses.Value = value;
}

/// <summary>
/// Gets or sets a value that determines if <see cref="BindingInfo.BindingSource"/> for collection types
/// (<see cref="ModelMetadata.IsCollectionType"/>).
/// <para>
/// When <see langword="true" />, the binding source for collection types is inferred as <see cref="BindingSource.Query"/>.
/// Otherwise <see cref="BindingSource.Body"/> is inferred.
/// </para>
/// </summary>
/// <value>
/// The default value is <see langword="false"/> if the version is
/// <see cref="CompatibilityVersion.Version_2_2"/> or later; <see langword="true"/> otherwise.
/// </value>
/// <remarks>
/// <para>
/// This property is associated with a compatibility switch and can provide a different behavior depending on
/// the configured compatibility version for the application. See <see cref="CompatibilityVersion"/> for
/// guidance and examples of setting the application's compatibility version.
/// </para>
/// <para>
/// Configuring the desired value of the compatibility switch by calling this property's setter takes
/// precedence over the value implied by the application's <see cref="CompatibilityVersion"/>.
/// </para>
/// <para>
/// If the application's compatibility version is set to <see cref="CompatibilityVersion.Version_2_1"/> or
/// lower then this setting will have the value <see langword="true"/> unless explicitly configured.
/// </para>
/// <para>
/// If the application's compatibility version is set to <see cref="CompatibilityVersion.Version_2_2"/> or
/// higher then this setting will have the value <see langword="false"/> unless explicitly configured.
/// </para>
/// </remarks>
public bool AllowInferringBindingSourceForCollectionTypesAsFromQuery
{
get => _allowInferringBindingSourceForCollectionTypesAsFromQuery.Value;
set => _allowInferringBindingSourceForCollectionTypesAsFromQuery.Value = value;
}

/// <summary>
/// Gets a map of HTTP status codes to <see cref="ClientErrorData"/>. Configured values
/// are used to transform <see cref="IClientErrorActionResult"/> to an <see cref="ObjectResult"/>
Expand Down
22 changes: 8 additions & 14 deletions src/Microsoft.AspNetCore.Mvc.Core/ApplicationModels/ApiBehaviorApplicationModelProvider.cs
View file
Open in desktop
Expand Up @@ -48,14 +48,15 @@ internal class ApiBehaviorApplicationModelProvider : IApplicationModelProvider
var defaultErrorTypeAttribute = new ProducesErrorResponseTypeAttribute(defaultErrorType);
ActionModelConventions.Add(new ApiConventionApplicationModelConvention(defaultErrorTypeAttribute));

var inferParameterBindingInfoConvention = new InferParameterBindingInfoConvention(modelMetadataProvider)
if (!options.SuppressInferBindingSourcesForParameters)
{
SuppressInferBindingSourcesForParameters = options.SuppressInferBindingSourcesForParameters
};
ControllerModelConventions = new List<IControllerModelConvention>
{
inferParameterBindingInfoConvention,
};
var convention = new InferParameterBindingInfoConvention(modelMetadataProvider)
{
AllowInferringBindingSourceForCollectionTypesAsFromQuery = options.AllowInferringBindingSourceForCollectionTypesAsFromQuery,
};

ActionModelConventions.Add(convention);
}
}

/// <remarks>
Expand All @@ -66,8 +67,6 @@ internal class ApiBehaviorApplicationModelProvider : IApplicationModelProvider

public List<IActionModelConvention> ActionModelConventions { get; }

public List<IControllerModelConvention> ControllerModelConventions { get; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see we don't need IControllerModelConventions anymore. But, what about our customers? Suggest leaving the property and the application of any items in the list. That is, limit change to initializing it as empty.

If this is a removal we've already decided on, remove the IControllerModelConvention interface as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type is internal, so this does not affect users.


public void OnProvidersExecuted(ApplicationModelProviderContext context)
{
}
Expand All @@ -91,11 +90,6 @@ public void OnProvidersExecuting(ApplicationModelProviderContext context)
convention.Apply(action);
}
}

foreach (var convention in ControllerModelConventions)
{
convention.Apply(controller);
}
}
}

Expand Down
108 changes: 29 additions & 79 deletions src/Microsoft.AspNetCore.Mvc.Core/ApplicationModels/InferParameterBindingInfoConvention.cs
View file
Open in desktop
Expand Up @@ -2,9 +2,7 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.Linq;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Mvc.Internal;
using Microsoft.AspNetCore.Mvc.ModelBinding;
using Microsoft.AspNetCore.Routing.Template;
Expand All @@ -13,13 +11,18 @@
namespace Microsoft.AspNetCore.Mvc.ApplicationModels
{
/// <summary>
/// A <see cref="IControllerModelConvention"/> that
/// <list type="bullet">
/// <item>infers binding sources for parameters</item>
/// <item><see cref="BindingInfo.BinderModelName"/> for bound properties and parameters.</item>
/// </list>
/// An <see cref="IActionModelConvention"/> that infers <see cref="BindingInfo.BindingSource"/> for parameters.
/// </summary>
public class InferParameterBindingInfoConvention : IControllerModelConvention
/// <remarks>
/// The goal of this covention is to make intuitive and easy to document <see cref="BindingSource"/> inferences. The rules are:
/// <list type="number">
/// <item>A previously specified <see cref="BindingInfo.BindingSource" /> is never overwritten.</item>
/// <item>A complex type parameter (<see cref="ModelMetadata.IsComplexType"/>) is assigned <see cref="BindingSource.Body"/>.</item>
/// <item>Parameter with a name that appears as a route value in ANY route template is assigned <see cref="BindingSource.Path"/>.</item>
/// <item>All other parameters are <see cref="BindingSource.Query"/>.</item>
/// </list>
/// </remarks>
public class InferParameterBindingInfoConvention : IActionModelConvention
{
private readonly IModelMetadataProvider _modelMetadataProvider;

Expand All @@ -29,41 +32,27 @@ public class InferParameterBindingInfoConvention : IControllerModelConvention
_modelMetadataProvider = modelMetadataProvider ?? throw new ArgumentNullException(nameof(modelMetadataProvider));
}

/// <summary>
/// Gets or sets a value that determines if model binding sources are inferred for action parameters on controllers is suppressed.
/// </summary>
public bool SuppressInferBindingSourcesForParameters { get; set; }
internal bool AllowInferringBindingSourceForCollectionTypesAsFromQuery { get; set; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this not public?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This convention type is new to 2.2, so users wouldn't need a compat flag.


protected virtual bool ShouldApply(ControllerModel controller) => true;
protected virtual bool ShouldApply(ActionModel action) => true;

public void Apply(ControllerModel controller)
public void Apply(ActionModel action)
{
if (controller == null)
if (action == null)
{
throw new ArgumentNullException(nameof(controller));
throw new ArgumentNullException(nameof(action));
}

if (!ShouldApply(controller))
if (!ShouldApply(action))
{
return;
}

InferBoundPropertyModelPrefixes(controller);

foreach (var action in controller.Actions)
{
InferParameterBindingSources(action);
InferParameterModelPrefixes(action);
}
InferParameterBindingSources(action);
}

internal void InferParameterBindingSources(ActionModel action)
{
if (SuppressInferBindingSourcesForParameters)
{
return;
}

for (var i = 0; i < action.Parameters.Count; i++)
{
var parameter = action.Parameters[i];
Expand Down Expand Up @@ -95,56 +84,17 @@ internal void InferParameterBindingSources(ActionModel action)
// Internal for unit testing.
internal BindingSource InferBindingSourceForParameter(ParameterModel parameter)
{
if (ParameterExistsInAnyRoute(parameter.Action, parameter.ParameterName))
if (IsComplexTypeParameter(parameter))
{
return BindingSource.Path;
return BindingSource.Body;
}

var bindingSource = IsComplexTypeParameter(parameter) ?
BindingSource.Body :
BindingSource.Query;

return bindingSource;
}

// For any complex types that are bound from value providers, set the prefix
// to the empty prefix by default. This makes binding much more predictable
// and describable via ApiExplorer
internal void InferBoundPropertyModelPrefixes(ControllerModel controllerModel)
{
foreach (var property in controllerModel.ControllerProperties)
if (ParameterExistsInAnyRoute(parameter.Action, parameter.ParameterName))
{
if (property.BindingInfo != null &&
property.BindingInfo.BinderModelName == null &&
property.BindingInfo.BindingSource != null &&
!property.BindingInfo.BindingSource.IsGreedy &&
!IsFormFile(property.ParameterType))
{
var metadata = _modelMetadataProvider.GetMetadataForProperty(
controllerModel.ControllerType,
property.PropertyInfo.Name);
if (metadata.IsComplexType && !metadata.IsCollectionType)
{
property.BindingInfo.BinderModelName = string.Empty;
}
}
return BindingSource.Path;
}
}

internal void InferParameterModelPrefixes(ActionModel action)
{
foreach (var parameter in action.Parameters)
{
var bindingInfo = parameter.BindingInfo;
if (bindingInfo?.BindingSource != null &&
bindingInfo.BinderModelName == null &&
!bindingInfo.BindingSource.IsGreedy &&
!IsFormFile(parameter.ParameterType) &&
IsComplexTypeParameter(parameter))
{
parameter.BindingInfo.BinderModelName = string.Empty;
}
}
return BindingSource.Query;
}

private bool ParameterExistsInAnyRoute(ActionModel action, string parameterName)
Expand All @@ -171,13 +121,13 @@ private bool IsComplexTypeParameter(ParameterModel parameter)
// No need for information from attributes on the parameter. Just use its type.
var metadata = _modelMetadataProvider
.GetMetadataForType(parameter.ParameterInfo.ParameterType);
return metadata.IsComplexType && !metadata.IsCollectionType;
}

private static bool IsFormFile(Type parameterType)
{
return typeof(IFormFile).IsAssignableFrom(parameterType) ||
typeof(IEnumerable<IFormFile>).IsAssignableFrom(parameterType);
if (AllowInferringBindingSourceForCollectionTypesAsFromQuery && metadata.IsCollectionType)
{
return false;
}

return metadata.IsComplexType;
}
}
}
1 change: 1 addition & 0 deletions src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorOptionsSetup.cs
View file
Open in desktop
Expand Up @@ -35,6 +35,7 @@ public class ApiBehaviorOptionsSetup :
{
dictionary[nameof(ApiBehaviorOptions.SuppressMapClientErrors)] = true;
dictionary[nameof(ApiBehaviorOptions.SuppressUseValidationProblemDetailsForInvalidModelStateResponses)] = true;
dictionary[nameof(ApiBehaviorOptions.AllowInferringBindingSourceForCollectionTypesAsFromQuery)] = true;
}

return dictionary;
Expand Down
16 changes: 4 additions & 12 deletions ...oft.AspNetCore.Mvc.Core.Test/ApplicationModels/ApiBehaviorApplicationModelProviderTest.cs
View file
Open in desktop
Expand Up @@ -98,15 +98,8 @@ public void Constructor_SetsUpConventions()
{
var convention = Assert.IsType<ApiConventionApplicationModelConvention>(c);
Assert.Equal(typeof(ProblemDetails), convention.DefaultErrorResponseType.Type);
});

Assert.Collection(
provider.ControllerModelConventions,
c =>
{
var convention = Assert.IsType<InferParameterBindingInfoConvention>(c);
Assert.False(convention.SuppressInferBindingSourcesForParameters);
});
},
c => Assert.IsType<InferParameterBindingInfoConvention>(c));
}

[Fact]
Expand Down Expand Up @@ -140,14 +133,13 @@ public void Constructor_DoesNotAddConsumesConstraintForFormFileParameterConventi
}

[Fact]
public void Constructor_SetsSuppressInferBindingSourcesForParametersIsSet()
public void Constructor_DoesNotAddInferParameterBindingInfoConvention_IfSuppressInferBindingSourcesForParametersIsSet()
{
// Arrange
var provider = GetProvider(new ApiBehaviorOptions { SuppressInferBindingSourcesForParameters = true });

// Act & Assert
var convention = Assert.Single(provider.ControllerModelConventions.OfType<InferParameterBindingInfoConvention>());
Assert.True(convention.SuppressInferBindingSourcesForParameters);
Assert.Empty(provider.ActionModelConventions.OfType<InferParameterBindingInfoConvention>());
}

[Fact]
Expand Down