search

aws / event-ruler

Event Ruler is a Java library that allows matching many thousands of Events per second to any number of expressive and sophisticated rules.
Apache License 2.0
569 stars 65 forks source link

Something weird in testWildcardLeadingWildcardCharacterNotUsedByExactMatch()? #181

Closed timbray closed 2 months ago

timbray commented 2 months ago

It's possible that I just don't understand how testPatternPermutations() works, because it's a complex tangle of stream idioms that I happily forgot the instant I was no longer doing Java every day.

But the string "hello" appears in both the list of strings that should match the wildcard "*hello" and the noMatches list of strings that (if I read the code correctly) shouldn't match the machine after all the patterns have been added.

baldawar commented 2 months ago

Looks like this is tied to matchesValue being a Set. https://github.com/aws/event-ruler/blob/96bb056848cbb3e98023244543d23a1557137653/src/test/software/amazon/event/ruler/ByteMachineTest.java#L2693.

timbray commented 2 months ago

So it works because of the order of writes to that map. Not a real problem then; but probably best to remove "hello" from the noMatches list in some future PR.

baldawar commented 2 months ago

Yup. I'm looking for more such cases, e.g. testWildcardSecondLastCharWildcardOccursBeforeDivergentFinalCharacterOfExactMatch and helo. Will send a PR after that.

timbray commented 2 months ago

Of course, I was copying Ruler unit tests for Quamina and I couldn't get this one to pass. Had me seriously baffled for a few minutes.

baldawar commented 2 months ago

It looks like the tests were copied from other tests and fixed for any failures. Given the number of tests Shawn added, totally can see how few edge cases were missed.

Hopefully the new fix is enough to flag the missing cases. I couldn’t find anything else that would be worth changing/adding yet but I’ll follow along Quamina’s changes to see if there’s more I can add to ruler.

baldawar commented 2 months ago

Thanks for pointing this out @timbray