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

Represent non-stringified JSON request body as an [object Object] string #881

Merged
merged 2 commits into from Feb 17, 2021
Merged

Represent non-stringified JSON request body as an [object Object] string #881

merged 2 commits into from Feb 17, 2021

Conversation

kettanaito
Copy link
Contributor

Changes

Ensures request._bodyInit (the internal representation of a request body) is always set to a [object Object] string representation of a body in case it has a non-stringified JSON body and the Content-Type header stating an expected JSON body.

Why

@kettanaito
Copy link
Contributor Author

kettanaito commented Dec 17, 2020

I'd like to ask for help with adding a test for this. Following the existing tests as examples, I've written this test:

test('converts a non-stringified json body to [object Object] string', function() {
  return fetch('/request', {
    method: 'post',
    headers: {
      'content-type': 'application/json'
    },
    body: {stringified: false}
  })
    .then(function(response) {
      return response.json()
    })
    .then(function(request) {
      assert.equal(request.data, '[object Object]')
    })
})

However, request.data is a public request body that gets correctly set to [object Object]. The test doesn't fail when the fix from this pull request is reverted. I can only assume that request.data doesn't equal the request body provided to xhr.send() at the end of a request:

fetch/fetch.js

Line 594 in a8aa427

xhr.send(typeof request._bodyInit === 'undefined' ? null : request._bodyInit)

Assertions on request._bodyInit fail, that doesn't seem to be public property (dangle implies).

The way I've verified this fix works is by running tests in my reproduction repository that asserts the arguments of the XMLHttpRequest.send call.

if (typeof body === 'string') {
this.headers.set('content-type', 'text/plain;charset=UTF-8')
} else if (this._bodyBlob && this._bodyBlob.type) {
this.headers.set('content-type', this._bodyBlob.type)
} else if (support.searchParams && URLSearchParams.prototype.isPrototypeOf(body)) {
this.headers.set('content-type', 'application/x-www-form-urlencoded;charset=UTF-8')
}
} else if (contentType.includes('json') && typeof this._bodyInit !== 'string') {
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 isn't a reliable JSON request body content-type header check. Would you recommend to provide application/json; charset=UTF-8 instead?

I'm thinking that this behavior should also affect any JSON-like content-type header (i.e. application/hal+json).

@lektor2034
Copy link

thk sir

@kettanaito kettanaito marked this pull request as draft Dec 17, 2020
@@ -246,14 +246,20 @@ function Body() {
this._bodyText = body = Object.prototype.toString.call(body)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternatively, we can set this._bodyInit to equal this._bodyText in this closure. It fixes the issue, but the solution becomes less deterministic not being connected with the request's content-type. I'm not sure it would be a good idea to always reset the body to its textual counterpart, but I would appreciate some feedback.

Choose a reason for hiding this comment

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

this._bodyText

ghost
ghost approved these changes Jan 26, 2021
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

@github github deleted a comment from marcin543 Feb 17, 2021
@github github deleted a comment from marcin543 Feb 17, 2021
@github github deleted a comment from marcin543 Feb 17, 2021
@github github deleted a comment from marcin543 Feb 17, 2021
@github github deleted a comment Feb 17, 2021
@JakeChampion JakeChampion marked this pull request as ready for review Feb 17, 2021
@JakeChampion JakeChampion linked an issue Feb 17, 2021 that may be closed by this pull request
@JakeChampion JakeChampion merged commit 5c6b055 into github:master Feb 17, 2021
@kettanaito kettanaito deleted the 856-non-stringified-json-body branch Feb 17, 2021
@JakeChampion
Copy link
Collaborator

This is released in v3.6.0 -- https://github.com/github/fetch/releases/tag/v3.6.0

This was referenced Jun 6, 2021
Copy link

@Apple1D Apple1D left a comment

Choose a reason for hiding this comment

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

  • [ ]

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

Successfully merging this pull request may close these issues.

Request body object is passed as-is
7 participants