From 2f9e1cee048c70ab18f2fe72dd12d95bfc40dd26 Mon Sep 17 00:00:00 2001 From: Petr Voronov Date: Fri, 4 Jul 2025 10:01:29 +0300 Subject: [PATCH 1/5] Fixes #19633. Added new test cases to handle cases where the field being compared (x) doesn't exist in the data dictionary, which would result in None when trying to access it, and you want these comparisons to safely return False rather than raising an error. For this moment it returns and brake process_event_rules(). TypeError: expected string or bytes-like object, got 'NoneType'. --- netbox/extras/tests/test_conditions.py | 77 +++++++++++++++++--------- 1 file changed, 50 insertions(+), 27 deletions(-) diff --git a/netbox/extras/tests/test_conditions.py b/netbox/extras/tests/test_conditions.py index dfe460f99..8d38802ca 100644 --- a/netbox/extras/tests/test_conditions.py +++ b/netbox/extras/tests/test_conditions.py @@ -62,66 +62,89 @@ class ConditionTestCase(TestCase): def test_eq(self): c = Condition('x', 1, 'eq') - self.assertTrue(c.eval({'x': 1})) - self.assertFalse(c.eval({'x': 2})) + self.assertTrue(c.eval({'x': 1})) # 1 == 1 → True + self.assertFalse(c.eval({'x': 2})) # 2 == 1 → False + self.assertFalse(c.eval({'x': None})) # None == 1 → False + self.assertFalse(c.eval({'z': 1})) # Missing 'x' → treated as None → False def test_eq_negated(self): c = Condition('x', 1, 'eq', negate=True) - self.assertFalse(c.eval({'x': 1})) - self.assertTrue(c.eval({'x': 2})) + self.assertFalse(c.eval({'x': 1})) # not (1 == 1) → False + self.assertTrue(c.eval({'x': 2})) # not (2 == 1) → True + self.assertTrue(c.eval({'x': None})) # not (None == 1) → True + self.assertTrue(c.eval({'z': 1})) # Missing 'x' → treated as None → True def test_gt(self): c = Condition('x', 1, 'gt') - self.assertTrue(c.eval({'x': 2})) - self.assertFalse(c.eval({'x': 1})) + self.assertTrue(c.eval({'x': 2})) # 2 > 1 → True + self.assertFalse(c.eval({'x': 1})) # 1 > 1 → False + self.assertFalse(c.eval({'x': None})) # None > 1 → False (safe handling) + self.assertFalse(c.eval({'z': 1})) # Missing 'x' → treated as None → False def test_gte(self): c = Condition('x', 1, 'gte') - self.assertTrue(c.eval({'x': 2})) - self.assertTrue(c.eval({'x': 1})) - self.assertFalse(c.eval({'x': 0})) + self.assertTrue(c.eval({'x': 2})) # 2 >= 1 → True + self.assertTrue(c.eval({'x': 1})) # 1 >= 1 → True + self.assertFalse(c.eval({'x': 0})) # 0 >= 1 → False + self.assertFalse(c.eval({'x': None})) # None >= 1 → False + self.assertFalse(c.eval({'z': 1})) # Missing 'x' → False def test_lt(self): c = Condition('x', 2, 'lt') - self.assertTrue(c.eval({'x': 1})) - self.assertFalse(c.eval({'x': 2})) + self.assertTrue(c.eval({'x': 1})) # 1 < 2 → True + self.assertFalse(c.eval({'x': 2})) # 2 < 2 → False + self.assertFalse(c.eval({'x': None})) # None < 2 → False + self.assertFalse(c.eval({'z': 1})) # Missing 'x' → False def test_lte(self): c = Condition('x', 2, 'lte') - self.assertTrue(c.eval({'x': 1})) - self.assertTrue(c.eval({'x': 2})) - self.assertFalse(c.eval({'x': 3})) + self.assertTrue(c.eval({'x': 1})) # 1 <= 2 → True + self.assertTrue(c.eval({'x': 2})) # 2 <= 2 → True + self.assertFalse(c.eval({'x': 3})) # 3 <= 2 → False + self.assertFalse(c.eval({'x': None})) # None <= 2 → False + self.assertFalse(c.eval({'z': 1})) # Missing 'x' → False def test_in(self): c = Condition('x', [1, 2, 3], 'in') - self.assertTrue(c.eval({'x': 1})) - self.assertFalse(c.eval({'x': 9})) + self.assertTrue(c.eval({'x': 1})) # 1 in [1,2,3] → True + self.assertFalse(c.eval({'x': 9})) # 9 in [1,2,3] → False + self.assertFalse(c.eval({'x': None})) # None in [1,2,3] → False + self.assertFalse(c.eval({'z': 1})) # Missing 'x' → False def test_in_negated(self): c = Condition('x', [1, 2, 3], 'in', negate=True) - self.assertFalse(c.eval({'x': 1})) - self.assertTrue(c.eval({'x': 9})) + self.assertFalse(c.eval({'x': 1})) # not (1 in [1,2,3]) → False + self.assertTrue(c.eval({'x': 9})) # not (9 in [1,2,3]) → True + self.assertTrue(c.eval({'x': None})) # not (None in [1,2,3]) → True + self.assertTrue(c.eval({'z': 1})) # Missing 'x' → True def test_contains(self): c = Condition('x', 1, 'contains') - self.assertTrue(c.eval({'x': [1, 2, 3]})) - self.assertFalse(c.eval({'x': [2, 3, 4]})) + self.assertTrue(c.eval({'x': [1, 2, 3]})) # 1 in [1,2,3] → True + self.assertFalse(c.eval({'x': [2, 3, 4]})) # 1 in [2,3,4] → False + self.assertFalse(c.eval({'x': None})) # 1 in None → False + self.assertFalse(c.eval({'z': [1, 2, 3]})) # Missing 'x' → False def test_contains_negated(self): c = Condition('x', 1, 'contains', negate=True) - self.assertFalse(c.eval({'x': [1, 2, 3]})) - self.assertTrue(c.eval({'x': [2, 3, 4]})) + self.assertFalse(c.eval({'x': [1, 2, 3]})) # not (1 in [1,2,3]) → False + self.assertTrue(c.eval({'x': [2, 3, 4]})) # not (1 in [2,3,4]) → True + self.assertTrue(c.eval({'x': None})) # not (1 in None) → True + self.assertTrue(c.eval({'z': [1, 2, 3]})) # Missing 'x' → True def test_regex(self): c = Condition('x', '[a-z]+', 'regex') - self.assertTrue(c.eval({'x': 'abc'})) - self.assertFalse(c.eval({'x': '123'})) + self.assertTrue(c.eval({'x': 'abc'})) # 'abc' matches regex → True + self.assertFalse(c.eval({'x': '123'})) # '123' doesn't match → False + self.assertFalse(c.eval({'x': None})) # None doesn't match → False + self.assertFalse(c.eval({'z': 'abc'})) # Missing 'x' → False def test_regex_negated(self): c = Condition('x', '[a-z]+', 'regex', negate=True) - self.assertFalse(c.eval({'x': 'abc'})) - self.assertTrue(c.eval({'x': '123'})) - + self.assertFalse(c.eval({'x': 'abc'})) # not (match) → False + self.assertTrue(c.eval({'x': '123'})) # not (no match) → True + self.assertTrue(c.eval({'x': None})) # not (None match) → True + self.assertTrue(c.eval({'z': 'abc'})) # Missing 'x' → True class ConditionSetTest(TestCase): From 8c8797599baa1601af52c6f8b570026ea840fcc2 Mon Sep 17 00:00:00 2001 From: Petr Voronov Date: Fri, 4 Jul 2025 10:19:22 +0300 Subject: [PATCH 2/5] Fixes #19633. Handle case that was the reason of TypeError and raise exeption: "TypeError: argument of type 'NoneType' is not iterable" with lead to breaking all events in the same object_type. --- netbox/extras/conditions.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/netbox/extras/conditions.py b/netbox/extras/conditions.py index 5680be444..8dec541bd 100644 --- a/netbox/extras/conditions.py +++ b/netbox/extras/conditions.py @@ -59,6 +59,7 @@ class Condition: if op not in self.TYPES[type(value)]: raise ValueError(_("Invalid type for {op} operation: {value}").format(op=op, value=type(value))) + self.op = op self.attr = attr self.value = value self.eval_func = getattr(self, f'eval_{op}') @@ -79,7 +80,12 @@ class Condition: except TypeError: # Invalid key path value = None - result = self.eval_func(value) + if value is None: + # Hande comparison case when value is None. + if self.op in (self.GT, self.GTE, self.LT, self.LTE, self.IN, self.CONTAINS, self.REGEX): + result = False + else: + result = self.eval_func(value) if self.negate: return not result From ee2aaa6508da81f60cd147253900f203ac9c1b51 Mon Sep 17 00:00:00 2001 From: Petr Voronov Date: Tue, 8 Jul 2025 20:11:03 +0300 Subject: [PATCH 3/5] Fixes #19633 fix PEP8 compliance error of previous commit in test_conditions.py --- netbox/extras/tests/test_conditions.py | 1 + 1 file changed, 1 insertion(+) diff --git a/netbox/extras/tests/test_conditions.py b/netbox/extras/tests/test_conditions.py index 8d38802ca..0ddb89cf3 100644 --- a/netbox/extras/tests/test_conditions.py +++ b/netbox/extras/tests/test_conditions.py @@ -146,6 +146,7 @@ class ConditionTestCase(TestCase): self.assertTrue(c.eval({'x': None})) # not (None match) → True self.assertTrue(c.eval({'z': 'abc'})) # Missing 'x' → True + class ConditionSetTest(TestCase): def test_empty(self): From ca9b2947ba6895cca4fdc09127fe7974eb4807b0 Mon Sep 17 00:00:00 2001 From: Petr Voronov Date: Wed, 9 Jul 2025 16:23:07 +0300 Subject: [PATCH 4/5] Fixes #19633 typo error. --- netbox/extras/conditions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/netbox/extras/conditions.py b/netbox/extras/conditions.py index 8dec541bd..9e2dd3809 100644 --- a/netbox/extras/conditions.py +++ b/netbox/extras/conditions.py @@ -81,7 +81,7 @@ class Condition: # Invalid key path value = None if value is None: - # Hande comparison case when value is None. + # Handle comparison case when value is None. if self.op in (self.GT, self.GTE, self.LT, self.LTE, self.IN, self.CONTAINS, self.REGEX): result = False else: From f41da06d9b4a9da1e4871e60bc318bfa7732936b Mon Sep 17 00:00:00 2001 From: Petr Voronov Date: Thu, 10 Jul 2025 17:31:06 +0300 Subject: [PATCH 5/5] Closes #19633 conditions.py - Change condition with self.negate to result XOR self.negate for compactness. context_managers.py added context manager suppress_event_exp to prevent missing exceptions in cycles related to sequential event handling events.py - using cm suppress_event_exp. test_event_rules.py - added test to emulate the exception in process_event_function and verify that exception was logged properly. --- netbox/extras/conditions.py | 11 +- netbox/extras/events.py | 154 ++++++++++++------------ netbox/extras/tests/test_event_rules.py | 64 ++++++++++ netbox/netbox/context_managers.py | 20 ++- 4 files changed, 167 insertions(+), 82 deletions(-) diff --git a/netbox/extras/conditions.py b/netbox/extras/conditions.py index 9e2dd3809..7de5a0a90 100644 --- a/netbox/extras/conditions.py +++ b/netbox/extras/conditions.py @@ -80,16 +80,15 @@ class Condition: except TypeError: # Invalid key path value = None + if value is None: # Handle comparison case when value is None. if self.op in (self.GT, self.GTE, self.LT, self.LTE, self.IN, self.CONTAINS, self.REGEX): - result = False - else: - result = self.eval_func(value) + return False ^ self.negate - if self.negate: - return not result - return result + result = self.eval_func(value) + + return result ^ self.negate # Equivalency diff --git a/netbox/extras/events.py b/netbox/extras/events.py index 95170e18d..5e3db226c 100644 --- a/netbox/extras/events.py +++ b/netbox/extras/events.py @@ -11,6 +11,7 @@ from django_rq import get_queue from core.events import * from netbox.config import get_config from netbox.constants import RQ_QUEUE_DEFAULT +from netbox.context_managers import suppress_event_exp from netbox.registry import registry from users.models import User from utilities.api import get_serializer_for_model @@ -86,71 +87,73 @@ def process_event_rules(event_rules, object_type, event_type, data, username=Non for event_rule in event_rules: - # Evaluate event rule conditions (if any) - if not event_rule.eval_conditions(data): - continue + with suppress_event_exp(logger): - # Compile event data - event_data = event_rule.action_data or {} - event_data.update(data) + # Evaluate event rule conditions (if any) + if not event_rule.eval_conditions(data): + continue - # Webhooks - if event_rule.action_type == EventRuleActionChoices.WEBHOOK: + # Compile event data + event_data = event_rule.action_data or {} + event_data.update(data) - # Select the appropriate RQ queue - queue_name = get_config().QUEUE_MAPPINGS.get('webhook', RQ_QUEUE_DEFAULT) - rq_queue = get_queue(queue_name) + # Webhooks + if event_rule.action_type == EventRuleActionChoices.WEBHOOK: - # Compile the task parameters - params = { - "event_rule": event_rule, - "model_name": object_type.model, - "event_type": event_type, - "data": event_data, - "snapshots": snapshots, - "timestamp": timezone.now().isoformat(), - "username": username, - "retry": get_rq_retry() - } - if snapshots: - params["snapshots"] = snapshots - if request_id: - params["request_id"] = request_id + # Select the appropriate RQ queue + queue_name = get_config().QUEUE_MAPPINGS.get('webhook', RQ_QUEUE_DEFAULT) + rq_queue = get_queue(queue_name) - # Enqueue the task - rq_queue.enqueue( - "extras.webhooks.send_webhook", - **params - ) + # Compile the task parameters + params = { + "event_rule": event_rule, + "model_name": object_type.model, + "event_type": event_type, + "data": event_data, + "snapshots": snapshots, + "timestamp": timezone.now().isoformat(), + "username": username, + "retry": get_rq_retry() + } + if snapshots: + params["snapshots"] = snapshots + if request_id: + params["request_id"] = request_id - # Scripts - elif event_rule.action_type == EventRuleActionChoices.SCRIPT: - # Resolve the script from action parameters - script = event_rule.action_object.python_class() + # Enqueue the task + rq_queue.enqueue( + "extras.webhooks.send_webhook", + **params + ) - # Enqueue a Job to record the script's execution - from extras.jobs import ScriptJob - ScriptJob.enqueue( - instance=event_rule.action_object, - name=script.name, - user=user, - data=event_data - ) + # Scripts + elif event_rule.action_type == EventRuleActionChoices.SCRIPT: + # Resolve the script from action parameters + script = event_rule.action_object.python_class() - # Notification groups - elif event_rule.action_type == EventRuleActionChoices.NOTIFICATION: - # Bulk-create notifications for all members of the notification group - event_rule.action_object.notify( - object_type=object_type, - object_id=event_data['id'], - object_repr=event_data.get('display'), - event_type=event_type - ) + # Enqueue a Job to record the script's execution + from extras.jobs import ScriptJob + ScriptJob.enqueue( + instance=event_rule.action_object, + name=script.name, + user=user, + data=event_data + ) - else: - raise ValueError(_("Unknown action type for an event rule: {action_type}").format( - action_type=event_rule.action_type - )) + # Notification groups + elif event_rule.action_type == EventRuleActionChoices.NOTIFICATION: + # Bulk-create notifications for all members of the notification group + event_rule.action_object.notify( + object_type=object_type, + object_id=event_data['id'], + object_repr=event_data.get('display'), + event_type=event_type + ) + + else: + raise ValueError(_("Unknown action type for an event rule: {action_type}").format( + action_type=event_rule.action_type + )) def process_event_queue(events): @@ -160,27 +163,28 @@ def process_event_queue(events): events_cache = defaultdict(dict) for event in events: - event_type = event['event_type'] - object_type = event['object_type'] + with suppress_event_exp(logger): + event_type = event['event_type'] + object_type = event['object_type'] - # Cache applicable Event Rules - if object_type not in events_cache[event_type]: - events_cache[event_type][object_type] = EventRule.objects.filter( - event_types__contains=[event['event_type']], - object_types=object_type, - enabled=True + # Cache applicable Event Rules + if object_type not in events_cache[event_type]: + events_cache[event_type][object_type] = EventRule.objects.filter( + event_types__contains=[event['event_type']], + object_types=object_type, + enabled=True + ) + event_rules = events_cache[event_type][object_type] + + process_event_rules( + event_rules=event_rules, + object_type=object_type, + event_type=event['event_type'], + data=event['data'], + username=event['username'], + snapshots=event['snapshots'], + request_id=event['request_id'] ) - event_rules = events_cache[event_type][object_type] - - process_event_rules( - event_rules=event_rules, - object_type=object_type, - event_type=event['event_type'], - data=event['data'], - username=event['username'], - snapshots=event['snapshots'], - request_id=event['request_id'] - ) def flush_events(events): diff --git a/netbox/extras/tests/test_event_rules.py b/netbox/extras/tests/test_event_rules.py index 2565e5bde..e4d27da82 100644 --- a/netbox/extras/tests/test_event_rules.py +++ b/netbox/extras/tests/test_event_rules.py @@ -390,6 +390,70 @@ class EventRuleTest(APITestCase): with patch.object(Session, 'send', dummy_send): send_webhook(**job.kwargs) + def test_suppression_flush_events(self): + + # Use context manager to verify that exception was logged as we expected + with self.assertLogs('netbox.events_processor', level='ERROR') as cm: + + # Get the known event + event = EventRule.objects.get(name='Event Rule 2') + + site = Site.objects.create(name='Site 1', slug='site-1', status=SiteStatusChoices.STATUS_PLANNED) + + # Update an object via the REST API + data = { + 'name': 'Site X', + 'status': SiteStatusChoices.STATUS_ACTIVE, + } + url = reverse('dcim-api:site-detail', kwargs={'pk': site.pk}) + self.add_permissions('dcim.change_site') + response = self.client.patch(url, data, format='json', **self.header) + self.assertHttpStatus(response, status.HTTP_200_OK) + + # Verify that a background task was queued for the updated object + self.assertEqual(self.queue.count, 1) + job = self.queue.jobs[0] + self.assertEqual(job.kwargs['event_rule'], event) + self.assertEqual(job.kwargs['event_type'], OBJECT_UPDATED) + self.assertEqual(job.kwargs['model_name'], 'site') + self.assertEqual(job.kwargs['data']['id'], site.pk) + self.assertEqual(job.kwargs['data']['status']['value'], SiteStatusChoices.STATUS_ACTIVE) + self.assertEqual(job.kwargs['snapshots']['prechange']['name'], 'Site 1') + self.assertEqual(job.kwargs['snapshots']['prechange']['status'], SiteStatusChoices.STATUS_PLANNED) + self.assertEqual(job.kwargs['snapshots']['postchange']['name'], 'Site X') + self.assertEqual(job.kwargs['snapshots']['postchange']['status'], SiteStatusChoices.STATUS_ACTIVE) + + # Update the event with non-existent + event.action_type = 'non-existent-action-type' + event.save() + + # Cleanup queue + self.queue.empty() + + # Verify that a queue is empty + self.assertEqual(self.queue.count, 0) + + # Update an object via the REST API + data = { + 'name': 'Site X', + 'status': SiteStatusChoices.STATUS_PLANNED, + } + url = reverse('dcim-api:site-detail', kwargs={'pk': site.pk}) + self.add_permissions('dcim.change_site') + response = self.client.patch(url, data, format='json', **self.header) + self.assertHttpStatus(response, status.HTTP_200_OK) + + # Verify that a queue is still empty + self.assertEqual(self.queue.count, 0) + + # Verify that we have only one ERROR message in our log + self.assertEqual(len(cm.output), 1) + + # Verify message format + pattern = (r"Error ValueError in ValueError at .*? - Unknown action type for" + r" an event rule: non-existent-action-type") + self.assertRegex(cm.output[0], pattern) + def test_duplicate_triggers(self): """ Test for erroneous duplicate event triggers resulting from saving an object multiple times diff --git a/netbox/netbox/context_managers.py b/netbox/netbox/context_managers.py index 7b01cce94..91d22f92a 100644 --- a/netbox/netbox/context_managers.py +++ b/netbox/netbox/context_managers.py @@ -1,8 +1,8 @@ +import traceback from contextlib import contextmanager from netbox.context import current_request, events_queue from netbox.utils import register_request_processor -from extras.events import flush_events @register_request_processor @@ -14,6 +14,8 @@ def event_tracking(request): :param request: WSGIRequest object with a unique `id` set """ + from extras.events import flush_events + current_request.set(request) events_queue.set({}) @@ -26,3 +28,19 @@ def event_tracking(request): # Clear context vars current_request.set(None) events_queue.set({}) + + +@contextmanager +def suppress_event_exp(logger=None): + """ + Suppress exceptions that may occur during event handling. + """ + try: + yield + except Exception as e: + if logger: + tb = e.__traceback__ + last_frame = tb.tb_frame if tb else None + filename = last_frame.f_code.co_filename if last_frame else 'unknown' + lineno = tb.tb_lineno if tb else 0 + logger.error(f"Error {e.__class__.__name__} in {e.__class__.__name__} at {filename}:{lineno} - {str(e)}")