Open huyphan opened 4 years ago
Thanks for opening an issue, and the great repro steps! I think you've identified an opportunity to simplify error handling, although I'm not sold on an additional subclass.
The exception is raised in SessionWrapper.load_items
which should preserve the original exception. As a first step, you can access this from ex.__cause__
:
The expression following from must be an exception or
None
. It will be set as__cause__
on the raised exception.
The code to check the error message is a little bit tedious since botocore exceptions are afaik just json blobs wrapped in a ClientError
class. This looks like:
except bloop.BloopException as ex:
code = ex.__cause__.response["Error"]["Code"]
if code == "ResourceNotFoundException": # <-- string literal TBD
# handle missing resource
BloopException
Long term I'd like to avoid re-wrapping the inner response code as a new subclass of BloopException
. Raising up the exception codes to individual classes adds mental overhead when debugging, a synchronicity problem when new codes are created, and complexity when seeking help on the aws forums or this issue tracker.
However, easy access to the error code seems like a great enhancement. Something like BloopException.boto_error_code
as an Optional[str]
would simplify common error handling without requiring a new version every time the Dynamo team adds a new error code.
What are your thoughts?
It makes sense regarding the overhead of having explicit exception class for each error code.
I did not realize that we can access the underlying error code via __cause__
attribute. I'm not a big fan of dunder attribute usage, so it would be cool if we can access the error code in a nicer way -- and your suggestion on BloopException.boto_error_code
is very reasonable.
Another improvement that maybe easier to add, is making the underlying error code part of string representation of BloopException
. So instead of just returning this:
>>> str(ex)
Unexpected error while loading items
... it could return this:
>>> str(ex)
Unexpected error while loading items: ResourceNotFoundException.
This improvement does not add much value to error handling though, it just makes the (unexpected) errors more informative.
For now I'll merge a PR that improves the error string, if you're up for making the change.
I'd like to think about the larger BloopException
change for a few days before making such a large change. That's the kind of structure people build error handling around, and as such is not easy to back out even in a major version bump.
I can contribute PR for this but I'm leaving the issue here first to collect some thoughts.
Currently if we skip creating table when binding a model and the table does not exist yet, loading objects will result generic exception
bloop.exceptions.BloopException
:I enabled debug log and found the underlying exception returned by DynamoDB:
For
get_item
/batch_get_item
API, I think it's safe to assume thatResourceNotFoundException
is only returned when the table does not exist. Hence we should be able to raise a specific exception here.The issue with that assumption is that it might not be future-proof. So one safe option is to still return a generic exception, but less generic than
BloopException
-- I'm thinking ofMissingResource
so it's consistent with existingMIssingObjects
exception.