Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions posthog/api/person.py
Original file line number Diff line number Diff line change
Expand Up @@ -727,7 +727,7 @@
],
responses=PersonListRecordSerializer,
)
def list(self, request: request.Request, *args: Any, **kwargs: Any) -> response.Response:

Check warning on line 730 in posthog/api/person.py

View workflow job for this annotation

GitHub Actions / Python code quality (depot-ubuntu-24.04)

lint:complexity

`list` has cyclomatic complexity 16 (warn >10)

Check warning on line 730 in posthog/api/person.py

View workflow job for this annotation

GitHub Actions / Python code quality (depot-ubuntu-24.04)

`list` has cyclomatic complexity 16 (warn >10)
tag_queries(product=ProductKey.PERSONS, feature=Feature.QUERY)
client_query_id = request.GET.get("client_query_id")
tag_client_query_id(client_query_id)
Expand Down Expand Up @@ -1343,6 +1343,11 @@
unknown = set(distinct_ids_to_split) - set(distinct_ids)
if unknown:
raise ValidationError({"distinct_ids_to_split": f"not on this person: {sorted(unknown)}"})
if set(distinct_ids) <= set(distinct_ids_to_split):
raise ValidationError({"distinct_ids_to_split": "must leave at least one distinct ID on this person"})
elif main_distinct_id and main_distinct_id not in distinct_ids:
# An unknown main ID would split off every distinct ID and leave the person unreachable.
raise ValidationError({"main_distinct_id": "not on this person"})

split_person.delay(
person.id,
Expand Down
18 changes: 18 additions & 0 deletions posthog/api/test/test_person.py
Original file line number Diff line number Diff line change
Expand Up @@ -1385,6 +1385,24 @@ def test_split_people_partial_rejects_unknown_distinct_id(self) -> None:
assert original is not None
self.assertCountEqual(original.distinct_ids, ["a", "b"])

@parameterized.expand(
[
("every_id_listed", {"distinct_ids_to_split": ["b", "a", "a"]}, "must leave at least one distinct ID"),
("unknown_main_id", {"main_distinct_id": "not_on_this_person"}, "not on this person"),
]
)
def test_split_people_rejects_a_split_that_leaves_no_distinct_id(
self, _name: str, payload: dict, message: str
) -> None:
person1 = _create_person(team=self.team, distinct_ids=["a", "b"], properties={}, immediate=True)

with mock.patch("posthog.api.person.split_person.delay") as split_task:
response = self.client.post(f"/api/person/{person1.pk}/split/", payload, content_type="application/json")

self.assertEqual(response.status_code, 400)
self.assertIn(message, response.content.decode())
split_task.assert_not_called()

def test_split_people_partial_rejects_combined_with_main_distinct_id(self) -> None:
person1 = _create_person(
team=self.team,
Expand Down
34 changes: 28 additions & 6 deletions posthog/models/person/person.py
Original file line number Diff line number Diff line change
Expand Up @@ -271,16 +271,26 @@ def split_person(
)

if distinct_ids_to_split is not None:
self._split_explicit_ids(distinct_ids_to_split)
self._split_explicit_ids(client, distinct_ids_to_split)
else:
if main_distinct_id:
self._require_own_distinct_id(client, main_distinct_id)
self._split_all_ids(client, main_distinct_id, max_splits)

def _split_explicit_ids(self, distinct_ids_to_split: list[str]) -> None:
"""Partial split: caller specifies exactly which IDs to move.
def _require_own_distinct_id(self, client: Any, distinct_id: str) -> None:
"""An unowned main ID would split off every ID and strand the person, so reject it before any batch runs."""
from posthog.personhog_client.proto import GetPersonByDistinctIdRequest

resp = client.get_person_by_distinct_id(
GetPersonByDistinctIdRequest(team_id=self.team_id, distinct_id=distinct_id)
)
if not resp.person or resp.person.id != self.pk:
raise ValueError(f"main_distinct_id is not on person_id={self.pk} (team_id={self.team_id})")

def _split_explicit_ids(self, client: Any, distinct_ids_to_split: list[str]) -> None:
"""Partial split: the caller names the IDs to move. The RPC checks ownership; a bounded fetch checks one ID stays."""
from posthog.personhog_client.proto import GetDistinctIdsForPersonRequest

The RPC validates that every ID belongs to this person — no need to
fetch all distinct IDs upfront.
"""
seen: set[str] = set()
distinct_ids_to_process: list[str] = []
for did in distinct_ids_to_split:
Expand All @@ -291,6 +301,18 @@ def _split_explicit_ids(self, distinct_ids_to_split: list[str]) -> None:
if not distinct_ids_to_process:
return

# One more ID than the request names: if every returned ID is in the request, none would remain.
remaining = client.get_distinct_ids_for_person(
Comment thread
eli-r-ph marked this conversation as resolved.
GetDistinctIdsForPersonRequest(
team_id=self.team_id, person_id=self.pk, limit=len(distinct_ids_to_process) + 1
)
)
if all(d.distinct_id in seen for d in remaining.distinct_ids):
raise ValueError(
f"split would leave no distinct ID on person_id={self.pk} (team_id={self.team_id}), "
"so the person would be unreachable"
)

logger.info(
"split_person will split explicit distinct_ids",
person_id=self.pk,
Expand Down
28 changes: 28 additions & 0 deletions posthog/models/person/test/test_split_person.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,12 @@
from posthog.test.base import BaseTest
from unittest.mock import patch

from parameterized import parameterized

from posthog.models import Person
from posthog.models.person.missing_person import uuidFromDistinctId
from posthog.models.person.util import get_person_by_distinct_id, get_person_by_id
from posthog.models.utils import UUIDT
from posthog.personhog_client.fake_client import FakePersonHogClient, fake_personhog_client
from posthog.test.persons import add_distinct_id, create_person

Expand Down Expand Up @@ -233,6 +236,31 @@ def test_partial_split_rejects_unknown_distinct_id(self, mock_create_pdi, mock_c
mock_create_pdi.assert_not_called()
mock_create_person.assert_not_called()

@parameterized.expand(
[
("every_id", {"main_distinct_id": None, "distinct_ids_to_split": ["id2", "id1", "id1"]}),
("unknown_main_id", {"main_distinct_id": "not_on_this_person"}),
("main_id_of_another_person", {"main_distinct_id": "other_persons_id"}),
]
)
def test_split_rejects_leaving_the_person_with_no_distinct_id(
self, mock_create_pdi, mock_create_person, _name: str, split_kwargs: dict
):
with fake_personhog_client() as fake:
person = self._setup_person(fake, ["id1", "id2"], mock_create_pdi, mock_create_person)
fake.add_person(
team_id=self.team.id, person_id=person.id + 1, uuid=str(UUIDT()), distinct_ids=["other_persons_id"]
)

with self.assertRaises(ValueError):
person.split_person(**split_kwargs)

fake.assert_not_called("split_person")
assert fake._persons_by_distinct_id[(self.team.id, "id1")].id == person.id
assert fake._persons_by_distinct_id[(self.team.id, "id2")].id == person.id
mock_create_pdi.assert_not_called()
mock_create_person.assert_not_called()

def test_partial_split_ignores_main_distinct_id_and_max_splits(self, mock_create_pdi, mock_create_person):
with fake_personhog_client() as fake:
person = self._setup_person(fake, ["a", "b", "c", "d"], mock_create_pdi, mock_create_person)
Expand Down
Loading