search

microsoft / typed-rest-client

Node Rest and Http Clients with typings for use with TypeScript
Other
675 stars 118 forks source link

Failing to read error message #123

Closed mmajcica closed 5 years ago

mmajcica commented 5 years ago

At the following line, in case of, let's say a 400 response that returns a body that is not JSON content

https://github.com/Microsoft/typed-rest-client/blob/17af59a9fdd8fcc1ed8e60064c46df20bcce6f52/lib/RestClient.ts#L221

the code will fail. This means that no matter which responseProcessor function we specify, it will never be reached. A better approach would be to just wrap that line in something like this:

try {
 obj = JSON.parse(contents);
 } catch (e) {
 }

This will allow us to specify a custom responseProcessor function which then will bring the simple error string in the exception.

If you agree, I'll submit a PR.

stephenmichaelf commented 5 years ago

Agreed this seems like a bug.

I am not sure we need to provide the option to allow a custom processor function. It will be either JSON or a string, right? We could try both then fail.

mmajcica commented 5 years ago

That's right. I'll give it a try.

mmajcica commented 5 years ago

In is not trully a bug, but it is expecting the body to always be a JSON response. Unfortunatelly, in this non perfect world, we have API's that do replay JSON objects in case all goes well, however, in case of the exceptions the do just replay the error message as a string in the body.

What I would suggest is to expect response with status code minor then 300 to be a JSON. Meanwhile in case of errors to pick up the consider also strings as JSON in the response.

Does this sound resonable?

mmajcica commented 5 years ago

In this way we do not break compatibility and add the possiblity to accept the error messages as strings.

damccorm commented 5 years ago

Closing since this was resolved by #124