diff --git a/.sampo/changesets/group-identify-validates-group-identity.md b/.sampo/changesets/group-identify-validates-group-identity.md new file mode 100644 index 00000000..ac43fcbb --- /dev/null +++ b/.sampo/changesets/group-identify-validates-group-identity.md @@ -0,0 +1,5 @@ +--- +pypi/posthog: patch +--- + +`group_identify()` now validates the group identity before enqueuing. Previously `group_identify("company", None)` (or an empty-string `group_type` / `group_key`) sent a `$groupidentify` event with a null/empty `$group_type` or `$group_key`, which cannot address a group profile and just adds an unusable event to the project. Missing values are now dropped with a warning instead, matching the sdk-specs `group-identify` contract. Valid values, including non-string group keys, are passed through unchanged. diff --git a/posthog/__init__.py b/posthog/__init__.py index e45fae7e..8bfcc663 100644 --- a/posthog/__init__.py +++ b/posthog/__init__.py @@ -578,8 +578,10 @@ def group_identify( Set properties on a group. Args: - group_type: Type of your group - group_key: Unique identifier of the group + group_type: Type of your group. Required - the call is dropped with a + warning if it is missing or empty. + group_key: Unique identifier of the group. Required - the call is + dropped with a warning if it is missing or empty. properties: Properties to set on the group timestamp: Optional timestamp for the event uuid: Optional UUID for the event diff --git a/posthog/client.py b/posthog/client.py index a10ef6bc..050401c9 100644 --- a/posthog/client.py +++ b/posthog/client.py @@ -1652,8 +1652,10 @@ def group_identify( Identify a group and set its properties. Args: - group_type: The type of group (e.g., 'company', 'team'). - group_key: The unique identifier for the group. + group_type: The type of group (e.g., 'company', 'team'). Required - + the call is dropped with a warning if it is missing or empty. + group_key: The unique identifier for the group. Required - the call + is dropped with a warning if it is missing or empty. properties: A dictionary of properties to set on the group. timestamp: The timestamp of the event. uuid: A unique identifier for the event. If provided, it must be a @@ -1675,6 +1677,18 @@ def group_identify( Note: This method will not raise exceptions. Errors are logged. """ + if not stringify_id(group_type): + self.log.warning( + "group_identify() called without a group_type, dropping the $groupidentify event" + ) + return None + + if not stringify_id(group_key): + self.log.warning( + "group_identify() called without a group_key, dropping the $groupidentify event" + ) + return None + properties = properties or {} # group_identify is purposefully always personful diff --git a/posthog/test/test_client.py b/posthog/test/test_client.py index f030ff94..f280fa9a 100644 --- a/posthog/test/test_client.py +++ b/posthog/test/test_client.py @@ -1893,6 +1893,49 @@ def test_advanced_group_identify_with_distinct_id(self): ) self.assertEqual(msg["timestamp"], "2014-09-03T00:00:00+00:00") + @parameterized.expand( + [ + ("none", None), + ("empty_string", ""), + ] + ) + def test_group_identify_without_group_type_is_dropped(self, _name, group_type): + with mock.patch("posthog.client.batch_post") as mock_post: + client = Client(FAKE_TEST_API_KEY, on_error=self.set_fail, sync_mode=True) + with self.assertLogs("posthog", level="WARNING") as logs: + msg_uuid = client.group_identify(group_type, "id:5") + + self.assertIsNone(msg_uuid) + mock_post.assert_not_called() + self.assertIn("group_type", logs.output[0]) + + @parameterized.expand( + [ + ("none", None), + ("empty_string", ""), + ] + ) + def test_group_identify_without_group_key_is_dropped(self, _name, group_key): + with mock.patch("posthog.client.batch_post") as mock_post: + client = Client(FAKE_TEST_API_KEY, on_error=self.set_fail, sync_mode=True) + with self.assertLogs("posthog", level="WARNING") as logs: + msg_uuid = client.group_identify("organization", group_key) + + self.assertIsNone(msg_uuid) + mock_post.assert_not_called() + self.assertIn("group_key", logs.output[0]) + + def test_group_identify_accepts_falsy_non_string_group_key(self): + with mock.patch("posthog.client.batch_post") as mock_post: + client = Client(FAKE_TEST_API_KEY, on_error=self.set_fail, sync_mode=True) + msg_uuid = client.group_identify("organization", 0) + self.assertIsNotNone(msg_uuid) + + mock_post.assert_called_once() + msg = mock_post.call_args[1]["batch"][0] + # The group key is validated, not normalized - it goes out as passed. + self.assertEqual(msg["properties"]["$group_key"], 0) + def test_basic_alias(self): with mock.patch("posthog.client.batch_post") as mock_post: client = Client(FAKE_TEST_API_KEY, on_error=self.set_fail, sync_mode=True)