From fe930369825024783771a6eef2dbfa355abb7eb5 Mon Sep 17 00:00:00 2001 From: psrok1 Date: Tue, 5 Mar 2024 21:07:54 +0100 Subject: [PATCH] Fix negated filters logic: non-boolean AND/OR --- karton/core/task.py | 71 +++++++++++++++++-------- tests/test_task_filters.py | 105 +++++++++++++++++++++++++++++++++++++ 2 files changed, 154 insertions(+), 22 deletions(-) diff --git a/karton/core/task.py b/karton/core/task.py index 51cba9a..c8e2ddd 100644 --- a/karton/core/task.py +++ b/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"): diff --git a/tests/test_task_filters.py b/tests/test_task_filters.py index 035b5ae..ae89270 100644 --- a/tests/test_task_filters.py +++ b/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)) + # 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_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)) + + 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))