chore(Persons): Remove foreign key constraints from Persons and related models #27636
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR removes foreign key constraints from Person-related models, replacing them with integer fields to support database separation and improve scalability.
- Removes Django admin interfaces for Person and PersonDistinctId models in
/posthog/admin/admins/person_admin.pyand/posthog/admin/admins/person_distinct_id_admin.py - Replaces ForeignKey fields with IntegerField (team_id, is_user_id) in
/posthog/models/person/person.py - Removes ManyToManyField 'people' from Cohort model and updates CohortPeople to use cohort_id in
/posthog/models/cohort/cohort.py - Updates FeatureFlagHashKeyOverride unique constraint to use team_id instead of team in
/posthog/models/feature_flag/feature_flag.py
8 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile
| team_id = ( | ||
| models.IntegerField() | ||
| ) # Note: in order to move persons to separate database, this is a int instead of foreingkey |
There was a problem hiding this comment.
style: Consider adding a db_index=True to team_id since it was previously indexed as a foreign key
| @@ -376,7 +375,7 @@ def get_and_update_pending_version(cohort: Cohort): | |||
|
|
|||
| class CohortPeople(models.Model): | |||
| id = models.BigAutoField(primary_key=True) | |||
| cohort = models.ForeignKey("Cohort", on_delete=models.CASCADE) | |||
| cohort_id = models.IntegerField() | |||
There was a problem hiding this comment.
logic: removing foreign key constraint allows invalid cohort_ids to be inserted - consider adding application-level validation
| team_id = models.IntegerField() | ||
| 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) |
There was a problem hiding this comment.
logic: Removing ForeignKey constraints means application code must ensure team_id and is_user_id values are valid. Consider adding validation in save() method.
| @@ -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() | |||
There was a problem hiding this comment.
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?
Problem
We want to move persons and related models into its own DB. In order to do that, we need to remove foreign key constraints.
Changes
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Does this work well for both Cloud and self-hosted?
How did you test this code?