trailblazer / trailblazer-endpoint

Generic HTTP endpoints to deal with different results from your operations.
23 stars 19 forks source link

Should we implement error handling for common errors. #5

Open konung opened 7 years ago

konung commented 7 years ago

In my current TRB 1.x app I have a monster method that catches common problems and provides several generic error messages, as well as passes on a specific error message. Another method converts into the format originally requested (json, xml, html, pdf, xls etc)

Bottom line - are we going to implement common responses for things like


method_not_allowed
route_not_found
unknown_action
record_not_found
record_not_unique
record_range_error
bad_request 

Or we leaving it up to the framework ( Rails, Hanami, etc) to handle?

Right now I'm essentially letting Rack/Rails handle it - I'm just injecting addition info into response, and formating it better. So instead of a 500 Error white screen of death, I'm sending 500 as a response code, and providing a nicely formatted message, in html, json or whatever.

P.S.: I'm just starting to play with Rails 5 & TRB2 , to see what I need to do to upgrade my TRB 1x app

apotonick commented 7 years ago

The business errors should be detected by Endpoint, such as "record not found". How those are then "forwarded" in Rails or Hanami, or whatever, is job of the concrete endpoint implementation. We should collect the errors you're catching here and discuss.

konung commented 7 years ago

Do you think route_not_found -type of errors are business or not?

Meaning does my end-user care what generated the error - their own mistake or something in my system - applications error vs business error? Isn't it better if all the errors are caught "uniformly" and presented in the same manner, with the same API? I'm not saying expose the inner details and the whole call stack via an error message, just a better message and the uniform API. I really hate getting calls: "The page says, contact admin - 500 error" or your API keeps saying "Bad Request" Like I need more details than that or I would like to provide more details than that in the error message. So I don't need to get that call in the first place.. You know. And I understand that you can run honeybadger and what not. But ...

IMHO all errors should return a response / status code like twitter is doing, but also duplicate that status code and an actual error message in the body of the error. This way regardless of which style of error is expected by client - you are presenting both. This is makes it easier for the users of your API to contact you with errors and also make for a better debugging experience in production. And then use the same Error API to through back all kinds of validation errors as well.

Here are some typical error responses I'm sending

I do similar catch for route_not_found errors and stuff.

404 Error

http://localhost/sales_orders/10063986.json

{
  "errors": [
    {
      "name": "Not Found",
      "message": "Couldn't find SalesOrder with 'id'=10063986 ",
      "status": 404,
      "timestamp": "2017-02-17 12:32:12.078-06:00"
    }
  ]
}

401 Error

To the same resource without an API key

{
  "errors": [
    {
      "name": "Unauthorized",
      "message": " You need to log in or provide API token!. Log Tag - [5961f882] at 2017-02-17 12:37:10 -0600 .Please use this Log Tag & Timestamp if you contact system admin. ",
      "status": 401,
      "timestamp": "2017-02-17 12:37:10.250-06:00"
    }
  ]
}

Finally I distinguish between production and dev, so it doesn't break my debugging process in dev, by trying to catch weird edge cases.

Here is a sample validation error, using the same Error API:

406 Not Acceptable

{
  "errors": [
    {
      "name": "tracking_number",
      "message": "Not a valid number. Only alphanumeric characters allowed. Should be between 6 and 40 characters or empty.",
      "status": 406,
      "timestamp": "2017-02-17T12:55:41.421-06:00"
    },
    {
      "name": "tracking_number",
      "message": "is too short (minimum is 6 characters)",
      "status": 406,
      "timestamp": "2017-02-17T12:55:41.421-06:00"
    }
  ]
}

@apotonick What do you think? Am I full of it? Or does it make sense?

philsturgeon commented 7 years ago

I look the look of this. Would you folks consider RFC7870?

apotonick commented 7 years ago

The RFC is a great idea @philsturgeon !

@konung Totally agree on that and very happy that someone finally comes up with some concrete ideas. The great news is that this totally fits into TRB 2.1 where you can have additional endings (e.g. End.current_user_missing, so the endpoint has to guess less.

These are basically two tasks (ignoring the 2.1 stuff I will bring on):

  1. Write matchers for those cases
  2. Implementing a generic problems representer following @philsturgeon's RFC to render those errors.