S3: fix validation parity of `PutBucketNotificationConfiguration` by bentsku · Pull Request #13091 · localstack/localstack · GitHub
Skip to content
This repository was archived by the owner on Mar 23, 2026. It is now read-only.

S3: fix validation parity of PutBucketNotificationConfiguration#13091

Merged
bentsku merged 4 commits into
mainfrom
fix-s3-internal-errors
Sep 4, 2025
Merged

S3: fix validation parity of PutBucketNotificationConfiguration#13091
bentsku merged 4 commits into
mainfrom
fix-s3-internal-errors

Conversation

@bentsku

@bentsku bentsku commented Sep 2, 2025

Copy link
Copy Markdown
Contributor

Motivation

While looking at error reports, I've spotted the following:

exception while calling s3.PutBucketNotificationConfiguration: 'Name'

This indicates a key access to a non-existent key. Looking at the code, I could spot where we had an unsafe access to the filter_rule["Name"] (https://docs.aws.amazon.com/AmazonS3/latest/API/API_FilterRule.html).

Validated with AWS, I could check that those fields were indeed mandatory and not optional.

I've taken the opportunity to remove a very old util we had added when doing the S3 migration to ASF (before, you had to manually attach additional fields that were not part of the spec, but this got fixed with #6865), so I removed the utility and used the proper exception which makes it much more readable.

Changes

  • fix the validation of the Notification Configuration to properly validate the XML shape
  • move some tests around for CRUD validation (one stayed in SQS notifications tests because it needs Queue policy)
  • removed _create_invalid_argument_exc utility and properly used the exception instead

completes FLC-38

@bentsku bentsku added this to the 4.8 milestone Sep 2, 2025
@bentsku bentsku self-assigned this Sep 2, 2025
@bentsku bentsku added aws:s3 Amazon Simple Storage Service semver: patch Non-breaking changes which can be included in patch releases docs: skip Pull request does not require documentation changes labels Sep 2, 2025
@github-actions

github-actions Bot commented Sep 2, 2025

Copy link
Copy Markdown

@github-actions

github-actions Bot commented Sep 2, 2025

Copy link
Copy Markdown

S3 Image Test Results (AMD64 / ARM64)

    2 files  ±0    2 suites  ±0   7m 45s ⏱️ -12s
  521 tests +2  469 ✅ ±0   52 💤 +2  0 ❌ ±0 
1 042 runs  +4  938 ✅ ±0  104 💤 +4  0 ❌ ±0 

Results for commit a33e99d. ± Comparison against base commit 3a06ba7.

This pull request removes 1 and adds 3 tests. Note that renamed tests count towards both.
tests.aws.services.s3.test_s3_notifications_sqs.TestS3NotificationsToSQS ‑ test_bucket_notification_with_invalid_filter_rules
tests.aws.services.s3.test_s3_api.TestS3BucketNotificationConfiguration ‑ test_bucket_notification_with_invalid_filter_rules
tests.aws.services.s3.test_s3_api.TestS3BucketNotificationConfiguration ‑ test_bucket_notification_with_missing_values_in_rule
tests.aws.services.s3.test_s3_notifications_sqs.TestS3NotificationsToSQS ‑ test_filter_rules_empty_value
This pull request removes 1 skipped test and adds 3 skipped tests. Note that renamed tests count towards both.
tests.aws.services.s3.test_s3_notifications_sqs.TestS3NotificationsToSQS ‑ test_bucket_notification_with_invalid_filter_rules
tests.aws.services.s3.test_s3_api.TestS3BucketNotificationConfiguration ‑ test_bucket_notification_with_invalid_filter_rules
tests.aws.services.s3.test_s3_api.TestS3BucketNotificationConfiguration ‑ test_bucket_notification_with_missing_values_in_rule
tests.aws.services.s3.test_s3_notifications_sqs.TestS3NotificationsToSQS ‑ test_filter_rules_empty_value

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 2, 2025

Copy link
Copy Markdown

Test Results (amd64) - Acceptance

7 tests  ±0   5 ✅ ±0   3m 8s ⏱️ -4s
1 suites ±0   2 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit a33e99d. ± Comparison against base commit 3a06ba7.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 2, 2025

Copy link
Copy Markdown

Test Results (amd64) - Integration, Bootstrap

    5 files      5 suites   1h 25m 37s ⏱️
1 954 tests 1 675 ✅ 279 💤 0 ❌
1 960 runs  1 675 ✅ 285 💤 0 ❌

Results for commit a33e99d.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 2, 2025

Copy link
Copy Markdown

@bentsku bentsku marked this pull request as ready for review September 2, 2025 23:52
@bentsku bentsku requested a review from k-a-il as a code owner September 2, 2025 23:52

@k-a-il k-a-il left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

@bentsku bentsku force-pushed the fix-s3-internal-errors branch from 6f8a9c8 to a33e99d Compare September 4, 2025 10:00
@bentsku bentsku merged commit 746b6af into main Sep 4, 2025
48 checks passed
@bentsku bentsku deleted the fix-s3-internal-errors branch September 4, 2025 13:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

aws:s3 Amazon Simple Storage Service docs: skip Pull request does not require documentation changes semver: patch Non-breaking changes which can be included in patch releases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants