eventbridge: fix handling of list elements #10600
Conversation
1162b52 to
0465c03
Compare
There was a problem hiding this comment.
Thank you @baermat for tackling this tricky event pattern matching 🙏
@baermat Can you apply the patch to the refactored version in the new utils.py after rebasing to resolve the merge conflicts?
Sorry for the inconvenience after merging #10599. I think it makes sense to have the test suite available to catch any potential regressions.
You can make use of the new test suite added in #10599. Use the test test_test_event_pattern and uncomment any "Failing tests" that this PR fixes or add a new scenario much easier by creating a file under event_pattern_templates. Check out my Demo from last Thursday for details. It also links the relevant Notion backlog items with more information.
| or raw_message.startswith("END") | ||
| or raw_message.startswith( | ||
| "REPORT" | ||
| ) # necessary until tail is updated in docker images. See this PR: |
There was a problem hiding this comment.
That PR links a patch from 2015 🤔 Is there a better open issue we could link to track this?
There was a problem hiding this comment.
I have to admit that I didn't check this PR at all. The fix here is mostly that a message can contain the words we filter for by accident (which is exactly what happened), while we should only filter for these words at the start
There was a problem hiding this comment.
Thank you for the clarification. Would be a nice-to-have clarifying comment 👍
| @@ -1,4 +1,7 @@ | |||
| { | |||
There was a problem hiding this comment.
I wonder why the new test test_put_events_with_target_lambda_list_entry does not show up here?
There was a problem hiding this comment.
I was wondering the same thing really, no idea
|
|
||
| assert "FailedEntryCount" in rs | ||
| assert "FailedEntries" in rs | ||
| assert rs["FailedEntryCount"] == 0 |
| @@ -573,8 +573,11 @@ def filter_event(event_pattern_filter: dict[str, Any], event: dict[str, Any]): | |||
| elif isinstance(value, (str, dict)): | |||
| try: | |||
| value = json.loads(value) if isinstance(value, str) else value | |||
| if isinstance(value, dict) and not filter_event(value, event_value): | |||
| return False | |||
| if isinstance(event_value, list): | |||
There was a problem hiding this comment.
nice addition of being able to handle lists of operators following the AND logic 👍 👏
0465c03 to
b324a55
Compare
| @@ -70,13 +70,16 @@ def matches_event(event_pattern: dict[str, any], event: dict[str, Any]) -> bool: | |||
| ): | |||
| return False | |||
|
|
|||
| # 3. recursively call filter_event(..) for dict types | |||
| # 3. recursively call matches_event(..) for dict types | |||
There was a problem hiding this comment.
nice catch. Thank you @baermat 🙏
| or raw_message.startswith("END") | ||
| or raw_message.startswith( | ||
| "REPORT" | ||
| ) # necessary until tail is updated in docker images. See this PR: |
There was a problem hiding this comment.
Thank you for the clarification. Would be a nice-to-have clarifying comment 👍
| @@ -1,4 +1,7 @@ | |||
| { | |||
Motivation
A customer reported internal errors when submitting events that contained a list within a dict. This PR adds the presumably correct behavior as AWS does it (checked with 2 validated tests). During the replication, another issue surfaced in our own testing code: The function that collects the event logs will also discard actual messages if the message happens to contain certain key words, while we should only discard messages that start with said keywords.
/cc @maxhoheiser
Changes
TODO
What's left to do:
test_put_events_with_target_lambda_list_entry?