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

Parse empty response bodies without throwing #415

Closed
ionutcirja opened this issue Jan 4, 2022 · 12 comments · Fixed by #459
Closed

Parse empty response bodies without throwing #415

ionutcirja opened this issue Jan 4, 2022 · 12 comments · Fixed by #459
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@ionutcirja
Copy link

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
Copy link
Author

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

@ionutcirja
Copy link
Author

PR created: #416

@szmarczak
Copy link
Collaborator

szmarczak commented Jan 4, 2022

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.

@ionutcirja
Copy link
Author

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?

@szmarczak
Copy link
Collaborator

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

@sholladay
Copy link
Collaborator

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

@szmarczak
Copy link
Collaborator

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

@sholladay
Copy link
Collaborator

sholladay commented Jan 7, 2022

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.

@kazzkiq
Copy link

kazzkiq commented Mar 15, 2022

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.

@sholladay sholladay added enhancement New feature or request help wanted Extra attention is needed labels Mar 22, 2022
@sholladay sholladay changed the title 202 Accepted: Unexpected end of JSON input Allow an empty response body with various 2xx status codes Mar 22, 2022
@sholladay sholladay changed the title Allow an empty response body with various 2xx status codes Parse empty response bodies without throwing Mar 22, 2022
@joaopedrocampos
Copy link

joaopedrocampos commented Jul 11, 2022

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

@Moshyfawn
Copy link

Moshyfawn commented Jul 29, 2022

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>)

@sholladay
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants