Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SupportNonNullableReferenceTypes not working for query parameters #2019

Closed
Eneuman opened this issue Feb 16, 2021 · 5 comments
Closed

SupportNonNullableReferenceTypes not working for query parameters #2019

Eneuman opened this issue Feb 16, 2021 · 5 comments
Labels
p1 High priority

Comments

@Eneuman
Copy link
Contributor

Eneuman commented Feb 16, 2021

Using v6.0.5 with .net Core 5 with non nullable reference types enabled.

It looks like c.SupportNonNullableReferenceTypes(); is not working for querystring parameters.

In a controller method I have the following parameter defined:

    [SwaggerOperation(OperationId = "GetGrid")]
    public async Task<ActionResult<ViewModels.Result>> GetGridAsync(string sorteringskolumn)

This generates the following json:

            "name": "sorteringsordning",
            "in": "query",
            "schema": {
              "type": "string"
            }

If I try and call this method without the parameter, the .Net Core Model Validation will give me back a HTTP 400 Bad Request and tell me that "sorteringskolumn" is required.

The problem here is that when generating a client from the above json, the client will not understand that the parameter is required since normaly, a string query parameter cannot be null and is by default "", but since non nullable reference type is enabled, the model validation behavior changes.

I think when using c.SupportNonNullableReferenceTypes () a more correct representation would be:

        "name": "sorteringsordning",
        "in": "query",
        "required": true,
        "schema": {
          "type": "string"
        }

The workaround is to add a [Required] infront of the parameter name in the controller, but this is awkvard since [Required] is not used when using non nullable reference types.

@Eneuman Eneuman changed the title SupportNonNullableReferenceTypes not wokring for query parameters SupportNonNullableReferenceTypes not working for query parameters Feb 16, 2021
@domaindrivendev
Copy link
Owner

Def a missing feature - specifically SB doesn't currently support "implicit required for non nullable reference types". Off the top my head, there's the hack approach and the much cleaner approach that ties in with some broader refactors that I want to make. Flagging as p1 for now.

@domaindrivendev domaindrivendev added the p1 High priority label Feb 16, 2021
@domaindrivendev
Copy link
Owner

With that said, it does look like ASP.NET Core currently honors this feature on the ModelMetadata.IsRequired property and so you could create a fairly simple IParameterFilter to workaround the issue for now:

// Startup.cs
services.AddSwaggerGen(c =>
{
    ...
    c.ParameterFilter<ImplicitRequiredParameterFilter>();
});
// ImplicitRequiredParameterFilter
public class ImplicitRequiredParameterFilter : IParameterFilter
{
    public void Apply(OpenApiParameter parameter, ParameterFilterContext context)
    {
        parameter.Required = context.ApiParameterDescription.ModelMetadata.IsRequired;
    }
}

@Eneuman
Copy link
Contributor Author

Eneuman commented Feb 17, 2021

Thank you for the workaround, it works great!

@Eneuman
Copy link
Contributor Author

Eneuman commented Feb 17, 2021

Now that we dont need to decorate the strings with [Required], we just need to get rid of the need to decorate the reference types with [BindRequired].

Since I have enabled non nullable referencetypes in my project, VS also wants me to explicit tell if a reference type is supposed to be null by adding a ? at the end of the declaration. As I understand it, this doesn't actually change the IL code, its more like static analysis of the code.

Like this:

[SwaggerOperation(OperationId = "GetGrid")]
public async Task<ActionResult<ViewModels.Grid>> GetGridAsync(MyComplexClass myType, string userId, [BindRequired] int articleId, int? orgId, int? personalId)

Is there any way to also remove the need for [BindRequired] ?
(Note that myType will is treated as it had [FromBody])

@martincostello
Copy link
Collaborator

To make issue tracking a bit less overwhelming for the new maintainers (see #2778), I've created a new tracking issue to roll-up various nullability issues here: #2793.

We'll refer back to this issue from there and include it as part of resolving that issue, but I'm going to close this one to help prune the backlog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p1 High priority
Projects
None yet
Development

No branches or pull requests

3 participants