nchaulet/node-geocoder

Fetch adapter should not call body.text() if body.json() throws an error

holly-cummins opened this issue · 1 comments

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 print the-text without trying to read the stream twice.

@martinpfannemueller your fix is not visible here? maybe you can check with my fix: #346 and vote here so that it gets merged?