Regression in error responses from /api/v0/files/rm

[lidel]

When one causes an error, eg. by attempting to remove an MFS file that does not exist, the error response returned by go-ipfs 0.11.0 is not a valid JSON.

Problem (0.11.0)

Error response in 0.11.0 is plain text:

> curl -X POST -vvv 'http://127.0.0.1:5001/api/v0/files/rm?arg=/foo/keks'                                                                                                                                          ~
*   Trying 127.0.0.1:5001...
* Connected to 127.0.0.1 (127.0.0.1) port 5001 (#0)
> POST /api/v0/files/rm?arg=/foo/keks HTTP/1.1
> Host: 127.0.0.1:5001
> User-Agent: curl/7.79.1
> Accept: */*
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Access-Control-Allow-Headers: X-Stream-Output, X-Chunked-Output, X-Content-Length
< Access-Control-Expose-Headers: X-Stream-Output, X-Chunked-Output, X-Content-Length
< Content-Type: application/json
< Server: go-ipfs/0.11.0
< Trailer: X-Stream-Error
< Vary: Origin
< X-Chunked-Output: 1
< Date: Tue, 14 Dec 2021 01:12:02 GMT
< Transfer-Encoding: chunked
<
"/foo/keks: parent lookup: file does not exist"
* Connection #0 to host 127.0.0.1 left intact

Before (0.10.0)

0.10.0 had correct error response (JSON):

> curl -X POST -vvv 'http://127.0.0.1:5001/api/v0/files/rm?arg=/foo/keks'                                                                                                                                          ~
*   Trying 127.0.0.1:5001...
* Connected to 127.0.0.1 (127.0.0.1) port 5001 (#0)
> POST /api/v0/files/rm?arg=/foo/keks HTTP/1.1
> Host: 127.0.0.1:5001
> User-Agent: curl/7.79.1
> Accept: */*
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 500 Internal Server Error
< Access-Control-Allow-Headers: X-Stream-Output, X-Chunked-Output, X-Content-Length
< Access-Control-Expose-Headers: X-Stream-Output, X-Chunked-Output, X-Content-Length
< Content-Type: application/json
< Server: go-ipfs/0.10.0
< Trailer: X-Stream-Error
< Vary: Origin
< Date: Tue, 14 Dec 2021 01:07:30 GMT
< Transfer-Encoding: chunked
<
{"Message":"parent lookup: file does not exist","Code":0,"Type":"error"}
* Connection #0 to host 127.0.0.1 left intact

[aschmahmann]

@petar this is likely due to https://github.com/ipfs/go-ipfs/pull/8449 and the way it chose to deal with multiple errors. If you could take a look at a fix here when you get a chance that'd be great 🙏.

@lidel IIUC this regression was detected in some CI, but didn't trigger in go-ipfs letting us know we'd broken something. Where did things fall through the cracks here? While we could add specific tests for this kind of regression, I'd suspect these would likely come from HTTP Client tests (e.g. interop or js-ipfs-http-client).

We may also need to add more docs to go-ipfs-cmds explaining how to use it so we're less likely to run into these sorts of problems as we ramp up newer contributors.

[lidel]

@aschmahmann we simply don't test response format in go-ipfs sharness :upside_down_face:

We only test "positive paths" in sharness and when we do, we just grep strings. We only have two "error path" tests where we only check exit code and as long its 1 on error we are passing (first, second).

In most places, we have no tests for the JSON error response format. This was caught in js-ipfs-http-client tests (https://github.com/ipfs/js-ipfs/pull/3922) where various error responses are covered (JS lib needs to translate them to correct subtype of Error object, so we have pretty good coverage there).

Proposed rule of thumb for testing HTTP RPC API repsonses

We should start checking error messages.

Grepping if error string is present in the output is not enough:

ipfs files rm --enc=json /foo/keks  | grep "file does not exist"

We should replace those naive checks with jq which will ensure valid JSON is returned, aiming at something like:

$ ipfs files rm --enc=json /foo/keks 
{"Message":"parent lookup: file does not exist","Code":0,"Type":"error"}

$ ipfs files rm --enc=json /foo/keks | jq .Message 
"parent lookup: file does not exist"

Caveat: many of our commands have --enc=json broken at the moment (https://github.com/ipfs/go-ipfs/issues/7050), including files rm :skull: :upside_down_face: and we need to fix that first, or use curl for verifying JSON responses.

[aschmahmann]

This was caught in js-ipfs-http-client tests

go-ipfs CI already runs the go-ipfs-http-client and interop tests, should we add in the js-ipfs-http-client tests as well?

Caveat: many of our commands have --enc=json broken at the moment

Yep, ideally we should be testing the outputs from all the relevant output types. Although given the primary consumers of the JSON format are HTTP clients that already have tests, it might be a bigger payoff to get those running in go-ipfs CI.

[lidel]

go-ipfs CI already runs the go-ipfs-http-client and interop tests, should we add in the js-ipfs-http-client tests as well?

We could do that, yes. What we want to run is npm run test:interface:http-go in https://github.com/ipfs/js-ipfs/tree/master/packages/ipfs-http-client, but against a custom go-ipfs, not the release version from package.json

[gammazero]

What should the error output look like when there are multiple errors?

Multiple json messages:

{"Message":"parent lookup: someFile1: file does not exist","Code":0,"Type":"error"}

{"Message":"parent lookup: someFile2: file does not exist","Code":0,"Type":"error"}

Multiple errors in json list:

[
  {"Message":"parent lookup: someFile1: file does not exist","Code":0,"Type":"error"},
  {"Message":"parent lookup: someFile2: file does not exist","Code":0,"Type":"error"}
]

Only one error, stopping when first error occurs:

{"Message":"parent lookup: someFile1: file does not exist","Code":0,"Type":"error"},

[lidel]

Hm.. files rm was designed with CLI in mind, and good CLI tools are usually aborting on first error. Print first error and exit with code other than 0 – should be enough.