Bitcoin-com / rest.bitcoin.com

Bitcoin.com's Cloud's REST API
https://rest.bitcoin.com
MIT License
19 stars 26 forks source link

Errors need to be thrown, but returned as a string. #496

Closed christroutner closed 4 years ago

christroutner commented 5 years ago

Chris Troutner:

Would love to see some GitHub issues with examples of the error strings you're seeing. I know we're not handling all our errors as per best practice/standards. I want to fix that.

Andreas Brekken: 2019-07-31T03:41:47.029419+00:00 app[worker.1]: ERROR Error not instance of Error class { error: [32m'258: txn-mempool-conflict' [39m }

The outer error is my wrapping, so the error thrown is the string [32m'258: txn-mempool-conflict' [39m

Chris Troutner: So that error is coming from the full node and is being passed through rest.

But your saying, the error is not being thrown? It's not activating a try/catch statement?

That's something that needs to be fixed.

christroutner commented 4 years ago

This is a long-running issue. We need to evaluate the error output of rest with Postman to understand what the best-practice is, what we're doing, where/if any conflict lives, and figure out how to fix it.

christroutner commented 4 years ago

When rest throws an error, it returns a 4xx or 5xx http status code, and an object that looks like this:

{
    "error": "addresses needs to be an array. Use GET for single address."
}

Here is a great article comparing the error response to common large websites like Facebook and others: https://blog.restcase.com/rest-api-error-codes-101/

To summarize that article:

christroutner commented 4 years ago

Here is another article on best practices for throwing errors in a REST API https://nordicapis.com/best-practices-api-error-handling/

I personally like this format for returning errors. It's very similar to the current implementation. However, by adding the http status code to the body returned by the API, it gives downstream developers two ways to read and react to errors.

{
  "error" : {
    "status" : 502,
    "message" : "Bad gateway."
  }
}
christroutner commented 4 years ago

This RFC7807 contains a formal approach to returning error information in the body of a REST API error response.

christroutner commented 4 years ago

Closing this issue as the scope of it was research-based in scope and I think it's done a good job of capturing the nuance and a clear action item for moving forward. Issue #543 captures the action item, which the team can either prioritize or reject.