Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork4.5k
Extend Moto exception translation to cover more error types#13414
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
55464dd to9750c07Comparegithub-actionsbot commentedNov 25, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Test Results - Preflight, Unit22 670 tests +1 20 902 ✅ +1 6m 30s ⏱️ +11s Results for commite326aaf. ± Comparison against base commit2877bfd. This pull requestremoves 1 andadds 2 tests.Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
github-actionsbot commentedNov 25, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
github-actionsbot commentedNov 25, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 2h 37m 58s ⏱️ Results for commite326aaf. ♻️ This comment has been updated with latest results. |
github-actionsbot commentedNov 25, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
bentsku left a comment
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.
Nice, I think this is pretty neat! I have mostly one comment about testing to make sure we're always testing theRESTError type handling in case the internal implementation ofmoto changes. The rest is more of personal preferences / ideas, but nothing to act on.
Thanks for jumping on this 🚀
| self._moto_service_exception=types.EllipsisType | ||
| self._moto_rest_error=types.EllipsisType |
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.
note: I guess this is a matter of taste, I personally liked the "try moto then error_type is X except Exception then error is Y", but both are equivalent so it does not matter 😄
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 switched it to avoid a fail-fast situation which could happen if an exception is raised in the first assignment, leading to the second member attribute not being defined at all.
| ifisinstance(exception, (ServiceException,self._moto_service_exception)): | ||
| ifisinstance( | ||
| exception, (ServiceException,self._moto_service_exception,self._moto_rest_error) | ||
| ): |
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.
note: here it is seems that we do not really care what type the exception really is (I mean betweenServiceException andRESTError).
It might be easier to declare a tuple of type once? So that it would be easier to extend if we were to add yet another error type? That way, we also don't need theEllipsis type here. But this is just personal preference, really no need to act on this note.
Something like:
try:importmoto.core.exceptionsself._service_error_types= (ServiceException,moto.core.exceptions.ServiceException,moto.core.exceptions.RESTError, )except (ModuleNotFoundError,AttributeError):# Moto may not be available in stripped-down versions of LocalStack, like LocalStack S3 image.self._service_error_types= (ServiceException, )...ifisinstance(exception,self._service_error_types):return
What do you think?
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 a good idea. Implemented in6236f11
| }, | ||
| ) | ||
| msg="The keypair 'some-key-pair' does not exist." | ||
| moto_exception=InvalidKeyPairNameError(msg) |
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.
nit: if the internal implementation of theInvalidKeyPairNameError is switched to be ofServiceException, then we won't test theRESTError behavior anymore.
Should we manually define a subclass ofRESTError in the unit test to be sure we're handling that case properly?
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 a very good point, if Moto were to migrate EC2 exceptions toServiceException, this would silently invalidate the test.
I redefined the exceptions within the tests ine326aaf, what do you think?
bentsku left a comment
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.
LGTM! Thanks a lot for improving on the test, I think it's going to be a bit more solid to future changes in individual services of Moto 👍 looks great! thanks for keeping on improving this 💯
c43bcca intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Background
#13153 added the ability to understand certain Moto exceptions so that were transparently reported to the client
Changes
This PR extends support for this translation system to cover RESTError. While services that use Moto's new response serialiser useServiceException (which subclasses
Exception), some older code raisesRESTError (based on werkzeugHTTPException).Tests
Unit tests are included
Related
Closes PNX-528