restify / clients

HttpClient, StringClient, and JsonClient extracted from restify
MIT License
57 stars 34 forks source link

Handling JSON parsing errors in JSONClient #147

Closed DonutEspresso closed 6 years ago

DonutEspresso commented 6 years ago

I have a proposal for better JSONClient ergonomics. Currently, the client swallows potential JSON.parse errors, then creates an empty object and sets that as the parsed result: https://github.com/restify/clients/blob/master/lib/JsonClient.js#L65

This is confusing and can be very misleading. JSONClient should do best effort parsing on incoming payloads. Seeing that the JSON RFC has been expanded to include strings as valid JSON, i.e., '"hello"', I propose the following:

1) If the client fails to parse the payload it should return a sensible parse error with useful information 2) If there is an HTTP error, that error trumps the parse error. Alternatively, we could return a MultiError - open to discussion on that. 2) If the client fails to parse the payload, it should return the original string payload instead of an empty object

These proposals would address #138 and #140 for more comprehensive JSON handling.

This breaks:

Thoughts? cc @yunong @rajatkumar @retrohacker @hekike

hekike commented 6 years ago

Just for the record, based on the PR we don't do item 3. However, I think it would be a good idea to add the original string payload to the error.

DonutEspresso commented 6 years ago

Good call, missed a commit locally, just updated PR.