feat(evaluation): multi-value array property support for non-set operators#65
feat(evaluation): multi-value array property support for non-set operators#65kyeh-amp wants to merge 6 commits into
Conversation
Restructure matchCondition so coerceStringArray runs for every non-null, non-boolean value. Set operators keep matchSet semantics; non-set operators against array-valued properties now delegate to a new matchStringsNonSet helper that returns true if any element satisfies the predicate -- matching Amplitude analytics behavior. Scalar strings short-circuit via a strncmp prefix check in coerceStringArray to avoid json_decode overhead on every eval.
…rray Address code review feedback: - coerceStringArray now drops only null elements (matching the spec's filterNotNull semantic). Previously array_filter without a callback silently dropped "0" and "" as well, which the new any-match path would have made more user-visible. - Remove the unreachable try/catch around json_decode (PHP returns null on error by default; the is_array check already handles it). Drop the now-unused Exception import. - Add two unit assertions pinning the new falsy-string behavior.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 wrongly use any-match for arrays
- Modified matchStringsNonSet to use all-must-satisfy logic for negation operators (IS_NOT, DOES_NOT_CONTAIN, REGEX_DOES_NOT_MATCH), ensuring they require all array elements to satisfy the negation rather than just any element.
Or push these changes by commenting:
@cursor push 56abe827eb
You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 2045fa9. Configure here.
| } | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Negation operators wrongly use any-match for arrays
High Severity
matchStringsNonSet applies "return true if ANY element satisfies" logic uniformly to all operators, including negation ones (is not, does not contain, regex does not match). This causes both is X and is not X to return true for the same array value (e.g., ['a', 'b'] with filter ['a']), breaking complementarity. For targeting, is not X on an array almost certainly means "X is not among the values" (all-must-satisfy / none-matches-positive), not "some element differs from X" which is trivially true for any multi-element array. The integration tests use non-overlapping values that pass under both interpretations, masking this issue.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 2045fa9. Configure here.
There was a problem hiding this comment.
It's a valid assertion of the logic, but the relevance is off. In the end the implementation mirrors the observed filtering logic in charts/analytics.
2 participants
Summary
Non-set operators (
is,is not,contains,does not contain,greater,less, regex, version) now match array-valued user properties element-wise, returning true if any element satisfies the predicate. Previously, array values were stringified viacoerceStringand compared as a single blob, diverging from Amplitude analytics/charts behavior.Applies to both native PHP arrays and JSON-array-string properties (e.g.
'["a","b"]'). Set operators retain their existing semantics. Boolean short-circuit and scalar-string paths are unchanged.Changes
matchConditionrestructured socoerceStringArrayis called for every non-null, non-boolean value. NewmatchStringsNonSethelper delegates tomatchStringper element.coerceStringArraygains a[-prefix pre-check (viastrncmp, PHP 7.4-safe) so scalar strings short-circuit beforejson_decode.coerceStringArraynow correctly preserves"0"and""elements — it used to drop them via defaultarray_filtertruthiness, which contradicted the spec's "filter nulls only" contract.try/catcharoundjson_decoderemoved (PHP returnsnullon parse failure; the existingis_arraycheck already handles it).Tests
EvaluationEngineTest::testMultiValueArrayPropertyMatchingcovering scalars, JSON-array strings, PHP collections, malformed JSON, empty arrays, leading-whitespace strings, and falsy-string elements across both set and non-set operators.EvaluateIntegrationTestexercising the array-property flags on the shared evaluation deployment.Test plan
./vendor/bin/phpunit --no-coverage./vendor/bin/phpstan analyseisoperator against a user property set to a PHP array — should now match any element (previously requiredsetoperator or JSON-array string contortions)Note
Medium Risk
Changes core flag targeting semantics: non-set operators now evaluate array/JSON-array user properties element-wise, which can alter rollout/segment matches for existing flags. Risk is mitigated by added unit/integration coverage but still impacts evaluation behavior broadly.
Overview
Adds multi-value property matching for non-set operators.
matchConditionnow attempts to coerce non-null, non-boolean properties into string arrays and, for non-set operators (e.g.is,contains, comparisons, regex, version), returns true if any element matches via a newmatchStringsNonSethelper; set operators keep existing semantics.Tightens array coercion behavior.
coerceStringArraynow fast-paths non-array strings that don’t start with[before attemptingjson_decode, removes the unusedtry/catch, and filters out onlynullelements (preserving falsy strings like"0"/"") viafilterNonNullStrings.Updates tests. Adds comprehensive unit coverage for scalar/array/JSON-array cases and edge conditions, and extends integration tests for array/JSON-array targeting; also updates the integration deployment key used to fetch test flags.
Reviewed by Cursor Bugbot for commit 2045fa9. Bugbot is set up for automated code reviews on this repo. Configure here.