Got: Cannot pass string as json body with json = true option

[ikokostya]

Versions

got: 8.3.1

Code example

Try to call got with string as json body:

got(url, {
  method: 'POST',
  json: true,
  body: 'str'
});

Expected behavior

No errors. The code above is similar to:

got(url, {
   headers: {
       'content-type': 'applicaiton/json'
   },
   body: JSON.stringify('str')
})

Also I can pass any other primitive types (null, number) which are valid in JSON.

Actual behavior

I got the following error:

 TypeError: The `body` option must be a plain Object or Array when the `form` or `json` option is used

[brandon93s]

The json option requires a JSON request body and a JSON response. When this is not the case, you can set the headers manually to support the endpoints you're interacting with.

More info: https://github.com/sindresorhus/got/issues/467#issuecomment-403189894

[ikokostya]

@brandon93s What you mean under JSON request body? Body wrapped with JSON.stringify() is valid json body, no matter it's object (hash) or not.

[brandon93s]

To clarify, the json option does the stringify and parse operations for you while also configuring headers to support JSON. Because of this, a plan object or an array are required so that got can handle the conversion to JSON. When passing a string, got cannot confirm that the input is valid JSON (without a parse anyways) and therefore does not want to make invalid assumptions in configuring headers on your behalf.

[ikokostya]

When passing a string, got cannot confirm that the input is valid JSON

Why? got converts body to string with JSON.stringify: https://github.com/sindresorhus/got/blob/ad7b361dcb2490c3864b845b979b756f13f7d89b/index.js#L583

JSON.stringify() takes any type:

> JSON.parse(JSON.stringify(null))
null
> JSON.parse(JSON.stringify(1))
1
> JSON.parse(JSON.stringify('str'))
'str

[ikokostya]

@brandon93s ?

[ikokostya]

To be clear: json = true option is a convenience method to convert body to valid JSON string and setup corresponding request header. Current behavior is incorrect and unobvious.

Unlike request, response body always parsed https://github.com/sindresorhus/got/blob/ad7b361dcb2490c3864b845b979b756f13f7d89b/index.js#L377 without checking its type, but it can be primitive type.

[ikokostya]

@sindresorhus ?

[alextes]

@ikokostya the key bit in the docs is:

body must be a plain object or array and will be stringified.

So when asking got to make JSON communication easier for you, it assumes it will be stringifying your body for you. Assuming this, we've discussed in depth what the simplest approach would be in #174 . See specifically this comment by Sindre. The most relevant bit being:

The JSON option is just meant to cover the common case. If someone wants more advanced configurability they can just not use it.

Now I actually suggested to implement the behavior you expect, but Sindre felt it more complicated. If I had to guess why it's because there will be more users that accidentally pass a JSON string than users that want us to stringify a JavaScript string or number. So, to keep things helpful and simple, we only accept plain objects and arrays.

I actually agree more with @sindresorhus now then I did then, still, your 'wish' is heard. Maybe you can add what your use-case is for making the body a string? That would be helpful 🙏 .

Does that fully answer your questions @ikokostya 😄 ?

[ikokostya]

@alextes Thanks for the exhaustive answer.

Maybe you can add what your use-case is for making the body a string?

I want to send POST request with string body as JSON:

got(url, {
   headers: {
       'content-type': 'applicaiton/json'
   },
   body: JSON.stringify('str')
})

Of course, I try to use json: true option to simplify my code.

If I had to guess why it's because there will be more users that accidentally pass a JSON string than users that want us to stringify a JavaScript string or number.

Now I get TypeError when provide non-object as request body. Without this restriction body will be successfully converted to string and another service can return response with 400 status code. Technically it's more correct behavior, because we send correct JSON payload, but in invalid format. There are no big difference between object and non-object payload, because user can also send object payload in invalid format. That's why I think current restriction is not very helpful. If user want to check body shape for another service, it should make it explicitly.

[sindresorhus]

@ikokostya You're the first one, from what I can remember, to request this. I have also never seen any API ever require you to send a top-level JSON serialized string.

The current validation is helpful in case users don't realize (or forget) we do the JSON stringifying for them. {json: true, body: JSON.stringify({foo: true})} would be double stringified and could cause hard to debug errors.

Even if your solution is more techincally correct, I prefer to be pragmatic and optimize for the common-case instead.

[alextes]

Yea, pretty much completely agree with Sindre now.
I think this answers the question. Our json option is just not for your use-case 😅. We've chosen a different but I think very tasteful trade-off. It creates a problem where there technically is none for you (false positive) but probably prevents a lot of incorrect use resulting in bugs for others (false negatives).

You could either:

1. Not use the option, set the correct header, stringify, and parse yourself.
2. Change the API from "str" to something like {"descriptiveKey": "str"}.

[clocked0ne]

@alextes The documentation does not make it clear for the Request body option that setting json: true will cause this behaviour, it should be documented that object is a valid type to provide when json: true is also present.

I had a thought that I would be caught out and tested using the Node REPL to see

TypeError: The body option must be an Object or Array when the json option is used

[alextes]

@clocked0ne currently our docs say:

json

Type: boolean
Default: false

If you use got.stream(), this option will be ignored.

If set to true and Content-Type header is not set, it will be set to application/json.

Parse response body with JSON.parse and set accept header to application/json. If used in conjunction with the form option, the body will the stringified as querystring and the response parsed as JSON.

body must be a plain object or array and will be stringified.

Note specifically the line:

body must be a plain object or array and will be stringified.

We can only code got to go with the most sensible default, and document exactly what that means in our case. You seem to dispute we have the second nailed down. I'm always happy to improve the documentation! How would you suggest we change it 🤔😉?

[clocked0ne]

@alextes I would suggest to add to the body section as well, so it is obvious the two properties are linked:

body

Type: string Buffer stream.Readable form-data instance

_If you provide this option, got.stream() will be read-only._

The body that will be sent with a POST request.

If present in options and options.method is not set, options.method will be set to POST.

The content-length header will be automatically set if body is a string / Buffer / fs.createReadStream instance / form-data instance, and content-length and transfer-encoding are not manually set in options.headers.

Add to the Type: line object or [object] and below this line a another italicized line:
_If you set options.json to true then body must be a plain object or array_

So altogether this could look like:

body

Type: string Buffer stream.Readable form-data instance [object]

_If you provide this option, got.stream() will be read-only._
_If you set options.json to true then body must be a plain object or array_

The body that will be sent with a POST request.

If present in options and options.method is not set, options.method will be set to POST.

The content-length header will be automatically set if body is a string / Buffer / fs.createReadStream instance / form-data instance, and content-length and transfer-encoding are not manually set in options.headers.

Then it is absolutely clear in either property what happens when both are used.

[alextes]

I'm against putting object or list of object as the type for body. That list should not depend on anything. I don't like the confusion there around the fact it _can_ be but again, json is just for convenience.

I'm not too opposed but still hesitant to the idea of copying the clarification to the body description 🤔. After all, the issue is with the json option, people not using that seem to be fine. Are you sure you think this would help?

We only put things in the documentation that we feel provide value. Doubling warnings, adding emphasis, or increasing fonts, has a cost and should be used sparingly in my opinion. You can pass a JSON string just fine, as long as you're not also using the json option. If you're using the json option but not reading what it does, I'm not sure there's much we can do. In short, are you sure doubling documentation is the helpful approach rather than users reading the documentation in the first place?

[clocked0ne]

I can only think that this was a a gotcha that could easily have been avoided, especially for someone migrating from another library such as request or axios. Properties should not impact each other in ways that are not documented fully with either property. An alternative might have been to not make json a Boolean and instead make it the actual JSON body to be used instead, thereby removing the need for any explanation around the body property, then you could have simply noted that body would be ignored in favour of json. You could then have put a real check in for an Object or String and only stringified the value if necessary, avoiding the problem sindresorhus described.

I would suggest either adding this clarification to the body so that people do not have to wait until they execute a call in this way to see the TypeError, or go further and make an additional body field for JSON, such as jsonBody so the two are no longer conflated (this could also behave as suggested above and ignore the body parameter). Under this circumstance If the body parameter was present but not jsonBody then the original behavour could resume, so this would be a non-breaking change.

Personally I'd rather just see the additional clarification in the body property documentation though. Given the prevalence of usage of JSON it is hardly a niche thing to support, I expect as many people use the json flag as do not.

If you won't do any of that, at least make:

body must be a plain object or array and will be stringified.

bold so it won't be so easy to miss.

[szmarczak]

An alternative might have been to not make json a Boolean and instead make it the actual JSON body to be used instead

Tip: do not make things ambiguous. This beautiful sentence solves your problem:

are you sure doubling documentation is the helpful rather than users reading the documentation in the first place?

Anyway, I don't mind a note in the README that the body is stringified (though it's obvious) :)

[alextes]

@szmarczak the sentence is not grammatically correct, but I'm glad you found beauty in it haha 😄 .

Alright. I remain quite unconvinced. Most users running into confusion around this option seem to because of their assumptions, not because of ambiguous documentation. That's understandable, but not helpful to try and compensate.

As always, anyone reading along hit by the same issue, please upvote the first comment. Currently, there are zero upvotes which makes me reluctant to change things. Still, I'm in a good mood, let me try to hit some middle ground 😁 .

[szmarczak]

671 is gonna solve this :)

[alextes]

@szmarczak hadn't seen that one yet! I think it's kinda beside the point but let's hope it solves it anyway 😁 !

[clocked0ne]

An alternative might have been to not make json a Boolean and instead make it the actual JSON body to be used instead

I don't see how that is ambiguous vs adding to the body property to say it accepts objects, which it does, but only if you read further down to where the json property is mentioned...

Maybe just link to the json property to reference it from the body property and it will probably be enough, I just don't understand the big defensive stance over the docs. Most developers write inadequate or terrible, assumptive documentation and this is actually quite good. I migrated from request because its bloated, then from axios because it is full of bugs and inconsistent, undocumented behaviour. I'm just trying to suggest a small change to benefit everyone and I really don't see the supposed detriment to making things clearer but maybe 671 solves it like you said anyway. This did make me laugh though:

Anyway, I don't mind a note in the README that the body is stringified _(though it's obvious)_ :)

Most users running into confusion around this option seem to because of their _assumptions_

I hope this doesn't come across as snarky, so happy holidays guys and thanks for the work on this package 🥂