From b5cb67bdbd67599c7d300c5a20faabb30fb1286b Mon Sep 17 00:00:00 2001 From: Peter Eckel Date: Wed, 21 Feb 2024 14:29:23 +0000 Subject: [PATCH 1/6] Fixes: #15194 - Check the whole queue for matching queued webhooks --- netbox/extras/signals.py | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/netbox/extras/signals.py b/netbox/extras/signals.py index 2813ed7aeb7..0799d849b50 100644 --- a/netbox/extras/signals.py +++ b/netbox/extras/signals.py @@ -55,16 +55,21 @@ def run_validators(instance, validators): clear_events = Signal() -def is_same_object(instance, webhook_data, request_id): +def last_matching_index(instance, queue, request_id): """ - Compare the given instance to the most recent queued webhook object, returning True - if they match. This check is used to avoid creating duplicate webhook entries. + Find the latest queued webhook object matching the given instance, returning its index. + If no object is found, return None. This check is used to avoid creating duplicate webhook + entries. """ - return ( - ContentType.objects.get_for_model(instance) == webhook_data['content_type'] and - instance.pk == webhook_data['object_id'] and - request_id == webhook_data['request_id'] - ) + try: + return max( + index_ for index_, webhook_data in enumerate(queue) + if ContentType.objects.get_for_model(instance) == webhook_data['content_type'] and + instance.pk == webhook_data['object_id'] and + request_id == webhook_data['request_id'] + ) + except ValueError: + return None @receiver((post_save, m2m_changed)) @@ -112,12 +117,12 @@ def handle_changed_object(sender, instance, **kwargs): objectchange.request_id = request.id objectchange.save() - # If this is an M2M change, update the previously queued webhook (from post_save) + # Update the previously queued webhook from a preceeding post_save queue = events_queue.get() - if m2m_changed and queue and is_same_object(instance, queue[-1], request.id): + if queue and (last_index := last_matching_index(instance, queue, request.id)) is not None: instance.refresh_from_db() # Ensure that we're working with fresh M2M assignments - queue[-1]['data'] = serialize_for_event(instance) - queue[-1]['snapshots']['postchange'] = get_snapshots(instance, action)['postchange'] + queue[last_index]['data'] = serialize_for_event(instance) + queue[last_index]['snapshots']['postchange'] = get_snapshots(instance, action)['postchange'] else: enqueue_object(queue, instance, request.user, request.id, action) events_queue.set(queue) From 9f3c750786b99aa1c95bde094cb861588785f1e3 Mon Sep 17 00:00:00 2001 From: Peter Eckel Date: Thu, 30 May 2024 15:15:24 +0000 Subject: [PATCH 2/6] HACK: Make get_serializer_for_model() work with dummy_plugin --- netbox/netbox/tests/dummy_plugin/models.py | 1 + netbox/utilities/api.py | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/netbox/netbox/tests/dummy_plugin/models.py b/netbox/netbox/tests/dummy_plugin/models.py index 9bd39a46b1c..ade46f76805 100644 --- a/netbox/netbox/tests/dummy_plugin/models.py +++ b/netbox/netbox/tests/dummy_plugin/models.py @@ -8,6 +8,7 @@ class DummyModel(models.Model): number = models.IntegerField( default=100 ) + serializer_label = 'netbox.tests.dummy_plugin' class Meta: ordering = ['name'] diff --git a/netbox/utilities/api.py b/netbox/utilities/api.py index 11b91481110..3cedfc4a36e 100644 --- a/netbox/utilities/api.py +++ b/netbox/utilities/api.py @@ -30,7 +30,12 @@ def get_serializer_for_model(model, prefix=''): """ Return the appropriate REST API serializer for the given model. """ - app_label, model_name = model._meta.label.split('.') + if hasattr(model, 'serializer_label'): + app_label = model.serializer_label + model_name = model._meta.label.split('.')[1] + else: + app_label, model_name = model._meta.label.split('.') + serializer_name = f'{app_label}.api.serializers.{prefix}{model_name}Serializer' try: return import_string(serializer_name) From 8fd508560d99a97f3c67455b31b9cff2fa1a82ca Mon Sep 17 00:00:00 2001 From: Peter Eckel Date: Thu, 30 May 2024 15:16:35 +0000 Subject: [PATCH 3/6] Correct the name of the serializer for DummyModel --- netbox/netbox/tests/dummy_plugin/api/serializers.py | 2 +- netbox/netbox/tests/dummy_plugin/api/views.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/netbox/netbox/tests/dummy_plugin/api/serializers.py b/netbox/netbox/tests/dummy_plugin/api/serializers.py index 239d7d998b0..0a641209579 100644 --- a/netbox/netbox/tests/dummy_plugin/api/serializers.py +++ b/netbox/netbox/tests/dummy_plugin/api/serializers.py @@ -2,7 +2,7 @@ from netbox.tests.dummy_plugin.models import DummyModel -class DummySerializer(ModelSerializer): +class DummyModelSerializer(ModelSerializer): class Meta: model = DummyModel diff --git a/netbox/netbox/tests/dummy_plugin/api/views.py b/netbox/netbox/tests/dummy_plugin/api/views.py index 58f221285cc..3fd51bcf23f 100644 --- a/netbox/netbox/tests/dummy_plugin/api/views.py +++ b/netbox/netbox/tests/dummy_plugin/api/views.py @@ -1,8 +1,8 @@ from rest_framework.viewsets import ModelViewSet from netbox.tests.dummy_plugin.models import DummyModel -from .serializers import DummySerializer +from .serializers import DummyModelSerializer class DummyViewSet(ModelViewSet): queryset = DummyModel.objects.all() - serializer_class = DummySerializer + serializer_class = DummyModelSerializer From e512027a91d4e5ed133f1c4011102ec0b416026f Mon Sep 17 00:00:00 2001 From: Peter Eckel Date: Thu, 30 May 2024 15:18:12 +0000 Subject: [PATCH 4/6] Modified DummyModel to make it trigger event rules --- netbox/netbox/tests/dummy_plugin/migrations/0001_initial.py | 2 ++ netbox/netbox/tests/dummy_plugin/models.py | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/netbox/netbox/tests/dummy_plugin/migrations/0001_initial.py b/netbox/netbox/tests/dummy_plugin/migrations/0001_initial.py index b7241b51de5..9cd597ff4be 100644 --- a/netbox/netbox/tests/dummy_plugin/migrations/0001_initial.py +++ b/netbox/netbox/tests/dummy_plugin/migrations/0001_initial.py @@ -15,6 +15,8 @@ class Migration(migrations.Migration): ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False)), ('name', models.CharField(max_length=20)), ('number', models.IntegerField(default=100)), + ('created', models.DateField(auto_now_add=True, null=True)), + ('last_updated', models.DateTimeField(auto_now=True, null=True)), ], options={ 'ordering': ['name'], diff --git a/netbox/netbox/tests/dummy_plugin/models.py b/netbox/netbox/tests/dummy_plugin/models.py index ade46f76805..19500e61919 100644 --- a/netbox/netbox/tests/dummy_plugin/models.py +++ b/netbox/netbox/tests/dummy_plugin/models.py @@ -1,7 +1,8 @@ from django.db import models +from netbox.models import EventRulesMixin, ChangeLoggingMixin -class DummyModel(models.Model): +class DummyModel(EventRulesMixin, ChangeLoggingMixin, models.Model): name = models.CharField( max_length=20 ) From ecdd30778a9ef6f6fbcf31dd356dceec21203756 Mon Sep 17 00:00:00 2001 From: Peter Eckel Date: Thu, 30 May 2024 15:49:43 +0000 Subject: [PATCH 5/6] Added essential tests for the double event rule invocation problem --- netbox/extras/tests/test_event_rules.py | 47 ++++++++++++++++++- .../dummy_plugin/migrations/0001_initial.py | 2 +- netbox/netbox/tests/dummy_plugin/models.py | 4 ++ 3 files changed, 51 insertions(+), 2 deletions(-) diff --git a/netbox/extras/tests/test_event_rules.py b/netbox/extras/tests/test_event_rules.py index 8cea2078a4c..80375ba82fb 100644 --- a/netbox/extras/tests/test_event_rules.py +++ b/netbox/extras/tests/test_event_rules.py @@ -16,6 +16,8 @@ from extras.models import EventRule, Tag, Webhook from extras.webhooks import generate_signature, send_webhook from utilities.testing import APITestCase +from netbox.tests.dummy_plugin.models import DummyModel +from netbox.context import events_queue class EventRuleTest(APITestCase): @@ -31,6 +33,7 @@ def setUp(self): def setUpTestData(cls): site_type = ObjectType.objects.get_for_model(Site) + dummy_type = ObjectType.objects.get_for_model(DummyModel) DUMMY_URL = 'http://localhost:9000/' DUMMY_SECRET = 'LOOKATMEIMASECRETSTRING' @@ -65,7 +68,7 @@ def setUpTestData(cls): ), )) for event_rule in event_rules: - event_rule.object_types.set([site_type]) + event_rule.object_types.set([site_type, dummy_type]) Tag.objects.bulk_create(( Tag(name='Foo', slug='foo'), @@ -377,3 +380,45 @@ def dummy_send(_, request, **kwargs): # Patch the Session object with our dummy_send() method, then process the webhook for sending with patch.object(Session, 'send', dummy_send) as mock_send: send_webhook(**job.kwargs) + + def test_webhook_double_save_create(self): + data = { + 'name': 'Dummy', + } + url = reverse('plugins-api:dummy_plugin-api:dummymodel-list') + self.add_permissions('dummy_plugin.add_dummymodel') + response = self.client.post(url, data, format='json', **self.header) + self.assertHttpStatus(response, status.HTTP_201_CREATED) + self.assertEqual(DummyModel.objects.count(), 1) + + dummy = DummyModel.objects.first() + + self.assertEqual(self.queue.count, 1) + job = self.queue.jobs[0] + self.assertEqual(job.kwargs['event_rule'], EventRule.objects.get(type_create=True)) + self.assertEqual(job.kwargs['event'], ObjectChangeActionChoices.ACTION_CREATE) + self.assertEqual(job.kwargs['model_name'], 'dummymodel') + self.assertEqual(job.kwargs['data']['id'], dummy.pk) + self.assertEqual(job.kwargs['snapshots']['postchange']['name'], 'Dummy') + + def test_webhook_double_save_update(self): + dummy = DummyModel.objects.create(name='Dummy') + + data = { + 'name': 'New Dummy', + 'number': 42, + } + url = reverse('plugins-api:dummy_plugin-api:dummymodel-detail', kwargs={'pk': dummy.pk}) + self.add_permissions('dummy_plugin.change_dummymodel') + response = self.client.patch(url, data, format='json', **self.header) + self.assertHttpStatus(response, status.HTTP_200_OK) + self.assertEqual(DummyModel.objects.count(), 1) + + self.assertEqual(self.queue.count, 1) + job = self.queue.jobs[0] + self.assertEqual(job.kwargs['event_rule'], EventRule.objects.get(type_update=True)) + self.assertEqual(job.kwargs['event'], ObjectChangeActionChoices.ACTION_UPDATE) + self.assertEqual(job.kwargs['model_name'], 'dummymodel') + self.assertEqual(job.kwargs['data']['id'], dummy.pk) + self.assertEqual(job.kwargs['snapshots']['postchange']['name'], 'New Dummy') + self.assertEqual(job.kwargs['snapshots']['postchange']['number'], 42) diff --git a/netbox/netbox/tests/dummy_plugin/migrations/0001_initial.py b/netbox/netbox/tests/dummy_plugin/migrations/0001_initial.py index 9cd597ff4be..9ba1547d3c1 100644 --- a/netbox/netbox/tests/dummy_plugin/migrations/0001_initial.py +++ b/netbox/netbox/tests/dummy_plugin/migrations/0001_initial.py @@ -15,7 +15,7 @@ class Migration(migrations.Migration): ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False)), ('name', models.CharField(max_length=20)), ('number', models.IntegerField(default=100)), - ('created', models.DateField(auto_now_add=True, null=True)), + ('created', models.DateTimeField(auto_now_add=True, null=True)), ('last_updated', models.DateTimeField(auto_now=True, null=True)), ], options={ diff --git a/netbox/netbox/tests/dummy_plugin/models.py b/netbox/netbox/tests/dummy_plugin/models.py index 19500e61919..0c9d8e6dc84 100644 --- a/netbox/netbox/tests/dummy_plugin/models.py +++ b/netbox/netbox/tests/dummy_plugin/models.py @@ -13,3 +13,7 @@ class DummyModel(EventRulesMixin, ChangeLoggingMixin, models.Model): class Meta: ordering = ['name'] + + def save(self, *args, **kwargs): + super().save() + super().save() From 773b6e413070c9909fc9cd3885fc07af166e1690 Mon Sep 17 00:00:00 2001 From: Peter Eckel Date: Thu, 30 May 2024 19:19:33 +0000 Subject: [PATCH 6/6] PEP8 fixes --- netbox/netbox/tests/dummy_plugin/models.py | 1 + 1 file changed, 1 insertion(+) diff --git a/netbox/netbox/tests/dummy_plugin/models.py b/netbox/netbox/tests/dummy_plugin/models.py index 0c9d8e6dc84..2e4a2e09f17 100644 --- a/netbox/netbox/tests/dummy_plugin/models.py +++ b/netbox/netbox/tests/dummy_plugin/models.py @@ -2,6 +2,7 @@ from netbox.models import EventRulesMixin, ChangeLoggingMixin + class DummyModel(EventRulesMixin, ChangeLoggingMixin, models.Model): name = models.CharField( max_length=20