From d7e4fd6f38d7d6e050840264c5f835da95d681cb Mon Sep 17 00:00:00 2001 From: Gopalakrishna Palem Date: Thu, 16 May 2019 18:58:48 +0530 Subject: [PATCH 1/8] Update stream-to-json-value.js Better error reporting in case of `json.parse` failures --- src/utils/stream-to-json-value.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/stream-to-json-value.js b/src/utils/stream-to-json-value.js index e42de2fc6..2ae83e50d 100644 --- a/src/utils/stream-to-json-value.js +++ b/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) From 4c2405f7d670ba65d1fe86a1ae81a74c3e815eee Mon Sep 17 00:00:00 2001 From: Gopalakrishna Palem Date: Thu, 16 May 2019 21:33:05 +0530 Subject: [PATCH 2/8] parseError taking non-JSON into account Making the `parseError` function compatible with non-JSON responses from server. - `isJson = true` parameter added that is backwards compatible with existing code Ref: https://github.com/ipfs/js-ipfs-http-client/issues/1000#issuecomment-493087784 --- src/utils/send-request.js | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/utils/send-request.js b/src/utils/send-request.js index 219327adf..82e7aee7c 100644 --- a/src/utils/send-request.js +++ b/src/utils/send-request.js @@ -13,9 +13,21 @@ const log = require('debug')('ipfs-http-client:request') // -- Internal -function parseError (res, cb) { +function parseError (res, cb, isJson = true) { const error = new Error(`Server responded with ${res.statusCode}`) + if (!isJson) { + return streamToValue(res, (err, data) => { + // 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) { + error.message = data.toString() + } + cb(error) + }) + } + streamToJsonValue(res, (err, payload) => { if (err) { return cb(err) @@ -44,7 +56,7 @@ function onRes (buffer, cb) { } if (res.statusCode >= 400 || !res.statusCode) { - return parseError(res, cb) + return parseError(res, cb, isJson) } // Return the response stream directly From f914a2730c78be732f0710ac8d811e24e0a1bb6d Mon Sep 17 00:00:00 2001 From: Gopalakrishna Palem Date: Fri, 17 May 2019 04:49:38 +0530 Subject: [PATCH 3/8] data should be non-empty (in addition to being non-null) `parseError` method should print default error in case data is null or empty ("") Co-Authored-By: Alan Shaw --- src/utils/send-request.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/send-request.js b/src/utils/send-request.js index 82e7aee7c..8fe5a6f90 100644 --- a/src/utils/send-request.js +++ b/src/utils/send-request.js @@ -21,7 +21,7 @@ function parseError (res, cb, isJson = true) { // 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) { + if (data && data.length) { error.message = data.toString() } cb(error) From f8ab9bb8d863d4cd492d42ebc2b3bcc4dfaa1f55 Mon Sep 17 00:00:00 2001 From: Gopalakrishna Palem Date: Fri, 17 May 2019 19:45:11 +0530 Subject: [PATCH 4/8] Moved isJson identification into a function This makes `parseError` more self-sufficient. --- src/utils/send-request.js | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/utils/send-request.js b/src/utils/send-request.js index 8fe5a6f90..a3a6afafc 100644 --- a/src/utils/send-request.js +++ b/src/utils/send-request.js @@ -13,10 +13,15 @@ const log = require('debug')('ipfs-http-client:request') // -- Internal -function parseError (res, cb, isJson = true) { +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}`) - if (!isJson) { + if (!hasJSONHeaders(res)) { return streamToValue(res, (err, data) => { // the `err` here refers to errors in stream processing, which // we ignore here, since we already have a valid `error` response @@ -46,8 +51,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) @@ -56,7 +60,7 @@ function onRes (buffer, cb) { } if (res.statusCode >= 400 || !res.statusCode) { - return parseError(res, cb, isJson) + return parseError(res, cb) } // Return the response stream directly From dbed72e854fe1115c75940f608723391155914fe Mon Sep 17 00:00:00 2001 From: Gopalakrishna Palem Date: Fri, 17 May 2019 19:55:34 +0530 Subject: [PATCH 5/8] removing trailing-space as per the lint 35:1 error Trailing spaces not allowed no-trailing-spaces --- src/utils/send-request.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/send-request.js b/src/utils/send-request.js index a3a6afafc..20212f815 100644 --- a/src/utils/send-request.js +++ b/src/utils/send-request.js @@ -32,7 +32,7 @@ function parseError (res, cb) { cb(error) }) } - + streamToJsonValue(res, (err, payload) => { if (err) { return cb(err) From 1419fbe9555b4a7ff1e196e2e20fc136ba683962 Mon Sep 17 00:00:00 2001 From: Gopalakrishna Palem Date: Tue, 21 May 2019 16:47:29 +0530 Subject: [PATCH 6/8] adding status code to the error This is needed for the callers to know when the error needs a retry (such as 401, 403 etc. --- src/utils/send-request.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/utils/send-request.js b/src/utils/send-request.js index 20212f815..b9f850561 100644 --- a/src/utils/send-request.js +++ b/src/utils/send-request.js @@ -29,6 +29,7 @@ function parseError (res, cb) { if (data && data.length) { error.message = data.toString() } + error.code = res.statusCode cb(error) }) } From 18edde2a8f117f8f228f130fc5c8b717f37985a3 Mon Sep 17 00:00:00 2001 From: Alan Shaw Date: Tue, 21 May 2019 21:26:39 +0100 Subject: [PATCH 7/8] feat: always add status code to error message Uses `statusCode` since `code` is being used in JSON responses. License: MIT Signed-off-by: Alan Shaw --- src/utils/send-request.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/utils/send-request.js b/src/utils/send-request.js index b9f850561..1645969f2 100644 --- a/src/utils/send-request.js +++ b/src/utils/send-request.js @@ -20,16 +20,16 @@ function hasJSONHeaders (res) { 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) => { + 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() } - error.code = res.statusCode cb(error) }) } From 333ca9f494bed72b8206277f47a9117f80378240 Mon Sep 17 00:00:00 2001 From: Alan Shaw Date: Tue, 21 May 2019 21:28:12 +0100 Subject: [PATCH 8/8] test: add tests for error handling License: MIT Signed-off-by: Alan Shaw --- test/request-api.spec.js | 74 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/test/request-api.spec.js b/test/request-api.spec.js index 02f342e53..a9b2fcf2e 100644 --- a/test/request-api.spec.js +++ b/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) + }) + }) + }) +})