Skip to content

Commit

Permalink
fix(plugin-server): applyEventPropertyUpdates returns whether it did …
Browse files Browse the repository at this point in the history
…an update, rather than needing to use deep-equals
  • Loading branch information
bretthoerner committed Oct 5, 2023
1 parent bf8dcd7 commit ef79531
Showing 1 changed file with 19 additions and 15 deletions.
34 changes: 19 additions & 15 deletions plugin-server/src/worker/ingestion/person-state.ts
Original file line number Diff line number Diff line change
@@ -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'
Expand Down Expand Up @@ -215,10 +214,9 @@ export class PersonState {

private async updatePersonProperties(person: Person): Promise<Person> {
const update: Partial<Person> = {}
const updatedProperties = this.applyEventPropertyUpdates(person.properties || {})

if (!equal(person.properties, updatedProperties)) {
update.properties = updatedProperties
if (person.properties && this.applyEventPropertyUpdates(person.properties)) {
update.properties = person.properties
}
if (this.updateIsIdentified && !person.is_identified) {
update.is_identified = true
Expand All @@ -231,33 +229,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 (personProperties[propertyKey]) {
updated = true
delete personProperties[propertyKey]
}
})

return updatedProperties
return updated
}

// Alias & merge
Expand Down Expand Up @@ -439,8 +443,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,
Expand Down

0 comments on commit ef79531

Please sign in to comment.