Match an empty prefix #12147
Match an empty prefix #12147
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
5b29261 to
1b44de8
Compare
I was using this pattern to match all events: https://stackoverflow.com/questions/62406933/aws-eventbridge-pattern-to-capture-all-events It works in AWS and in localstack before 4.0.3. Because the new rule engine was using truthiness to determine if the condition was a prefix one in `condition.get('prefix')`, it would assume a prefix pattern of `''` isn't actually a prefix pattern. This instead checks for `None` to determine the condition type and gets empty prefixes to match again.
1b44de8 to
cc69af2
Compare
|
I have read the CLA Document and I hereby sign the CLA |
|
I don't believe I can add labels to this PR, but I think it should be |
There was a problem hiding this comment.
Nice catch! This is a great fix, thanks a lot for the contribution!
The new test case should be run against AWS to be sure it properly works against it. We have documentation around it here: https://github.com/localstack/localstack/tree/master/docs/testing/integration-tests#test-against-amazon-web-services
Let me know if it is possible for you to run the validation, otherwise we could probably merge if we manually try it. You could probably add the suffix test case as well.
Regarding the fix, I think it might run deeper: we use if <condition> := dict.get(<key>) quite a lot in _evaluate_condition, and I think your fix could probably generalized and used throughout, for example in line 71 and 73.
Let me know if you'd be interested in tackling the issue throughout, or you'd rather merge it as is and somebody else will tackle the rest. This is already a great fix in itself 👌
|
Thanks! I was able to find and test this without actually building localstack myself. Y'all have excellent test cases! It'll probably be a couple weeks till I can get time to set things up locally, run the validation, and so on. As such, I'd rather we merge it as is and leave the rest for later. I do use the empty prefix pattern in AWS' EventBridge, so I have manually verified that it works there. I haven't used the empty suffix pattern, so we may want to remove that change till someone can verify it. Let me know what you'd prefer! |
There was a problem hiding this comment.
Thanks again for the PR! I've ran your test case against AWS and it indeed works, so I'll approve and merge this PR!
I'll tackle the follow-up, thank you so much for providing the fix for the most common use case and validating it. I've also validated the empty suffix works, so that's all good!
Welcome to the list of contributors of LocalStack 🙌
Motivation
#12148
I was using this pattern to match all events:
https://stackoverflow.com/questions/62406933/aws-eventbridge-pattern-to-capture-all-events
It works in AWS and in localstack before 4.0.3.
Changes
The new rule engine was using truthiness via
condition.get('prefix')to determine if a condition was a prefix condition.That assumes a prefix pattern of
''isn't actually a prefix pattern.This change instead checks for
Noneto determine the condition type and gets empty prefixes to match again.Testing
Added a new JSON test case for this