node-geocoder icon indicating copy to clipboard operation
node-geocoder copied to clipboard

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

Open holly-cummins opened this issue 3 years ago • 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 https://github.com/node-fetch/node-fetch/issues/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.

holly-cummins avatar Jun 08 '22 21:06 holly-cummins

@martinpfannemueller your fix is not visible here? maybe you can check with my fix: https://github.com/nchaulet/node-geocoder/pull/346 and vote here so that it gets merged?

Ephigenia avatar May 22 '23 07:05 Ephigenia