Fixes #15194: Prevent enqueuing duplicate events for an object

[jeremystretch]

Fixes: #15194

• Change the events_queue context variable from a list to a dictionary, to enable identifying each object by a unique key.
• Modify the logic within enqueue_object() to designate a unique key for each object in the events queue, instead of comparing each new object to the previous entry.
• Remove the now-obsolete is_same_object() function.
• Add a test for duplicate event triggers.

A huge thank you to @peteeckel for his thorough research into this issue and his initial work on a solution under PR #15318!

[peteeckel]

Hi @jeremystretch, thank you very much for this PR! Looks very promising so far, though I still seem to have one issue. I'm just in the process of writing a couple of tests for NetBox DNS, and I found something I couldn't track down yet.

This test

    def test_update_zone_add_nameservers(self):
        zone = Zone.objects.create(name="zone1.example.com", **self.zone_data)
        zone.snapshot()

        url = reverse("plugins:netbox_dns:zone_edit", kwargs={"pk": zone.pk})
        request = RequestFactory().get(url)
        request.id = uuid.uuid4()
        request.user = self.user

        self.assertEqual(self.queue.count, 0, msg="Unexpected jobs found in queue")

        with event_tracking(request):
            zone.nameservers.add(self.nameservers[1])
            zone.nameservers.add(self.nameservers[2])
            zone.save()

        self.assertEqual(self.queue.count, 1)
        job = self.queue.jobs[0]
        print(job.kwargs['snapshots'])
        self.assertEqual(len(job.kwargs['data']['nameservers']), 2)
        self.assertIsNotNone(job.kwargs['snapshots'].get('prechange'))
        self.assertIsNotNone(job.kwargs['snapshots'].get('postchange'))
        self.assertEqual(len(job.kwargs['snapshots']['prechange']['nameservers']), 0)
        self.assertEqual(len(job.kwargs['snapshots']['postchange']['nameservers']), 2)

fails with the check that the prechange snapshot is None, which it shouldn't be.

If I insert an additional save() before the first M2M change, the pre-change snapshot is not None. My assumption (unproven yet) is that if an M2M change comes before the first update in the queue, the pre-change snapshot is lost.

        with event_tracking(request):
            zone.save()
            zone.nameservers.add(self.nameservers[1])
            zone.nameservers.add(self.nameservers[2])
            zone.save()

As I said, I haven't thoroughly investigated this, but it seems there is still something missing somewhere (might well be on my side).

[peteeckel]

For reference:

    def test_update_zone(self):
        zone = Zone.objects.create(name="zone1.example.com", **self.zone_data)
        zone.snapshot()

        url = reverse("plugins:netbox_dns:zone_edit", kwargs={"pk": zone.pk})
        request = RequestFactory().get(url)
        request.id = uuid.uuid4()
        request.user = self.user

        self.assertEqual(self.queue.count, 0, msg="Unexpected jobs found in queue")

        with event_tracking(request):
            zone.soa_refresh = 86400
            zone.save()

        self.assertEqual(self.queue.count, 1)
        job = self.queue.jobs[0]
        self.assertEqual(job.kwargs['data']['soa_refresh'], 86400)
        self.assertIsNotNone(job.kwargs['snapshots'].get('prechange'))
        self.assertIsNotNone(job.kwargs['snapshots'].get('postchange'))
        self.assertEqual(job.kwargs['snapshots']['prechange']['soa_refresh'], Zone.get_defaults()['soa_refresh'])
        self.assertEqual(job.kwargs['snapshots']['postchange']['soa_refresh'], 86400)

This test succeeds, so it's definitely linked to M2M updates somehow.

[peteeckel]

Two additional things:

• Removing M2M links does not have the problem, just adding them loses the pre-change snapshot (see below)
• Exactly the same happens with tags (so it's not a problem with my m2m_changed handler)

    def test_update_zone_remove_nameservers(self):
        zone = Zone.objects.create(name="zone1.example.com", **self.zone_data)
        zone.nameservers.set(self.nameservers[1:])
        zone.snapshot()

        url = reverse("plugins:netbox_dns:zone_edit", kwargs={"pk": zone.pk})
        request = RequestFactory().get(url)
        request.id = uuid.uuid4()
        request.user = self.user

        self.assertEqual(self.queue.count, 0, msg="Unexpected jobs found in queue")

        with event_tracking(request):
            zone.nameservers.set([])
            zone.save()

        self.assertEqual(self.queue.count, 1)
        job = self.queue.jobs[0]
        self.assertEqual(len(job.kwargs['data']['nameservers']),0 )
        self.assertIsNotNone(job.kwargs['snapshots'].get('prechange'))
        self.assertIsNotNone(job.kwargs['snapshots'].get('postchange'))
        self.assertEqual(len(job.kwargs['snapshots']['prechange']['nameservers']), 2)
        self.assertEqual(len(job.kwargs['snapshots']['postchange']['nameservers']), 0)

[peteeckel]

Hi @jeremystretch, apart from the problem I found and described in the comments this works absolutely great for me. I ran a couple of tests with the webhook_receiver and there the issue with the M2M updates losing the pre-change snapshot does not occur.

So this PR fixes the issue for me. If I can find what causes the issue with my tests and it's actually a problem with NetBox I will open a new issue. For now I'm perfectly fine with it - thank you very much for fixing this so elegantly and showing me a much better way of testing event queue-related stuff than I devised. That's really awesome.

[peteeckel]

Update to the test issue: That only seems to happen in the event_tracker context, with a 'real' request the pre-change snapshots are fine. So my remarks are not relevant at all.

[jeremystretch]

@peteeckel I think the issue with your test is that the zone instance doesn't get refreshed following the first call to snapshot() immediately after being created, so the pre-change data is still null. Thanks for testing!