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

Error handling design can be improved #158

Open
kohsuke opened this issue Mar 28, 2020 · 0 comments
Open

Error handling design can be improved #158

kohsuke opened this issue Mar 28, 2020 · 0 comments

Comments

@kohsuke
Copy link

kohsuke commented Mar 28, 2020

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 #153 (comment) 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.

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

1 participant