Exceptions & error handling

[wiverson]

I'm noticing a few things about the exception handling, had some Qs/thoughts:

There are a number of unchecked exceptions with no base class for various requests (I'm mostly focusing on signing up & signing in via anon key client). This is making it a bit hard to work with, as I have to make sure I go through and capture everything.

I think what I'd like would be for requests to throw two kinds of checked exception, one intended for user-level problems (e.g. existing account, invalid email/pass) and one intended for developers (e.g. bad anon key, bad url, etc). That way I could have two exceptions to track (which my IDE can automatically generate catches for) instead of the current set.

I'm also noticing that there is some inconsistency in how the exceptions are caught/thrown - for example, some of the user errors appear to be hidden in a BadRequest.

I guess the question is how critical are the current exceptions vs a rework to make them easier to deal with? Does this make sense? Should I go ahead and send a PR proposal, or...?

[acupofjose]

Totally agree the exception handling is not ideal currently. If you can outline a proposal I'd appreciate it!

[wiverson]

Ok, here's my initial thoughts:

All of the requests have two checked exceptions. One is ConfigurationException, one is UserException. ConfigurationException covers anything that only a developer would be expected to resolve (e.g. missing api keys, bad URL, etc). UserException would cover anything a user would be expected to resolve (e.g. existing user, password problem, etc).

The existing exceptions would get folded into one of these two exceptions, with perhaps an enum to distinguish between types instead of different classes.

I think there are some error conditions that don't really map to existing exceptions right now, e.g. user already registered. I think the only way to distinguish between some of these right now is by matching text to the message back from the server. I think part of the idea would be to try to do a best case match between the exception and the content from the server to set the enum, if nothing matches it either falls back on a generic User exception or a Configuration exception.

I just saw you added a base GoTrueException. I would make both Configuration and User descend from those. :)

I'd also move Response, Content, etc up to GoTrueException as nullable.

The idea is that a user could either catch GoTrueException (e.g. for demos just catch and dump the exception to a log), but for a production app the developer can catch user and configuration and present the info as needed (e.g. for User it's "fix X" and for Configuration it's "Developer needs to fix"

[acupofjose]

I like the two error base classes. But most libraries I see use separate classes for exceptions, not enums. I'd been trying to avoid message parsing because of magic strings and all that, but it may be unavoidable...

Thoughts on two base classes GotrueConfigurationException and GotrueUserException with appropriate inherited classes that may/may-not require request message parsing?

[wiverson]

Yeah, normally I'd say go for different exceptions. Here's kind of what I'm thinking:

Right now everything is declared with unchecked exceptions. This is great for demos, but is a Real Bummer(tm) as I'm trying to build a production-worthy bullet-proof version. What I'm having to do right now is just kind of assume that I'm covering everything by looking at the list of exceptions and then manually having to cover them all for each test case. You can see what that looks like in this (very janky) gist.

I really like it when my IDE can help me out with this stuff. So, I think these are the options:

• Leave it as it is. :\
• Add two base checked exceptions, and then leave it to the user to implement logic to disambiguate via a method, presumably via pattern-matching or similar to discriminate based on type? Would still have to manually check to make sure all of the subcases are covered.
• Add two base checked exceptions, and then use an enum to discriminate. This has the advantage that if a new enum/error is added later, IDEs such as Rider will automatically pick this up as a code quality issue and offer to auto-gen additional branches. It also means IDEs will automatically generate the checked exceptions as well, which would make it a lot more of a "hit the button and the IDE fills it out for you" kind of thing.

I can do a pass at this in a PR if you'd like to see what it looks like.

[acupofjose]

Oh interesting - you make a strong case for the enums there. I’m down for it! It’ll be good to see it and implement it on the other repos!

[wiverson]

I'm thinking it would be Good For Me(tm) to submit some changes as PRs. Tho, I gotta ask...

Is that the preferred style for C#? ;)

[acupofjose]

Yep! Opening bracket on next line. Visual Studio ought to auto format for that.

[wiverson]

I'm using Rider (I'm on macOS). Rider has a feature to detect code style from an existing code base, I just ran that and added it to a cleanup PR. :)

[wiverson]

Resolved over several PRs including #57

Woot!