Inference of "required" field for parameters is inconsistent and confusing

[domaindrivendev]

There's been several issues posted in relation to the required field for parameters, describing scenarios where it should or shouldn't be set. Keeping track of them independently has been difficult, and so I've decided to roll them into one issue, to hopefully come up with a general strategy that optimizes for the common case and is acceptable for the rest. If you've been directed here from one of the related issues, please read through the following comments and provide input around next steps.

Here's some of the suggestions that have been surfaced:

1. Non-nullable => required, nullable => not required

public IActionResult(int param1, int? param2, string param3)
// param1 is required, param2 & param3 are not required

This is NOT an acceptable approach because it's possible for a parameter in a JSON API to be both required and nullable.

2. Regular parameter => required, optional parameter => not required

public IActionResult(int param1, int param2 = 10)
// param1 is required, param2 is not required

This is a good approach because it leverages existing C# syntax. It doesn't require additional annotations and it allows a default value to be provided - something that's often useful for API consumers to be aware of. However, this approach won't work for model-bound parameters. For example

public IActionResult(QueryParameters params)
...
public class QueryParameters
{
    public int Param1 { get; set; }
    public int Param2 { get; set; }  // how do we say this is optional
}

3. Decorated with RequiredAttribute => required, Not decorated => not required

public IActionResult([Required]int param1, int param2)
// param1 is required, param2 is not required

This approach could be applied for action parameters and model-bound parameters. It's also the de facto method for implementing required field validation on model-bound parameters. However, it means that all fields will be described as optional unless they're decorated. If you assume that most API's have more required parameters than optional ones, you could argue this doesn't optimize for the most common case.

Summary

The current implementation leverages the ApiParameterDescription.ModelMetadata.IsRequired property that's surfaced through ApiExplorer. Under the hood, this uses a mix of approaches 1 and 2. As discussed, 1 doesn't really make sense for API parameters and the mix of both is clearly causing confusion. So, going forward I'd like to propose a mix of option 2 & 3, or a pure option 3. In other words ...

1. For action parameters, default to required unless it's an optional method parameter. For model-bound parameters, default to not required unless decorated with the RequiredAttribute

OR

2. For all parameters (action and model-bound), default to not required unless decorated with the RequiredAttribute

[nphmuller]

tl;dr: Option 2 (with an exception for path bound, and required attributes on non-nullable types)

I think the best solution is one that gives the same result as the build in Model Validator in MVC. Especially since Swagger doesn't really protect your API, it only documents it. With this I mean that if a parameter is marked as Required in Swagger, but isn't required when you actually call the API, it would be really confusing and maybe even a bit dangerous.

ASP.NET Core 2.1 also supports validation attributes directly on parameters. So that also takes care of a bit of the problem. As previously you would have to write your own ActionFilter to validate action parameters.

Anyway, this would give the following solution:

• Path parameters are always required.
• Both Query and Body parameters are always optional unless decorated with a Required attribute.
  • If there's a Required attribute and the type is non-nullable, it's still not required because it's always set to its default value (which is why it doesn't fail model validation in MVC). See the docs

Taking your 3 problem samples:

1

public IActionResult(int param1, int? param2, string param3)
// param1, param2 and param3 are not required, since they don't have a Required attribute.

2

public IActionResult(int param1, int param2 = 10)
// param1 and param are not required.

...
public class QueryParameters
{
    public int Param1 { get; set; } // Non-nullable, so always set to it's default value. I.e. non-required.
   // Can be made required like this:
   [Required]
   public int? Param2 { get; set; }
}

3

public IActionResult([Required] int param1, int param2)
// param1 and param2 are not required

[domaindrivendev]

Thanks @nphmuller - nicely articulated and I agree with your rationale. However, following this logic, shouldn't param1 be "not required" in the last example, as it's non-nullable?

public IActionResult([Required]int param1, int param2)

The only downside to this general approach is the fact that you can't document a parameter that is both required and nullable. In fact, you can't even implement this if you're using built-in model validation. The reality is, there's a subtle impedance mismatch between the meaning of the RequiredAttribute and the meaning of the required field in the Swagger/JSONSchema spec. But, in the spirit of optimizing for the most common case, I'm willing to live with this. Interested to get thoughts from other folks, but in the meantime I'm going to start a branch for these changes.

As a side note, it's unfortunate that the ApiExplorer that ships with ASP.NET Core doesn't follow this logic in it's assignment of the ApiParameterDescription.ModelMetadata.IsRequired property. In fact, it follows a kind of inverse - non-nullable fields are automatically marked required!

[nphmuller]

@domaindrivendev Yeah, it should be not required. I'll update my post above. Demonstrates how confusing it is...

you can't document a parameter that is both required and nullable.

You mean both required and non-nullable, right? I think it's a really confusing implementation from the MVC team. If a parameter is documented with the RequiredAttribute, the validation should fail if it's not part of the json. I understand why it's implemented the way it currently is (the validation is ran after the json is deserialized, so the non-nullable parameter already has its default value), but it's really error-prone this way.

I think it already helps if Swashbuckle reflects this, because it makes it more obvious that the required attribute actually has not effect on a non-nullable parameter. But the base is still pretty confusing.

About ApiExplorer: I'll create an issue over there, because this makes a confusing part even more confusing. Let's see what the team has to say about it. :)

[nphmuller]

@domaindrivendev It seems something like the snippet I wrote in aspnet/Mvc#7399 (comment) might be the recommended way for now. Agreed? Do you want me to create a PR for it?

[domaindrivendev]

Yep looks good to me. If you were willing to drive the PR that would be great. Could you copy the snippet into this issue, so we have it for reference. I'm still certain were going to have folks complaining about this implementation so good to have all the decision points in one place for reference :)

Also, would you mind adding some documentation as part of the PR? Given the confusion around this, I think that will be helpful. Thanks so much for your input here!

[nphmuller]

Sure, this is the snippet. It's currently the recommended way of inferring the IsRequired state according to the responses in this issue: aspnet/Mvc#7399

private bool IsRequiredInRequest(ApiParameterDescription desc)
{
    var modelMetadata = desc.ModelMetadata;
    if (desc.RouteInfo?.IsOptional == false)
        return true;
    if (modelMetadata.IsBindingRequired)
        return true;
    if (modelMetadata.IsRequired && modelMetadata.IsReferenceOrNullableType && HasDefaultNullValue(desc))
        return true;

    return false;
}

private bool HasDefaultNullValue(ApiParameterDescription desc)
{
    var modelMetadata = desc.ModelMetadata;
    switch (modelMetadata.MetadataKind)
    {
        case ModelMetadataKind.Property:
        {
            // Determine the default value of a property by creating a new instance
            // of the model and checking the property's value.
            var container = Activator.CreateInstance(modelMetadata.ContainerType);
            var propertyInfo = container.GetType().GetProperty(modelMetadata.PropertyName);
            return propertyInfo.GetValue(container) == null;
        }

        // Never triggered in ASP.NET Core 2.0, but might be in 2.1. This is because
        // modelMetadata.IsRequired is always false in 2.0, since validation attributes
        // directly on on action parameters aren't support. In 2.1 however this is support,
        // so this case might be hit then.
        case ModelMetadataKind.Type:
        {
            var controllerParameterDescriptor = desc.ParameterDescriptor as ControllerParameterDescriptor;
            if (controllerParameterDescriptor == null)
                throw new NotSupportedException("No way to determine the default value.");
            return controllerParameterDescriptor.ParameterInfo.DefaultValue == null;
        }

        default:
            throw new NotSupportedException();
    }

}

I'm also making progress in the implementation, although I don't have as much free time as expected.

I really want this to work with ASP.NET Core 2.1. And since they introduce support for validation attributes on action parameters I'm still working out how to infer if the parameter is required.

Posted this issue to figure that out: aspnet/Mvc#7435

[josemaia]

Is the following scenario under your number 2?

• Model-bound parameter Paging, i.e. public IActionResult(int param1, Paging pagingInfo)
• Paging has two properties (page, pageSize), both ints
• Paging() sets default values on both of them

In this scenario, a few months back I used to get both page and pageSize showing up as non-required in Swagger; currently I get both seen as required, forcing us to fill in two extra parameters.

I think this'd work with @nphmuller's example, as HasDefaultNullValue would return true, but I'm not sure how the properties being separated would influence that scenario.

[nphmuller]

@josemaia With the sample I posted the page and pageSize properties would be optional for the following reasons:

• They're both of type int. Which is non-nullable and always optional.
• They have default values. So even if they would have been annotated with [Required] and they would be nullable (int?), they still would be optional because of the default values.

By the way: the problem you described is almost exactly the same I ran into when updating to Swashbuckle.AspNetCore 1.1.0. See: #595

[alexvaluyskiy]

@nphmuller i tried this code snippet in ASP.NET Core 2.1 preview-2 and it works fine

public static class ApiParameterDescriptionExtensions
{
    public static bool IsRequired(this ApiParameterDescription parameterDescription)
    {
        if (parameterDescription.RouteInfo?.IsOptional == false)
            return true;
        if (parameterDescription.ModelMetadata.IsBindingRequired)
            return true;
        if (parameterDescription.ModelMetadata.IsRequired && DefaultValue(parameterDescription) == null)
            return true;

        return false;
    }

    public static object DefaultValue(this ApiParameterDescription parameterDescription)
    {
        switch (parameterDescription.ModelMetadata.MetadataKind)
        {
            case ModelMetadataKind.Property:
            {
                var container = Activator.CreateInstance(parameterDescription.ModelMetadata.ContainerType);
                var val = parameterDescription.ModelMetadata.PropertyGetter(container);

                // check that value is not default(T)
                if (parameterDescription.Type.IsValueType && Equals(val, Activator.CreateInstance(parameterDescription.Type)))
                {
                    return null;
                }

                return val;
            }
            case ModelMetadataKind.Parameter
                when parameterDescription.ParameterDescriptor is ControllerParameterDescriptor descriptor:
            {
                return descriptor.ParameterInfo.HasDefaultValue
                    ? descriptor.ParameterInfo.DefaultValue
                    : null;
            }
            default:
                return null;
        }
    }
}

I have created a controller with two actions

public class CrudActionsController : Controller
{
    [HttpPost("test")]
    public void Test(int test0, [BindRequired]int test1, [Required]int test2, int? test3, int test4 = 21, int? test5 = 53)
    {
    }

    [HttpPost("test2")]
    public void Test2(MyRequestModel model)
    {
    }

    public class MyRequestModel
    {
        public int Param1 { get; set; } = 5;

        [Required]
        public int Param2 { get; set; }

        public int? Param3 { get; set; }

        [Required]
        public int? Param4 { get; set; }
    }
}

For the Test action it returns

test0 - Required: true
test1 - Required: true
test2 - Required: true
test3 - Required: false
test4 - Required: false
test5 - Required: false

For the Test2 action it returns

Param1 - Required: false
Param2 - Required: true
Param3 - Required: false
Param4 - Required: false

Also DefaultValue method could be used to set default property for parameters (only in ASP.NET Core 2.0 and higher)

[domaindrivendev]

@nphmuller @alexlukelevy - on further reflection, I'm wondering if the additional predicate to check for a null default value is going to be worth the additional complexity of creating instances, containing instances etc..

In practice, I can't think of a good reason to use the RequiredAttribute on a property that doesn't have a null default value. Therefore, it's a reasonable assumption to say - when the attribute is used, it's most likely the developers intention for the underlying property to be required.

That's not to say it won't happen though. Developers, including myself, can fall into the trap of using it on a value type without realizing that a subsequent validation can't possibly fail. On principle, the additional predicate to match "actual" behavior more closely makes sense, but I'm a little skeptical of the complexity it introduces.

What do you think of doing the following compromise:

if (modelMetadata.IsRequired && !parameterDescription.Type.GetTypeInfo().IsValueType)
    return true;

It would catch the most common case described above, but not the case where a property value get's set in the containing types constructor. But, I'm contemplating this a good tradeoff

[gereon77]

From my point of view this topic is still not solved properly. Applying [Required] on a FromQuery parameter still results in required: false in my swagger doc.

[domaindrivendev]

@gereon77 - are you using ASP.NET Core's built-in validation mechanism (i.e. ModelState.IsValid) to actually validate the presence of fields that you want to describe as required?

This is the standard approach for request validation and therefore is the one that will yield the best results with Swashbuckle. For non-standard approaches, you'll have to leverage it's extensibility points (e.g. Operation & Schema filters). This is 100% by design.

If you are using the standard approach but still don't see the required flag set accordingly, then please post some example code so I can troubleshoot.

[gereon77]

@domaindrivendev : Thanks for your reply. Lets assume following demo code:

...
[HttpPost]
[ValidateModelState]
public IActionResult PostAnnouncement([FromBody] Announcement announcement)
{
    this.announcementService.Write(announcement);
    return this.Ok(announcement);
}
...

[DataContract]
public class Announcement
{
    [DataMember]
    [Required]
    public string Message { get; internal set; }

    [DataMember]
    [Required]
    public string Title { get; internal set; }
}

public class ValidateModelStateAttribute : ActionFilterAttribute
{
    public override void OnActionExecuting(ActionExecutingContext context)
    {
        if (!context.ModelState.IsValid)
        {
            context.Result = new BadRequestObjectResult(context.ModelState);
        }
    }
}

Here everything works perfect and the Swagger UI shows Message and Title as required.

But now lets assume this:

...
[HttpGet]
public IActionResult GetSomething([RequiredFromQuery] string key) // same as ([FromQuery][Required] string key)
{
    if (this.repository.ContainsKey(key))
    {
        return this.Ok(this.repository[key]);
    }

    return this.NotFound();
}
...

using System.ComponentModel.DataAnnotations;
using Microsoft.AspNetCore.Mvc.ModelBinding;

[AttributeUsage(AttributeTargets.Property | AttributeTargets.Parameter, AllowMultiple = false, Inherited = true)]
public class RequiredFromQueryAttribute : RequiredAttribute, IBindingSourceMetadata, IModelNameProvider
{
    public BindingSource BindingSource => BindingSource.Query;

    public string Name { get; set; }
}

I would expect that "key" is now marked as required, but it isn't. I know I could work with a wrapper class with an applied DataContract attribute, even though I haven't tested this with FromQuery params yet. As strings as query parameters are optional in AspNetCore I implemented an IActionModelConvention which adds custom filters for all parameters with my custom attribute and this filter ensures that the parameter is present and returns 400 if not:

...
var parameterNames = action
                        .Parameters
                        .Where(p => p.Attributes.OfType<RequiredFromQueryAttribute>().Any())
                        .Select(p => p.ParameterName);

foreach (var parameterName in parameterNames)
{
    action.Filters.Add(new FromQueryRequiredActionFilter(parameterName));
}
...

It would be very nice if Swagger UI would show string query params with Required attribute as required also like it does for simple types. Hope it's more clear now.

[domaindrivendev]

ModelState.IsValid does not honor Required attributes on parameters directly. So, how are you actually enforcing them to be required?

Remember, Swashbuckle describes what it believes to be the actual behavior. Now, it’s worth noting (and this is covered in the readme) that AsP.NET Core v2.1 will support validation on parameters directly. So, this will be the path to your desired behavior but in the meantime you would need to wrap in a class

[gereon77]

I enforce it to be required this way:

public class FromQueryRequiredConvention: IActionModelConvention
{
    public void Apply(ActionModel action)
    {
        var parameterNames = action
                                .Parameters
                                .Where(p => p.Attributes.OfType<RequiredAttribute>().Any())
                                .Select(p => p.ParameterName);

        foreach (var parameterName in parameterNames)
        {
            action.Filters.Add(new FromQueryRequiredActionFilter(parameterName));
        }
    }


    private class FromQueryRequiredActionFilter : IActionFilter
    {
        private readonly string parameterName;

        public FromQueryRequiredActionFilter(string parameterName)
        {
            this.parameterName = parameterName;
        }

        public void OnActionExecuted(ActionExecutedContext context)
        {
        }

        public void OnActionExecuting(ActionExecutingContext context)
        {
            context.ActionArguments.TryGetValue(this.parameterName, out var value);

            var str = value?.ToString();

            if (string.IsNullOrWhiteSpace(str))
            {
                context.Result = new BadRequestObjectResult($"Parameter '{this.parameterName}' is required");
            }
        }
    }
}

Would be cool if Swagger could handle this but in fact it's just a beauty issue and I can wait. :)

[domaindrivendev]

@nphmuller - I pulled down 2.1 preview 2 (2.1.0-preview2-final) but still don't see model validation working on top-level parameters. I thought this functionality was being added as part of 2.1 - do you know am I missing something?

public IActionResult Search([Required]string keywords)
{
    if (!ModelState.IsValid)  // IsValid returns true even if the request doesn't include keywords
        return BadRequest();
}

[rcollette]

I'm not seeing this behavior implemented

public IActionResult(int param1, int param2 = 10)
// param1 is required, param2 is not required

If not was this intentional and what would be the proper method of indicating a parameter that has a default value when not supplied?

[domaindrivendev]

Need to re-open as the latest implementation is causing equal, if not further confusion. There’s clearly different schools of thought around this. I’m going to take another stab at summarizing what those are and would encourage everyone on this thread to chip in their two cents.

[gereon77]

As explained above I would welcome a solution where the RequiredAttribute is recognized by the swagger API even if 2.1 won't bring model validation on top-level parameters.

[Yousefjb]

I didn't read all the comments above but can I suggest another approach ?

        /// <summary>
        /// Test parsing array of integers separted by , char
        /// </summary>
        /// <param name="id" required>see the required attribute here</param>
        /// <param name="numbers">your int array to parse</param>
        /// <returns>the text "you passed: " + the array joined with ","</returns>
        [HttpGet("test/{id}")]
        public string GetList(int id, string numbers)

< param name="id" required>

[renanmt]

Bumping this one cause it can make a huge difference when documenting APIs.
My problem today is this:

        [HttpPut("favorites")]
        [ProducesResponseType(204)]
        public IActionResult AddFavorites([FromQuery] Guid contentId) { ... }

        [HttpDelete("favorites")]
        [ProducesResponseType(2004)]
        public IActionResult RemoveFavorites([FromQuery] Guid contentId) { ... }

Having the contentId as required is exactly what I need to properly document this api.

[chrisoverzero]

@Yousefjb Unfortunately, that's neither well-formed XML nor a valid XML Documentation Comment.

[domaindrivendev]

*** ALL *** Based on further feedback, I'm considering an implementation that honors Required/BindRequired attributes on ALL parameters, both top-level and property-based, regardless of how they're treated by ASP.NET Core's built in model validation. I have a pending PR (see above) for this and have published a corresponding preview package to myget - https://www.myget.org/feed/domaindrivendev/package/nuget/Swashbuckle.AspNetCore/3.0.0-preview-0507.

I'd like to get a vote on this approach from everyone on this thread and ideally from anyone who was directed here from the related issues. So, please download the preview (link above), try it out and use the emoticons below to vote in favor or against this approach.

[gereon77]

Thank you very very much @domaindrivendev for implementing the Required attribute check!

[domaindrivendev]

Will be generally available with the 3.0.0 release (coming soon). In the meantime, feel free to pull down the latest preview from myget.org and try it out.

[mkontula]

Thanks @domaindrivendev! This was much anticipated feature!

[gereon77]

Hi @domaindrivendev ... Unfortunately with version 3 this cool feature is broken again. In Version 2.5 when I declared my query params with [Required][FromQuery] it was shown as required in the doc. With version 3.0 it's not. So I will downgrade again.... If needed I can paste screenshots or any other desired details.
Thx in advance

[domaindrivendev]

If you can post the details to clearly demonstrate the issue that would be really helpful. Thanks

[gereon77]

Hi @domaindrivendev , thanks for your reply.

Here is my little helper attribute which simplifies [Required][FromQuery]

using System.ComponentModel.DataAnnotations;
using Microsoft.AspNetCore.Mvc.ModelBinding;

[AttributeUsage(AttributeTargets.Property | AttributeTargets.Parameter, AllowMultiple = false, Inherited = true)]
public class RequiredFromQueryAttribute : RequiredAttribute, IBindingSourceMetadata, IModelNameProvider
{
    public BindingSource BindingSource => BindingSource.Query;

    public string Name { get; set; }
}

In V2.5.0 following declaration

public async Task<IActionResult> Demo([RequiredFromQuery] string value)

Returned:

"/api/Demo": {
      "get": {
        "tags": [ "Demo" ],
        "operationId": "ApiDemoGet",
        "consumes": [],
        "produces": [],
        "parameters": [
          {
            "name": "value",
            "in": "query",
            "required": true, // !!!!!!!!!!!!!!!!
            "type": "string"
          }]
      }
    }

In V3.0.0 the required flag is false again. :(

[gereon77]

@domaindrivendev Any chance that this will work again in a future version? I upgraded to Swashbuckle 4 and the problem still remains... Very sad to stay on 2.5.0 just because of this...

[gereon77]

Hi @domaindrivendev any news about this? I wonder because this doesn't seem so hard to be fixed...