Cleanup InferParameterBindingInfoConvention #8665
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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 | ||
}; | ||
} | ||
|
||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor but s/will take/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"/> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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> | ||
|
@@ -66,8 +67,6 @@ internal class ApiBehaviorApplicationModelProvider : IApplicationModelProvider | |
|
||
public List<IActionModelConvention> ActionModelConventions { get; } | ||
|
||
public List<IControllerModelConvention> ControllerModelConventions { get; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can see we don't need If this is a removal we've already decided on, remove the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
{ | ||
} | ||
|
@@ -91,11 +90,6 @@ public void OnProvidersExecuting(ApplicationModelProviderContext context) | |
convention.Apply(action); | ||
} | ||
} | ||
|
||
foreach (var convention in ControllerModelConventions) | ||
{ | ||
convention.Apply(controller); | ||
} | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please copy over my doc comment improvement for Also s/ |
||
/// <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; | ||
|
||
|
@@ -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; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this not There was a problem hiding this comment. Choose a reason for hiding this commentThe 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]; | ||
|
@@ -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) | ||
|
@@ -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; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mention what binding source is chosen otherwise.