search

what3words / w3w-node-wrapper

Node wrapper for the What3Words API
https://docs.what3words.com/api/v3/
MIT License
44 stars 10 forks source link

Added try/catch in JSON.parse #12

Closed StephanGeorg closed 7 years ago

StephanGeorg commented 7 years ago

It is necessary to wrap the JSON.parse in a try/catch. In some cases the endpoint answers w/ html, this will crash the library:

<html>
^
SyntaxError: Unexpected token <
    at Object.parse (native)
[import]    at Request._callback (/bundle/bundle/programs/server/npm/node_modules/w3w-node-wrapper/lib/W3W.Geocoder.js:282:38)
    at Request.self.callback (/bundle/bundle/programs/server/npm/node_modules/request/request.js:186:22)
    at emitTwo (events.js:87:13)
    at Request.emit (events.js:172:7)
StephanGeorg commented 7 years ago

I thing the problem is the nginx 404 site, which is returned because the endpoints looks down.

joe-jordan commented 7 years ago

Thanks for this, we're investigating (both the published wrappers and the API 404s!)

joe-jordan commented 7 years ago

Thank you for this contribution. We have now patched the API configuration so that we will always return JSON errors even in the rare cases that the endpoint is down. This should mean we don't need this patch (or to update any of our other wrappers).

Thanks again!

StephanGeorg commented 7 years ago

Thanks Joe. Ok, when you can guarantee that the response is always JSON, because in my case it was the nginx proxy which returned HTML. Wrapping the JSON.parse in a try/catch would be a additional protection.

vicchi commented 7 years ago

Hi @StephanGeorg, I think that conceptually this PR is entirely valid and has actually uncovered a logic error in lib/W3W.Geocoder.js.

At W3W.Geocoder.js#L279, I think the conditional should actually read as follows: 

if (params.format && (params.format.toLowerCase() !== 'json' || (params.format.toLowerCase() !== 'geojson')) {
...
}

This is because the what3words API supports JSON, GeoJSON and XML payload formats.

The only sticking point in all of this is what to return as the HTTP status code for the reject statement as hard-coding 500 make me go "errr" a little bit. I'm open to suggestions on this.

I'll re-open this PR ... do you want to re-push to include GeoJSON support and with a rework of the reject statement, even if you feel that leaving it as is is the right thing to do?

This PR was always really a response to the symptoms of a bug in the underlying API, rather than the solution to that bug. We've changed the back end configuration, as @joe-jordan mentioned, so that where a 4xxx or 5xxxx series error occurs, we can guarantee that the caller will get a JSON response and not the HTML version of Nginx's default error_page handlers.

StephanGeorg commented 7 years ago

The only sticking point in all of this is what to return as the HTTP status code for the reject statement as hard-coding 500 make me go "errr" a little bit. I'm open to suggestions on this.

Yes, you're totally right. But per definition I would say it is

500 Internal Server Error A generic error message, given when an unexpected condition was encountered and no more specific message is suitable.

Because you cannot say when this error is fired but my experience is that it is better to protect a JSON.parse in a try/catch.

tsamaya commented 7 years ago

Great stuff