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(#2767): Fix serilization issues of bigint in json body #2773

Merged
merged 7 commits into from
Aug 27, 2024

Conversation

helloanoop
Copy link
Contributor

Fixes: #2767

@helloanoop helloanoop marked this pull request as draft August 7, 2024 14:31
Copy link

github-actions bot commented Aug 7, 2024

Test Results

  1 files  ±0   59 suites  +1   21s ⏱️ -1s
 84 tests ±0   84 ✅ ±0  0 💤 ±0  0 ❌ ±0 
131 runs  +1  131 ✅ +1  0 💤 ±0  0 ❌ ±0 

Results for commit 00e9845. ± Comparison against base commit b4fd350.

♻️ This comment has been updated with latest results.

@helloanoop
Copy link
Contributor Author

The serialization is now lossless. Just need to fix the deserialization next by providing a toggle at collection level setting to disable parsing response as json (when the content type is application/json)

image

<pre className="line request">
<span className="arrow">{'>'}</span> data {requestData}
<span className="arrow">{'>'}</span> data <pre>{request.data}</pre>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This causes a horizontal overflow, if the text is too long

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch. Fixed

return this.req.data;
}

setBody(data) {
if (typeof data === 'object') {
Copy link
Member

@Its-treason Its-treason Aug 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably also check if the Object is a "plain" object with data.constructor === Object (or something else). Setting a FormData object as the body does not work now, but worked previously.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense. Will have this updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I have updated the logic to also check if content type is json (based on headers).
This should cover the use case of people setting body as FormData object.

If someone has set the Content-Type as json in the header (or it gets auto added if they have chosen JSON as body in UI), and then they try to pass a non-plain object while invoking setBody() - they are essentially shooting themselves in the foot and we don't need to gracefully handle that.

The reason why are supporting the raw option is to support use cases where people want to still have the content type as json, but want to pass the json strings themselves instead of passing the json object

@Its-treason lmk if you differ or have further suggestions

setBody(data, options = {}) {
    if (options.raw) {
      this.req.data = data;
      this.body = data;
      return;
    }

    const isJson = hasJSONContentType(this.req.headers);
    if (isJson && this.__isObject(data)) {
      this.body = data;
      this.req.data = this.__safeStringifyJSON(data);
      return;
    }

    this.req.data = data;
    this.body = data;
  }

* @param {boolean} options.raw - If true, return the raw body without parsing it
* @returns
*/
getBody(options = {}) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about req.body that is set inside the constructor? I think this should also be JSON parsed like req.getBody

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 good catch (this could have resulted in breakages for folks who were accessing req.body in scripts)

Have updated the code with a comment

constructor(req) {
    // existing code

    /**
     * We automatically parse the JSON body if the content type is JSON
     * This is to make it easier for the user to access the body directly
     * 
     * It must be noted that the request data is always a string and is what gets sent over the network
     * If the user wants to access the raw data, they can use getBody({raw: true}) method 
     */
    const isJson = hasJSONContentType(this.req.headers);
    if (isJson) {
      this.body = this.__safeParseJSON(req.data);
    }
  }

@pull-request-size pull-request-size bot added size/L and removed size/M labels Aug 8, 2024
@helloanoop
Copy link
Contributor Author

helloanoop commented Aug 8, 2024

@Its-treason I have addressed all your comments and have also made some updates. Let me know if you have further comments

@lohxt1 will take over from here and add further changes on this PR to add a toggle in bruno.json to support disabling the behaviour of bruno automatically parsing response as json (if the response is a valid json). This is aimed at solving the problem around the response text losing its fidelity during the JSON.parse operation

bruno.json

{
    parseResponseJson: true
}

@helloanoop
Copy link
Contributor Author

helloanoop commented Aug 8, 2024

Another option that I am thinking to solve problems for folks who don’t want us to to automatically parse response as json (where the response loses fidelity in some cases) is to provide an api that can disable this.

req.disableParsingResponseJson()

They can use this then at request level scripting, or apply it to folder level or collection level scripts as they see fit.

We can consider providing a UI toggle much later if there is demand for it.

lohxt1 and others added 2 commits August 8, 2024 18:36
* disable response json parse flag

* fix: pr review comments
@helloanoop helloanoop marked this pull request as ready for review August 8, 2024 13:17
@helloanoop
Copy link
Contributor Author

The PR is GTG from my side.

Folks who don’t want us to to automatically parse response as json (where the response text loses fidelity in some cases) can use the new api method req.disableParsingResponseJson()

They can use this at request level scripting, or apply it to folder level or collection level scripts as they see fit.

@Its-treason @jwetzell let me know if you have any further comments. I plan to have this merged tomorrow.

lohxt1 and others added 2 commits August 8, 2024 19:07
* disable response json parse flag

* fix: pr review comments

* update bru req function name
@MeskaKeyne
Copy link

hello, fix seems not working in 1.27.0

eg
{
"id": 1.0000000000000493e+21,
"dateFrom": "2019-08-08",
}

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

Successfully merging this pull request may close these issues.

Lossless Serialization of Decimals and Large Numbers in JSON
4 participants