alerts: fix race conditions in recipient determination

docs/user_docs/source/notifications.rst

@@ -74,7 +74,7 @@ More information about each field will be provided in the following section wher
 
 A few notes here:
 
-- The label query field allows a complex filter on the requirement for which labels are/are not applied to an event or superevent.  To construct a label query, combine label names with binary AND: ('&' or ',') or binary OR: '|'. They can also be negated with '~' or '-'. For N labels, there must be exactly N-1 binary operators. Parentheses are not allowed. It is suggested to specify at least one label that is not negated, or you may receive more notifications than you would like.
+- The label query field allows a complex filter on the requirement for which labels are/are not applied to an event or superevent.  To construct a label query, combine label names with binary AND: ('&' or ',') or binary OR: '|'. They can also be negated with '~' or '-'. For N labels, there must be exactly N-1 binary operators. Parentheses are not allowed. It is suggested to specify at least one label that is not negated, or you may receive more notifications than you would like. Operator precedence is: NOT, AND, OR.
 - You can specify either a set of labels or a label query, but not both.
 - An event or superevent is considered a neutron star candidate if its source is thought to be a compact binary system where the secondary object has a mass less than 3 M\ :sub:`sun`\ . You can restrict your alerts to only events or superevents which are considered to be a neutron star candidate by checking the corresponding box in the form.
 

gracedb/alerts/constants.py

+from pyparsing import oneOf, Literal
+
+
+# List of characters which are equivalent to the corresponding
+# logical operator
+AND = ['&', ',']
+OR = ['|']
+NOT = ['~', '-']
+
+
+# Pyparsing parsers
+OPERATORS = {
+    'NOT': oneOf(NOT),
+    'AND': oneOf(AND),
+    'OR': Literal(OR),
+}

gracedb/alerts/recipients.py

 from django.conf import settings
 from django.db.models import Q
 
+from events.models import Label
 from events.shortcuts import is_event
-from search.query.labels import filter_for_labels
 from superevents.shortcuts import is_superevent
 from .models import Contact, Notification
+from .utils import evaluate_label_queries
 
 
 class CreationRecipientGetter(object):
     queryset = Notification.objects.all()
 
-    def __init__(self, event_or_superevent, **kwargs):
-        self.es = event_or_superevent
-        self.is_event_alert = is_event(event_or_superevent)
-        self.event = event_or_superevent if self.is_event_alert \
-            else event_or_superevent.preferred_event
+    def __init__(self, es, **kwargs):
+        # NOTE: es = event_or_superevent
+        self.is_event_alert = is_event(es)
+        self.event = es if self.is_event_alert else es.preferred_event
         self.process_kwargs(**kwargs)
-        # event_or_superevent queryset - used for filtering by labels
-        self.es_qs = self.es._meta.model.objects.filter(pk=self.es.pk)
+
+        # Explicitly get the values for a few things and store them on a
+        # class instance.  This is because there is a possibility of race
+        # conditions if the event or superevent is updated while we are trying
+        # to figure out which notifications should trigger, which can take
+        # several seconds or more in production.
+        self.far = self.event.far
+        self.is_ns_candidate = self.event.is_ns_candidate()
+        # Force queryset evaluation with list()
+        self.label_names = list(es.labels.values_list('name', flat=True))
 
     def process_kwargs(self, **kwargs):
         pass
@@ -29,12 +37,12 @@ class CreationRecipientGetter(object):
 
     def get_far_filter(self):
         query = Q(far_threshold__isnull=True)
-        if self.event.far:
-            query |= Q(far_threshold__gt=self.event.far)
+        if self.far:
+            query |= Q(far_threshold__gt=self.far)
         return query
 
     def get_nscand_filter(self):
-        if self.event.is_ns_candidate():
+        if self.is_ns_candidate:
             return Q()
         return Q(ns_candidate=False)
 
@@ -64,16 +72,23 @@ class CreationRecipientGetter(object):
         return Q()
 
     def filter_for_labels(self, notifications):
+        # Check notifications which do NOT have a label query. Check whether
+        # their labels are a subset of what is attached to the event or
+        # superevent.  Notifications with no labels are automatically a
+        # subset.
+        # In this case, all the labels attached to the notification should
+        # be in the set attached to the event/superevent.
         notification_pks = []
-        for n in notifications:
-            if n.labels.exists() and not n.label_query:
-                if not set(n.labels.all()).issubset(self.es.labels.all()):
-                    continue
-            elif n.label_query:
-                qs_out = filter_for_labels(self.es_qs, n.label_query)
-                if not qs_out.exists():
-                    continue
-            notification_pks.append(n.pk)
+        label_set = set(self.label_names)
+        for n in notifications.filter(label_query__isnull=True):
+            n_label_set = set(n.labels.values_list('name', flat=True))
+            if n_label_set.issubset(label_set):
+                notification_pks.append(n.pk)
+
+        # Check those with label queries
+        notification_qs = notifications.filter(label_query__isnull=False)
+        pks = evaluate_label_queries(self.label_names, notification_qs)
+        notification_pks.extend(pks)
         return Notification.objects.filter(pk__in=notification_pks)
 
     def get_contacts_for_notifications(self, notifications):
@@ -139,13 +154,13 @@ class UpdateRecipientGetter(CreationRecipientGetter):
         query = Q(pk__in=[])
 
         # Then we add other options that could possibly match
-        if self.event.far is not None:
+        if self.far is not None:
             if self.old_far is None:
-                query |= Q(far_threshold__gt=self.event.far)
+                query |= Q(far_threshold__gt=self.far)
             else:
                 query |= (Q(far_threshold__lte=self.old_far) &
-                    Q(far_threshold__gt=self.event.far))
-        if self.old_nscand is False and self.event.is_ns_candidate():
+                    Q(far_threshold__gt=self.far))
+        if self.old_nscand is False and self.is_ns_candidate:
             query |= Q(ns_candidate=True)
         return query
 
@@ -183,14 +198,9 @@ class LabelRemovedRecipientGetter(LabelAddedRecipientGetter):
         # Only notifications with a label query should be triggered
         # by a label_removed alert, since notifications with a
         # label set can only have non-negated labels.
-        notification_pks = []
-        for n in notifications:
-            if n.label_query:
-                qs_out = filter_for_labels(self.es_qs, n.label_query)
-                if not qs_out.exists():
-                    continue
-                notification_pks.append(n.pk)
-        return Notification.objects.filter(pk__in=notification_pks)
+        notification_qs = notifications.filter(label_query__isnull=False)
+        pks = evaluate_label_queries(self.label_names, notification_qs)
+        return Notification.objects.filter(pk__in=pks)
 
 
 # Dict which maps alert types to recipient getter classes

gracedb/alerts/tests/test_recipients.py

@@ -1699,7 +1699,7 @@ def test_event_label_removed_alerts(
 # Other tests -----------------------------------------------------------------
 @pytest.mark.django_db
 def test_complex_label_query(superevent):
-    # NOTE: L1 & ~L2 | L3 == L1 & (~L2 | L3)
+    # NOTE: L1 & ~L2 | L3 == (L1 & ~L2) | L3
     n = Notification.objects.create(
         label_query='L1 & ~L2 | L3',
         user=superevent.submitter,
@@ -1733,7 +1733,8 @@ def test_complex_label_query(superevent):
     superevent.labelling_set.create(creator=superevent.submitter, label=l3)
     recipient_getter = LabelAddedRecipientGetter(superevent, label=l3)
     matched_notifications = recipient_getter.get_notifications()
-    assert matched_notifications.count() == 0
+    assert matched_notifications.count() == 1
+    assert matched_notifications.first().description == n.description
 
     # Test label added recipients for L1 being added (with L2)
     l1 = Label.objects.get(name='L1')

gracedb/alerts/utils.py

 from __future__ import absolute_import
-from pyparsing import oneOf, Literal, Optional, ZeroOrMore, StringEnd, Suppress
+from pyparsing import (
+    oneOf, Optional, ZeroOrMore, StringEnd, Suppress, infixNotation, opAssoc,
+)
 import re
 
-from django.db.models import Q
-
 from events.models import Label
+from .constants import OPERATORS
+
 
+def get_label_parser():
+    return oneOf(list(Label.objects.values_list('name', flat=True)))
 
-OPERATORS = {
-    'AND': oneOf(", &"),
-    'OR': Literal("|"),
-    'NOT': oneOf("- ~"),
-}
 
+def parse_label_query(s, keep_binary_ops=False):
+    """
+    Parses a label query into a list. The output is a list whose elements
+    depend on the options:
 
-def parse_label_query(s):
-    """Parses a label query into a list of label names"""
+    >>> parse_label_query('A & B', keep_binary_ops=False)
+        ['A', 'B']
+    >>> parse_label_query('A & B', keep_binary_ops=True)
+        ['A', '&', 'B']
+    """
     # Parser for one label name
-    label = oneOf(list(Label.objects.all().values_list('name', flat=True)))
+    label = get_label_parser()
 
     # "intermediate" parser - between labels should be AND or OR and then
     # an optional NOT
-    im = Suppress((OPERATORS['AND'] ^ OPERATORS['OR']) +
-        Optional(OPERATORS['NOT']))
+    im = (OPERATORS['AND'] ^ OPERATORS['OR']) + Optional(OPERATORS['NOT'])
+    if not keep_binary_ops:
+        im = Suppress(im)
 
     # Full parser: optional NOT and a label, then zero or more
     # "intermediate" + label combos, then string end
-    labelQ = Suppress(Optional(OPERATORS['NOT'])) + label + \
+    optional_initial_not = Optional(OPERATORS['NOT'])
+    if not keep_binary_ops:
+        optional_initial_not = Suppress(optional_initial_not)
+    label_expr = optional_initial_not + label + \
         ZeroOrMore(im + label) + StringEnd()
-    
-    return labelQ.parseString(s).asList()
+
+    return label_expr.parseString(s).asList()
 
 
 def convert_superevent_id_to_speech(sid):
@@ -44,3 +54,33 @@ def convert_superevent_id_to_speech(sid):
     # and make uppercase
     twilio_str = " ".join(grps).replace(' 0', ' O').upper()
     return twilio_str
+
+
+def get_label_query_parser(label_name_list):
+    label_parser = get_label_parser()
+    label_parser.setParseAction(lambda toks: toks[0] in label_name_list)
+    label_query_parser = infixNotation(label_parser,
+        [
+            (OPERATORS['NOT'], 1, opAssoc.RIGHT, lambda toks: not toks[0][1]),
+            (OPERATORS['AND'], 2, opAssoc.LEFT, lambda toks: all(toks[0][0::2])),
+            (OPERATORS['OR'], 2, opAssoc.LEFT, lambda toks: any(toks[0][0::2])),
+        ]
+    )
+    return label_query_parser
+
+
+def evaluate_label_queries(label_name_list, notifications):
+    """
+    Takes in a list of label names and a queryset of Notifications,
+    returns pks of Notifications which have a label_query and that
+    label_query is evaluates to True
+    """
+    label_query_parser = get_label_query_parser(label_name_list)
+
+    notification_pks = []
+    for n in notifications.filter(label_query__isnull=False):
+        valid = label_query_parser.parseString(n.label_query)[0]
+        if valid:
+            notification_pks.append(n.pk)
+
+    return notification_pks