Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
viren-nadkarni merged 4 commits intomainfrommoto-exc-translation
Dec 1, 2025

Conversation

@viren-nadkarni
Copy link
Member

@viren-nadkarniviren-nadkarni commentedNov 25, 2025
edited
Loading

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 subclassesException), some older code raisesRESTError (based on werkzeugHTTPException).

Tests

Unit tests are included

Related

Closes PNX-528

@viren-nadkarniviren-nadkarni self-assigned thisNov 25, 2025
@viren-nadkarniviren-nadkarni added docs: skipPull request does not require documentation changes notes: skipPull request does not have to be mentioned in the release notes semver: minorNon-breaking changes which can be included in minor releases, but not in patch releases labelsNov 25, 2025
@viren-nadkarniviren-nadkarni added this to the4.12 milestoneNov 25, 2025
@github-actions
Copy link

github-actionsbot commentedNov 25, 2025
edited
Loading

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 commite326aaf. ± Comparison against base commit2877bfd.

This pull requestremoves 1 andadds 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_translatedtests.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-actionsbot commentedNov 25, 2025
edited
Loading

Test Results (amd64) - Acceptance

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

Results for commite326aaf. ± Comparison against base commit2877bfd.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actionsbot commentedNov 25, 2025
edited
Loading

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 commite326aaf.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actionsbot commentedNov 25, 2025
edited
Loading

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 commite326aaf. ± Comparison against base commit2877bfd.

♻️ This comment has been updated with latest results.

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

@bentskubentsku 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 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 🚀

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
MemberAuthor

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.

bentsku reacted with thumbs up emoji
ifisinstance(exception, (ServiceException,self._moto_service_exception)):
ifisinstance(
exception, (ServiceException,self._moto_service_exception,self._moto_rest_error)
):
Copy link
Contributor

@bentskubentskuNov 25, 2025
edited
Loading

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?

Copy link
MemberAuthor

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)
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 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?

Copy link
MemberAuthor

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 reacted with thumbs up emojibentsku reacted with eyes emoji
@viren-nadkarniviren-nadkarni requested review frombentsku and removed request fordmacvicarNovember 28, 2025 11:42
Copy link
Contributor

@bentskubentsku 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-nadkarniviren-nadkarni merged commitc43bcca intomainDec 1, 2025
42 checks passed
@viren-nadkarniviren-nadkarni deleted the moto-exc-translation branchDecember 1, 2025 05:53
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@bentskubentskubentsku approved these changes

@thrauthrauAwaiting requested review from thrauthrau is a code owner

Assignees

@viren-nadkarniviren-nadkarni

Labels

docs: skipPull request does not require documentation changesnotes: skipPull request does not have to be mentioned in the release notesreview: merge when readySignals to the reviewer that a PR can be merged if acceptedsemver: minorNon-breaking changes which can be included in minor releases, but not in patch releases

Projects

None yet

Milestone

4.12

Development

Successfully merging this pull request may close these issues.

3 participants

@viren-nadkarni@bentsku

[8]ページ先頭

©2009-2025 Movatter.jp