Exception handling in the crud module

[MoritzWeber0]

@TimoBbz and I had a dicussion about the general exception handling in the backend.

I will briefly describe the situation:
To create new models, we have a crud module that handles the database connection. The crud module has a function create_model. When calling the function, you have to pass the model information (including a tool variable) and the model gets created in the database. All possible tools are defined in a separate database table.

Therefore, it can happen that the referenced tool doesn't exist during model creation. In this case, SQLAlchemy throws a DatabaseError (which is a general error and can occur in many cases). In my opinion, we should catch this possible error in the function itself and throw a custom defined exception, e.g. ToolNotFoundError, which can be handled by the calling function.

However, @TimoBbz argues that a developer should always pass valid values to the function and handle potential errors before.

I'd like to hear some general opinions and feedback about the way of handling exceptions.

[TimoBbz]

That’s not exactly my point though. I think that in general, setting custom exceptions if you’re not creating a reusable module is not recommended as there is rapidly a lot of cases to cover and can easily get out of hand. I think that we need to rely as most as possible on already existing errors provided by the functions we use. For example:

def create_book(title: string, genre_id: int, db: Session) -> Book:
  genre = db.exec(select(Genre).where(Genre.id == genre_id).one() #raises GenreError with details if None
  book = Book(title=title, genre=genre)
  db.add(book) #raises BookError with details if title is already used
  db.refresh(book)
  return book

This is how it should work for me. No custom errors, and in the same time, meaningful errors. If this kind of errors is not available, then we should split the crud functions so they raise no more than one error per function, meaning that we require valid arguments:

# in crud
def get_genre_by_id(genre_id: int, db: Session) -> Genre | None:
  genre = db.exec(select(Genre).where(Genre.id == genre_id).first() #no error raised

def create_book(title: str, genre: Genre, db: Session) -> Book: #genre must not be none bc type annotation
  assert genre is not None # type annotation does not raise error, so we add this assertion to raise error
  book = Book(title=title, genre=genre)
  db.add(book) #raises DatabaseError with details if title is already used
  db.refresh(book)
  return book

# in route
@route.post()
def create_book(title: str, genre_id, db: Session = Depends()) -> Book:
  genre = crud.get_genre_by_id(genre_id, db)
  if genre is None:
    raise HTTPException(404) from None
  try:
    return crud.create_book(title, genre, db)
  except DatabaseError as e:
    raise HTTPException(409) from e

[ewuerger]

Imo in general you can do what ever you like as the developer to inform the user of your code about errors, so there is no (invisible, higher) rule that says only use custom exceptions in your described cases. If you think the standard exception types do not cover your ill-state or you have a catchy name that hits the bullseye with its name then it is fine to declare and use this special exception.
There is something that is called anti-patterns and is described here (asking for permission instead of forgiveness).

Also I want to raise awareness that relying on assertions to maintain healthy state is ok if you are deploying un-optimized python code (i.e. __debug__ is True). Else all assertion statements will be removed at compilation. You want to prohibit database corruption and it is wise to error as soon as possible if such a state is met. Doing that in a readable and obvious way that leaves almost no room for questions for users and other devs is the art here.

So for me it is perfectly fine to pass arguments that do not match the wanted state (but type else your static type checker should already alert) and re-raising too broad exception with precisely named custom exceptions with short messages. That doesn't imply to always raise custom exceptions. But I'd like to know if ToolNotFoundError is already good enough... I can't really tell what this tool is, probably the MBSETool? Then I'd suggest MBSEToolNotFoundError("Could not create model: Passed tool does not exist in the database.").

[Wuestengecko]

I agree partially with Moritz in that a "GenreError" and a "BookError" aren't really meaningful exception names, but what exactly "meaningful" itself means is wide open to interpretation anyways (looking at you, ValueError).

But let's start at the beginning.

---

Looking at this example:

def create_book(title: string, genre_id: int, db: Session) -> Book:
    genre = db.exec(some_query)  # may fail
    book = Book(title=title, genre=genre)
    db.add(book)  # may fail
    db.refresh(book)
    return book

In my opinion, "custom exceptions or not" is an A/B problem (in this example – production code might look different of course). What I see here is much more an abstraction problem.

create_book returns a Book instance, which represents a book in the database, and which has some uniqueness constraint. A book also has a genre, which it references from another table by the genre's ID.

It would make much more sense to have a class that represents such a Genre in the database, and pass that around instead of raw IDs. As a side effect, this would also allow the type checker to actually know what's going on.

def find_genre_by_id(id: int, db: Session) -> Genre:
    return db.exec(some_query)  # may fail

def create_book(title: string, genre: Genre, db: Session) -> Book:
    book = Book(title=title, genre=genre)
    db.add(book)  # may fail
    db.refresh(book)
    return book

This separates out the two database queries, making the caller responsible for getting a reference to the Genre from the database first (which might fail) and only then creating a Book with that reference (which also might fail). However, as the caller does both steps explicitly, he knows exactly which step failed without us having to introduce custom exceptions to distinguish between them.

This doesn't entirely solve the underlying issue though:

SQLAlchemy throws a DatabaseError (which is a general error and can occur in many cases).

We still don't know for example whether the Genre we're looking for doesn't exist, or whether we just lost connection and should retry later. However, now that we have a find_genre_by_id function, we can extend that to make a pre-flight check and throw a standard ValueError if the ID is bad, and all other conditions (like TOCTOU races, which we're now susceptible to) would lead to a DatabaseError and crash the entire transaction. Similarly, create_book can make another check to see whether the uniqueness would still hold up and throw a ValueError there, which we can distinguish from the first one because they happen in two different function calls.

If we were to actually introduce a GenreNotFoundError, I also want to say that I'm in favor of eliminating the "human-readable" message then. Normally exceptions are printed like ExceptionType: message, and if the exception type already says everything then there's no need to repeat it in the message again – an error like "Cannot create book: GenreNotFoundError: 3" tells me just as much as "Cannot create book: GenreNotFoundError: No genre with ID 3", while also making the ID easily extractable from the exception for further post-processing.

---

I would also like to briefly side-step on one more point from Timo's rewritten example, specifically the two lines:

raise HTTPException(404) from None
raise HTTPException(409) from e

I know of at least one HTTP library off the top of my head (aiohttp's server component) which uses one exception subclass per HTTP error code - there you would raise the errors HTTPNotFound and HTTPConflict instead. While yes it is a slightly different use case than with SQLAlchemy (you're throwing exceptions at the library, and not catching ones from it), in my opinion this leads to much easier readable and understandable code, even better than a crutch with global constants like HTTPException(HTTP_ERROR_NOT_FOUND). And with the exceptions being organized in a neat hierarchy (builtin Exception > HTTPException > HTTPError > HTTPClientError > HTTPNotFound), the argument that it gets out of hand too quickly IMHO falls apart as well, because you can catch exceptions at any point in the hierarchy if you don't care too much about the details, while still having the option of handling some specific cases differently.

---

To bring all my points together: In my opinion, custom exception types are completely fine and even needed in some cases to distinguish between different error conditions programmatically, or to make code more readable. However the issue could just as well be that different unrelated error conditions can even occur in the same function in the first place, in which case I'm in favor of redesigning the function to separate out the concerns – which might eliminate the need for custom exceptions entirely.

[Wuestengecko]

To address some other points made in this discussion:

---

@MoritzWeber0

In general, in my opinion it should be clear which errors can occur when calling a public function, in whose implementation I am not interested.

I agree with the idea, but reality unfortunately works slightly differently. I've seen it often enough that some deeply nested underlying function inadvertently throws a KeyError from a failed dict lookup or something, which ultimately traces back to something not being inserted into the dict way earlier that should have. So you should always be prepared that in theory any exception could fly at any time, and prepare your error handling accordingly – e.g. roll back an active transaction on every BaseException that flies, instead of only listening for the specific errors that are documented to be possible, and potentially leaving the transaction in an undefined state. Don't expect the libraries you use to always work perfectly as documented, and remember the wise words:

---

@ewuerger

Imo in general you can do what ever you like as the developer to inform the user of your code about errors

That would include printing an error message to stdout and raising SystemExit ;)

Jokes aside, there are still some better and some worse practices. In my opinion, when writing a library, you should always keep the needs of the eventual user in mind too. Otherwise you end up with custom exceptions that are still over-broad, like SQLAlchemy's DatabaseError, or you could end up fracturing your exceptions into too many small errors without a clear structure or hierarchy (for which I fortunately don't have an example).

---

@ewuerger

But I'd like to know if ToolNotFoundError is already good enough... I can't really tell what this tool is, probably the MBSETool? Then I'd suggest MBSEToolNotFoundError("Could not create model: Passed tool does not exist in the database.").

Without knowing too much about the code, I would still say that "ToolNotFoundError" is good enough for a start. And if it turns out to not be, we could later in the future further subclass it with things like "MBSEToolNotFoundError", "VCSToolNotFoundError" etc., whatever type of tool exists.

[TimoBbz]

After some searches, there are some subclasses of the SQLAlchemy’s DatabaseError, that are actually the subclasses of the DB API (in our case psycopg2). These errors are narrower, such as in this case IntegrityError, that is triggered by any error of Class 23 — Integrity Constraint Violation (See Appendix A. PostgreSQL Error Codes).

[TimoBbz]

@Wuestengecko
I agree with the idea, but reality unfortunately works slightly differently. I've seen it often enough that some deeply nested underlying function inadvertently throws a KeyError from a failed dict lookup or something, which ultimately traces back to something not being inserted into the dict way earlier that should have. So you should always be prepared that in theory any exception could fly at any time, and prepare your error handling accordingly – e.g. roll back an active transaction on every BaseException that flies, instead of only listening for the specific errors that are documented to be possible, and potentially leaving the transaction in an undefined state.

Although there could be some unexpected errors, FastAPI automatically sends a default error in case of unhandled errors, and this default error can be redefined. So no real need to handle the default exception.
Also, the transactions in Collab Manager are not autocommited, so any interrupted transaction results in a rollback.

[amolenaar]

What I miss in the conversation so far is the user-facing behavior. What should the user see? If it all boils down to a "Something went wrong" message on the frontend it does not really matter if you define different error classes. But maybe we want to return different response codes from the API in different circumstances.

So, what's the behavior you're after?

[TimoBbz]

This is not really a user-level problem we’re dealing with there (except if you’re talking about the developer as a user of the crud module). We want to know what errors should the crud raise, and in more general, how should it be organised.

[MoritzWeber0]

We deal with this case already by raising custom HTTPExceptions from the backend itself. As @TimoBbz mentioned, both opinions in the discussion have the exact same user behavior.

If you want to learn more about the syntax of the HTTPException, you can find a documentation here: https://dsd-dbs.github.io/capella-collab-manager/backend/exception/