feat: match array properties element-wise for non-set operators#92
feat: match array properties element-wise for non-set operators#92
Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Negation operators on arrays use wrong quantifier
- Changed match_strings_non_set to use all? for negation operators (IS_NOT, DOES_NOT_CONTAIN, REGEX_DOES_NOT_MATCH) instead of any?, ensuring correct 'no element matches' semantics.
Or push these changes by commenting:
@cursor push 130164c824
Preview (130164c824)
diff --git a/lib/experiment/evaluation/evaluation.rb b/lib/experiment/evaluation/evaluation.rb
--- a/lib/experiment/evaluation/evaluation.rb
+++ b/lib/experiment/evaluation/evaluation.rb
@@ -82,9 +82,22 @@
end
def match_strings_non_set(prop_values, op, filter_values)
- prop_values.any? { |v| match_string(v, op, filter_values) }
+ if negation_operator?(op)
+ prop_values.all? { |v| match_string(v, op, filter_values) }
+ else
+ prop_values.any? { |v| match_string(v, op, filter_values) }
+ end
end
+ def negation_operator?(op)
+ case op
+ when Operator::IS_NOT, Operator::DOES_NOT_CONTAIN, Operator::REGEX_DOES_NOT_MATCH
+ true
+ else
+ false
+ end
+ end
+
def get_hash(key)
Murmur3.hash32x86(key)
endYou can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit c9f7cb1. Configure here.
|
|
||
| def match_strings_non_set(prop_values, op, filter_values) | ||
| prop_values.any? { |v| match_string(v, op, filter_values) } | ||
| end |
There was a problem hiding this comment.
Negation operators on arrays use wrong quantifier
Medium Severity
match_strings_non_set uses any? uniformly for all operators, including negation ones like IS_NOT, DOES_NOT_CONTAIN, and REGEX_DOES_NOT_MATCH. Because match_string already negates internally (e.g. !matches_is?), any? produces "at least one element doesn't match" rather than "no element matches." This means a property like ["A","B"] simultaneously satisfies both IS "A" and IS NOT "A", which is a logical contradiction. The existing tests only use non-overlapping values for negation, so they don't distinguish between any? and the correct all? (or equivalently, !any? { positive_match }) semantics for negated operators.
Reviewed by Cursor Bugbot for commit c9f7cb1. Configure here.



Summary
Adds element-wise (any-match) evaluation for multi-valued user properties when paired with non-set operators (
is,contains,greater, etc.), aligning the local evaluation engine with Amplitude analytics/charts behavior.Previously, non-set operators always stringified the whole property value via
coerce_string— so an array property like["a","b"]would never satisfyis "a". Now:match_conditioncallscoerce_string_arrayfor every non-null value. If the value is multi-valued (native array or JSON array string) and the operator is non-set, it delegates to a newmatch_strings_non_sethelper that returns true if any element satisfies the operator.is not,does not contain) also use any-match semantics —is not "A"on["A","B"]istruebecause"B"is not"A".coerce_string→match_stringpath unchanged.Also fixes two issues in
coerce_string_array:start_with?('[')pre-check so scalar strings skipJSON.parseand avoid exception-driven control flow on every evaluation.[value]instead ofnil, which caused set operators to spuriously match single-string properties.Tests
spec/experiment/evaluation/evaluation_spec.rbwith an inline test harness (flag_with_condition,context_with_prop,evaluate,assert_match,assert_no_match) and 14 unit cases covering scalars, native arrays, JSON array strings, malformed JSON, and leading-whitespace edge cases.operator testsblock; the deployment key is consolidated toserver-VVhLULXCxxY0xqmszXouXxiEzoeJWmSh(a superset of the prior key, containing all existing flags plus the new multi-value flags).Full suite: 271 examples, 0 failures. Rubocop: clean.
Checklist
Note
Medium Risk
Changes core flag targeting semantics when a property is an array/JSON array string, which can alter which users qualify for segments. Also tightens
coerce_string_arrayparsing behavior, potentially changing outcomes for previously mis-coerced scalar values.Overview
The evaluation engine now treats multi-valued properties (native arrays or JSON array strings) as element-wise inputs for non-set operators like
is,contains, and comparisons, matching if any element satisfies the operator (via newmatch_strings_non_set).coerce_string_arrayis tightened to only parse JSON when the string looks like an array (starts with'[') and to returnnilfor non-array/malformed JSON instead of coercing scalars into single-element arrays, reducing accidental set-operator matches. Tests are expanded with new unit coverage and additional integration cases, plus the integration spec caches the flags URI and updates the deployment key.Reviewed by Cursor Bugbot for commit c73b5c6. Bugbot is set up for automated code reviews on this repo. Configure here.