Add IsRequired and DefaultValue to ApiParameterDescription #7957
Conversation
| /// A parameter is considered required if | ||
| /// a) If it's bound from the request body (<see cref="BindingSource.Body"/>). | ||
| /// b) If it's a required route value. | ||
| /// c) If it has annotations (e.g. BindRequiredAttribute) that indicate it's required. |
There was a problem hiding this comment.
- This list will be all in one line unless you switch to
<list type="bullet">or<list type="number">syntax. - Don't repeat "If" in these items.
| @@ -324,7 +324,7 @@ public override bool IsReadOnly | |||
|
|
|||
| /// <inheritdoc /> | |||
| public override bool IsRequired | |||
| { | |||
| { | |||
| @@ -23,7 +21,7 @@ namespace Microsoft.AspNetCore.Mvc.ApiExplorer | |||
| /// Implements a provider of <see cref="ApiDescription"/> for actions represented | |||
| /// by <see cref="ControllerActionDescriptor"/>. | |||
| /// </summary> | |||
| public class DefaultApiDescriptionProvider : IApiDescriptionProvider | |||
| public partial class DefaultApiDescriptionProvider : IApiDescriptionProvider | |||
| parameter.IsRequired = true; | ||
| } | ||
|
|
||
| if (parameter.ModelMetadata != null && parameter.ModelMetadata.IsBindingRequired) |
There was a problem hiding this comment.
When would ModelMetadata property be null e.g. is this only for route parameters that don't match an actual parameter (but are added to Results)?
| parameter.IsRequired = true; | ||
| } | ||
|
|
||
| if (parameter.Source == BindingSource.Path && parameter.RouteInfo != null && !parameter.RouteInfo.IsOptional) |
There was a problem hiding this comment.
When would RouteInfo property be null e.g. is this only for BindingSource.Path parameters that don't match actual route parameters?
| else | ||
| { | ||
| if (parameter.ParameterDescriptor is ControllerParameterDescriptor controllerParameter && | ||
| ParameterDefaultValues.TryGetDeclaredParameterDefaultValue(controllerParameter.ParameterInfo, out var defaultValue)) |
| } | ||
|
|
||
| [Fact] | ||
| public void ProcessIsRequired_SetsTrue_ForParametersWithBindRequired() |
There was a problem hiding this comment.
Test is contradictory, description is for a parameter but metadata is for a property.
There was a problem hiding this comment.
ApiParameters cover parameters and properties
There was a problem hiding this comment.
🆗 but this test still covers a C# property, making the name confusing. Suggest changing the name to ..._ForApiParameterWithBindRequired or ..._ForPropertyWithBindRequired.
| } | ||
|
|
||
| [Fact] | ||
| public void ProcessIsRequired_DoesNotSetToTrue_ForParametersWithValidationRequired() |
There was a problem hiding this comment.
Test is contradictory, description is for a parameter but metadata is for a property.
There was a problem hiding this comment.
ApiParameters cover both parameters and properties
There was a problem hiding this comment.
🆗 but this test still covers a C# property, making the name confusing. Suggest changing the name to ..._ForApiParameterWithValidationRequired or ..._ForPropertyWithValidationRequired.
| } | ||
|
|
||
| [Fact] | ||
| public void ProcessDefaultValue_SetsDefaultRouteValue_FromParameterInfo() |
There was a problem hiding this comment.
Rename because test is about setting the ApiParameterDescription.DefaultValue property when the parameter is bound to a Query value.
| parameter.IsRequired = true; | ||
| } | ||
|
|
||
| if (parameter.ModelMetadata != null && parameter.ModelMetadata.IsBindingRequired) |
| parameter.IsRequired = true; | ||
| } | ||
|
|
||
| if (parameter.Source == BindingSource.Path && parameter.RouteInfo != null && !parameter.RouteInfo.IsOptional) |
| } | ||
|
|
||
| [Fact] | ||
| public void ProcessIsRequired_SetsTrue_ForParametersWithBindRequired() |
There was a problem hiding this comment.
ApiParameters cover parameters and properties
| } | ||
|
|
||
| [Fact] | ||
| public void ProcessIsRequired_DoesNotSetToTrue_ForParametersWithValidationRequired() |
There was a problem hiding this comment.
ApiParameters cover both parameters and properties
| } | ||
|
|
||
| [Fact] | ||
| public void ProcessIsRequired_SetsTrue_ForParametersWithBindRequired() |
There was a problem hiding this comment.
🆗 but this test still covers a C# property, making the name confusing. Suggest changing the name to ..._ForApiParameterWithBindRequired or ..._ForPropertyWithBindRequired.
| } | ||
|
|
||
| [Fact] | ||
| public void ProcessIsRequired_DoesNotSetToTrue_ForParametersWithValidationRequired() |
There was a problem hiding this comment.
🆗 but this test still covers a C# property, making the name confusing. Suggest changing the name to ..._ForApiParameterWithValidationRequired or ..._ForPropertyWithValidationRequired.
Fixes #7563