fix(plugin-server): applyEventPropertyUpdates returns whether it did …

…an update, rather than needing to use deep-equals
PostHog · Oct 5, 2023 · 151e025 · 151e025
1 parent bf8dcd7
commit 151e025
Show file tree

Hide file tree

Showing 2 changed files with 34 additions and 21 deletions.
diff --git a/plugin-server/src/worker/ingestion/person-state.ts b/plugin-server/src/worker/ingestion/person-state.ts
@@ -1,6 +1,5 @@
 import { PluginEvent, Properties } from '@posthog/plugin-scaffold'
 import * as Sentry from '@sentry/node'
-import equal from 'fast-deep-equal'
 import { StatsD } from 'hot-shots'
 import { ProducerRecord } from 'kafkajs'
 import { DateTime } from 'luxon'
@@ -214,11 +213,11 @@ export class PersonState {
     }
 
     private async updatePersonProperties(person: Person): Promise<Person> {
-        const update: Partial<Person> = {}
-        const updatedProperties = this.applyEventPropertyUpdates(person.properties || {})
+        person.properties ||= {}
 
-        if (!equal(person.properties, updatedProperties)) {
-            update.properties = updatedProperties
+        const update: Partial<Person> = {}
+        if (this.applyEventPropertyUpdates(person.properties)) {
+            update.properties = person.properties
         }
         if (this.updateIsIdentified && !person.is_identified) {
             update.is_identified = true
@@ -231,33 +230,39 @@ export class PersonState {
         return person
     }
 
-    private applyEventPropertyUpdates(personProperties: Properties): Properties {
-        const updatedProperties = { ...personProperties }
-
+    /**
+     * @param personProperties Properties of the person to be updated, these are updated in place.
+     * @returns true if the properties were changed, false if they were not
+     */
+    private applyEventPropertyUpdates(personProperties: Properties): boolean {
         const properties: Properties = this.eventProperties['$set'] || {}
         const propertiesOnce: Properties = this.eventProperties['$set_once'] || {}
         const unsetProps = this.eventProperties['$unset']
         const unsetProperties: Array<string> = Array.isArray(unsetProps)
             ? unsetProps
             : Object.keys(unsetProps || {}) || []
 
-        // Figure out which properties we are actually setting
+        let updated = false
         Object.entries(propertiesOnce).map(([key, value]) => {
             if (typeof personProperties[key] === 'undefined') {
-                updatedProperties[key] = value
+                updated = true
+                personProperties[key] = value
             }
         })
         Object.entries(properties).map(([key, value]) => {
             if (personProperties[key] !== value) {
-                updatedProperties[key] = value
+                updated = true
+                personProperties[key] = value
             }
         })
-
         unsetProperties.forEach((propertyKey) => {
-            delete updatedProperties[propertyKey]
+            if (propertyKey in personProperties) {
+                updated = true
+                delete personProperties[propertyKey]
+            }
         })
 
-        return updatedProperties
+        return updated
     }
 
     // Alias & merge
@@ -439,8 +444,8 @@ export class PersonState {
         //   that guarantees consistency of how properties are processed regardless of persons created_at timestamps and rollout state
         //   we're calling aliasDeprecated as we need to refresh the persons info completely first
 
-        let properties: Properties = { ...otherPerson.properties, ...mergeInto.properties }
-        properties = this.applyEventPropertyUpdates(properties)
+        const properties: Properties = { ...otherPerson.properties, ...mergeInto.properties }
+        this.applyEventPropertyUpdates(properties)
 
         if (this.poEEmbraceJoin) {
             // Optimize merging persons to keep using the person id that has longer history,

diff --git a/plugin-server/tests/worker/ingestion/person-state.test.ts b/plugin-server/tests/worker/ingestion/person-state.test.ts
@@ -244,16 +244,24 @@ describe('PersonState.update()', () => {
 
     describe('on person update', () => {
         it('updates person properties', async () => {
-            await hub.db.createPerson(timestamp, { b: 3, c: 4 }, {}, {}, teamId, null, false, uuid.toString(), [
-                'new-user',
-            ])
+            await hub.db.createPerson(
+                timestamp,
+                { b: 3, c: 4, toString: {} },
+                {},
+                {},
+                teamId,
+                null,
+                false,
+                uuid.toString(),
+                ['new-user']
+            )
 
             const person = await personState({
                 event: '$pageview',
                 distinct_id: 'new-user',
                 properties: {
                     $set_once: { c: 3, e: 4 },
-                    $set: { b: 4 },
+                    $set: { b: 4, toString: 1 },
                 },
             }).updateProperties()
             await hub.db.kafkaProducer.flush()
@@ -262,7 +270,7 @@ describe('PersonState.update()', () => {
                 expect.objectContaining({
                     id: expect.any(Number),
                     uuid: uuid.toString(),
-                    properties: { b: 4, c: 4, e: 4 },
+                    properties: { b: 4, c: 4, e: 4, toString: 1 },
                     created_at: timestamp,
                     version: 1,
                     is_identified: false,