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

Inferring FromBody.AllowEmptyBehavior = Allow based nullability information #39754

Closed
Tracked by #33403
brunolins16 opened this issue Jan 25, 2022 · 5 comments · Fixed by #39917
Closed
Tracked by #33403

Inferring FromBody.AllowEmptyBehavior = Allow based nullability information #39754

brunolins16 opened this issue Jan 25, 2022 · 5 comments · Fixed by #39917
Assignees
Labels
area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels breaking-change This issue / pr will introduce a breaking change, when resolved / merged. feature-model-binding
Milestone

Comments

@brunolins16
Copy link
Member

brunolins16 commented Jan 25, 2022

Current design

Today, MVC infer optionality for parameters/properties based on nullability or if they have default value for all Binding Sources, eg.:

#nullable enable
    public IActionResult Post([FromBody] string? name);
    public IActionResult Get([FromQuery] string? name);
    public IActionResult Post([FromForm] string? name);
#nullable restore

Or

#nullable disable
    public IActionResult Post([FromBody] string name = null) ;
    public IActionResult Get([FromQuery] string name = null);
    public IActionResult Post([FromForm] string name = null);
#nullable restore

All examples above will allow the null value for the parameter name, however, when the binding From Body (inferred or not) the current default behavior is fail when the Content-Length == 0.

{
    "type": "https://tools.ietf.org/html/rfc7231#section-6.5.1",
    "title": "One or more validation errors occurred.",
    "status": 400,
    "traceId": "00-1a27854a7f1211e53fbc64e054801c11-a79404ee342e26ff-00",
    "errors": {
        "": [
            "A non-empty request body is required."
        ]
    }
}

Currently, are available 2 options to override this behavior and any of those available options are based on the Nullability information and need to be explicitly specified by the user.

Globally:

builder.Services.Configure<MvcOptions>(options =>
{
    options.AllowEmptyInputInBodyModelBinding = true;
});

Scoped:

public IActionResult Post([FromBody(EmptyBodyBehavior = EmptyBodyBehavior.Allow)] string? name);

Proposed Change

As part of the idea to bring Minimal APIs enhancements to MVC the proposal is applying the same default behavior and allow empty input body (Content-Length = 0) when the parameter/property is nullable or has the default value.

The suggestion is to always allow empty input (Content-Length = 0) when a [FromBody] parameter/property sets a default value or is nullable without allowing or requiring configuration.

After this change the following scenarios will allow an empty input:

#nullable disable
    public IActionResult Action([FromBody] string value);
    public IActionResult Action([FromBody] string value = null);
#nullable restore
#nullable enable
    public IActionResult Action([FromBody] string? value);
    public IActionResult Action([FromBody] string? value = null);
#nullable restore

Usage Examples

If the users are affected by this change and they would like a different behavior, the simplest way to avoid it is to define the action parameter/property correctly based on the optionality, as the examples listed below.

Required parameter in a disabled nullable context

Before

#nullable disable
    public IActionResult Action([FromBody] string value);
    public IActionResult Action([FromBody] string value = null);
#nullable restore

After

#nullable disable
    public IActionResult Action([FromBody][Required] string value);
#nullable restore

Required parameter in an enabled nullable context

Before

#nullable enable
    public IActionResult Action([FromBody] string? value);
    public IActionResult Action([FromBody] string? value = null);
#nullable restore

After

#nullable enable
    public IActionResult Action([FromBody] string value);
#nullable restore
@brunolins16 brunolins16 added breaking-change This issue / pr will introduce a breaking change, when resolved / merged. feature-model-binding Priority:0 Work that we can't release without area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels Cost:S labels Jan 25, 2022
@brunolins16 brunolins16 added this to the .NET 7 Planning milestone Jan 25, 2022
@ghost
Copy link

ghost commented Jan 25, 2022

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@brunolins16 brunolins16 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jan 25, 2022
@brunolins16 brunolins16 self-assigned this Jan 25, 2022
@brunolins16 brunolins16 removed the Priority:0 Work that we can't release without label Jan 25, 2022
@brunolins16 brunolins16 removed their assignment Jan 25, 2022
@brunolins16 brunolins16 removed the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jan 31, 2022
@pranavkm
Copy link
Contributor

This looks great.

The suggestion is to always allow empty input (Content-Length = 0)

Super unrelated to this change, but could we update the check to use the feature instead of relying on Content-Length: 0:

public interface IHttpRequestBodyDetectionFeature

@brunolins16
Copy link
Member Author

This looks great.

The suggestion is to always allow empty input (Content-Length = 0)

Super unrelated to this change, but could we update the check to use the feature instead of relying on Content-Length: 0:

public interface IHttpRequestBodyDetectionFeature

Actually, I was looking at exactly that because of this issue #29570. Thanks for mention that.

@brunolins16
Copy link
Member Author

I have been talking with @halter73 offline and one aspect of this change that we agreed is that having a mechanism to infer the EmptyBody based on optionality even when the AllowEmptyInputInBodyModelBinding = false seems weird and might be worth instead just change the default to true and let the actual validation mechanism check the parameter validity without any inferred behavior for the EmptyBody.

cc: @pranavkm.

@brunolins16
Copy link
Member Author

After, design conversations the final the infer mechanism will allow empty body only when:

modelMetadata.NullabilityState == NullabilityState.Nullable || 
modelMetadata.IsNullableValueType || 
modelMetadata.HasDefaultValue

That means the following scenario will still not allowing empty body as default, unless the default is changed by the current options available:

#nullable disable
    public IActionResult Ref([FromBody] TestClass val) => Ok(val);
#nullable restore

@ghost ghost locked as resolved and limited conversation to collaborators Mar 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels breaking-change This issue / pr will introduce a breaking change, when resolved / merged. feature-model-binding
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants