Integrate Moto exception translation into handler chain #13153
Conversation
viren-nadkarni
added
semver: patch
Test Results - Preflight, Unit22 298 tests ±0 20 555 ✅ ±0 14m 4s ⏱️ - 2m 8s Results for commit 751aed4. ± Comparison against base commit 658d6fb. This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
|
The issue here is that the base runtime does not have the moto dependency, and the idea was to not have a moto dependency in our "core runtime" as part of the S3 image project. Handling service exception in the I think the easiest way here would be to add an exception handler in |
viren-nadkarni
force-pushed
the
exc-translate-asf
branch
from
fd07dd0 to
e74123b
Compare
viren-nadkarni
force-pushed
the
exc-translate-asf
branch
from
e74123b to
02abf1a
Compare
viren-nadkarni
changed the title
alexrashed
added
the
notes: skip
|
Thanks for the pointer @bentsku! I moved this into the exception handler in the chain and also accommodated for possible unavailability of Moto, although I'm not sure whether this is the cleanest approach. |
There was a problem hiding this comment.
LGTM! it's great to see that we now consider compatibility in environments where moto is not a dependency, like the s3 image.
i was wondering whether the cleaner approach would have been to write separate handlers for Moto exceptions, but only add those to the handler chain if moto is available, rather than conditionally inlining the moto exception handling as we do here. but ultimately i think it makes no difference, since all these handlers are hyper-specific to AWS anyway.
| self._moto_service_exception = types.EllipsisType | ||
| with contextlib.suppress(ModuleNotFoundError, AttributeError): | ||
| self._moto_service_exception = importlib.import_module( | ||
| "moto.core.exceptions" | ||
| ).ServiceException |
There was a problem hiding this comment.
nit: i think i would prefer this, simply because the dynamic import via import_module makes it harder/impossible for an IDE to detect the import, whereas from moto.core.exceptions import ServiceException will show up in searches, module uses, etc.
try:
from moto.core.exceptions import ServiceException
self._moto_service_exception = ServiceException
except (ModuleNotFoundError, AttributeError):
self._moto_service_exception = types.EllipsisTypeThere was a problem hiding this comment.
You can even use the newer things, like it's done for the Moto patches conditionally applied for S3:
if find_spec("moto"):
from moto.core.exceptions import ServiceException
self._moto_service_exception = ServiceException
else:
self._moto_service_exception = types.EllipsisTypeIt's the new advised way, except ModuleNotError might raise some linting warnings 👍
| # Moto may not be available in stripped-down versions of LocalStack, like LocalStack S3 image. | ||
| self._moto_service_exception = types.EllipsisType | ||
| try: | ||
| self._moto_service_exception = importlib.import_module( | ||
| "moto.core.exceptions" | ||
| ).ServiceException | ||
| except (ModuleNotFoundError, AttributeError) as exc: | ||
| LOG.debug("Unable to set up Moto ServiceException translation: %s", exc) | ||
|
|
bentsku
left a comment
bentsku
left a comment
There was a problem hiding this comment.
LGTM, nothing much more to add than what @thrau as said 👍 I also think the conditionally added handlers might be a bit cleaner and decoupled, but now that I think of it a bit more, it won't update exception parameter itself of the ServiceExceptionSerializer, so we'd probably need additional work (attach it to the context as service_exception) and verify if there is something attached before doing the isinstance call on the exception parameter.
I think this will work nicely 👍 thanks for tackling this!
Changes
#13129 introduced a context manager for translating Moto exceptions into LS serialisable ones.
This PR integrates this mechanism with the handler chain itself and opts to remove the context manager. With this, the complexity and overhead of handling these cases is abstracted away from providers.
Tests
Unit tested. Integration tested by
test_send_email_raises_message_rejectedRelated
Supersedes: #13129
Closes: PNX-105