Making merged filters short-circuit (#1350)

* Making merged filters short-circuit

* Add notes to docs about short-circuiting

telegram/ext/callbackcontext.py

@@ -50,6 +50,8 @@ class CallbackContext(object):
         matches (List[:obj:`re match object`], optional): If the associated update originated from
             a regex-supported handler or had a :class:`Filters.regex`, this will contain a list of
             match objects for every pattern where ``re.search(pattern, string)`` returned a match.
+            Note that filters short circuit, so combined regex filters will not always
+            be evaluated.
         args (List[:obj:`str`], optional): Arguments passed to a command if the associated update
             is handled by :class:`telegram.ext.CommandHandler`, :class:`telegram.ext.PrefixHandler`
             or :class:`telegram.ext.StringCommandHandler`. It contains a list of the words in the

telegram/ext/filters.py

@@ -49,6 +49,16 @@ class BaseFilter(object):
         >>> (Filters.text & (Filters.entity(URL) | Filters.entity(TEXT_LINK)))
         >>> Filters.text & (~ Filters.forwarded)
 
+    Note:
+        Filters use the same short circuiting logic that pythons `and`, `or` and `not`.
+        This means that for example:
+
+            >>> Filters.regex(r'(a?x)') | Filters.regex(r'(b?x)')
+
+        With a message.text of `x`, will only ever return the matches for the first filter,
+        since the second one is never evaluated.
+
+
     If you want to create your own filters create a class inheriting from this class and implement
     a `filter` method that returns a boolean: `True` if the message should be handled, `False`
     otherwise. Note that the filters work only as class instances, not actual class objects
@@ -177,21 +187,27 @@ class MergedFilter(BaseFilter):
         # We need to check if the filters are data filters and if so return the merged data.
         # If it's not a data filter or an or_filter but no matches return bool
         if self.and_filter:
-            comp_output = self.and_filter(update)
-            if base_output and comp_output:
-                if self.data_filter:
-                    merged = self._merge(base_output, comp_output)
-                    if merged:
-                        return merged
-                return True
+            # And filter needs to short circuit if base is falsey
+            if base_output:
+                comp_output = self.and_filter(update)
+                if comp_output:
+                    if self.data_filter:
+                        merged = self._merge(base_output, comp_output)
+                        if merged:
+                            return merged
+                    return True
         elif self.or_filter:
-            comp_output = self.or_filter(update)
-            if base_output or comp_output:
+            # Or filter needs to short circuit if base is truthey
+            if base_output:
                 if self.data_filter:
-                    merged = self._merge(base_output, comp_output)
-                    if merged:
-                        return merged
+                    return base_output
                 return True
+            else:
+                comp_output = self.or_filter(update)
+                if comp_output:
+                    if self.data_filter:
+                        return comp_output
+                    return True
         return False
 
     def __repr__(self):
@@ -251,6 +267,15 @@ class Filters(object):
             you want your pattern to be case insensitive. This approach is recommended
             if you need to specify flags on your pattern.
 
+        Note:
+            Filters use the same short circuiting logic that pythons `and`, `or` and `not`.
+            This means that for example:
+
+                >>> Filters.regex(r'(a?x)') | Filters.regex(r'(b?x)')
+
+            With a message.text of `x`, will only ever return the matches for the first filter,
+            since the second one is never evaluated.
+
         Args:
             pattern (:obj:`str` | :obj:`Pattern`): The regex pattern.
         """

tests/test_filters.py

@@ -122,12 +122,9 @@ class TestFilters(object):
         matches = result['matches']
         assert isinstance(matches, list)
         assert all([type(res) == SRE_TYPE for res in matches])
+        # Should not give a match since it's a or filter and it short circuits
         result = (Filters.command | Filters.regex(r'linked param'))(update)
-        assert result
-        assert isinstance(result, dict)
-        matches = result['matches']
-        assert isinstance(matches, list)
-        assert all([type(res) == SRE_TYPE for res in matches])
+        assert result is True
 
     def test_regex_complex_merges(self, update):
         SRE_TYPE = type(re.match("", ""))
@@ -725,3 +722,83 @@ class TestFilters(object):
         assert Filters.update.edited_channel_post(update)
         assert Filters.update.channel_posts(update)
         assert Filters.update(update)
+
+    def test_merged_short_circuit_and(self, update):
+        update.message.text = '/test'
+
+        class TestException(Exception):
+            pass
+
+        class RaisingFilter(BaseFilter):
+            def filter(self, _):
+                raise TestException
+
+        raising_filter = RaisingFilter()
+
+        with pytest.raises(TestException):
+            (Filters.command & raising_filter)(update)
+
+        update.message.text = 'test'
+        (Filters.command & raising_filter)(update)
+
+    def test_merged_short_circuit_or(self, update):
+        update.message.text = 'test'
+
+        class TestException(Exception):
+            pass
+
+        class RaisingFilter(BaseFilter):
+            def filter(self, _):
+                raise TestException
+
+        raising_filter = RaisingFilter()
+
+        with pytest.raises(TestException):
+            (Filters.command | raising_filter)(update)
+
+        update.message.text = '/test'
+        (Filters.command | raising_filter)(update)
+
+    def test_merged_data_merging_and(self, update):
+        update.message.text = '/test'
+
+        class DataFilter(BaseFilter):
+            data_filter = True
+
+            def __init__(self, data):
+                self.data = data
+
+            def filter(self, _):
+                return {'test': [self.data]}
+
+        result = (Filters.command & DataFilter('blah'))(update)
+        assert result['test'] == ['blah']
+
+        result = (DataFilter('blah1') & DataFilter('blah2'))(update)
+        assert result['test'] == ['blah1', 'blah2']
+
+        update.message.text = 'test'
+        result = (Filters.command & DataFilter('blah'))(update)
+        assert not result
+
+    def test_merged_data_merging_or(self, update):
+        update.message.text = '/test'
+
+        class DataFilter(BaseFilter):
+            data_filter = True
+
+            def __init__(self, data):
+                self.data = data
+
+            def filter(self, _):
+                return {'test': [self.data]}
+
+        result = (Filters.command | DataFilter('blah'))(update)
+        assert result
+
+        result = (DataFilter('blah1') | DataFilter('blah2'))(update)
+        assert result['test'] == ['blah1']
+
+        update.message.text = 'test'
+        result = (Filters.command | DataFilter('blah'))(update)
+        assert result['test'] == ['blah']