Swashbuckle.AspNetCore: Inference of "required" field for parameters is inconsistent and confusing

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

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

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.

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.

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

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>

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.

@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

@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: https://github.com/domaindrivendev/Swashbuckle.AspNetCore/issues/595

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!