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

JSON "null" request body rejected as an empty request body #40415

Open
1 task done
madelson opened this issue Feb 25, 2022 · 13 comments
Open
1 task done

JSON "null" request body rejected as an empty request body #40415

madelson opened this issue Feb 25, 2022 · 13 comments
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 candidate Indicates that the issues may be a good fit for community to help with. Requires work from eng. team
Milestone

Comments

@madelson
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

I have an action method which accepts a posted null value for the body:

		[HttpPost("acceptsNull")]
		public string Upload(WeatherForecast? forecast = null)
		{
			return JsonConvert.SerializeObject(forecast);
		}

However, when I post a JSON null to this endpoint I get a 400 response complaining that the body is empty.

Expected Behavior

Since the body is not empty and JSON-deserializes to the model parameter type just fine, I expect it to invoke the action with a null model.

If it can't do that, I at least expect an error message that indicates nulls are not permitted rather than one claiming the body is empty (although if the body were actually empty instead of valid JSON I'd probably want this request to be rejected).

Steps To Reproduce

Call the above endpoint with:

using var client = new HttpClient();
using var response = client.PostAsJsonAsync(".../acceptsNull", default(WeatherForecast));
Console.WriteLine(response.StatusCode); // 400
Console.WriteLine(await response.Content.ReadAsStringAsync()); //  ...A non-empty request body is required....

Exceptions (if any)

No response

.NET Version

6.0.100

Anything else?

The workaround I found is to set [FromBody(EmptyBodyBehavior = EmptyBodyBehavior.Allow)], but I don't really want to have to do this because I'd like to reject empty bodies and accept non-empty bodies which are JSON null.

@javiercn javiercn added the area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Feb 25, 2022
@brunolins16
Copy link
Member

@madelson we are changing the EmptyBody Behavior inference for nullable or parameters with default value in .NET 7 Preview 3 #39754, and both use cases (empty body and JSON null) will be treated as the same (invoke the action with null model).

Can you share why you are accepting JSON null and not empty bodies?

@madelson
Copy link
Author

madelson commented Mar 3, 2022

Can you share why you are accepting JSON null and not empty bodies?

To be clear, we aren't forced into this nor are we very strongly opinionated regarding it.

In general, it feels strange to me not to differentiate between a valid JSON payload consisting of the null literal with an empty request body. For example, I could see cases where the missing request body might be a mistake on the caller's part that goes uncaught because the endpoint accepts null.

On the other hand, if the [FromBody] parameter has a default value as in my example then this behavior does make perfect sense however.

@brunolins16
Copy link
Member

@madelson I understand you point, however, since you have an action with a nullable parameter or a parameter with a default value, this parameter is actually optional, that means even if the caller mistakes and failed to send the request body your API is accepting it because the parameter is not even required. That is what happens if you have a nullable/default value parameter FromQuery and the caller is not sending the correct Querystring.

As I mentioned, your scenario is fixed in .NET 7 Preview 3 where both will be accepted, however, if you have a non-nullable type /or without default value the request will be rejected with the message that you mentioned "A non-empty request body is required" that I agree that could not be the best for the JSON "null" since the body is not empty but that is how MVC works right now.

@brunolins16 brunolins16 added the Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. label Mar 4, 2022
@ghost
Copy link

ghost commented Mar 4, 2022

Hi @madelson. We have added the "Needs: Author Feedback" label to this issue, which indicates that we have an open question for you before we can take further action. This issue will be closed automatically in 7 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.

@madelson
Copy link
Author

madelson commented Mar 4, 2022

a nullable parameter or a parameter with a default value, this parameter is actually optional

A parameter with a default value should definitely be treated as optional. I think it's less obvious that nullable parameters are automatically optional (certainly they are not optional in C#, for example).

That is what happens if you have a nullable/default value parameter FromQuery and the caller is not sending the correct Querystring.

A difference here is that query strings do not have a native encoding for null whereas JSON does so it is possible to differentiate between a value that is null and a value that is missing in JSON.

In the HTTP world generally, does it make sense/is it conventional to send an empty request body (which cannot be parsed as JSON) with a content type header that implies JSON encoding? I'm honestly not sure.

request will be rejected with the message that you mentioned "A non-empty request body is required" that I agree that could not be the best for the JSON "null" since the body is not empty but that is how MVC works right now.

Moreso than the above points I would consider this a bug that ought to be fixed. Had the message been correct it would have saved me saved me some debugging time. Do you agree?

@ghost ghost added Needs: Attention 👋 This issue needs the attention of a contributor, typically because the OP has provided an update. and removed Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. labels Mar 4, 2022
@brunolins16
Copy link
Member

@madelson I agree that the message is not obvious and should not be the same as A non-empty request body is required, if the null is not a valid value, the JSON null scenario must fail during the validation instead. With that I agree with make it part of our backlog but I would say it is a enhancement instead of a bug, since the behavior will not block anymore due to the change in Preview 3 , ok?

Also, it will not be, unfortunately, a high priority right now but if you have time to contribute will be a pleasure helping you to make it works.

@brunolins16 brunolins16 added this to the .NET 7 Planning milestone Mar 7, 2022
@ghost
Copy link

ghost commented Mar 7, 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 help wanted Up for grabs. We would accept a PR to help resolve this issue enhancement This issue represents an ask for new feature or an enhancement to an existing one and removed Needs: Attention 👋 This issue needs the attention of a contributor, typically because the OP has provided an update. labels Mar 7, 2022
@madelson
Copy link
Author

madelson commented Mar 8, 2022

Sounds reasonable. Thanks @brunolins16 !

@senbar
Copy link
Contributor

senbar commented Mar 9, 2022

@madelson have You started working on this, or is it up for grabs still?

@madelson
Copy link
Author

@senbar go for it. I haven't picked it up.

@rjgotten
Copy link

A difference here is that query strings do not have a native encoding for null

Technically they don't have a native encoding at all.
Everything after the ? save for the # and whatever meaning a server wants to attribute to it, is fair game.
However, there are some particular schemes which grew into ad-hoc quote-unquote 'standards' such as the way the ASP.NET query string parsers and model binder work; the way jQuery's serialization of objects to query strings works (which iirc is a derivative of how PHP parses query strings?)

One convention for representing null query string values, or rather: a value-less key - is simply ?key.
Contrast with ?key= which represents the empty string.

The former is not supported entirely as such by ASP.NET, afaik.
I think all the parsing methods the framework supports, either drop the key entirely or rather than a value-less key turn it into a key-less value - i.e. a null key with a value of "key".

(Well; at least it's consistently inconsistent with the rest of the world 🤷‍♂️ )

@senbar
Copy link
Contributor

senbar commented Apr 1, 2022

What's the error message I should put in? Is something like "Null is not permitted in request body" fine?
Edit: I have opened pr draft to decide on what message should look like

@DamianEdwards DamianEdwards changed the title JSON "null" response body rejected as an empty response body JSON "null" request body rejected as an empty response body Apr 11, 2022
@DamianEdwards DamianEdwards changed the title JSON "null" request body rejected as an empty response body JSON "null" request body rejected as an empty request body Apr 11, 2022
@ghost
Copy link

ghost commented Oct 11, 2022

Thanks for contacting us.

We're moving this issue to the .NET 8 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.

@captainsafia captainsafia added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates and removed area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Jun 20, 2023
@mkArtakMSFT mkArtakMSFT added the help candidate Indicates that the issues may be a good fit for community to help with. Requires work from eng. team label Oct 28, 2023
@mkArtakMSFT mkArtakMSFT removed the help wanted Up for grabs. We would accept a PR to help resolve this issue label Oct 28, 2023
@captainsafia captainsafia modified the milestones: .NET 8 Planning, Backlog Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 candidate Indicates that the issues may be a good fit for community to help with. Requires work from eng. team
Projects
No open projects
Status: No status
Development

Successfully merging a pull request may close this issue.

8 participants