search

HubSpot / slack-client

An asynchronous HTTP client for Slack's web API
Apache License 2.0
114 stars 53 forks source link

Error handling design can be improved #158

Open kohsuke opened 4 years ago

kohsuke commented 4 years ago

Describe the bug The current API design uses SlackErrorIF as the error definition, which really just consists of one property error. This despite the fact that Slack server responds with a richer information.

As a result, users of this library are forced to spend unnecessary time digging into the root cause of the problem. See https://github.com/HubSpot/slack-client/issues/153#issuecomment-605504469 where two users misdiagnosed the problem.

Getting developers back on track from a failure is a critical component of the library usability, and II have to say this library is currently failing that bar for me (and considering the fact that the said error, in my case, comes from the very first, simplest possible use of this library to post a hello world message!)

The current workaround is ResponseDebugger, though in my case because it didn't occur to me to install SLF4J binding, so the message was simply swallowed to void. Even if it does go to logging, its output is not correlated with the location in the program where the problem happened, nor any other contextual information that is leading up to the failed invocation.

To Reproduce Use version 1.6 of this library, and run the PostAMessage example.

Exception in thread "main" java.lang.IllegalStateException: SlackError{type=UNKNOWN, error=invalid_arguments}
    at com.hubspot.algebra.Result.lambda$unwrapOrElseThrow$1(Result.java:68)
    at com.hubspot.algebra.Result.lambda$unwrapOrElseThrow$0(Result.java:64)
    at java.util.Optional.orElseThrow(Optional.java:290)
    at com.hubspot.algebra.Result.unwrapOrElseThrow(Result.java:60)
    at com.hubspot.algebra.Result.unwrapOrElseThrow(Result.java:64)
    at com.hubspot.algebra.Result.unwrapOrElseThrow(Result.java:68)

Expected behavior The actual response coming from Slack API is as follows:

{"ok":false,"error":"invalid_arguments","deprecated_argument":"as_user","warning":"missing_charset","response_metadata":{"warnings":["missing_charset"]}}

So first,SlackErrorResponseIF is a better representation of a problem than SlackError.

In additiona, Slack API doesn't really specify exactly what fields are in the error response, and as a case in point in the above error, there's an otherwise undefined field name deprecated_argument. So it's futile to try to do a full databinding. A better thing to do IMO is to simply carry forward JsonNode that represents the full information of a failure.

See https://github.com/kohsuke/slack-client where I made that change.

If this is too disruptive a change of existing users of this library, please consider allowing SlackErrorIF to point back to the parent SlackErrorResponseIF, so that the information can be retained in a much more compatible manner.