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

Conversation

pranavkm
Copy link
Contributor

Fixes #8657

@dougbu
Copy link
Member

dougbu commented Oct 30, 2018

@pranavkm I think I should hold off reviewing 'til you split this in two. LMK if I need to review this up front.

@pranavkm pranavkm force-pushed the prkrishn/8657 branch 7 times, most recently from eb248c4 to 9bb5480 Compare October 30, 2018 23:22
@pranavkm
Copy link
Contributor Author

🆙 📅

* 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
@dougbu
Copy link
Member

dougbu commented Oct 30, 2018

@pranavkm the commit description and the title are way wonky. This is about a new option (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"/>.
Copy link
Member

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.

/// 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
Copy link
Member

Choose a reason for hiding this comment

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

minor but s/will take/takes/

@@ -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.

/// 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>
Copy link
Member

Choose a reason for hiding this comment

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

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; }
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.

@pranavkm pranavkm changed the title Respect ApiBehaviorOptions.SuppressInferBindingSourcesForParameters when specified Cleanup InferParameterBindingInfoConvention Oct 31, 2018
Copy link
Contributor Author

@pranavkm pranavkm left a comment

Choose a reason for hiding this comment

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

🆙 📅

@@ -66,8 +67,6 @@ internal class ApiBehaviorApplicationModelProvider : IApplicationModelProvider

public List<IActionModelConvention> ActionModelConventions { get; }

public List<IControllerModelConvention> ControllerModelConventions { get; }
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.

/// 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
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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants