Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(Persons): Remove foreign key constraints from Persons and related models #27636

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
+19 −54
Draft

chore(Persons): Remove foreign key constraints from Persons and related models #27636

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions posthog/admin/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@
PluginAdmin,
TextAdmin,
CohortAdmin,
PersonAdmin,
PersonDistinctIdAdmin,
SurveyAdmin,
DataWarehouseTableAdmin,
ProjectAdmin,
Expand Down Expand Up @@ -70,8 +68,6 @@
admin.site.register(Text, TextAdmin)

admin.site.register(Cohort, CohortAdmin)
admin.site.register(Person, PersonAdmin)
admin.site.register(PersonDistinctId, PersonDistinctIdAdmin)

admin.site.register(Survey, SurveyAdmin)

Expand Down
2 changes: 0 additions & 2 deletions posthog/admin/admins/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@
from .insight_admin import InsightAdmin
from .instance_setting_admin import InstanceSettingAdmin
from .organization_admin import OrganizationAdmin
from .person_admin import PersonAdmin
from .person_distinct_id_admin import PersonDistinctIdAdmin
from .plugin_admin import PluginAdmin
from .plugin_config_admin import PluginConfigAdmin
from .survey_admin import SurveyAdmin
Expand Down
19 changes: 0 additions & 19 deletions posthog/admin/admins/person_admin.py
View file
Open in desktop

This file was deleted.

11 changes: 0 additions & 11 deletions posthog/admin/admins/person_distinct_id_admin.py
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion posthog/migrations/max_migration.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
0545_insight_filters_to_query
0546_remove_featureflaghashkeyoverride_unique_hash_key_for_a_user_team_feature_flag_combo_and_more
3 changes: 1 addition & 2 deletions posthog/models/cohort/cohort.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@
deleted = models.BooleanField(default=False)
filters = models.JSONField(null=True, blank=True)
query = models.JSONField(null=True, blank=True)
people = models.ManyToManyField("Person", through="CohortPeople")
version = models.IntegerField(blank=True, null=True)
pending_version = models.IntegerField(blank=True, null=True)
count = models.IntegerField(blank=True, null=True)
Expand Down Expand Up @@ -268,7 +267,7 @@
for i in range(0, len(items), batchsize):
batch = items[i : i + batchsize]
persons_query = (
Person.objects.filter(team_id=team_id)

Check failure on line 270 in posthog/models/cohort/cohort.py

View workflow job for this annotation

GitHub Actions / Python code quality checks 

Cannot resolve keyword 'cohort' into field. Choices are: cohortpeople, created_at, featureflaghashkeyoverride, id, is_identified, is_user_id, persondistinctid, properties, properties_last_operation, properties_last_updated_at, team_id, uuid, version
.filter(
Q(
persondistinctid__team_id=team_id,
Expand Down Expand Up @@ -328,7 +327,7 @@
for i in range(0, len(items), batchsize):
batch = items[i : i + batchsize]
persons_query = (
Person.objects.filter(team_id=team_id).filter(uuid__in=batch).exclude(cohort__id=self.id)

Check failure on line 330 in posthog/models/cohort/cohort.py

View workflow job for this annotation

GitHub Actions / Python code quality checks 

Cannot resolve keyword 'cohort' into field. Choices are: cohortpeople, created_at, featureflaghashkeyoverride, id, is_identified, is_user_id, persondistinctid, properties, properties_last_operation, properties_last_updated_at, team_id, uuid, version
)
if insert_in_clickhouse:
insert_static_cohort(
Expand Down Expand Up @@ -376,7 +375,7 @@

class CohortPeople(models.Model):
id = models.BigAutoField(primary_key=True)
cohort = models.ForeignKey("Cohort", on_delete=models.CASCADE)
cohort_id = models.IntegerField()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: removing foreign key constraint allows invalid cohort_ids to be inserted - consider adding application-level validation

person = models.ForeignKey("Person", on_delete=models.CASCADE)
version = models.IntegerField(blank=True, null=True)

Expand Down
6 changes: 4 additions & 2 deletions posthog/models/feature_flag/feature_flag.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,13 +371,15 @@ class FeatureFlagHashKeyOverride(models.Model):
# and we only ever want to get the key.
feature_flag_key = models.CharField(max_length=400)
person = models.ForeignKey("Person", on_delete=models.CASCADE)
team = models.ForeignKey("Team", on_delete=models.CASCADE)
team_id = (
models.IntegerField()
) # Note: in order to move persons to separate database, this is a int instead of foreingkey
Comment on lines +374 to +376
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Consider adding a db_index=True to team_id since it was previously indexed as a foreign key

hash_key = models.CharField(max_length=400)

class Meta:
constraints = [
models.UniqueConstraint(
fields=["team", "person", "feature_flag_key"],
fields=["team_id", "person", "feature_flag_key"],
name="Unique hash_key for a user/team/feature_flag combo",
)
]
Expand Down
26 changes: 13 additions & 13 deletions posthog/models/person/person.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,9 @@ class Person(models.Model):
# used for evaluating if we need to override the value or not (value: set or set_once)
properties_last_operation = models.JSONField(null=True, blank=True)

team = models.ForeignKey("Team", on_delete=models.CASCADE)
team_id = models.IntegerField()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally this all makes sense. Long term I think we will want to ditch the Django models alltogether and handle migrations from the node service instead (or whatever service "owns" Persons).

I guess we lose cascading deleted here although I'm not sure if that ever actually happened?

properties = models.JSONField(default=dict)
is_user = models.ForeignKey("User", on_delete=models.CASCADE, null=True, blank=True)
is_user_id = models.IntegerField(null=True, blank=True)
Comment on lines +40 to +42
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Removing ForeignKey constraints means application code must ensure team_id and is_user_id values are valid. Consider adding validation in save() method.

is_identified = models.BooleanField(default=False)
uuid = models.UUIDField(db_index=True, default=UUIDT, editable=False)

Expand Down Expand Up @@ -118,35 +118,35 @@ def split_person(self, main_distinct_id: Optional[str], max_splits: Optional[int


class PersonDistinctId(models.Model):
team = models.ForeignKey("Team", on_delete=models.CASCADE, db_index=False)
team_id = models.IntegerField(db_index=False)
person = models.ForeignKey(Person, on_delete=models.CASCADE)
distinct_id = models.CharField(max_length=400)

# current version of the id, used to sync with ClickHouse and collapse rows correctly for new clickhouse table
version = models.BigIntegerField(null=True, blank=True)

class Meta:
constraints = [models.UniqueConstraint(fields=["team", "distinct_id"], name="unique distinct_id for team")]
constraints = [models.UniqueConstraint(fields=["team_id", "distinct_id"], name="unique distinct_id for team")]


class PersonlessDistinctId(models.Model):
id = models.BigAutoField(primary_key=True)
team = models.ForeignKey("Team", on_delete=models.CASCADE, db_index=False)
team_id = models.IntegerField(db_index=False)
distinct_id = models.CharField(max_length=400)
is_merged = models.BooleanField(default=False)
created_at = models.DateTimeField(auto_now_add=True, blank=True)

class Meta:
constraints = [
models.UniqueConstraint(fields=["team", "distinct_id"], name="unique personless distinct_id for team")
models.UniqueConstraint(fields=["team_id", "distinct_id"], name="unique personless distinct_id for team")
]


class PersonOverrideMapping(models.Model):
"""A model of persons to be overriden in merge or merge-like events."""

id = models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name="ID")
team_id = models.BigIntegerField()
team_id = models.IntegerField()
uuid = models.UUIDField()

class Meta:
Expand All @@ -169,7 +169,7 @@ class PersonOverride(models.Model):
"""

id = models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name="ID")
team = models.ForeignKey("Team", on_delete=models.CASCADE)
team_id = models.IntegerField()

old_person_id = models.ForeignKey(
"PersonOverrideMapping",
Expand All @@ -190,7 +190,7 @@ class PersonOverride(models.Model):
class Meta:
constraints = [
models.UniqueConstraint(
fields=["team", "old_person_id"],
fields=["team_id", "old_person_id"],
name="unique override per old_person_id",
),
models.CheckConstraint(
Expand Down Expand Up @@ -228,7 +228,7 @@ class PendingPersonOverride(models.Model):
"""

id = models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name="ID")
team_id = models.BigIntegerField()
team_id = models.IntegerField()
old_person_id = models.UUIDField()
override_person_id = models.UUIDField()
oldest_event = models.DateTimeField()
Expand Down Expand Up @@ -273,7 +273,7 @@ class FlatPersonOverride(models.Model):
"""

id = models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name="ID")
team_id = models.BigIntegerField()
team_id = models.IntegerField()
old_person_id = models.UUIDField()
override_person_id = models.UUIDField()
oldest_event = models.DateTimeField()
Expand Down Expand Up @@ -317,12 +317,12 @@ def get_distinct_ids_for_subquery(person: Person | None, team: Team) -> list[str

if person is not None:
first_ids = (
PersonDistinctId.objects.filter(person=person, team=team)
PersonDistinctId.objects.filter(person=person, team_id=team.pk)
.order_by("id")
.values_list("distinct_id", flat=True)[:first_ids_limit]
)
last_ids = (
PersonDistinctId.objects.filter(person=person, team=team)
PersonDistinctId.objects.filter(person=person, team_id=team.pk)
.order_by("-id")
.values_list("distinct_id", flat=True)[:last_ids_limit]
)
Expand Down
Loading