Address PR review, convert exclude(~Q) magic into regular filters

events/api.py

@@ -24,7 +24,7 @@
 from django.core.cache import cache
 from django.core.exceptions import PermissionDenied
 from django.db import transaction
-from django.db.models import Count, F, Prefetch, Q
+from django.db.models import Count, Exists, F, OuterRef, Prefetch, Q, QuerySet
 from django.db.models.functions import Greatest
 from django.db.utils import IntegrityError
 from django.http import Http404, HttpResponsePermanentRedirect
@@ -2525,15 +2525,23 @@ def _find_keyword_replacements(keyword_ids: list[str]) -> tuple[list[Keyword], b
     return list(set(replaced_keywords)), found_all_keywords
 
 
-def _filter_keyword_or(queryset, param):
-    keywords, __ = _find_keyword_replacements(param.split(","))
+def _filter_events_keyword_or(queryset: QuerySet, keyword_ids: list[str]) -> QuerySet:
+    """
+    Given an Event queryset, apply OR filter on keyword ids
+    :param queryset:
+    :param keyword_ids:
+    :return:
+    """
+    keywords, __ = _find_keyword_replacements(keyword_ids)
     keyword_ids = [keyword.pk for keyword in keywords]
 
-    # Use negated exclude to avoid slower filter+distinct
-    queryset = queryset.exclude(
-        ~(Q(keywords__pk__in=keyword_ids) | Q(audience__pk__in=keyword_ids))
+    kw_qs = Event.keywords.through.objects.filter(
+        event=OuterRef("pk"), keyword__in=keyword_ids
     )
-    return queryset
+    audience_qs = Event.audience.through.objects.filter(
+        event=OuterRef("pk"), keyword__in=keyword_ids
+    )
+    return queryset.filter(Exists(kw_qs) | Exists(audience_qs))
 
 
 def _filter_event_queryset(queryset, params, srs=None):  # noqa: C901
@@ -2542,7 +2550,8 @@ def _filter_event_queryset(queryset, params, srs=None):  # noqa: C901
     (e.g. self.request.query_params ingit EventViewSet)
     """
     # Please keep in mind that .distinct() will absolutely kill
-    # performance of queries.
+    # performance of queries. To avoid duplicate rows in filters
+    # consider using Exists instead.
 
     # filter to get events with or without a registration.
     val = params.get("registration", None)
@@ -2642,26 +2651,29 @@ def _filter_event_queryset(queryset, params, srs=None):  # noqa: C901
     vals = params.get("keyword_set_AND", None)
     if vals:
         vals = vals.split(",")
-        keyword_sets = KeywordSet.objects.filter(id__in=vals)
-        q = Q()
-        for keyword_set in keyword_sets:
-            keywords = keyword_set.keywords.all()
-            q &= Q(keywords__in=keywords)
-
-        # Use negated exclude to avoid slower filter+distinct
-        queryset = queryset.exclude(~q)
+        kw_sets = KeywordSet.objects.filter(id__in=vals).prefetch_related(
+            Prefetch("keywords", Keyword.objects.all().only("id"))
+        )
+        for kw_set in kw_sets:
+            kw_ids = [kw.id for kw in kw_set.keywords.all()]
+            subqs = Event.objects.filter(id=OuterRef("pk"), keywords__in=kw_ids)
+            queryset = queryset.filter(Exists(subqs))
 
     vals = params.get("keyword_set_OR", None)
     if vals:
         vals = vals.split(",")
-        keyword_sets = KeywordSet.objects.filter(id__in=vals)
+        keyword_sets = KeywordSet.objects.filter(id__in=vals).prefetch_related(
+            Prefetch("keywords", Keyword.objects.all().only("id"))
+        )
         all_keywords = set()
         for keyword_set in keyword_sets:
             keywords = keyword_set.keywords.all()
             all_keywords.update(keywords)
 
-        # Use negated exclude to avoid slower filter+distinct
-        queryset = queryset.exclude(~Q(keywords__in=all_keywords))
+        kw_qs = Event.keywords.through.objects.filter(
+            event=OuterRef("pk"), keyword__in=all_keywords
+        )
+        queryset = queryset.filter(Exists(kw_qs))
 
     if "local_ongoing_OR_set" in "".join(params):
         count = 1
@@ -2724,9 +2736,20 @@ def _filter_event_queryset(queryset, params, srs=None):  # noqa: C901
         for i in all_sets:
             val = params.get(i, None)
             if val:
-                val = val.split(",")
-                # Use negated exclude to avoid slower filter+distinct
-                queryset = queryset.exclude(~Q(keywords__pk__in=val))
+                vals = val.split(",")
+
+                keyword_sets = KeywordSet.objects.filter(id__in=vals).prefetch_related(
+                    Prefetch("keywords", Keyword.objects.all().only("id"))
+                )
+                all_keywords = set()
+                for keyword_set in keyword_sets:
+                    keywords = keyword_set.keywords.all()
+                    all_keywords.update(keywords)
+
+                kw_qs = Event.keywords.through.objects.filter(
+                    event=OuterRef("pk"), keyword__in=all_keywords
+                )
+                queryset = queryset.filter(Exists(kw_qs))
 
     val = params.get("internet_based", None)
     if val and parse_bool(val, "internet_based"):
@@ -2762,20 +2785,20 @@ def _filter_event_queryset(queryset, params, srs=None):  # noqa: C901
                 else ["fi", "sv", "en"]
             )
             tri = [TrigramSimilarity(f"name_{i}", val) for i in langs]
-            keywords = (
+            keywords = list(
                 Keyword.objects.annotate(simile=Greatest(*tri))
                 .filter(simile__gt=0.2)
                 .order_by("-simile")[:3]
             )
-            if keywords:
-                val_q |= Q(keywords__in=keywords)
+
+            val_q |= Q(keywords__in=keywords)
 
             combined_q &= val_q
 
-        # Use negated exclude to avoid slower filter+distinct
-        # In this case filter+distinct is so slow it gets a
-        # 504 gateway timeout in prod!
-        queryset = queryset.exclude(~combined_q)
+        # FYI: Biggest slowdown in this filter is the ordering of keywords simile
+        queryset = queryset.filter(
+            Exists(Event.objects.filter(combined_q, id=OuterRef("pk")).only("id"))
+        )
 
     val = params.get("text", None)
     if val:
@@ -2903,11 +2926,11 @@ def _filter_event_queryset(queryset, params, srs=None):  # noqa: C901
         queryset = queryset.filter(location_id__in=val)
 
     if val := params.get("keyword", None):
-        queryset = _filter_keyword_or(queryset, val)
+        queryset = _filter_events_keyword_or(queryset, val.split(","))
 
     # 'keyword_OR' behaves the same way as 'keyword'
     if val := params.get("keyword_OR", None):
-        queryset = _filter_keyword_or(queryset, val)
+        queryset = _filter_events_keyword_or(queryset, val.split(","))
 
     # Filter by keyword ids requiring all keywords to be present in event
     val = params.get("keyword_AND", None)
@@ -2918,12 +2941,18 @@ def _filter_event_queryset(queryset, params, srs=None):  # noqa: C901
         if not found_all_keywords:
             return queryset.none()
 
-        q = Q(keywords__pk=keywords[0].pk) | Q(audience__pk=keywords[0].pk)
-        for keyword in keywords[1:]:
+        q = Q()
+        for keyword in keywords:
             q &= Q(keywords__pk=keyword.pk) | Q(audience__pk=keyword.pk)
 
-        # Use negated exclude to avoid slower filter+distinct
-        queryset = queryset.exclude(~q)
+        for keyword in keywords:
+            kw_qs = Event.keywords.through.objects.filter(
+                event=OuterRef("pk"), keyword=keyword
+            )
+            audience_qs = Event.audience.through.objects.filter(
+                event=OuterRef("pk"), keyword=keyword
+            )
+            queryset = queryset.filter(Exists(kw_qs) | Exists(audience_qs))
 
     # Negative filter for keyword ids
     val = params.get("keyword!", None)
@@ -3019,27 +3048,37 @@ def _filter_event_queryset(queryset, params, srs=None):  # noqa: C901
                 short_desc_arg = {"short_description_" + lang + "__isnull": False}
                 q = (
                     q
-                    | Q(in_language__id=lang)
+                    | Exists(
+                        Event.in_language.through.objects.filter(
+                            event=OuterRef("pk"), language=lang
+                        )
+                    )
                     | Q(**name_arg)
                     | Q(**desc_arg)
                     | Q(**short_desc_arg)
                 )
             else:
-                q = q | Q(in_language__id=lang)
+                q = q | Exists(
+                    Event.in_language.through.objects.filter(
+                        event=OuterRef("pk"), language=lang
+                    )
+                )
 
-        # Use negated exclude to avoid filter+distinct
-        queryset = queryset.exclude(~q)
+        queryset = queryset.filter(q)
 
     # Filter by in_language field only
     val = params.get("in_language", None)
     if val:
         val = val.split(",")
         q = Q()
         for lang in val:
-            q = q | Q(in_language__id=lang)
+            q = q | Exists(
+                Event.in_language.through.objects.filter(
+                    event=OuterRef("pk"), language=lang
+                )
+            )
 
-        # Use negated exclude to avoid filter+distinct
-        queryset = queryset.exclude(~q)
+        queryset = queryset.filter(q)
 
     val = params.get("starts_after", None)
     param = "starts_after"
@@ -3126,7 +3165,9 @@ def _filter_event_queryset(queryset, params, srs=None):  # noqa: C901
     if val and val.lower() in ["true", "false"]:
         # Include events that have at least one free offer
         if val.lower() == "true":
-            # Use negated exclude to avoid slower filter+distinct
+            queryset = queryset.filter(
+                Exists(Offer.objects.filter(event=OuterRef("pk"), is_free=True))
+            )
             queryset = queryset.exclude(~Q(offers__is_free=True))
         # Include events that have no free offers
         elif val.lower() == "false":