diff --git a/posthog/api/person.py b/posthog/api/person.py index d3f311067a82..70696df727a6 100644 --- a/posthog/api/person.py +++ b/posthog/api/person.py @@ -1343,6 +1343,11 @@ def split(self, request: request.Request, pk=None, **kwargs) -> response.Respons 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, diff --git a/posthog/api/test/test_person.py b/posthog/api/test/test_person.py index c043708b397b..82e336cd09e3 100644 --- a/posthog/api/test/test_person.py +++ b/posthog/api/test/test_person.py @@ -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, diff --git a/posthog/models/person/person.py b/posthog/models/person/person.py index f29e9c371b4d..d35529623825 100644 --- a/posthog/models/person/person.py +++ b/posthog/models/person/person.py @@ -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: @@ -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( + 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, diff --git a/posthog/models/person/test/test_split_person.py b/posthog/models/person/test/test_split_person.py index 8b1d24e0fb9b..6b94e3321c79 100644 --- a/posthog/models/person/test/test_split_person.py +++ b/posthog/models/person/test/test_split_person.py @@ -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 @@ -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)