Cleanup InferParameterBindingInfoConvention#8665
Conversation
4447cd1 to
22dc79c
Compare
|
@pranavkm I think I should hold off reviewing 'til you split this in two. LMK if I need to review this up front. |
eb248c4 to
9bb5480
Compare
|
🆙 📅 |
* 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
9bb5480 to
d98c27f
Compare
|
@pranavkm the commit description and the title are way wonky. This is about a new option ( |
|
|
||
| /// <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"/>. |
There was a problem hiding this comment.
Mention what binding source is chosen otherwise.
| /// 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.
minor but s/will take/takes/
|
|
||
| public List<IActionModelConvention> ActionModelConventions { get; } | ||
|
|
||
| public List<IControllerModelConvention> ControllerModelConventions { get; } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This type is internal, so this does not affect users.
| /// 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.
Please copy over my doc comment improvement for ModelMetadata.IsComplexType.
Also s/<see cref="BindingSource.Body"/> is assigned/is assigned <see cref="BindingSource.Body"/>/. Same for the next two items.
| /// 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.
This convention type is new to 2.2, so users wouldn't need a compat flag.
|
|
||
| public List<IActionModelConvention> ActionModelConventions { get; } | ||
|
|
||
| public List<IControllerModelConvention> ControllerModelConventions { get; } |
There was a problem hiding this comment.
This type is internal, so this does not affect users.
| /// 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.
This convention type is new to 2.2, so users wouldn't need a compat flag.
Fixes #8657