diff --git a/.sampo/changesets/canonical-group-key-bucketing.md b/.sampo/changesets/canonical-group-key-bucketing.md new file mode 100644 index 000000000..67fe94098 --- /dev/null +++ b/.sampo/changesets/canonical-group-key-bucketing.md @@ -0,0 +1,5 @@ +--- +posthog: patch +--- + +Use canonical numeric group keys for local feature-flag bucketing and fall back to remote evaluation for unsupported key types. diff --git a/posthog/client.py b/posthog/client.py index d74ea56e3..235eae728 100644 --- a/posthog/client.py +++ b/posthog/client.py @@ -76,6 +76,7 @@ from posthog.feature_flags import ( InconclusiveMatchError, RequiresServerEvaluation, + _group_key_to_string, match_feature_flag_properties, resolve_bucketing_value, ) @@ -3582,7 +3583,7 @@ def _compute_flag_locally( f"Flag has no group properties for group '{group_name}'" ) focused_group_properties = group_properties[group_name] - group_key = groups[group_name] + group_key = _group_key_to_string(groups[group_name]) return match_feature_flag_properties( feature_flag, group_key, diff --git a/posthog/feature_flags.py b/posthog/feature_flags.py index 87da025b5..bc87faca4 100644 --- a/posthog/feature_flags.py +++ b/posthog/feature_flags.py @@ -444,7 +444,7 @@ def match_feature_flag_properties( is_inconclusive = True continue effective_properties = group_properties[group_name] - effective_bucketing = groups[group_name] + effective_bucketing = _group_key_to_string(groups[group_name]) else: effective_properties = properties effective_bucketing = bucketing_value @@ -613,6 +613,14 @@ def _json_value_to_string(value) -> str: ) +def _group_key_to_string(value) -> str: + if isinstance(value, str): + return value + if isinstance(value, (int, float)) and not isinstance(value, bool): + return _json_value_to_string(value) + raise InconclusiveMatchError("Group keys must be strings or finite numbers") + + def _value_to_string(value) -> str: if isinstance(value, str): return value diff --git a/posthog/test/test_feature_flags.py b/posthog/test/test_feature_flags.py index a126a0092..257992bef 100644 --- a/posthog/test/test_feature_flags.py +++ b/posthog/test/test_feature_flags.py @@ -118,6 +118,86 @@ def test_missing_nested_cohort_always_requires_server_evaluation(self): ) +class TestGroupKeyBucketing(unittest.TestCase): + def setUp(self): + self.client = Client(FAKE_TEST_API_KEY, send=False) + self.client.group_type_mapping = {"0": "company"} + + def tearDown(self): + self.client.shutdown() + + def set_group_flag(self, mixed=False, rollout=50): + condition = {"properties": [], "rollout_percentage": rollout} + filters = {"groups": [condition]} + if mixed: + condition["aggregation_group_type_index"] = 0 + else: + filters["aggregation_group_type_index"] = 0 + self.client.feature_flags = [ + {"id": 1, "key": "group-beta", "active": True, "filters": filters} + ] + + @parameterized.expand([(False,), (True,)]) + @mock.patch("posthog.client.flags") + def test_numeric_group_keys_use_server_representation(self, mixed, patch_flags): + self.set_group_flag(mixed) + # For this flag, "0.00001" hashes below 50%, but Python's "1e-05" + # hashes above it. Integer keys should keep their existing bucketing. + for group_key in (0.00001, "0.00001", 42, "42"): + with self.subTest(group_key=group_key): + self.assertTrue( + self.client.get_feature_flag( + "group-beta", + "user-1", + groups={"company": group_key}, + only_evaluate_locally=True, + send_feature_flag_events=False, + ) + ) + flags = self.client.evaluate_flags( + "user-1", + groups={"company": group_key}, + only_evaluate_locally=True, + ) + self.assertTrue(flags.get_flag("group-beta")) + patch_flags.assert_not_called() + + @parameterized.expand([(False,), (True,)]) + @mock.patch("posthog.client.flags") + def test_unsupported_group_keys_require_remote_evaluation(self, mixed, patch_flags): + self.set_group_flag(mixed, rollout=100) + patch_flags.return_value = {"featureFlags": {"group-beta": "remote"}} + for group_key in (True, False, None, [], {}, float("inf"), float("nan")): + with self.subTest(group_key=group_key): + self.assertIsNone( + self.client.get_feature_flag( + "group-beta", + "user-1", + groups={"company": group_key}, + only_evaluate_locally=True, + send_feature_flag_events=False, + ) + ) + flags = self.client.evaluate_flags( + "user-1", + groups={"company": group_key}, + only_evaluate_locally=True, + ) + self.assertNotIn("group-beta", flags.keys) + patch_flags.assert_not_called() + self.assertEqual( + self.client.get_feature_flag( + "group-beta", + "user-1", + groups={"company": group_key}, + send_feature_flag_events=False, + ), + "remote", + ) + patch_flags.assert_called_once() + patch_flags.reset_mock() + + class TestLocalEvaluation(unittest.TestCase): @classmethod def setUpClass(cls):