fix(gateway): return 500 for all /ip[nf]s/id failures #187
Conversation
| @@ -352,9 +352,11 @@ func TestGatewayGet(t *testing.T) { | |||
| }{ | |||
| {"127.0.0.1:8080", "/", http.StatusNotFound, "404 page not found\n"}, | |||
| {"127.0.0.1:8080", "/" + k.Cid().String(), http.StatusNotFound, "404 page not found\n"}, | |||
| {"127.0.0.1:8080", "/ipfs/this-is-not-a-cid", http.StatusInternalServerError, "failed to resolve /ipfs/this-is-not-a-cid: invalid path \"/ipfs/this-is-not-a-cid\": invalid CID: illegal base32 data at input byte 3\n"}, | |||
There was a problem hiding this comment.
I really don't like this tests where we compare the body for errors. Error information may change slightly depending on the backing implementation. I think rewriting these tests can be part of #156 or even #183.
Yes, we should compare the body for files we are expecting I think, and stuff like custom 404s, but not necessarily for errors. As long as the status is correct, the test should pass. Similarly to TestGatewayInternalServerErrorInvalidPath.
There was a problem hiding this comment.
@hacdias these don't do much for UX, but also we dont expect these messages to change often.
Once set in stone, make good canaries to hint someone in transitive dependencies messed up refactor :)
I would keep this, at least until go-libipfs/gateway and rhea work is in progress – better to have more tests than less.
I remember in the past an external library (like go-cid or go-multihash/base, don't remember which one) changed something, and we detected it in Kubo, 2 layers later, because a specific error message changed to a generic one.
ps. At some point we will add more friendly HTML error responses if a request was made with Accept that includes text/html (web browser), like we did for dag-cbor.
Codecov Report
@@ Coverage Diff @@
## main #187 +/- ##
==========================================
- Coverage 29.75% 29.70% -0.06%
==========================================
Files 100 100
Lines 11267 11263 -4
==========================================
- Hits 3353 3346 -7
- Misses 7548 7550 +2
- Partials 366 367 +1
|
hacdias
changed the title
| errors.Is(err, routing.ErrNotFound) || | ||
| err == routing.ErrNotFound || |
There was a problem hiding this comment.
💭 👍 for removing these, it looked like a dead code that could cause hard-to-debug bugs in the futture.
Closes #185.