Fix negated filters logic: non-boolean AND/OR (#247)

docs/requirements.txt

@@ -1,4 +1,4 @@
-Sphinx==4.5.0
+Sphinx==5.3.0
 sphinx-autodoc-annotation==1.0.post1
 sphinx-autodoc-typehints==1.10.3
 sphinx-rtd-theme==0.5.0

docs/task_headers_payloads.rst

@@ -107,6 +107,18 @@ Filter logic can be used to fulfill specific use-cases:
 ``[{"foo": "!*"}]``                   'foo' header must be not defined.
 ====================================  ==============================================================================
 
+Excluding (negated) filters come with specific corner-cases. Regular filters require specific value to be defined in header, while
+negated filters are accepting all possible values except specified in filter.
+
+==================================================================================  =============================================================================================================================================
+   ``filters`` value                                                                  Meaning
+----------------------------------------------------------------------------------  ---------------------------------------------------------------------------------------------------------------------------------------------
+``[{"type": "sample", "stage": "!*"}]``                                             matches only tasks that have type 'sample' but no 'stage' key
+``[{"platform": "!linux"}, {"platform": "!windows"}]``                              matches **all** tasks (even with no headers) but not these with platform 'linux' or 'windows'
+``[{"foo": "bar", "platform": "!linux"}, {"foo": "bar", "platform": "!windows"}]``  'foo' header is required and must have 'bar' value, but platform can't be 'linux' or 'windows'
+``[{"foo": "bar", "platform": "!linux"}, {"foo": "baz", "platform": "!windows"}]``  'foo' header is required and must have 'bar' value and no 'linux' in platform key, or foo must be 'baz', but then platform can't be 'windows'
+==================================================================================  =============================================================================================================================================
+
 .. warning::
 
     It's recommended to use only strings in filter and header values

karton/core/task.py

@@ -212,38 +212,65 @@ def matches_filters(self, filters: List[Dict[str, Any]]) -> bool:
         :meta private:
         """
 
-        matches = False
-        for task_filter in filters:
-            matched = []
-            for filter_key, filter_value in task_filter.items():
-                # Coerce to string for comparison
-                header_value = self.headers.get(filter_key)
+        def test_filter(headers: Dict[str, Any], filter: Dict[str, Any]) -> int:
+            """
+            Filter match follows AND logic, but it's non-boolean because filters may be
+            negated (task:!platform).
+
+            Result values are as follows:
+            - 1  - positive match, no mismatched values in headers
+                   (all matched)
+            - 0  - no match, found value that doesn't match to the filter
+                   (some are not matched)
+            - -1 - negative match, found value that matches negated filter value
+                   (all matched but found negative matches)
+            """
+            matches = 1
+            for filter_key, filter_value in filter.items():
+                # Coerce filter value to string
                 filter_value_str = str(filter_value)
-                header_value_str = str(header_value)
-
                 negated = False
                 if filter_value_str.startswith("!"):
                     negated = True
                     filter_value_str = filter_value_str[1:]
 
-                if header_value is None:
-                    matched.append(negated)
-                    continue
+                # If expected key doesn't exist in headers
+                if filter_key not in headers:
+                    # Negated filter ignores non-existent values
+                    if negated:
+                        continue
+                    # But positive filter doesn't
+                    return 0
 
+                # Coerce header value to string
+                header_value_str = str(headers[filter_key])
                 # fnmatch is great for handling simple wildcard patterns (?, *, [abc])
                 match = fnmatch.fnmatchcase(header_value_str, filter_value_str)
-                # if matches, but it's negated then we can return straight away
-                # since no matter the other filters
+                # If matches, but it's negated: it's negative match
                 if match and negated:
-                    return False
-
-                # else, apply a XOR logic to take care of negation matching
-                matched.append(match != negated)
-
-            # set the flag if all consumer filter fields match the task header.
-            # It will be set to True only if at least one filter matches the header
-            matches |= all(matched)
-
+                    matches = -1
+                # If doesn't match but filter is not negated: it's not a match
+                if not match and not negated:
+                    return 0
+            # If there are no mismatched values: filter is matched
+            return matches
+
+        # List of filter matches follow OR logic, but -1 is special
+        # If there is any -1, result is False
+        #   (any matched, but it's negative match)
+        # If there is any 1, but no -1's: result is True
+        #   (any matched, no negative match)
+        # If there are only 0's: result is False
+        #   (none matched)
+        matches = False
+        for task_filter in filters:
+            match_result = test_filter(self.headers, task_filter)
+            if match_result == -1:
+                # Any negative match results in False
+                return False
+            if match_result == 1:
+                # Any positive match but without negative matches results in True
+                matches = True
         return matches
 
     def set_task_parent(self, parent: "Task"):

tests/test_task_filters.py

@@ -57,6 +57,14 @@ def test_catch_all_filter(self):
         task_without_headers = Task(headers={})
         self.assertTrue(task_without_headers.matches_filters(filters))
 
+    def test_catch_none_filter(self):
+        filters = []
+        task_with_headers = Task(headers={"foo": "bar", "bar": "baz"})
+        self.assertFalse(task_with_headers.matches_filters(filters))
+
+        task_without_headers = Task(headers={})
+        self.assertFalse(task_without_headers.matches_filters(filters))
+
     def test_negated_filter(self):
         filters = [
             {
@@ -129,6 +137,68 @@ def test_negate_header_existence(self):
         })
         self.assertTrue(task_noplatform.matches_filters(filters))
 
+    def test_negate_header_existence_but_catch_all(self):
+        filters = [
+            {
+                "type": "sample",
+                "platform": "!*"
+            },
+            {}
+        ]
+        task_linux = Task(headers={
+            "type": "sample",
+            "kind": "runnable",
+            "platform": "linux"
+        })
+        self.assertFalse(task_linux.matches_filters(filters))
+
+        task_empty_string = Task(headers={
+            "type": "sample",
+            "kind": "runnable",
+            "platform": ""
+        })
+        self.assertFalse(task_empty_string.matches_filters(filters))
+
+        task_noplatform = Task(headers={
+            "type": "sample",
+            "kind": "runnable",
+        })
+        self.assertTrue(task_noplatform.matches_filters(filters))
+
+        # Platform requirement is defined for type=sample
+        task_different_type = Task(headers={
+            "type": "different",
+            "platform": "hehe",
+        })
+        self.assertTrue(task_different_type.matches_filters(filters))
+
+    def test_require_header_existence(self):
+        filters = [
+            {
+                "type": "sample",
+                "platform": "*"
+            }
+        ]
+        task_linux = Task(headers={
+            "type": "sample",
+            "kind": "runnable",
+            "platform": "linux"
+        })
+        self.assertTrue(task_linux.matches_filters(filters))
+
+        task_empty_string = Task(headers={
+            "type": "sample",
+            "kind": "runnable",
+            "platform": ""
+        })
+        self.assertTrue(task_empty_string.matches_filters(filters))
+
+        task_noplatform = Task(headers={
+            "type": "sample",
+            "kind": "runnable",
+        })
+        self.assertFalse(task_noplatform.matches_filters(filters))
+
     def test_non_string_headers(self):
         # It's not recommended but was allowed by earlier versions
         filters = [
@@ -156,3 +226,38 @@ def test_non_string_headers(self):
         })
         self.assertFalse(task_different_value.matches_filters(filters))
 
+    def test_negated_filter_for_different_type(self):
+        filters = [
+            {
+                "type": "sample",
+                "platform": "win32"
+            },
+            {
+                "type": "different",
+                "platform": "!win32"
+            }
+        ]
+
+        task_sample = Task(headers={
+            "type": "sample",
+            "platform": "win32"
+        })
+        self.assertTrue(task_sample.matches_filters(filters))
+
+        task_different_win32 = Task(headers={
+            "type": "different",
+            "platform": "win32"
+        })
+        self.assertFalse(task_different_win32.matches_filters(filters))
+
+        task_different_win64 = Task(headers={
+            "type": "different",
+            "platform": "win64"
+        })
+        self.assertTrue(task_different_win64.matches_filters(filters))
+
+        task_sample_win64 = Task(headers={
+            "type": "sample",
+            "platform": "win64"
+        })
+        self.assertFalse(task_sample_win64.matches_filters(filters))