Use proper exception handling

[danielcash]

In my opinion, we should remove anything related to error codes, status codes, etc from the business logic (aka controller). We would be better off just throwing exceptions (not generic exceptions, we should create specific custom exceptions, e.g. MultipleUsersReturnedException). Then in the views, we can catch these exceptions, and pass along the corresponding error codes, status codes, etc through the API endpoint.

There are a few reasons for doing this. First, it currently feels like a bit of a kludge to be passing status objects to every business logic function. These functions should not care about how the data will be passed on to the client. Second, it makes it very difficult to call these functions from other controller functions, and makes the logic for error checking quite a pain. Lastly, and most importantly, the catch-all "except Exception" in the views is masking exceptions and getting rid of any stack traces. This is making debugging and testing difficult, and is considered to be a nasty anti-pattern.

The solution, in my opinion, is to just throw explicit custom exceptions when applicable from the controller functions. Then, in the views we can catch the exceptions that we could expect, and pass along the corresponding error codes. If an exception occurs that we aren't expecting, we shouldn't necessarily be passing anything along. An unexpected exception should crash out and give us a stack trace to debug with so we can avoid running into the exception again. Django is robust enough to continue running despite these, and we shouldn't try to code around them.

[williamflaherty]

@Mimimiel @danielcash I'm gonna let you guys hash out. I agree with most of what Dan has said [to me] about cleaning up the code e.g. no models in the views etc. (And as far as I know Mischa agrees as well) From now on I would like to keep our conversations about this as public as possible on here so we can refer to them later.

I think everything Dan said here makes sense but I think Mischa had reasons for why she did things the way she did, she'll have to clarify. My biggest concern is in the amount of time it will take to make custom exceptions, but I understand the draw in having something easier to debug and I agree it's definitely clunky to pass around the status objects.

[danielcash]

For what it's worth, the time it takes to create custom exceptions is negligible. I prototyped this out a few days ago. Literally all we have to do to create an exception is something like:

# Generic Pearing API errors
class PearingApiException(Exception):
    pass

# We can sub-class exceptions, which is nice for a few reasons
class InvalidSecretKeyException(PearingApiException):
    pass

class SomeCrazyApiException(PearingApiException):
    pass

Then we can do stuff like:

try:
    some_function_that_throws_an_api_exception()
except InvalidSecretKeyException as e:
    # will catch just InvalidSecretKeyExceptions
except PearingApiException as e:
    # will catch all PearingApiExceptions and any exceptions that are subclassed (InvalidSecretKeyExceptions and SomeCrazyApiException)

Then, in the business logic we can do stuff like:

person = Person.objects.filter(username=user)
if not person:
    raise NonExistentPersonException("yo, that dude doesn't live here")
elif len(person) > 1:
    raise MultiplePeopleReturnedException("why are there multiple???")

# or...
try:
    person = Person.objects.get(username=user)
except MultipleObjectsReturnedError as e: # can't remember what the real django exception is called
    raise MultiplePeopleReturnedException("...")

# or even just not catch the exception and let the view catch MultipleObjectsReturnedError
# so many options

[Mimimiel]

Notes from the discussion between Dan and I:

Mischa A) I don't know what happens in the event of a completely unhandled/unplanned for exception, but I'm assuming that nothing gets returned from the API call. I don't want the server crashing and an unhandled exception behaving the same way. B) I probably wouldn't catch any exceptions in the controller and let the view deal with it C) I don't want to throw those exceptions back to willie. I'd want some kind of translator to mask those errors so that from a security standpoint we're not letting everything out there. i.e. an error code D) you mention masking the stack trace because of the overall try/catch. I'm not sure how to elegantly handle this in a production standpoint because again A

Dan A) Unhandled exceptions I believe will return some sort of response with an error code. I will definitely check on that and determine the exact behavior before moving forward. B) I think there may be some cases where it's smart to catch in the controller. I.e. let's say we have something like get_messages that calls get_person and for some weird reason we're okay with get_person failing. This case doesn't make sense, but there are definitely instances where it would. C) Agreed, that's why I was hoping we could just catch in the view and handle error codes from there. I have a huge example commit that I haven't PR'd that has a lot of this in it. D) Well, as long as we can fully log all of the unhandled exceptions, I'll be satisfied. I'll have to check Django's default behavior.

Dan A) It sounds like django will return a 500 internal server error and email everyone listed in settings when there's an unhandled exception. Which is annoying. Willie can install sentry. Ok, i vote for now we still catch all default exceptions in the view, but make sure we log them

[Mimimiel]

Thinking about it more, the controller should actually throw up the specific Pearing exception instead of letting the view try to interpret it. There's a lot of cases where the same exception would be generated for different reasons, and the view would have a hard time determining root cause. For example, in getting settings for a person, does "does not exist" stem from the person not existing or the settings not existing?

[Mimimiel]

Also, because this will drive me crazy otherwise, we have to determine a naming convention for the exceptions so that 1) the naming is consistent and 2) it is easy to identify if the desired exception already exists. So in your example above, NonExistentPersonException and MultiplePeopleReturnedException follow a AdjectiveNoun{Verb} pattern...but already 1) the Verb is optional and 2) If I'm searching for all Exceptions related to "Person", I might miss the MultiplePeopleReturnedException because it says People instead of Person.

I would opt for AdjectiveModelVerbException and to ignore plurality, for example NoPersonFoundException and MultiplePersonFoundException.

[nziebart]

I don't know much about Django, but I would say that the exceptions should be created based on the needs of the mobile app. If the UI doesn't care why a call failed, no point in having multiple exception types. That way we don't do unnecessary thinking and work.

On Wednesday, February 18, 2015, Mimimiel notifications@github.com wrote:

Also, because this will drive me crazy otherwise, we have to determine a naming convention for the exceptions so that 1) the naming is consistent and 2) it is easy to identify if the desired exception already exists. So in your example above, NonExistentPersonException and MultiplePeopleReturnedException follow a AdjectiveNoun{Verb} pattern...but already 1) the Verb is optional and 2) If I'm searching for all Exceptions related to "Person", I might miss the MultiplePeopleReturnedException because it says People instead of Person.

I would opt for AdjectiveModelVerbException and to ignore plurality, for example NoPersonFoundException and MultiplePersonFoundException.

— Reply to this email directly or view it on GitHub https://github.com/williamflaherty/pearing/issues/7#issuecomment-74996376 .

Sent from Gmail Mobile

[danielcash]

Mischa, I agree. Naming conventions would be good, and the one you proposed for exceptions seems great. And to your previous comment, that's what I'm meaning when I say throw explicit exceptions.

To Ziebart's point, I can see how having a lot of different exceptions could be frustrating to deal with. However, I think down the road we'll benefit from having that granularity, especially if we end up incorporating a web app (I don't want to think about that right now). Also, keep in mind that the "controller" functions aren't only going to be called from the views, and it can be important to have that granularity in other parts of the Django app.

Also, subclassing exceptions is really useful, since we can create a sort of hierarchy of exceptions. If the mobile app only cares about a certain type of error, then the view can just handle a superclass of those exceptions. The rest of the unhandled exceptions can either be caught and send a generic error response to the client, or we can just let Django's exception handler catch it and auto-send a 500 response.

[Mimimiel]

I agree with you Dan. The specific exceptions will be very useful for test cases, and, even though the iOS app may not care about certain exceptions, the developer trying to figure out why the app isn't working definitely will appreciate the verbose messages.

[nziebart]

Its not a big deal either way, but here is my reasoning.

For debugging purposes, logging and exception messages are your friends. Always be verbose in logs and exception messages. But the type of the exception is for when things need to catch it.

Having MultiplePersonReturnedExceptions doesn't necessarily hurt anything, but it leads to annoyances like consistency issues within the code. Do we also need a MultipleAccountsReturnedException? And since we are being specific there, don't we need a specific exception for xxx error case? And there's no real criteria for these decisions, so they take time. Then if you decide to stop, you end up with parts of the code that are specific, and others that are generic.

On the other hand, if your decision is based on whether or not something will actually handle the exception, you can make quick and consistent decisions.

Also, it's easy to go back later and change the exception to a specific type, if the need for it arises. So you don't really lose anything now by throwing generic ServiceExceptions and DatabaseExceptions, and you save time. Trying to predict what kinds of errors future code might care about is less productive.

I've dealt with this a lot in small startups, and pristine hierarchies of specific exceptions for every error case have never paid off. Even at the startup where I work now, we have a very complex system that involves banking and money transfer, and the exceptions are generic unless something can catch them and do something about it. The logs, however, are massive.

On Saturday, February 21, 2015, Mimimiel notifications@github.com wrote:

I agree with you Dan. The specific exceptions will be very useful when testing, and, even though the iOS app may not care about certain exceptions, the developer trying to figure out why the app isn't working definitely will appreciate the verbose messages.

— Reply to this email directly or view it on GitHub https://github.com/williamflaherty/pearing/issues/7#issuecomment-75380732 .

[danielcash]

It's not my intention to raise exceptions that won't be caught/used anywhere. Like you said, that would be a headache and not very useful to whoever/whatever is calling our API.

There are definitely cases where we'll want to catch specific exceptions outside of the view, but we should not be raising custom exceptions if we're not going to catch and handle them explicitly. I got carried away in my examples, but we'll definitely only implement exceptions if we need them at the time, custom or not.

Points definitely taken. We should be judicious about throwing exceptions, and logging will be very important.