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

Optional [FromBody] Model Binding #6878

Closed
douglasg14b opened this issue Jan 20, 2019 · 18 comments · Fixed by #22634
Closed

Optional [FromBody] Model Binding #6878

douglasg14b opened this issue Jan 20, 2019 · 18 comments · Fixed by #22634
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates enhancement This issue represents an ask for new feature or an enhancement to an existing one help wanted Up for grabs. We would accept a PR to help resolve this issue

Comments

@douglasg14b
Copy link

douglasg14b commented Jan 20, 2019

Is your feature request related to a problem? Please describe.

I am trying to use optional [FromBody] model binding on a per-endpoint basis.
Example:

        [HttpPost]
        public IActionResult Post([FromBody]SomeClass request = null)
        {
            return Ok();
        }

An empty request body results in the following being returned:

{
  "": [
    "A non-empty request body is required."
  ]
}

Describe the solution you'd like

For properties that have default value ie [FromBody] bool expandRelations = true to not be required in the body of the request.

Additional context

Similar issue here: aspnet/Mvc#6920

@pranavkm
Copy link
Contributor

Setting MvcOptions.AllowEmptyInputInBodyModelBinding should continue to work for this scenario. Are you seeing otherwise?

@pranavkm pranavkm added question area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Jan 22, 2019
@douglasg14b
Copy link
Author

douglasg14b commented Jan 22, 2019

@pranavkm On a per-endpoint basis similar to the example, not globally for the entire web application.

@pranavkm
Copy link
Contributor

@douglasg14b there isn't a feature to enable this on a per endpoint basis. One option might be to remove \ ignore specific that model validation error in your action.

@douglasg14b
Copy link
Author

I suppose this is a feature request then!

I can work around this for now, but it's a bit awkward.

@pranavkm pranavkm removed the question label Feb 15, 2019
@CrossBound
Copy link

I would also like to see a per-request option as mentioned by @hmvs here aspnet/Mvc#6920 (comment)

@mkArtakMSFT mkArtakMSFT added this to the Backlog milestone Jun 17, 2019
@mkArtakMSFT mkArtakMSFT added enhancement This issue represents an ask for new feature or an enhancement to an existing one PRI: 3 - Optional labels Jun 17, 2019
@pranavkm pranavkm added the c label Aug 22, 2019
@mkArtakMSFT
Copy link
Member

Thanks for contacting us, @douglasg14b.
We don't think we will get time to implement this feature.
However, this can be done by setting the MvcOptions.AllowEmptyInputInBodyModelBinding to true and implementing a custom ActionFilter which would validate the scenarios you care about explicitly.

@mkArtakMSFT mkArtakMSFT added the ✔️ Resolution: Won't Fix Resolved because we decided not to change the behavior reported in this issue. label Aug 28, 2019
@pentp
Copy link
Contributor

pentp commented Aug 28, 2019

Why close(+wontfix) this issue if it's just a matter of priority? There is no reasonably scoped workaround available and this problem won't go away in the foreseeable future.

@douglasg14b
Copy link
Author

douglasg14b commented Aug 28, 2019

@mkArtakMSFT Why close as wontfix if We don't think we will get time to implement this feature.? Sounds like this isn't a wontfix and is rather a low priority? Pentp's comment highlights the issue there.

If it's been decided that this will never be a feature in Asp.Net Core, that should probably be documented somewhere for clarification.

@mkArtakMSFT mkArtakMSFT removed the ✔️ Resolution: Won't Fix Resolved because we decided not to change the behavior reported in this issue. label Aug 29, 2019
@mkArtakMSFT
Copy link
Member

@pentp the reason is that keeping issues like this around just keeps accumulating our backlog without any good reason. If this is really something which community will benefit from a lot, we will hear similar feedback in the future again and we can reconsider then. There's just no point in accumulating backlog as it becomes unmanageable.

@douglasg14b I agree this is not really a "won't fix". It's a matter of priority and given our current priorities this doesn't and won't hit the bar in the near future.

@douglasg14b
Copy link
Author

douglasg14b commented Aug 29, 2019

Not to sound snide, but that sounds like your team needs an "On Hold" queue. It's what we use to manage items that won't be considered for a long time, or are blocked by a number of factors and need to sit for months.

@danroth27
Copy link
Member

There is no reasonably scoped workaround available

Why is the proposed solution of using the MvcOptions.AllowEmptyInputInBodyModelBinding setting + an action filter not workable?

@mkArtakMSFT
Copy link
Member

By the way, here is a working sample @pranavkm has put together with the proposed alternative: https://github.com/pranavkm/OptionalBodyBinding

@mkArtakMSFT mkArtakMSFT reopened this Aug 30, 2019
@mkArtakMSFT
Copy link
Member

@douglasg14b if you're willing to send a PR for this, we'll happily consider it.

@mkArtakMSFT mkArtakMSFT added the help wanted Up for grabs. We would accept a PR to help resolve this issue label Aug 30, 2019
@CrossBound
Copy link

I have to agree, even if this is low priority I hate to see this simply be closed because that means it completely leaves the radar.

@pentp
Copy link
Contributor

pentp commented Aug 31, 2019

I thought the scoping of the two parts of the workaround would mismatch, but actually it looks fine. The actual filter probably needs some tuning to avoid excessive reflection.

@pranavkm pranavkm removed the help wanted Up for grabs. We would accept a PR to help resolve this issue label Sep 2, 2019
@pranavkm pranavkm added help wanted Up for grabs. We would accept a PR to help resolve this issue and removed c labels Sep 2, 2019
@tmort93
Copy link

tmort93 commented Mar 6, 2020

My team experienced this same issue and we are also interested in a per-endpoint solution.
In the interim we are considering this work around which, in preliminary testing, has been able to handle an empty body without issue.

public class SomeClassModelBinder : IModelBinder
{
    public async Task BindModelAsync(ModelBindingContext bindingContext)
    {
        var stream = bindingContext.HttpContext.Request.Body;
        string body;
        using (var reader = new StreamReader(stream))
        {
            body = await reader.ReadToEndAsync();
        }
        var someClass = JsonConvert.DeserializeObject<SomeClass>(body);

        bindingContext.Result = ModelBindingResult.Success(someClass);
    }
}

[FromBody, ModelBinder(BinderType = typeof(SomeClassModelBinder))] SomeClass request,

@oliverjanik
Copy link

This is a 7th? revision of an MVC framework for .net and there's still workarounds required for common scenarios. This IMO violates the principle of least surprise. If object is declared nullable, doesn't that imply optional?

Another unfortunate side-effect is that it produces the ugly validation problem response:

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

pranavkm added a commit that referenced this issue Jun 7, 2020
pranavkm added a commit that referenced this issue Jun 10, 2020
pranavkm added a commit that referenced this issue Jun 22, 2020
@ghost ghost closed this as completed in #22634 Jun 22, 2020
ghost pushed a commit that referenced this issue Jun 22, 2020
* Add support for optional FromBody parameters

Fixes #6878

* Fixup nullable

* Changes per API review
pranavkm added a commit that referenced this issue Jun 22, 2020
* Add support for optional FromBody parameters

Fixes #6878

* Fixup nullable

* Changes per API review
@pranavkm
Copy link
Contributor

This should be addressed in 5.0-preview7. FromBody attributes accepts a property that allows configuring individual parameters to be optional:

public IActionResult Post([FromBody(EmptyBodyBehavior = EmptyBodyBehavior.Allow)] MyModel model)
...

@pranavkm pranavkm modified the milestones: Backlog, 5.0.0-preview7 Jun 23, 2020
mkArtakMSFT pushed a commit that referenced this issue Jun 23, 2020
* Add support for optional FromBody parameters (#22634)

* Add support for optional FromBody parameters

Fixes #6878

* Fixup nullable

* Changes per API review

* Fixup doc comment (#23229)
@ghost ghost locked as resolved and limited conversation to collaborators Jul 23, 2020
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates enhancement This issue represents an ask for new feature or an enhancement to an existing one help wanted Up for grabs. We would accept a PR to help resolve this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants