fix: cohort negation (#25327)

ee/clickhouse/models/test/test_filters.py

@@ -126,6 +126,7 @@ def test_simplify_cohorts(self):
                             {
                                 "key": "id",
                                 "value": cohort.pk,
+                                "negation": False,
                                 "type": "precalculated-cohort",
                             }
                         ],
@@ -141,6 +142,7 @@ def test_simplify_cohorts(self):
                         "values": [
                             {
                                 "key": "id",
+                                "negation": False,
                                 "value": cohort.pk,
                                 "type": "precalculated-cohort",
                             }
@@ -158,7 +160,7 @@ def test_simplify_static_cohort(self):
             {
                 "properties": {
                     "type": "AND",
-                    "values": [{"type": "static-cohort", "key": "id", "value": cohort.pk}],
+                    "values": [{"type": "static-cohort", "negation": False, "key": "id", "value": cohort.pk}],
                 }
             },
         )
@@ -172,7 +174,7 @@ def test_simplify_hasdone_cohort(self):
             {
                 "properties": {
                     "type": "AND",
-                    "values": [{"type": "cohort", "key": "id", "value": cohort.pk}],
+                    "values": [{"type": "cohort", "negation": False, "key": "id", "value": cohort.pk}],
                 }
             },
         )

posthog/hogql/property.py

@@ -482,7 +482,8 @@ def property_to_expr(
         return ast.CompareOperation(
             left=ast.Field(chain=["id" if scope == "person" else "person_id"]),
             op=ast.CompareOperationOp.NotInCohort
-            if property.operator == PropertyOperator.NOT_IN.value
+            # Kludge: negation is outdated but still used in places
+            if property.negation or property.operator == PropertyOperator.NOT_IN.value
             else ast.CompareOperationOp.InCohort,
             right=ast.Constant(value=cohort.pk),
         )

posthog/hogql_queries/legacy_compatibility/clean_properties.py

@@ -1,3 +1,6 @@
+from posthog.schema import PropertyOperator
+
+
 def clean_global_properties(properties: dict | list[dict] | None):
     if properties is None or len(properties) == 0:
         # empty properties
@@ -90,8 +93,15 @@ def clean_property(property: dict):
         cleaned_property["type"] = "cohort"
 
     # fix invalid property key for cohorts
-    if cleaned_property.get("type") == "cohort" and cleaned_property.get("key") != "id":
-        cleaned_property["key"] = "id"
+    if cleaned_property.get("type") == "cohort":
+        if cleaned_property.get("key") != "id":
+            cleaned_property["key"] = "id"
+        if cleaned_property.get("operator") is None:
+            cleaned_property["operator"] = (
+                PropertyOperator.NOT_IN.value if cleaned_property.get("negation", False) else PropertyOperator.IN_.value
+            )
+        if "negation" in cleaned_property:
+            del cleaned_property["negation"]
 
     # set a default operator for properties that support it, but don't have an operator set
     if is_property_with_operator(cleaned_property) and cleaned_property.get("operator") is None:
@@ -112,7 +122,7 @@ def clean_property(property: dict):
 
 
 def is_property_with_operator(property: dict):
-    return property.get("type") not in ("cohort", "hogql")
+    return property.get("type") not in ("hogql",)
 
 
 # old style dict properties e.g. {"utm_medium__icontains": "email"}

posthog/hogql_queries/legacy_compatibility/test/test_clean_properties.py

@@ -40,7 +40,7 @@ def test_handles_property_filter_lists(self):
                 "values": [
                     {
                         "type": "AND",
-                        "values": [{"key": "id", "type": "cohort", "value": 636}],
+                        "values": [{"key": "id", "type": "cohort", "operator": "in", "value": 636}],
                     }
                 ],
             },
@@ -61,7 +61,33 @@ def test_handles_property_group_filters(self):
                 "values": [
                     {
                         "type": "AND",
-                        "values": [{"key": "id", "type": "cohort", "value": 850}],
+                        "values": [{"key": "id", "type": "cohort", "operator": "in", "value": 850}],
+                    }
+                ],
+            },
+        )
+
+    def test_handles_cohort_negation(self):
+        properties = {
+            "type": "AND",
+            "values": [
+                {
+                    "type": "AND",
+                    "values": [{"key": "id", "type": "cohort", "value": 850, "operator": None, "negation": True}],
+                }
+            ],
+        }
+
+        result = clean_global_properties(properties)
+
+        self.assertEqual(
+            result,
+            {
+                "type": "AND",
+                "values": [
+                    {
+                        "type": "AND",
+                        "values": [{"key": "id", "type": "cohort", "operator": "not_in", "value": 850}],
                     }
                 ],
             },
@@ -82,7 +108,7 @@ def test_handles_property_group_filters_values(self):
                 "values": [
                     {
                         "type": "AND",
-                        "values": [{"key": "id", "type": "cohort", "value": 850}],
+                        "values": [{"key": "id", "type": "cohort", "operator": "in", "value": 850}],
                     }
                 ],
             },

posthog/models/filters/mixins/simplify.py

@@ -2,6 +2,7 @@
 
 from posthog.constants import PropertyOperatorType
 from posthog.models.property import GroupTypeIndex, PropertyGroup
+from posthog.schema import PropertyOperator
 
 if TYPE_CHECKING:  # Avoid circular import
     from posthog.models import Property, Team
@@ -112,7 +113,9 @@ def _simplify_property(self, team: "Team", property: "Property", **kwargs) -> "P
                 # :TODO: Handle non-existing resource in-query instead
                 return PropertyGroup(type=PropertyOperatorType.AND, values=[property])
 
-            return simplified_cohort_filter_properties(cohort, team, property.negation)
+            return simplified_cohort_filter_properties(
+                cohort, team, property.negation or property.operator == PropertyOperator.NOT_IN.value
+            )
 
         # PropertyOperatorType doesn't really matter here, since only one value.
         return PropertyGroup(type=PropertyOperatorType.AND, values=[property])