Explain why it's bad to use a middleware to coat error messages as exceptions

Question

We manage a backend application behind a FastAPI REST controller with multiple endpoints.

A member of our team decided to use a middleware on the application which parses the body of every incoming HTTP request after it has been processed by the application, see if there is any 'error' key in there (it assumes every response is a dictionary...), then sends a JSONResponse with status code 400.

They did this because the way errors are handled in the backend is by sending an 'error' message in the response, which most of the time is a dictionary. We received complaints from our clients because in this case the status code sent from the API was 200, even though it contained an error message.

I know there are so many things wrong in this approach, but I can't seem to find the words to give a constructive feedback as to why it's bad. All I can say is what's the good way to do it :

• All non-recoverable errors occurring in the backend should spark specific exceptions, and the controller should handle a set of reasonably generic exceptions as to what's the backend is supposed to be doing and send the right status code back to the client depending on those exceptions.
• Exceptions, handled or unhandled, will anyway propagate up to the top-level, which is the controller.

I'm looking for precise arguments to explain why the 'exception propagation' approach I suggested is better than using a middleware, for a constructive feedback.

NB: I'm also open for alternatives / constructive feedback if the latter approach is in fact wrong.

Answer 1

It all depends on the requirements. In other words, what your colleague is doing may actually be the correct approach.

In general, one would avoid such middlewares for multiple reasons:

• The presence of a error key is not necessarily a sign that there is an error. Basic example of a route that responds with a series of counters that make it possible to track the progress of a task:

  {
    "counters": {
      "processed": 158,
      "remaining": 1380,
      "errors": 0,
    }
  }
  

  Is the fact that there are absolutely no errors happening during the processing a good reason to send HTTP 400? I don't think so!

• The fact that there is an error (if there is a error) doesn't mean one needs to send HTTP 400. Nor to hide the error in the first place.

  There are many other alternatives, such as, for instance, sending HTTP 401, HTTP 403, HTTP 404, or any other valid HTTP 4.. or HTTP 5.. status code. There are even cases where one would respond with a success status code, such as HTTP 201: “hey, caller, the resource you requested was actually created, but, by the way, there is a error that means that you may have to double-check the newly created resource.”

• One may simply forget about this middleware. And when it hits you in a face, it hurts. Such as in “I wasted half of the sprint searching why the API misbehaves in that particular edge case, just to find that it's because that third party library responds with an object containing ‘error’ as one of its properties, and as we serialize this object and send it to the client, one of our middlewares intercept it and screws up.”

  This is essentially a reason why middlewares should usually remain stupid, i.e. do basic processing, but nothing too fancy, and especially nothing business-related. Compressing a response should be fine (although I had a case where changing compression from Brotli to Gzip broke some of the routes, which seemed very random at first). Tampering with the actual structure of the response to apply rounding to floating point numbers, or things like that, is however not a good idea in general, because it is really easy to miss.

  A simple example where a developer can waste time. Imagine the following structure for a task:

  class Task:
      self.name: str
      self.start_time: datetime
      self.end_time: Optional[datetime]
      self.error: Optional[str]
  

  Somewhere, the tasks are being aggregated into a response that ends up being sent to the client of the API.

  At a given moment, the team decides that there is no need to keep the actual error details in Task: all that matters is whether the task failed or not. So one of the developers changes the class to this:

  class Task:
      self.name: str
      self.start_time: datetime
      self.end_time: Optional[datetime]
      self.error: bool  # Notice the type change.
  

  He fixes the code that relies on this field. The unit tests are green, everything looks good. The next morning, somebody notices that a few dozen routes ran during the nightly build while executing system tests started to misbehave, responding with HTTP 400.

  How long would it take to find the cause? 😉

On the other hand, you may be working on a project where the requirements are very explicit, and require to catch any response containing error field. In this case, the middleware is acceptable. But make sure to document it thoroughly.

Answer 2

if there is any 'error' key in there (it assumes every response is a dictionary...), then sends a JSONResponse with status code 400.

This seems to be the most objectively wrong thing about this solution. It assumes that any error is a client error and, unless there's something about this that you haven't explained, that seems like a bad assumption. It's also a fairly broad error code. It's fine to use that when it makes sense, but I imagine there are other 400 codes that you might want to use.

There's nothing stopping the developers from adding a HTTP code, or even a custom error code, or something along those lines to provide more detail. The current approach seems really hacky and brittle. Your description suggests that these responses aren't homogeneous. If that's the case, trying to improve on this approach using that middleware is going to become a nasty mess of logic handling various cases of response structures. This logic is also going to be coupled to these various response creation routines.

I agree with your that exceptions are a decent solution here, both in general, and from my own experience with FastAPI. This is exactly the kind of thing that exceptions are good for: alternate execution paths which cannot succeed. I think a custom set of exception classes is a good approach as well because it can allow for changes to the specific code you want to return without modifying how the exception is raised.

Identifying the places where the exceptions should be raised should be fairly simple, at least to start. Wherever these error keys are being created and added to the response is likely the right place. Given the state of the solution you describe, I would expect more error cases that are not being handled properly, though.