fix json transform when data is pre-stringified

[gfortaine]

Just spent my Sunday afternoon on this nasty bug introduced by PR #3688 (to fix #2613). It should be OK for people :

• using interceptors with JSON data. Indeed, the fact is that when we retry the request with the request interceptor, because the data is already stringified the second time config.data has been changed in response interceptor #1386, we fall into this bug (the payload is stringified 2 times instead of only one) cc @ddolcimascolo @rdsedmundo : Error response interceptor receives JSON stringified payload on error.config.data #3986

• using pre-stringified data cc @kawanet : 0.21.2 has a breaking change with pre-stringified JSON request body #4018

cc @jasonsaayman @chinesedfan @DigitalBrainJS

[DigitalBrainJS]

It looks like I'm a little late with my fix :)

[kawanet]

LGTM

[gfortaine]

@DigitalBrainJS

stringifySafely

LGTM, so it looked like relevant to include it in this fix as well 👍

[kawanet]

@gfortaine @DigitalBrainJS

stringifySafely does execute unnecessarily parse() even when data is a pre-stringified well-formed JSON.
It would cause a performance decrease compared to 0.21.1 and before.

0.21.1

    if (utils.isObject(data)) {
      setContentTypeIfUnset(headers, 'application/json;charset=utf-8');
      return JSON.stringify(data);
    }
    return data;

commit f6ae669

    if (utils.isObject(data) || (headers && headers['Content-Type'] === 'application/json')) {
      setContentTypeIfUnset(headers, 'application/json');
      return stringifySafely(data);
    }
    return data;

Why not keep a better backward compatibility:

    var isJSON = (headers && headers['Content-Type'] === 'application/json');
    if (utils.isObject(data) || (isJSON && !utils.isString(data))) {
      setContentTypeIfUnset(headers, 'application/json');
      return stringifySafely(data);
    }
    return data;

[gfortaine]

@kawanet How do you handle #2613 (comment) with strings ?

[kawanet]

@slaout looks just want to support a data which is a naked number.
It's supported as utils.isString(data) returns false when data is a number.
Sending a naked number as a JSON is a new feature of axios and would not break any existing apps.
Sending a naked boolean would be supported as well. But not a naked string.

README.md doesn't mention JavaScript string nor number serialized.

By default, axios serializes JavaScript objects to JSON.

I rarely think there are any use-cases to send a naked string to be serialized (wrapped) as a JSON by axios's default behavior.

[jasonsaayman]

@kawanet, I agree that the readme, may not contain this behaviour however people could easily be lead to believe that we handle JSON as per the RFC. I therefore think the best course of action is to keep backward compatibility at the best levels while including compatibility with the RFC.

@gfortaine, can we include all the cases from #2613 and as below so that we don't break anything in the future:

• object => { example: true }
• array => [ 42, 43 ]
• string => "foo"
• number => -42
• "true" => true
• "false" => false
• "null" => null

[jasonsaayman]

@DigitalBrainJS is this inline with #4021, can I merge this and close your pull request?

[gfortaine]

@jasonsaayman all the cases => done (see tests) ✅

[kawanet]

Thank you for updating axios! I welcome 0.21.4 anyway!🍻

---

Behavior Summary:

(1) sending a valid JSON {"foo": "bar"} stringified by axios. compatible behavior. everybody need this.

axios.post(url, {"foo": "bar"}, options)

(2) sending a valid JSON {"foo": "bar"} pre-stringified by app. compatible behavior. my app uses this way.
0.21.2 and 0.21.3 has broken this. 0.21.4 fixes it.

axios.post(url, '{"foo": "bar"}', options)

(3) sending a valid JSON 123. new behavior since 0.21.2. @slaout has shown his use-case.

axios.post(url, 123, options)

(4) sending a JSON wrapped string "some text". new behavior since 0.21.2 but no use-cases shown though :)

axios.post(url, "some text", options)

(5) sending a JSON wrapped string "{\"foo\":\"bar\"" because it is not a valid JSON. treat it as same as (4).

axios.post(url, '{"foo":"bar"', options) // broken JSON missing trailing `}`

(7) sending a Buffer contains a pre-serialized JSON to bypass unnecessary parse in stringifySafely.

axios.post(url, Buffer.from('{"foo": "bar"}'), options)

[jasonsaayman]

Ok great, @gfortaine I will need to make some updates since the pull is coming from master and there are breaking changes already merged to master so I will have to change it to release/0.21.4.

[gfortaine]

@jasonsaayman rebased to be able to merge it safely in release/0.21.4

[vargaurav]

when are we planning for 0.21.4 release?

[jasonsaayman]

@gfortaine thanks looks great to me 👍 I will release tonight

[DigitalBrainJS]

@jasonsaayman Definitely yes. But it would be nice if someone could fix the syntax issue of the config object in the Readme as my PR contains these changes.
https://github.com/axios/axios/pull/4021/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5L477

[jasonsaayman]

@DigitalBrainJS I will include that in the release :)

[alexandrughinea]

This breaks a lot of stuff upstream, escaping is done too aggressively.

[DigitalBrainJS]

@alexandrughinea Why? Is using the right ContentType for requests so aggressive? Is there any reason to set ContentType to JSON to actually send non JSON data?

[alexandrughinea]

@DigitalBrainJS That't not the problem.
The problem is that folks get their dependency security alerts and probably will rush to upgrade the patch version for Axios (because timed audits, etc.) without actually knowing that a change in the production code is ALSO needed for each POST/PUT request etc. - otherwise it will break code. - finally found the details about this change, but it was hidden under a lot of detailed release notes.

Please better highlight breaking changes in release notes. 👍

[kawanet]

@alexandrughinea +1

Semver
Until axios reaches a 1.0 release, breaking changes will be released with a new minor version. For example 0.5.1, and 0.5.4 will have the same API, but 0.6.0 will have breaking changes.

It was not a problem that the commit 5ad6994 had a breaking change.
It was an aggressive thing that it was released with a patch version number 0.21.2, but not with a new minor version number 0.22.0.
I'd love maintainers to give a new major or minor version number when adding such incompatible features at the next time.
Thank you all maintainers anyway. ❤️

[jasonsaayman]

We will mend this, I am working harder now to determine what will bump what in the versioning. Next release will be a 0.22.0 and then I would like to cut a version 1. However there is some larger questions that really need to be answered before that v1 is cut to have a solid vision and steering for axios.

That being said I strive to be more vigilant!

[chinanderm]

This breaks a lot of stuff upstream, escaping is done too aggressively.
a change in the production code is ALSO needed for each POST/PUT request etc.

Care to elaborate @alexandrughinea?