Repository: PostHog/posthog
Pull request: fix(action-matching): Match on nulls properly
## Problem

Fixes https://github.com/PostHog/posthog/issues/12893

<!-- Who are we building for, what are their needs, why is this important? -->

## Changes

<!-- If there are frontend changes, please include screenshots. -->
<!-- If a reference design was involved, include a link to the relevant Figma frame! -->

👉 *Stay up-to-date with [PostHog coding conventions](https://posthog.com/docs/contribute/coding-conventions) for a smoother review.*

## How did you test this code?

<!-- Briefly describe the steps you took. -->
<!-- Include automated tests if possible, otherwise describe the manual testing routine. -->


Problem statement:
Webhook sends notifications on actions that don't meet the filter.
## Bug description

We have an action that is defined on an event and has two filters.
1. X event property is set.
2. X event != null.

The action is defined to _Post to webhook when this action is triggered_.

The bug is that PostHog wrongfully sends notification when the action is not triggered.
I thing this happens because only **one of the filters match but not both**.
Needless to say that the filters are defined with an AND operator.
In addition to that, I don't see any events under the _Matching events_ section below within the action's page.

Could you please make the webhook trigger only when the action is matched on both filters?
Thanks a lot.

## Environment

- [x] PostHog Cloud
- [ ] self-hosted PostHog, version/commit: please provide

Hints from issue discussion:
Hey Elon, mind sharing a link to the action that's problematic?
@neilkakkar Not sure if action IDs are global to all accounts, but here's mine - https://app.posthog.com/data-management/actions/20804.
The issue seems to be with the null exact match:

In Clickhouse matching:
```
(   has(['null'], replaceRegexpAll(JSONExtractRaw(properties, 'env__elementary_hosted_account_id'), '^"|"$', ''))
```

In plugin server matching:

the `"null"` string != `null`, hence the webhook firing incorrectly.
How could I fix it?
Is there anything I can do on my end to resolve this?

Also, is there a way to filter on nested objects?
I have to flat my event properties using an extension to allow myself to filter on them.
It's a bug on our end, I'm working on a fix right now :)

Commit message: one more test

Patch to review:
diff --git a/plugin-server/src/worker/ingestion/action-matcher.ts b/plugin-server/src/worker/ingestion/action-matcher.ts
index bbbbee282b09d..36f378f5d2083 100644
--- a/plugin-server/src/worker/ingestion/action-matcher.ts
+++ b/plugin-server/src/worker/ingestion/action-matcher.ts
@@ -52,6 +52,25 @@ export function castingCompare(
     b: any,
     operator: PropertyOperator.Exact | PropertyOperator.IsNot | PropertyOperator.LessThan | PropertyOperator.GreaterThan
 ): boolean {
+    // Check null case first
+    if (a === null || b === null) {
+        const aNullCast = String(a)
+        const bNullCast = String(b)
+        switch (operator) {
+            case PropertyOperator.Exact:
+                if (aNullCast === bNullCast) {
+                    return true
+                }
+                break
+            case PropertyOperator.IsNot:
+                if (aNullCast === bNullCast) {
+                    return false
+                }
+                break
+            default:
+                break
+        }
+    }
     // Check basic case first
     switch (operator) {
         case PropertyOperator.Exact:
diff --git a/plugin-server/tests/worker/ingestion/action-matcher.test.ts b/plugin-server/tests/worker/ingestion/action-matcher.test.ts
index 15e8bdcf82968..fc3da2c35eb91 100644
--- a/plugin-server/tests/worker/ingestion/action-matcher.test.ts
+++ b/plugin-server/tests/worker/ingestion/action-matcher.test.ts
@@ -940,6 +940,69 @@ describe('ActionMatcher', () => {
 
             expect(await actionMatcher.match(eventExampleOk1)).toEqual([actionDefinition])
         })
+
+        it('properly handles is_not null string coercion', async () => {
+            const actionDefinitionOpIsSet: Action = await createTestAction([
+                {
+                    properties: [
+                        { type: 'event', key: 'foo', operator: 'is_set' as PropertyOperator },
+                        { type: 'event', key: 'foo', value: ['null'], operator: 'is_not' as PropertyOperator },
+                    ],
+                },
+            ])
+
+            const eventFooBar = createTestEvent({ properties: { foo: 'bar' } })
+            const eventFooBarPolPot = createTestEvent({ properties: { foo: null, pol: 'pot' } })
+            const eventFooBaR = createTestEvent({ properties: { foo: 'baR' } })
+            const eventFooBaz = createTestEvent({ properties: { foo: 'baz' } })
+            const eventFooRabarbar = createTestEvent({ properties: { foo: 'rabarbar' } })
+            const eventFooNumber = createTestEvent({ properties: { foo: 7 } })
+            const eventNoNothing = createTestEvent()
+            const eventFigNumber = createTestEvent({ properties: { fig: 999 } })
+            const eventFooTrue = createTestEvent({ properties: { foo: true } })
+            const eventFooNull = createTestEvent({ properties: { foo: null } })
+
+            expect(await actionMatcher.match(eventFooBar)).toEqual([actionDefinitionOpIsSet])
+            expect(await actionMatcher.match(eventFooBarPolPot)).toEqual([])
+            expect(await actionMatcher.match(eventFooBaR)).toEqual([actionDefinitionOpIsSet])
+            expect(await actionMatcher.match(eventFooBaz)).toEqual([actionDefinitionOpIsSet])
+            expect(await actionMatcher.match(eventFooRabarbar)).toEqual([actionDefinitionOpIsSet])
+            expect(await actionMatcher.match(eventFooNumber)).toEqual([actionDefinitionOpIsSet])
+            expect(await actionMatcher.match(eventNoNothing)).toEqual([])
+            expect(await actionMatcher.match(eventFigNumber)).toEqual([])
+            expect(await actionMatcher.match(eventFooTrue)).toEqual([actionDefinitionOpIsSet])
+            expect(await actionMatcher.match(eventFooNull)).toEqual([])
+        })
+
+        it('properly handles exact null string coercion', async () => {
+            const actionDefinitionOpIsSet: Action = await createTestAction([
+                {
+                    properties: [{ type: 'event', key: 'foo', value: ['null'] }],
+                },
+            ])
+
+            const eventFooBar = createTestEvent({ properties: { foo: 'bar' } })
+            const eventFooBarPolPot = createTestEvent({ properties: { foo: null, pol: 'pot' } })
+            const eventFooBaR = createTestEvent({ properties: { foo: 'baR' } })
+            const eventFooBaz = createTestEvent({ properties: { foo: 'baz' } })
+            const eventFooRabarbar = createTestEvent({ properties: { foo: 'rabarbar' } })
+            const eventFooNumber = createTestEvent({ properties: { foo: 7 } })
+            const eventNoNothing = createTestEvent()
+            const eventFigNumber = createTestEvent({ properties: { fig: 999 } })
+            const eventFooTrue = createTestEvent({ properties: { foo: true } })
+            const eventFooNull = createTestEvent({ properties: { foo: null } })
+
+            expect(await actionMatcher.match(eventFooBar)).toEqual([])
+            expect(await actionMatcher.match(eventFooBarPolPot)).toEqual([actionDefinitionOpIsSet])
+            expect(await actionMatcher.match(eventFooBaR)).toEqual([])
+            expect(await actionMatcher.match(eventFooBaz)).toEqual([])
+            expect(await actionMatcher.match(eventFooRabarbar)).toEqual([])
+            expect(await actionMatcher.match(eventFooNumber)).toEqual([])
+            expect(await actionMatcher.match(eventNoNothing)).toEqual([])
+            expect(await actionMatcher.match(eventFigNumber)).toEqual([])
+            expect(await actionMatcher.match(eventFooTrue)).toEqual([])
+            expect(await actionMatcher.match(eventFooNull)).toEqual([actionDefinitionOpIsSet])
+        })
     })
 
     describe('#checkElementsAgainstSelector()', () => {


Code review comments:
diff_hunk: @@ -52,6 +52,25 @@ export function castingCompare(
     b: any,
     operator: PropertyOperator.Exact | PropertyOperator.IsNot | PropertyOperator.LessThan | PropertyOperator.GreaterThan
 ): boolean {
+    // Check null case first
+    if (a === null || b === null) {
path: plugin-server/src/worker/ingestion/action-matcher.ts
text: @user1:
I suppose this should be in the `if (typeof a !== typeof b)` block, since this `"null"` vs. `null` comparison only is relevant when one of the values is a string, and the other is a null.

@author:
That doesn't work, since the `a != b` in the basic check short circuits first, as `"null" != null` 😬 

@user1:
Ohh, right, so maybe we should do `if (a === null) a = "null"; if (b === null) b = "null"`? Trying to somehow reduce the line count here 😅