Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pattern is, reasonable, but my biggest concern here is having the
Exception
here being too broad.My question: what does
success=False
signal to a client? If it's supposed to specifically mean that the save failed due to say, integrity error, it'd be wiser to catch the database exception (https://docs.sqlalchemy.org/en/20/core/exceptions.html#sqlalchemy.exc.SQLAlchemyError).
Here are some counter points for why letting errors pass through sometimes is important:
Unit tests may silently pass that are failing if we catch exceptions too broadly. The most common way that a unit tests fails is that an exception is not handled during its execution. Are we certain that every unit test call site will check success=True in every case? Letting some exceptions pass through is healthy for simpler and robust testing.
Related, but what if add an additional line of logic before the return on line 280 that always fails? In that case,
success=False
may look like a temporary database error, when in fact it would be a broad logical failure.Retry semantics. It is important that network access to services understand atleast somewhat when a failure is safe to retry. There are multiple ways you can encode the retry semantic -- for instance, maybe success=False means, do not retry at all, and maybe success=False means feel free to retry. But returning 500 is useful because it definitely implies to clients to at the least back off. If a totally unexpected issue is occuring in the system and we throw 500s because the endpoint isn't gracefully handling an issue, it is wise for clients to always back off from retrying the service.
Basically, I think that catching all exceptions and treating them the same way hides the fact that 'errors' come in different classes and warrant different responses from the client. I think it would be easier to narrow the exception class and be explicit about what
success=False
means for the caller (which part failed? is it safe to retry gain? etc, etc)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good points. If we want to truly follow REST semantics then our API should use 4xx and 5xx response codes meaningfully. Right now all errors result in 5xx. Instead we should split our errors into two classes - user error and system error. User error means use can retry after fixing the error while 5xx means do not retry.
If I create two exception classes
ClientException
andServerException
, is it easy to return 400 for the first one and 500 for the second one, along with the error message?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's totally fair. And in my mind it doesn't matter to me about REST per say. I think my main concern is simply that we differentiate the error classes between "somewhat expected db failure" and "unexpected logical failure" and however we want to do that is indifferent to me.
I think it's ok to simply encode it this way:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case the only change would be merely to narrow the exception clause a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But just to be totally 100% honest, it is 100% ok for you to go with your gut and fuck around and find out. If you feel strongly about any particular path, I am ok with proceeding that way, too. This is my best effort perspective, but you know your logic better than me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with returning
False
for client errors (aka 400). Will update this PR. Thanks.