Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should we implement error handling for common errors. #5

Open
konung opened this issue Feb 16, 2017 · 4 comments
Open

Should we implement error handling for common errors. #5

konung opened this issue Feb 16, 2017 · 4 comments

Comments

@konung
Copy link

konung commented Feb 16, 2017

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
Copy link
Member

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
Copy link
Author

konung commented Feb 17, 2017

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
Copy link

philsturgeon commented Sep 11, 2017

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

@apotonick
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants