Add context manager for translating Moto exceptions #13129
Conversation
viren-nadkarni
added
semver: patch
Test Results - Preflight, Unit22 125 tests 20 387 ✅ 6m 18s ⏱️ Results for commit 70cf161. |
Test Results (amd64) - Acceptance7 tests 5 ✅ 3m 5s ⏱️ Results for commit 70cf161. |
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 2h 37m 24s ⏱️ Results for commit 70cf161. |
LocalStack Community integration with Pro 2 files 2 suites 1h 59m 8s ⏱️ Results for commit 70cf161. |
alexrashed
left a comment
alexrashed
left a comment
There was a problem hiding this comment.
Super nice implementation of the exception handling as a context manager which can easily be reused! 💯
I wonder if it would be possible to even further generalize the exception handling for services which have been migrated to moto's new core AWS serializer... 🤔
That's definitely something we can iterate on, nothing blocking a merge here!
| with translate_service_exception: | ||
| message = backend.send_raw_email(source, destinations, raw_data) |
There was a problem hiding this comment.
thought: Nice, this is a really clean, nice, and reusable way to apply the exception handling! I am just wondering if there is a way to maybe have a generic way to wrap all backend operation invocations in a way?
For example by using a decorator on the whole object? 🤔
There was a problem hiding this comment.
Thanks! Having a decorator for handler methods is great idea. I think it may be even better to integrate this in the ASF serialiser which will hide this complexity from provider level. I'll experiment with this in a follow up iteration.
| import moto.backends as moto_backends | ||
| from moto.core.base_backend import BackendDict | ||
| from moto.core.exceptions import RESTError | ||
| from moto.core.exceptions import RESTError, ServiceException |
There was a problem hiding this comment.
question: Is the ServiceException always being raised for issues like this (and only for issues like this), and would it be raised to the handler chain level? Otherwise there might be a chance to tackle this globally in the exception handler chain in app.py?
There was a problem hiding this comment.
It is only raised when importing and invoking Moto methods, not with FallbackDispatcher or call_moto(). For the latter, the serialised error response is returned from Moto, which can easily be passed down.
There was a problem hiding this comment.
Thanks! So it's only relevant for direct moto backend invocations in our service provider implementations.
Do you think this is done often enough such that it should be handled globally?
There was a problem hiding this comment.
Yes, I'll investigate this in the follow-up PR
Background
A common pattern in LocalStack providers is to import Moto module and invoke backend methods directly:
The problem here is that any exception raised by Moto is not understood by the ASF serialiser. This causes it to return a HTTP 5xx with an error log containing the trace:
Changes
This PR introduces a context manager that can be used to wrap such Moto invocations. It can translate Moto exceptions into ASF serialisable exceptions, which propagates the proper 4xx error code.
This context manager currently only supports ServiceException. Some Moto services use RESTError. Adding support for this is left for future iteration.
This PR also deploys this new context manager to an SES handler usage.
An alternative to this context manager could have been integrating this into ASF. I have no strong preference about this. It seemed better to keep this decoupled from ASF due to the changing landscape in Moto.
Tests
Includes unit tests for the context manager, plus an AWS-verified integration test for the SES change.
Related
Closes PNX-104