Skip to content

Conversation

@viren-nadkarni
Copy link
Member

@viren-nadkarni viren-nadkarni commented Nov 25, 2025

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 use ServiceException (which subclasses Exception), some older code raises RESTError (based on werkzeug HTTPException).

Tests

Unit tests are included

Related

Closes PNX-528

@viren-nadkarni viren-nadkarni self-assigned this Nov 25, 2025
@viren-nadkarni viren-nadkarni added docs: skip Pull request does not require documentation changes notes: skip Pull request does not have to be mentioned in the release notes semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases labels Nov 25, 2025
@viren-nadkarni viren-nadkarni added this to the 4.12 milestone Nov 25, 2025
@github-actions
Copy link

github-actions bot commented Nov 25, 2025

Test Results - Preflight, Unit

22 670 tests  +1   20 902 ✅ +1   6m 30s ⏱️ +11s
     1 suites ±0    1 768 💤 ±0 
     1 files   ±0        0 ❌ ±0 

Results for commit e326aaf. ± Comparison against base commit 2877bfd.

This pull request removes 1 and adds 2 tests. Note that renamed tests count towards both.
tests.unit.aws.handlers.test_service.TestServiceExceptionSerializer ‑ test_moto_exception_is_parsed
tests.unit.aws.handlers.test_service.TestServiceExceptionSerializer ‑ test_moto_rest_error_is_translated
tests.unit.aws.handlers.test_service.TestServiceExceptionSerializer ‑ test_moto_service_exception_is_translated

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Nov 25, 2025

Test Results (amd64) - Acceptance

7 tests  ±0   5 ✅ ±0   3m 21s ⏱️ +3s
1 suites ±0   2 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit e326aaf. ± Comparison against base commit 2877bfd.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Nov 25, 2025

Test Results (amd64) - Integration, Bootstrap

    5 files      5 suites   2h 37m 58s ⏱️
5 349 tests 4 811 ✅ 538 💤 0 ❌
5 355 runs  4 811 ✅ 544 💤 0 ❌

Results for commit e326aaf.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Nov 25, 2025

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   2h 1m 21s ⏱️ - 2m 56s
4 975 tests +6  4 597 ✅ +6  378 💤 ±0  0 ❌ ±0 
4 977 runs  +6  4 597 ✅ +6  380 💤 ±0  0 ❌ ±0 

Results for commit e326aaf. ± Comparison against base commit 2877bfd.

♻️ This comment has been updated with latest results.

@viren-nadkarni viren-nadkarni marked this pull request as ready for review November 25, 2025 13:36
@viren-nadkarni viren-nadkarni added the review: merge when ready Signals to the reviewer that a PR can be merged if accepted label Nov 25, 2025
Copy link
Contributor

@bentsku bentsku left a 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 the RESTError type handling in case the internal implementation of moto changes. The rest is more of personal preferences / ideas, but nothing to act on.

Thanks for jumping on this 🚀

Comment on lines 25 to 26
self._moto_service_exception = types.EllipsisType
self._moto_rest_error = types.EllipsisType
Copy link
Contributor

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 😄

Copy link
Member Author

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.

if isinstance(exception, (ServiceException, self._moto_service_exception)):
if isinstance(
exception, (ServiceException, self._moto_service_exception, self._moto_rest_error)
):
Copy link
Contributor

@bentsku bentsku Nov 25, 2025

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 between ServiceException and RESTError).

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 the Ellipsis type here. But this is just personal preference, really no need to act on this note.

Something like:

        try:
            import moto.core.exceptions
            self._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, )

...

        if isinstance(exception, self._service_error_types):
            return

What do you think?

Copy link
Member Author

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 in 6236f11

},
)
msg = "The keypair 'some-key-pair' does not exist."
moto_exception = InvalidKeyPairNameError(msg)
Copy link
Contributor

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 the InvalidKeyPairNameError is switched to be of ServiceException, then we won't test the RESTError behavior anymore.

Should we manually define a subclass of RESTError in the unit test to be sure we're handling that case properly?

Copy link
Member Author

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 to ServiceException, this would silently invalidate the test.

I redefined the exceptions within the tests in e326aaf, what do you think?

@viren-nadkarni viren-nadkarni requested review from bentsku and removed request for dmacvicar November 28, 2025 11:42
Copy link
Contributor

@bentsku bentsku left a 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 💯

@viren-nadkarni viren-nadkarni merged commit c43bcca into main Dec 1, 2025
42 checks passed
@viren-nadkarni viren-nadkarni deleted the moto-exc-translation branch December 1, 2025 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs: skip Pull request does not require documentation changes notes: skip Pull request does not have to be mentioned in the release notes review: merge when ready Signals to the reviewer that a PR can be merged if accepted semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants