Skip to content
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

implement SNS Filter/operators $or, suffix, equals-ignore-case, anything-but #10691

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Apr 19, 2024

Motivation

As reported in #9738, AWS launched new operators for SNS filtering. suffix and equals-ignore-case are pretty simple to implement, but $or necessitated a quite consequent rewrite, as the logic can get quite convoluted pretty fast. It quickly creates a lot of different forks, where one rule can have multiple branches and a payload can have multiple branches too if it contains array.

Also included better support for anything-but, which can accept any regular "plain" values like string, numbers and list of string & number. It can also accept the prefix operator, see https://docs.aws.amazon.com/sns/latest/dg/string-value-matching.html#string-anything-but-matching-prefix

Also, we still had an issue with some ordering of complex payload with arrays, see #10567 (comment)

Changes

  • switched the model for the policy, making it a list where each item of the list is an OR condition. This comes from the documentation of AWS: https://docs.aws.amazon.com/sns/latest/dg/and-or-logic.html
    • this is done via the improved flatten_policy, thanks a lot @cloutierMat for the great pairing session we've had, and you providing the final solution fixing all our issues! 🥳
  • added support for suffix and equals-ignore-case
  • improved support for anything-but
  • added a lot of unit tests for filtering
  • added a few AWS tests to validate the new behavior
  • remove the skip for the AWS validated $or test
  • fix the recursive function flatten_payload

fixes #10567, fixes #9738

@bentsku bentsku added aws:sns Amazon Simple Notification Service semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases labels Apr 19, 2024
@bentsku bentsku requested a review from joe4dev April 19, 2024 04:45
@bentsku bentsku self-assigned this Apr 19, 2024
Copy link

github-actions bot commented Apr 19, 2024

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 36m 42s ⏱️ -8s
2 951 tests +1  2 650 ✅ +2  301 💤  - 1  0 ❌ ±0 
2 953 runs  +1  2 650 ✅ +2  303 💤  - 1  0 ❌ ±0 

Results for commit fa53c0b. ± Comparison against base commit 9d80628.

♻️ This comment has been updated with latest results.

@bentsku bentsku changed the title implement SNS Filter/operators $or, suffix, equals-ignore-case implement SNS Filter/operators $or, suffix, equals-ignore-case, anything-but May 15, 2024
@bentsku bentsku marked this pull request as ready for review May 15, 2024 16:22
@bentsku bentsku requested a review from thrau as a code owner May 15, 2024 16:22
Copy link
Contributor

@cloutierMat cloutierMat left a comment

Choose a reason for hiding this comment

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

Lots of great work! 🚀 That is a pretty big step forward!

I like your simplification of the recursive function for the policy.


return any(
all(
any(
Copy link
Contributor

Choose a reason for hiding this comment

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

🤣 Ok, I understand why you want to keep it separate from the attributes policy...

filter_policy = {"key": [{"anything-but": {"suffix": "test"}}]}
_subscribe(filter_policy)
self._add_normalized_field_to_snapshot(e.value.response)
snapshot.match("error-condition-anything-but-suffix", e.value.response)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! I kept wondering if this was a thing we needed to add support for! ⚡

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah me too, I thought the documentation was lying... but no, they really do not support anything else than prefix 😆

@bentsku
Copy link
Contributor Author

bentsku commented May 16, 2024

I like your simplification of the recursive function for the policy.

Thanks @cloutierMat, it's actually heavily inspired by yours 🚀 you definitely fixed all the recursion of SNS! Thank you so much 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws:sns Amazon Simple Notification Service semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
2 participants