Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 24, 2025
Merged

SNS: fix Subscribe non existent topic exception #12175

merged 1 commit into from
Jan 24, 2025

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Jan 23, 2025

Motivation

As reported in #12174, we did not raise an exception if the topic did not exist

Changes

  • add a test for raising an exception if the topic did not exist
  • raise the exception

fixes #12174

@bentsku bentsku added aws:sns Amazon Simple Notification Service semver: patch Non-breaking changes which can be included in patch releases labels Jan 23, 2025
@bentsku bentsku added this to the 4.1 milestone Jan 23, 2025
@bentsku bentsku self-assigned this Jan 23, 2025
@bentsku bentsku requested a review from baermat as a code owner January 23, 2025 22:41
@github-actions GitHub Actions
Copy link

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   1h 11m 1s ⏱️ - 42m 42s
2 586 tests  - 1 430  2 478 ✅  - 1 221  108 💤  - 209  0 ❌ ±0 
2 588 runs   - 1 430  2 478 ✅  - 1 221  110 💤  - 209  0 ❌ ±0 

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. 
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…

tests.aws.services.sns.test_sns.TestSNSSubscriptionCrud ‑ test_subscribe_with_invalid_topic

@bentsku bentsku requested a review from anisaoshafi January 24, 2025 11:51
anisaoshafi
anisaoshafi approved these changes Jan 24, 2025
View reviewed changes
Copy link
Contributor

@anisaoshafi anisaoshafi left a 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 :shipit:

anisaoshafi
anisaoshafi reviewed Jan 24, 2025
View reviewed changes
localstack-core/localstack/services/sns/provider.py
@@ -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:
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@bentsku bentsku merged commit eff69ac into master Jan 24, 2025
38 checks passed
@bentsku bentsku deleted the fix-sns-non-existent-topic branch January 24, 2025 15:48
@codenem codenem mentioned this pull request May 28, 2025
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anisaoshafi anisaoshafi anisaoshafi approved these changes

@baermat baermat Awaiting requested review from baermat baermat is a code owner

Assignees

@bentsku bentsku

Labels
aws:sns Amazon Simple Notification Service semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
4.1
Development

Successfully merging this pull request may close these issues.

bug: SNS subscribe doesn't raise error when topic does not exist
2 participants
@bentsku @anisaoshafi