fix: error reporting for non-JSON responses (#1016)

Better error reporting by detecting `Content-Type` of the response and not attempting to parse JSON for non-`application/json` responses.

resolves #912
resolves #1000
closes #1001

License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>

Author: Alan Shaw

Diff for: src/utils/send-request.js

@@ -13,8 +13,26 @@ const log = require('debug')('ipfs-http-client:request')
 
 // -- Internal
 
+function hasJSONHeaders (res) {
+  return res.headers['content-type'] &&
+    res.headers['content-type'].indexOf('application/json') === 0
+}
+
 function parseError (res, cb) {
   const error = new Error(`Server responded with ${res.statusCode}`)
+  error.statusCode = res.statusCode
+
+  if (!hasJSONHeaders(res)) {
+    return streamToValue(res, (err, data) => { // eslint-disable-line handle-callback-err
+      // the `err` here refers to errors in stream processing, which
+      // we ignore here, since we already have a valid `error` response
+      // from the server above that we have to report to the caller.
+      if (data && data.length) {
+        error.message = data.toString()
+      }
+      cb(error)
+    })
+  }
 
   streamToJsonValue(res, (err, payload) => {
     if (err) {
@@ -34,8 +52,7 @@ function onRes (buffer, cb) {
   return (res) => {
     const stream = Boolean(res.headers['x-stream-output'])
     const chunkedObjects = Boolean(res.headers['x-chunked-output'])
-    const isJson = res.headers['content-type'] &&
-                   res.headers['content-type'].indexOf('application/json') === 0
+    const isJson = hasJSONHeaders(res)
 
     if (res.req) {
       log(res.req.method, `${res.req.getHeaders().host}${res.req.path}`, res.statusCode, res.statusMessage)

Diff for: src/utils/stream-to-json-value.js

@@ -24,7 +24,7 @@ function streamToJsonValue (res, cb) {
     try {
       res = JSON.parse(data)
     } catch (err) {
-      return cb(err)
+      return cb(new Error(`Invalid JSON: ${data}`))
     }
 
     cb(null, res)

Diff for: test/request-api.spec.js

@@ -65,3 +65,77 @@ describe('trailer headers', () => {
     })
   })
 })
+
+describe('error handling', () => {
+  it('should handle plain text error response', function (done) {
+    if (!isNode) return this.skip()
+
+    const server = require('http').createServer((req, res) => {
+      // Consume the entire request, before responding.
+      req.on('data', () => {})
+      req.on('end', () => {
+        // Write a text/plain response with a 403 (forbidden) status
+        res.writeHead(403, { 'Content-Type': 'text/plain' })
+        res.write('ipfs method not allowed')
+        res.end()
+      })
+    })
+
+    server.listen(6001, () => {
+      ipfsClient('/ip4/127.0.0.1/tcp/6001').config.replace('test/fixtures/r-config.json', (err) => {
+        expect(err).to.exist()
+        expect(err.statusCode).to.equal(403)
+        expect(err.message).to.equal('ipfs method not allowed')
+        server.close(done)
+      })
+    })
+  })
+
+  it('should handle JSON error response', function (done) {
+    if (!isNode) return this.skip()
+
+    const server = require('http').createServer((req, res) => {
+      // Consume the entire request, before responding.
+      req.on('data', () => {})
+      req.on('end', () => {
+        // Write a application/json response with a 400 (bad request) header
+        res.writeHead(400, { 'Content-Type': 'application/json' })
+        res.write(JSON.stringify({ Message: 'client error', Code: 1 }))
+        res.end()
+      })
+    })
+
+    server.listen(6001, () => {
+      ipfsClient('/ip4/127.0.0.1/tcp/6001').config.replace('test/fixtures/r-config.json', (err) => {
+        expect(err).to.exist()
+        expect(err.statusCode).to.equal(400)
+        expect(err.message).to.equal('client error')
+        expect(err.code).to.equal(1)
+        server.close(done)
+      })
+    })
+  })
+
+  it('should handle JSON error response with invalid JSON', function (done) {
+    if (!isNode) return this.skip()
+
+    const server = require('http').createServer((req, res) => {
+      // Consume the entire request, before responding.
+      req.on('data', () => {})
+      req.on('end', () => {
+        // Write a application/json response with a 400 (bad request) header
+        res.writeHead(400, { 'Content-Type': 'application/json' })
+        res.write('{ Message: ')
+        res.end()
+      })
+    })
+
+    server.listen(6001, () => {
+      ipfsClient('/ip4/127.0.0.1/tcp/6001').config.replace('test/fixtures/r-config.json', (err) => {
+        expect(err).to.exist()
+        expect(err.message).to.include('Invalid JSON')
+        server.close(done)
+      })
+    })
+  })
+})