JSON.parse(data) errors

[KrishnaPG]

It looks like the IPFS gateways return plain text errors, while the clients are expecting JSON, which is causing the json.parse to throw errors many times, (such as 404 not found, method not allowed etc.)

The problem is, while the json.parse line 27 throws the error it discards the original data, leaving caller have no clue why the parse failed.

For example, here is one error received on the client side returned by IPFS deamon: if you put a breakpoint and observe the data, it has some valuable information such as: argument "key" is required, which gets lost when the exception is thrown at line 27

The rethrown error at the call site contains stack with incorrect (and useless) error message:

"SyntaxError: Unexpected token a in JSON at position 0
    at JSON.parse (<anonymous>)
    at streamToValue (\node_modules\ipfs-http-client\src\utils\stream-to-json-value.js:25:18)
    at concat (\node_modules\ipfs-http-client\src\utils\stream-to-value.js:12:22)
    at ConcatStream.<anonymous> (\node_modules\concat-stream\index.js:32:43)
    at ConcatStream.emit (events.js:187:15)
    at finishMaybe (\node_modules\readable-stream\lib\_stream_writable.js:620:14)
    at afterWrite (\node_modules\readable-stream\lib\_stream_writable.js:466:3)
    at process._tickCallback (internal/process/next_tick.js:63:19)"

Suggestion:

1. If clients are expecting JSON data, the gateways should return errors in JSON format, Else
2. on the client side, e.g. at line 27, please include the original data received from the server in the error thrown so that callers get to know what the problem is, something like:

    try {
      res = JSON.parse(data)
    } catch (err) {
      return cb(new Error(data))   // <---- throw the data part of the error
    }

or something like return cb(new Error("Invalid JSON: ${data}"))

For beginners, being able to understand the server errors is very important, since it is not always possible get things right the first time.

[alanshaw]

@KrishnaPG would you be willing to submit a PR for the return cb(new Error("Invalid JSON: ${data}")) solution?

This would also resolve #912

[KrishnaPG]

Thank you. I am not sure if invalid json should be part of the error message. For example, here is what the error looks like without and with invalid json in the error string. I am placing the PR for both, please choose whichever you prefer and reject the other one. (IMO, the one without invalid json seems more accurate and less confusing, though I would defer the decision to you).

Without invalid json: (PR: #1002)

Error: ipfs method not allowed
    at streamToValue ( \node_modules\ipfs-http-client\src\utils\stream-to-json-value.js:27:17)
    at concat ( \node_modules\ipfs-http-client\src\utils\stream-to-value.js:12:22)
    at ConcatStream.<anonymous> ( \node_modules\concat-stream\index.js:32:43)
    at ConcatStream.emit (events.js:187:15)
    at finishMaybe ( \node_modules\readable-stream\lib\_stream_writable.js:620:14)
    at afterWrite ( \node_modules\readable-stream\lib\_stream_writable.js:466:3)
    at process._tickCallback (internal/process/next_tick.js:63:19)

With invalid json: (PR: #1001)

 Error: Invalid JSON: ipfs method not allowed
    at streamToValue ( \node_modules\ipfs-http-client\src\utils\stream-to-json-value.js:27:17)
    at concat ( \node_modules\ipfs-http-client\src\utils\stream-to-value.js:12:22)
    at ConcatStream.<anonymous> ( \node_modules\concat-stream\index.js:32:43)
    at ConcatStream.emit (events.js:187:15)
    at finishMaybe ( \node_modules\readable-stream\lib\_stream_writable.js:620:14)
    at afterWrite ( \node_modules\readable-stream\lib\_stream_writable.js:466:3)
    at process._tickCallback (internal/process/next_tick.js:63:19)

The code that produced the above errors:

const ipfsClient = require('ipfs-http-client');
const cid= "zdpuAxp26mzjJNVHLi1PB9hAJBc2bTJXAAeqYqy36WiNbc6TC";
const ipfs = new ipfsClient({ host: 'ipfs.infura.io', port: '5001', protocol: 'https' });

  ipfs.dht.findProvs(cid, { timeout:10000, maxNumProviders: 1 }, (err, peerInfos) => {
    console.log(err);
  })

[alanshaw]

Good point, I think we should iterate on #1001. "Invalid JSON" is the correct message here and having the data in the message is useful. The problem is with the error handling code:

https://github.com/ipfs/js-ipfs-http-client/blob/84117a136d05d9eb6a43e42e2a7e3dc52635efb9/src/utils/send-request.js#L16-L31

It uses streamToJsonValue but the server doesn't always return JSON.

We should pass isJson to parseError:

https://github.com/ipfs/js-ipfs-http-client/blob/84117a136d05d9eb6a43e42e2a7e3dc52635efb9/src/utils/send-request.js#L37-L38

https://github.com/ipfs/js-ipfs-http-client/blob/84117a136d05d9eb6a43e42e2a7e3dc52635efb9/src/utils/send-request.js#L47

In parseError we should use streamToJsonValue if isJson is true, otherwise it should just use streamToValue and use that value as the error message.

Does that make sense?

[KrishnaPG]

Interesting. So the whole thing is already inside an error handling mechanism (parseError). Let me look into it. If a mechanism already exists to identify the JSONness of response, we should be able to use it properly. Will get back on this after reviewing the code in more depth.

[KrishnaPG]

Added the isJSON check in the parseError method. This now generates error as below:

Error: ipfs method not allowed
    at parseError ( \node_modules\ipfs-http-client\src\utils\send-request.js:17:17)
    at ClientRequest.<anonymous> ( \node_modules\ipfs-http-client\src\utils\send-request.js:59:14)
    at Object.onceWrapper (events.js:273:13)
    at ClientRequest.emit (events.js:182:13)
    at HTTPParser.parserOnIncomingClient (_http_client.js:555:21)
    at HTTPParser.parserOnHeadersComplete (_http_common.js:109:17)
    at TLSSocket.socketOnData (_http_client.js:441:20)
    at TLSSocket.emit (events.js:182:13)
    at addChunk (_stream_readable.js:283:12)
    at readableAddChunk (_stream_readable.js:264:11)

In case, there is no valid textual message from the stream, the above error will look as below:

Error: Server responded with 403
    at parseError ( \node_modules\ipfs-http-client\src\utils\send-request.js:17:17)
    at ClientRequest.<anonymous> ( \node_modules\ipfs-http-client\src\utils\send-request.js:59:14)
    at Object.onceWrapper (events.js:273:13)
    at ClientRequest.emit (events.js:182:13)
    at HTTPParser.parserOnIncomingClient (_http_client.js:555:21)
    at HTTPParser.parserOnHeadersComplete (_http_common.js:109:17)
    at TLSSocket.socketOnData (_http_client.js:441:20)
    at TLSSocket.emit (events.js:182:13)
    at addChunk (_stream_readable.js:283:12)
    at readableAddChunk (_stream_readable.js:264:11)

And in case of the JSON parsing errors, the error will continue to have the format: Invalid JSON: ${data}

Looks like this covers the JSON parsing error with data + non-json error messages scenarios.

Combined both commits into the same PR: #1001