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

Commit

Permalink
Cleanup InferParameterBindingInfoConvention
Browse files Browse the repository at this point in the history 
* Infer BindingSource for collection parameters as Body. Fixes #8536
* Introduce a compat switch to keep 2.1.x LTS behavior for collection parameters
* Do not infer BinderModelName in InferParameterBindingInfoConvention
  • Loading branch information
@pranavkm
pranavkm committed Oct 30, 2018
1 parent 734b919 commit eb248c4
Show file tree
Hide file tree
Showing 7 changed files with 169 additions and 293 deletions.
36 changes: 36 additions & 0 deletions src/Microsoft.AspNetCore.Mvc.Core/ApiBehaviorOptions.cs
View file
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,39 @@ 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"/>) is inferred as <see cref="BindingSource.Query"/>.
/// </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 will take
/// 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
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; }

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
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"/>) <see cref="BindingSource.Body"/> is assigned.</item>
/// <item>If the parameter name appears in ANY route template, <see cref="BindingSource.Path"/> is assigned.</item>
/// <item>All other parameters, <see cref="BindingSource.Query"/> is assigned.</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; }

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
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
14 changes: 3 additions & 11 deletions ...oft.AspNetCore.Mvc.Core.Test/ApplicationModels/ApiBehaviorApplicationModelProviderTest.cs
View file
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 @@ -146,8 +139,7 @@ public void Constructor_SetsSuppressInferBindingSourcesForParametersIsSet()
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

0 comments on commit eb248c4

Please sign in to comment.