fix(common): serialize string to JSON if explicitly set in Content-Type

[dasois]

RFC 8259 and ECMA-404 define that a simple string is valid JSON.
If Content-Type has been explicitly set to 'application/json' the HttpClient should
assume that it needs to be serialized.

Closes #31973

PR Checklist

Please check if your PR fulfills the following requirements:

• [ x ] The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
• [ x ] Tests for the changes have been added (for bug fixes / features)
• [ x ] Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

• [ x ] Bugfix

What is the current behavior?

Issue Number: #31973

What is the new behavior?

If Content-Type is set to 'application/json' the body will also be serialized, if it is typeof string already.

Does this PR introduce a breaking change?

• [ x ] Yes

Existing POST requests, that have a pure string payload, are serializing the body before calling http.post at the moment. This change would cause them to be serialized a second time, which would break the consistency of their POST-request.

[Airblader]

Isn't this a breaking change?

In our project we have a setup of automatically generated bindings to the backend. This includes serializing the payload (there's some custom stuff going on, not just a simple JSON stringify). Since we also set the content type, this would probably break our framework.

[trotyl]

what if the request body string is already the stringified result? Say that:

this.http.post('/api', JSON.stringify(something));

[dasois]

Isn't this a breaking change?

In our project we have a setup of automatically generated bindings to the backend. This includes serializing the payload (there's some custom stuff going on, not just a simple JSON stringify). Since we also set the content type, this would probably break our framework.

Exactly, this is why it's marked as breaking change.

[dasois]

what if the request body string is already the stringified result? Say that:

this.http.post('/api', JSON.stringify(something));

http.post should not try to be "smart". If you are used to it stringifying everything (arrays, objects, strings) that you provide as a body parameter, if you set the Content-Type, then you would not stringify the body yourself and it behaves as expected.

[Airblader]

And how should affected users mitigate that breakage? Having to manually parse the string before sending it is hardly an improvement.

[dasois]

And how should affected users mitigate that breakage? Having to manually parse the string before sending it is hardly an improvement.

Well, your issue is that you want to disable automatic stringify of http.post entirely because you want to do it manually. This stands against the "convenience" of not having to stringify the payload yourself if it is e.g. a object. So one could think about an option to disable that default behaviour for example, do you have an idea?

[Airblader]

I don't have a proposal, and for the record I'm not entirely disagreeing with you*. But it's existing behavior, so if we break it we should make sure that users can fix their existing code (in a sane way), and ideally just fix it for them during the upgrade (if possible). :-)

*) That said, there's nothing "wrong" with not making use of the convenience, passing a stringified object is perfectly fine. So I'm not sure either one of the two scenarios is worth favoring, and in that case I'd let existing behavior win the battle rather than fixing one thing for some users while breaking the opposite scenario for others.

Random thought: a non-breaking solution would be detecting whether the passed string is a stringified object or not. This is a bit ugly, yes, but would preserve compatibility.

[trotyl]

if you set the Content-Type, then you would not stringify the body yourself and it behaves as expected.

I feel the opposite, if Angular helps you serialize to JSON, then Angular is responsible to add the Content-Type header. And only if you provide a serialization result you should manually set it.

[dasois]

Random thought: a non-breaking solution would be detecting whether the passed string is a stringified object or not. This is a bit ugly, yes, but would preserve compatibility.

I had that thought already, but I don't like such things to be "intelligent". Maybe it's doable though...
What do you think about the following: https://regex101.com/r/rJuSo4/1

[dasois]

I feel the opposite, if Angular helps you serialize to JSON, then Angular is responsible to add the Content-Type header. And only if you provide a serialization result you should manually set it.

Angular currently has no way to know if you have already provided a serialized result.

[Airblader]

What I meant is to try and JSON parse it, and if it succeeds check if it's of type object. If yes, then the user provided a serialized result. If it's of type string, they provided a serialized string. If it fails, they provided a plain string (or something invalid).

[dasois]

What I meant is to try and JSON parse it, and if it succeeds check if it's of type object. If yes, then the user provided a serialized result. If it's of type string, they provided a serialized string. If it fails, they provided a plain string (or something invalid).

What about arrays, null, true, false?
Also parsing can be unnecessarily expensive for large json, if you can just determine this by the first few chars.

[trotyl]

Angular currently has no way to know if you have already provided a serialized result.

Yep, and that's why enforced serialization not valid.

[dasois]

Closing and providing a new PR with a non-breaking way of dealing with this.

[Airblader]

I would leave both PRs open. I'm just giving my opinion here, the Angular team will have to decide. :-) Appreciate your time in this!

[dasois]

I feel the opposite, if Angular helps you serialize to JSON, then Angular is responsible to add the Content-Type header. And only if you provide a serialization result you should manually set it.

How should angular know if the server expects json and not something else?

[dasois]

I would leave both PRs open. I'm just giving my opinion here, the Angular team will have to decide. :-) Appreciate your time in this!

Thanks for providing your valuable feedback and your use case. I will reference the PRs to each other.

[jessicajaniuk]

This would be a hugely breaking change, but it's definitely a valid concern. We're going to close this PR for now, but we're discussing internally a good solution to this problem. There is no ETA at this moment.

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}