diff --git a/latest_migrations.manifest b/latest_migrations.manifest index c840797d2d77c..47fc72bba9b58 100644 --- a/latest_migrations.manifest +++ b/latest_migrations.manifest @@ -3,7 +3,7 @@ auth: 0012_alter_user_first_name_max_length axes: 0006_remove_accesslog_trusted contenttypes: 0002_remove_content_type_name ee: 0005_project_based_permissioning -posthog: 0172_person_properties_last_operation +posthog: 0173_should_update_person_props_function rest_hooks: 0002_swappable_hook_model sessions: 0001_initial -social_django: 0010_uid_db_index +social_django: 0010_uid_db_index \ No newline at end of file diff --git a/posthog/management/commands/test_migrations_are_null.py b/posthog/management/commands/test_migrations_are_null.py index be608811eed63..73c8f17b8e13b 100644 --- a/posthog/management/commands/test_migrations_are_null.py +++ b/posthog/management/commands/test_migrations_are_null.py @@ -15,7 +15,7 @@ def run_and_check_migration(variable): try: results = re.findall(r"([a-z]+)\/migrations\/([a-zA-Z_0-9]+)\.py", variable)[0] sql = call_command("sqlmigrate", results[0], results[1]) - if "NOT NULL" in sql and "Create model" not in sql: + if "NOT NULL" in sql and "Create model" not in sql and "-- not-null-ignore" not in sql: print( f"\n\n\033[91mFound a non-null field added to an existing model. This will lock up the table while migrating. Please add 'null=True, blank=True' to the field", "red", diff --git a/posthog/migrations/0173_should_update_person_props_function.py b/posthog/migrations/0173_should_update_person_props_function.py new file mode 100644 index 0000000000000..862eefa3c4c9e --- /dev/null +++ b/posthog/migrations/0173_should_update_person_props_function.py @@ -0,0 +1,39 @@ +# Generated by Django 3.1.12 on 2021-10-05 13:06 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("posthog", "0172_person_properties_last_operation"), + ] + + operations = [ + migrations.RunSQL( + """ + -- not-null-ignore + CREATE OR REPLACE FUNCTION should_update_person_prop( + in person_id int, + in prop_key text, + in _timestamp text, + in operation text, + out should_update bool + ) + AS $$ + SELECT NOT property_exists OR + (operation='set' AND _timestamp > stored_timestamp) OR + (last_operation='set_once' AND _timestamp < stored_timestamp) + FROM ( + SELECT + properties->prop_key IS NOT NULL as property_exists, + COALESCE(properties_last_updated_at ->> prop_key, '0') as stored_timestamp, + COALESCE(properties_last_operation ->> prop_key, 'set') as last_operation + FROM posthog_person + WHERE id=person_id + ) as person_props + $$ + LANGUAGE SQL; + """ + ) + ] diff --git a/posthog/test/test_should_update_person_prop.py b/posthog/test/test_should_update_person_prop.py new file mode 100644 index 0000000000000..c98f226f837c4 --- /dev/null +++ b/posthog/test/test_should_update_person_prop.py @@ -0,0 +1,239 @@ +from datetime import datetime + +from django.db import connection + +from posthog.models import Person +from posthog.test.base import BaseTest + +# How we expect this function to behave: +# | call | value exists | call TS is ___ existing TS | previous fn | write/override +# 1| set | no | N/A | N/A | yes +# 2| set_once | no | N/A | N/A | yes +# 3| set | yes | before | set | no +# 4| set | yes | before | set_once | yes +# 5| set | yes | after | set | yes +# 6| set | yes | after | set_once | yes +# 7| set_once | yes | before | set | no +# 8| set_once | yes | before | set_once | yes +# 9| set_once | yes | after | set | no +# 10| set_once | yes | after | set_once | no +# 11| set | yes | equal | set | no +# 12| set_once | yes | equal | set | no +# 13| set | yes | equal | set_once | no +# 14| set_once | yes | equal | set_once | no + +# Refers to migration 0173_should_update_person_props_function +# This is a Postgres function we use in the plugin server +class TestShouldUpdatePersonProp(BaseTest): + def test_update_without_properties_last_updated_at(self): + person = Person.objects.create( + team=self.team, + properties={"a": 0, "b": 0}, + properties_last_updated_at={}, + properties_last_operation={"a": "set", "b": "set_once"}, + ) + + with connection.cursor() as cursor: + cursor.execute( + f""" + SELECT + should_update_person_prop({person.id}, 'non-existent prop', now()::text, 'set'), + should_update_person_prop({person.id}, 'non-existent prop', now()::text, 'set_once') + """ + ) + + result = cursor.fetchall() + set_op_result = result[0][0] + set_once_op_result = result[0][1] + + self.assertEqual(set_op_result, True) + self.assertEqual(set_once_op_result, True) + + def test_update_without_properties_last_operation(self): + person = Person.objects.create( + team=self.team, + properties={"a": 0, "b": 0}, + properties_last_updated_at={ + "a": datetime(2050, 1, 1, 1, 1, 1).isoformat(), + "b": datetime(2050, 1, 1, 1, 1, 1).isoformat(), + }, + properties_last_operation={}, + ) + + with connection.cursor() as cursor: + cursor.execute( + f""" + SELECT + should_update_person_prop({person.id}, 'non-existent prop', now()::text, 'set'), + should_update_person_prop({person.id}, 'non-existent prop', now()::text, 'set_once') + """ + ) + + result = cursor.fetchall() + set_op_result = result[0][0] + set_once_op_result = result[0][1] + + self.assertEqual(set_op_result, True) + self.assertEqual(set_once_op_result, True) + + # tests cases 1 and 2 from the table + def test_update_non_existent_prop(self): + person = Person.objects.create( + team=self.team, properties={}, properties_last_updated_at={}, properties_last_operation={} + ) + + with connection.cursor() as cursor: + cursor.execute( + f""" + SELECT + should_update_person_prop({person.id}, 'non-existent prop', now()::text, 'set'), + should_update_person_prop({person.id}, 'non-existent prop', now()::text, 'set_once') + """ + ) + + result = cursor.fetchall() + set_op_result = result[0][0] + set_once_op_result = result[0][1] + + self.assertEqual(set_op_result, True) + self.assertEqual(set_once_op_result, True) + + # tests cases 3 and 4 from the table + def test_set_operation_with_earlier_timestamp(self): + person = Person.objects.create( + team=self.team, + properties={"a": 0, "b": 0}, + properties_last_updated_at={ + "a": datetime(2050, 1, 1, 1, 1, 1).isoformat(), + "b": datetime(2050, 1, 1, 1, 1, 1).isoformat(), + }, + properties_last_operation={"a": "set", "b": "set_once"}, + ) + + with connection.cursor() as cursor: + cursor.execute( + f""" + SELECT + should_update_person_prop({person.id}, 'a', now()::text, 'set'), + should_update_person_prop({person.id}, 'b', now()::text, 'set') + """ + ) + + result = cursor.fetchall() + previous_op_set_result = result[0][0] + previous_op_set_once_result = result[0][1] + + self.assertEqual(previous_op_set_result, False) + self.assertEqual(previous_op_set_once_result, True) + + # tests cases 5 and 6 from the table + def test_set_operation_with_older_timestamp(self): + person = Person.objects.create( + team=self.team, + properties={"a": 0, "b": 0}, + properties_last_updated_at={ + "a": datetime(2000, 1, 1, 1, 1, 1).isoformat(), + "b": datetime(2000, 1, 1, 1, 1, 1).isoformat(), + }, + properties_last_operation={"a": "set", "b": "set_once"}, + ) + + with connection.cursor() as cursor: + cursor.execute( + f""" + SELECT + should_update_person_prop({person.id}, 'a', now()::text, 'set'), + should_update_person_prop({person.id}, 'b', now()::text, 'set') + """ + ) + + result = cursor.fetchall() + previous_op_set_result = result[0][0] + previous_op_set_once_result = result[0][1] + + self.assertEqual(previous_op_set_result, True) + self.assertEqual(previous_op_set_once_result, True) + + # tests cases 7 and 8 from the table + def test_set_once_operation_with_earlier_timestamp(self): + person = Person.objects.create( + team=self.team, + properties={"a": 0, "b": 0}, + properties_last_updated_at={ + "a": datetime(2050, 1, 1, 1, 1, 1).isoformat(), + "b": datetime(2050, 1, 1, 1, 1, 1).isoformat(), + }, + properties_last_operation={"a": "set", "b": "set_once"}, + ) + + with connection.cursor() as cursor: + cursor.execute( + f""" + SELECT + should_update_person_prop({person.id}, 'a', now()::text, 'set_once'), + should_update_person_prop({person.id}, 'b', now()::text, 'set_once') + """ + ) + + result = cursor.fetchall() + previous_op_set_result = result[0][0] + previous_op_set_once_result = result[0][1] + + self.assertEqual(previous_op_set_result, False) + self.assertEqual(previous_op_set_once_result, True) + + # tests cases 9 and 10 from the table + def test_set_once_operation_with_older_timestamp(self): + person = Person.objects.create( + team=self.team, + properties={"a": 0, "b": 0}, + properties_last_updated_at={ + "a": datetime(2000, 1, 1, 1, 1, 1).isoformat(), + "b": datetime(2000, 1, 1, 1, 1, 1).isoformat(), + }, + properties_last_operation={"a": "set", "b": "set_once"}, + ) + + with connection.cursor() as cursor: + cursor.execute( + f""" + SELECT + should_update_person_prop({person.id}, 'a', now()::text, 'set_once'), + should_update_person_prop({person.id}, 'b', now()::text, 'set_once') + """ + ) + + result = cursor.fetchall() + previous_op_set_result = result[0][0] + previous_op_set_once_result = result[0][1] + + self.assertEqual(previous_op_set_result, False) + self.assertEqual(previous_op_set_once_result, False) + + # tests cases 11-14 from the table + def test_equal_timestamps(self): + timestamp = datetime(2000, 1, 1, 1, 1, 1).isoformat() + person = Person.objects.create( + team=self.team, + properties={"a": 0, "b": 0}, + properties_last_updated_at={"a": timestamp, "b": timestamp,}, + properties_last_operation={"a": "set", "b": "set_once"}, + ) + + with connection.cursor() as cursor: + cursor.execute( + f""" + SELECT + should_update_person_prop({person.id}, 'a', '{timestamp}', 'set_once'), + should_update_person_prop({person.id}, 'a', '{timestamp}', 'set'), + should_update_person_prop({person.id}, 'b', '{timestamp}', 'set_once'), + should_update_person_prop({person.id}, 'b', '{timestamp}', 'set') + """ + ) + + results = cursor.fetchall()[0] + + self.assertEqual(results[0], False) + self.assertEqual(results[1], False) + self.assertEqual(results[2], False) + self.assertEqual(results[3], False)