ky icon indicating copy to clipboard operation
ky copied to clipboard

Parse empty response bodies without throwing

Open ionutcirja opened this issue 4 years ago • 12 comments

In case of an 202 there is no way for the HTTP to later send an asynchronous response indicating the outcome of processing the request. When trying to call the json method we will get Unexpected end of JSON input.

ionutcirja avatar Jan 04 '22 20:01 ionutcirja

I will try to fix it and raise a PR asap.

ionutcirja avatar Jan 04 '22 20:01 ionutcirja

PR created: https://github.com/sindresorhus/ky/pull/416

ionutcirja avatar Jan 04 '22 20:01 ionutcirja

202 may have a response body[1]. An empty string is not valid JSON.

[1] https://datatracker.ietf.org/doc/html/rfc7231#section-6.3.3

The representation sent with this response ought to describe the request's current status and point to (or embed) a status monitor that can provide the user with an estimate of when the request will be fulfilled.

szmarczak avatar Jan 04 '22 22:01 szmarczak

202 may have a response body[1]. An empty string is not valid JSON.

[1] https://datatracker.ietf.org/doc/html/rfc7231#section-6.3.3

The representation sent with this response ought to describe the request's current status and point to (or embed) a status monitor that can provide the user with an estimate of when the request will be fulfilled.

Ok, I’m in that situation when 202 doesn’t have a response. What do you think it’s the solution as the library crashes right now?

ionutcirja avatar Jan 04 '22 23:01 ionutcirja

You can either try/catch but a better solution would be to manually parse this.

szmarczak avatar Jan 05 '22 12:01 szmarczak

We already special case 204 responses. Should probably do the same here for consistency, IMO.

sholladay avatar Jan 07 '22 05:01 sholladay

Then maybe let's do so for all content-length: 0?

szmarczak avatar Jan 07 '22 12:01 szmarczak

Yeah, I guess that's fair. I was thinking in terms of whether it's reasonable for a given status code to have an empty body. But I'm sure there are plenty of implementations out there that return an empty body with a 200 or other success codes. Maybe we should just allow that for convenience sake.

sholladay avatar Jan 07 '22 18:01 sholladay

Maybe we should just allow that for convenience sake.

@sholladay That should definitely be allowed and handled inside ky.

For instance 201 and 204 are other examples of pretty common HTTP statuses that generally do not return body content. Today ky throws whenever you .json() and one of those are returned.

kazzkiq avatar Mar 15 '22 03:03 kazzkiq

Hello guys!

Would love to work on this issue if possible, but I just wanted to confirm if I understood correctly: now we expect that, for any success status code (2xx), if the response body is empty ("invalid") when .json() gets called, instead of throwing an error we should reply and empty body ({}) and add the header content-length: 0 on the response.

Is that the expected behavior? 👀

joaopedrocampos avatar Jul 11 '22 22:07 joaopedrocampos

Hello guys!

Would love to work on this issue if possible, but I just wanted to confirm if I understood correctly: now we expect that, for any success status code (2xx), if the response body is empty ("invalid") when .json() gets called, instead of throwing an error we should reply and empty body ({}) and add the header content-length: 0 on the response.

Is that the expected behavior? 👀

I believe, the idea is to check the Content-Length header and if it's 0, than don't throw.

Also, not sure about an {} <empty object> for an empty response body. I'd expect it to be undefined instead (maybe "" <empty string>)

Moshyfawn avatar Jul 29 '22 00:07 Moshyfawn

When Content-Length is 0 or is missing, body must be returned as an empty string.

Let's not convert empty objects to undefined, that is too opinionated for Ky. Probably a good idea for a hook, though, which we could mention in the documentation.

sholladay avatar Aug 26 '22 16:08 sholladay