Fetch adapter should not call body.text() if body.json() throws an error
holly-cummins opened this issue · 1 comments
holly-cummins commented
If my geo data provider fails to give a healthy response, the error handling is not correct. I'm seeing errors of the following form
Error getting geography information for San Francisco, CA, USA
Error: HttpError: body used already for: https://nominatim.openstreetmap.org/search?addressdetails=1&q=San+Francisco%2C+CA%2C+USA&format=json
at /home/styff/platform/node_modules/node-geocoder/lib/httpadapter/fetchadapter.js:58:15
at tryCatcher (/home/stuff/platform/node_modules/bluebird/js/release/util.js:16:23)
This seems to be the same as node-fetch/node-fetch#533. The cause is this block in https://github.com/nchaulet/node-geocoder/blob/master/lib/httpadapter/fetchadapter.js:
try {
return await res.json();
} catch (e) {
throw new HttpError(await res.text(), {
code: res.statusCode
});
}
Because the response is a stream, calling res.json()
and then res.text()
is not valid. The recommended solution seems to be either:
- pre-check the response's status code before calling
res.json()
to reduce the likelihood of error - just call
res.text()
so there's always a response assigned to a variable, and then call JSON.parse(the-text
) and wrap that call in a try-catch. If the try-catch fails, then you can printthe-text
without trying to read the stream twice.
Ephigenia commented
@martinpfannemueller your fix is not visible here? maybe you can check with my fix: #346 and vote here so that it gets merged?