Fix: number based alerts evaluation isn't working (#4295)

* Fix: correctly evaluate numeric thresholds

* Missing import

* More missing imports

* Alert evaluation: support for booleans

Author: arikfr

redash/models/__init__.py

@@ -2,6 +2,7 @@
 import calendar
 import logging
 import time
+import numbers
 import pytz
 
 from six import text_type
@@ -775,6 +776,43 @@ def are_favorites(cls, user, objects):
         return [fav.object_id for fav in cls.query.filter(cls.object_id.in_([o.id for o in objects]), cls.object_type == object_type, cls.user_id == user)]
 
 
+OPERATORS = {
+    '>': lambda v, t: v > t,
+    '>=': lambda v, t: v >= t,
+    '<': lambda v, t: v < t,
+    '<=': lambda v, t: v <= t,
+    '==': lambda v, t: v == t,
+    '!=': lambda v, t: v != t,
+
+    # backward compatibility
+    'greater than': lambda v, t: v > t,
+    'less than': lambda v, t: v < t,
+    'equals': lambda v, t: v == t,
+}
+
+
+def next_state(op, value, threshold):
+    if isinstance(value, numbers.Number) and not isinstance(value, bool):
+        try:
+            threshold = float(threshold)
+        except ValueError:
+            return Alert.UNKNOWN_STATE
+    # If it's a boolean cast to string and lower case, because upper cased
+    # boolean value is Python specific and most likely will be confusing to
+    # users.
+    elif isinstance(value, bool):
+        value = str(value).lower()
+    else:
+        value = str(value)
+
+    if op(value, threshold):
+        new_state = Alert.TRIGGERED_STATE
+    else:
+        new_state = Alert.OK_STATE
+    
+    return new_state
+
+
 @generic_repr('id', 'name', 'query_id', 'user_id', 'state', 'last_triggered_at', 'rearm')
 class Alert(TimestampMixin, BelongsToOrgMixin, db.Model):
     UNKNOWN_STATE = 'unknown'
@@ -819,28 +857,12 @@ def evaluate(self):
         data = self.query_rel.latest_query_data.data
 
         if data['rows'] and self.options['column'] in data['rows'][0]:
-            operators = {
-                '>': lambda v, t: v > t,
-                '>=': lambda v, t: v >= t,
-                '<': lambda v, t: v < t,
-                '<=': lambda v, t: v <= t,
-                '==': lambda v, t: v == t,
-                '!=': lambda v, t: v != t,
-
-                # backward compatibility
-                'greater than': lambda v, t: v > t,
-                'less than': lambda v, t: v < t,
-                'equals': lambda v, t: v == t,
-            }
-            should_trigger = operators.get(self.options['op'], lambda v, t: False)
+            op = OPERATORS.get(self.options['op'], lambda v, t: False)
 
             value = data['rows'][0][self.options['column']]
             threshold = self.options['value']
 
-            if should_trigger(value, threshold):
-                new_state = self.TRIGGERED_STATE
-            else:
-                new_state = self.OK_STATE
+            new_state = next_state(op, value, threshold)
         else:
             new_state = self.UNKNOWN_STATE
 

tests/models/test_alerts.py

@@ -1,5 +1,6 @@
+from unittest import TestCase
 from tests import BaseTestCase
-from redash.models import Alert, db
+from redash.models import Alert, db, next_state, OPERATORS
 from redash.utils import json_dumps
 
 
@@ -44,21 +45,44 @@ def get_results(value):
 
 
 class TestAlertEvaluate(BaseTestCase):
-    def create_alert(self, results, column='foo'):
+    def create_alert(self, results, column='foo', value="1"):
         result = self.factory.create_query_result(data=results)
         query = self.factory.create_query(latest_query_data_id=result.id)
-        alert = self.factory.create_alert(query_rel=query, options={'op': 'equals', 'column': column, 'value': 1})
+        alert = self.factory.create_alert(query_rel=query, options={'op': 'equals', 'column': column, 'value': value})
         return alert
 
     def test_evaluate_triggers_alert_when_equal(self):
         alert = self.create_alert(get_results(1))
         self.assertEqual(alert.evaluate(), Alert.TRIGGERED_STATE)
 
+    def test_evaluate_number_value_and_string_threshold(self):
+        alert = self.create_alert(get_results(1), value="string")
+        self.assertEqual(alert.evaluate(), Alert.UNKNOWN_STATE)
+
     def test_evaluate_return_unknown_when_missing_column(self):
         alert = self.create_alert(get_results(1), column='bar')
         self.assertEqual(alert.evaluate(), Alert.UNKNOWN_STATE)
 
     def test_evaluate_return_unknown_when_empty_results(self):
         results = json_dumps({'rows': [], 'columns': [{'name': 'foo', 'type': 'STRING'}]})
         alert = self.create_alert(results)
-        self.assertEqual(alert.evaluate(), Alert.UNKNOWN_STATE)
+        self.assertEqual(alert.evaluate(), Alert.UNKNOWN_STATE)
+
+
+class TestNextState(TestCase):
+    def test_numeric_value(self):
+        self.assertEqual(Alert.TRIGGERED_STATE, next_state(OPERATORS.get('=='), 1, "1"))
+        self.assertEqual(Alert.TRIGGERED_STATE, next_state(OPERATORS.get('=='), 1, "1.0"))
+    
+    def test_numeric_value_and_plain_string(self):
+        self.assertEqual(Alert.UNKNOWN_STATE, next_state(OPERATORS.get('=='), 1, "string"))
+
+    def test_non_numeric_value(self):
+        self.assertEqual(Alert.OK_STATE, next_state(OPERATORS.get('=='), "1", "1.0"))
+
+    def test_string_value(self):
+        self.assertEqual(Alert.TRIGGERED_STATE, next_state(OPERATORS.get('=='), "string", "string"))
+    
+    def test_boolean_value(self):
+        self.assertEqual(Alert.TRIGGERED_STATE, next_state(OPERATORS.get('=='), False, 'false'))
+        self.assertEqual(Alert.TRIGGERED_STATE, next_state(OPERATORS.get('!='), False, 'true'))