-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
SNS: fix Subscribe non existent topic exception #12175
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
LocalStack Community integration with Pro 2 files ± 0 2 suites ±0 1h 11m 1s ⏱️ - 42m 42s Results for commit bcb1aaa. ± Comparison against base commit 44992c0. This pull request removes 1431 and adds 1 tests. Note that renamed tests count towards both.
|
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 took some time to understand the validation of arn format (400), vs topic existance (404). Interesting how aws handles these cases, at my first look I assumed it had to be a 400 error too.
Thanks! lgtm
@@ -651,6 +651,9 @@ def subscribe( | |||
parsed_topic_arn = parse_and_validate_topic_arn(topic_arn) | |||
store = self.get_store(account_id=parsed_topic_arn["account"], region_name=context.region) | |||
|
|||
if topic_arn not in store.topic_subscriptions: |
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.
question: does this error only happen for subscribe
fnx?
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.
Very good question! This error should happen for every operation targeting a Topic somehow. I think we were already properly raising an exception for Publish
for example, via the _get_topic
method. As the Subscribe
operation is quite complex, and does not really have the need for the _get_topic
method, we check it a bit differently.
Motivation
As reported in #12174, we did not raise an exception if the topic did not exist
Changes
fixes #12174