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

fix json transform when data is pre-stringified #4020

Merged

Conversation

@gfortaine
Copy link
Contributor

@gfortaine gfortaine commented Sep 5, 2021

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 #1386, we fall into this bug (the payload is stringified 2 times instead of only one) cc @ddolcimascolo @rdsedmundo : #3986

  • using pre-stringified data cc @kawanet : #4018

cc @jasonsaayman @chinesedfan @DigitalBrainJS

@gfortaine gfortaine force-pushed the bugfix/json-improvements branch from e19af1f to 1ec6f38 Sep 5, 2021
@DigitalBrainJS
Copy link
Contributor

@DigitalBrainJS DigitalBrainJS commented Sep 5, 2021

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

Loading

@kawanet
Copy link

@kawanet kawanet commented Sep 5, 2021

LGTM

Loading

@gfortaine gfortaine force-pushed the bugfix/json-improvements branch 2 times, most recently from 32e9c6d to fea1383 Sep 6, 2021
@gfortaine
Copy link
Contributor Author

@gfortaine gfortaine commented Sep 6, 2021

@DigitalBrainJS

stringifySafely

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

Loading

@kawanet
Copy link

@kawanet kawanet commented Sep 6, 2021

@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;

Loading

@gfortaine
Copy link
Contributor Author

@gfortaine gfortaine commented Sep 6, 2021

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

Loading

@kawanet
Copy link

@kawanet kawanet commented Sep 6, 2021

@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.

Loading

@jasonsaayman
Copy link
Member

@jasonsaayman jasonsaayman commented Sep 6, 2021

@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

Loading

@gfortaine gfortaine force-pushed the bugfix/json-improvements branch from f6ae669 to 5f45be2 Sep 6, 2021
@jasonsaayman
Copy link
Member

@jasonsaayman jasonsaayman commented Sep 6, 2021

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

Loading

@jasonsaayman jasonsaayman linked an issue that may be closed by this pull request Sep 6, 2021
@jasonsaayman jasonsaayman changed the base branch from master to release/0.21.4 Sep 6, 2021
@jasonsaayman jasonsaayman changed the base branch from release/0.21.4 to master Sep 6, 2021
@gfortaine gfortaine force-pushed the bugfix/json-improvements branch from 8f688bf to ff8e76e Sep 6, 2021
@gfortaine
Copy link
Contributor Author

@gfortaine gfortaine commented Sep 6, 2021

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

Loading

@kawanet
Copy link

@kawanet kawanet commented Sep 6, 2021

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)

Loading

@jasonsaayman
Copy link
Member

@jasonsaayman jasonsaayman commented Sep 6, 2021

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.

Loading

@gfortaine gfortaine force-pushed the bugfix/json-improvements branch from ff8e76e to 82b24e1 Sep 6, 2021
@gfortaine gfortaine changed the base branch from master to release/0.21.4 Sep 6, 2021
@gfortaine
Copy link
Contributor Author

@gfortaine gfortaine commented Sep 6, 2021

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

Loading

@gfortaine gfortaine force-pushed the bugfix/json-improvements branch from 82b24e1 to 9807d1e Sep 6, 2021
@vargaurav
Copy link

@vargaurav vargaurav commented Sep 6, 2021

when are we planning for 0.21.4 release?

Loading

@jasonsaayman
Copy link
Member

@jasonsaayman jasonsaayman commented Sep 6, 2021

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

Loading

@jasonsaayman jasonsaayman merged commit 0fc7248 into axios:release/0.21.4 Sep 6, 2021
@DigitalBrainJS
Copy link
Contributor

@DigitalBrainJS DigitalBrainJS commented Sep 6, 2021

@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

Loading

@jasonsaayman
Copy link
Member

@jasonsaayman jasonsaayman commented Sep 6, 2021

@DigitalBrainJS I will include that in the release :)

Loading

jasonsaayman added a commit that referenced this issue Sep 6, 2021
* fix json transform when data is pre-stringified (#4020)

* [Updating] incorrect JSON syntax in README.md

* [Releasing] v0.21.4

Co-authored-by: Guillaume FORTAINE <guillaume+github@fortaine.com>
DigitalBrainJS referenced this issue Sep 7, 2021
…be passed directly as payload for encoding to JSON #2613, #61, #907 (#3688)

* Draft

* Added support for primitive types to be converted to JSON if the request Content-Type is 'application/json';
Added throwing SyntaxError if JSON parsing failed and responseType is json;
Added transitional option object;
Added options validator to assert transitional options;
Added transitional option `silentJSONParsing= true` for backward compatibility;
Updated README.md;
Updated typings;

* Fixed isOlderVersion helper;
Fixed typo;
Added validator.spec.js;

* Added forcedJSONParsing transitional option #2791

* `transformData` is now called in the default configuration context if the function context is not specified (for tests compatibility);

* Added `transitional.clarifyTimeoutError` to throw ETIMEDOUT error instead of generic ECONNABORTED on request timeouts;
Added support of onloadend handler if available instead of onreadystatechange;
Added xhr timeout test;
Fixed potential bug of xhr adapter with proper handling timeouts&errors (FakeXMLHTTPRequest failed to handle timeouts);
@alexandrughinea
Copy link

@alexandrughinea alexandrughinea commented Sep 15, 2021

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

Loading

@DigitalBrainJS
Copy link
Contributor

@DigitalBrainJS DigitalBrainJS commented Sep 15, 2021

@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?

Loading

@alexandrughinea
Copy link

@alexandrughinea alexandrughinea commented Sep 15, 2021

@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. 👍

Loading

@kawanet
Copy link

@kawanet kawanet commented Sep 16, 2021

@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. ❤️

Loading

@jasonsaayman
Copy link
Member

@jasonsaayman jasonsaayman commented Sep 16, 2021

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!

Loading

@chinanderm
Copy link

@chinanderm chinanderm commented Sep 17, 2021

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?

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants