RFC: Web API error response_metadata is buried

[jmanian]

In some of the more recent Web API methods the most relevant error information is given in the response_metadata. For instance here's an example of Slack's response body for a bad call to views.open:

{
  "ok": false,
  "error": "invalid_arguments",
  "response_metadata": {
    "messages": [
      "[ERROR] missing required field: title [json-pointer:/view]",
      "[ERROR] missing required field: blocks [json-pointer:/view]",
      "[ERROR] missing required field: type [json-pointer:/view]"
    ]
  }
}

The error value of invalid_arguments is basically useless on its own — it's necessary to look at the response_metadata to understand the problem with the request.

Currently this library pulls out the error value and uses this for the message on SlackError (here). It's possible to access the reseponse_metadata with slack_error.response.body.response_metadata. However my problem is that when an error is raised my logs only show the error, because that is the message (and as far as I can tell, raise prints Class: message).

My goal is to get the response_metadata into my logs. Here's one idea that I have for a feature in this library that would achieve that:

1. Add a config option for verbose Web API errors (verbose_web_api_errors?).
2. If this config option is set then the message on SlackError will be the entire body as JSON, rather than just the error.
3. The option would default to false so as to not break anyone's code.
4. SlackError would also gain a new attribute, perhaps error, which would be the value of error from Slack's response, so that this is easily accessed even when the verbose option is being used.
5. For completeness, SlackError would also gain a new attribute response_metadata containing just the response_metadata (as a hash-like object).

What do you think of this approach? I don't know much about logging, so maybe there's a better approach to this, either within this library or simply in my own app.

[dblock]

I think metadata is designed to add additional detail and not be source of error. Would logging the error with complete metadata be more appropriate than throwing it into the error message?

[dblock]

I am fine with your approach btw, but I don’t think this should be optional. I don’t see why it wouldn’t become the new default. Nobody is relying or should be relying on text for conditional workflow and we can document this in upgrading just in case.

[jmanian]

Regarding your first suggestion:

Would logging the error with complete metadata be more appropriate than throwing it into the error message?

I agree — you might be right here, I just don't know the best way to log the complete metadata other than throwing it into the error message 😊. Whether it would be with a change in this library, or simply a configuration in my app. Do you have any suggestions for how to do that?

---

Regarding your second comment, making it the new default actually would break code in our app where we look at the error's message, so I assume others might be in the same boat. Here's an example:

rescue Slack::Web::Api::Error => e
  raise e unless e.message == 'not_in_channel'
end

My proposed change would change the value of e.message above. (This use case is the reason for item #4 in my proposed changes above, so that we can continue doing this sort of check.)

[jmanian]

Here's a related idea: I wonder if it would make more sense for each of Slack's error codes to have its own error class. For instance Slack::Web::Api::Errors::AccountInactive and Slack::Web::Api::Errors::NotInChannel.

All of them would be subclasses of Slack::Web::Api::Errors::SlackError, thus making it (mostly) backwards compatible, and making it easy to rescue all slack errors if desired, like you can do today. But it would also make it easier to handle a single error type when necessary.

That would simplify code like this:

rescue Slack::Web::Api::Errors::SlackError => e
  raise e unless e.error == 'not_in_channel'
  # do something
end

to this:

rescue Slack::Web::Api::Errors::NotInChannel
  # do something
end

---

If we like this idea, then it should be easy enough to pull all the types from the official API spec.

Here's one thing that gives me pause: I tried pulling the list of errors from the spec to see how many there are, and the number is 171, which seems like potentially too many error subclasses. Thoughts?

[dblock]

I think it's a great idea @jmanian and I wouldn't worry about the number of classes. It's a definite improvement for the developer as we constantly compare e.error in so many places.

[dblock]

Should we close this @jmanian ?