Potential bugs and issues with m2m logging

[rptmat57]

In the tests for m2m relationships, it only checks the same side for both tests (always checking related history, even though the original obj model is the one being tracked), is that really by design?

    def test_related_add_from_first_side(self):
        self.obj.related.add(self.related)
        self.assertEqual(
            LogEntry.objects.get_for_objects(self.obj.related.all()).first(),
            self.related.history.first(),
        )
        self.assertEqual(LogEntry.objects.count(), self.base_log_entry_count + 1)

    def test_related_add_from_other_side(self):
        self.related.related.add(self.obj)
        self.assertEqual(
            LogEntry.objects.get_for_objects(self.obj.related.all()).first(),
            self.related.history.first(),
        )
        self.assertEqual(LogEntry.objects.count(), self.base_log_entry_count + 1)

I would argue that m2m changes should create an entry for both sides, so if I want to track obj.related it should also track related.related (the reverse relationship). Here we also have some issues since the related_name is also related and I think this should be changed to related_reverse or something like that for clarity. Once you do that you realize there are some problems with which field name is used in LogEntry and which object is actually tracked.

Also, the test_related_add_from_first_side test for assertEqual passes but both arguments are None i.e. self.related.history.first() is None which in my view is incorrect.

I am simply opening up the discussion here, on what should really happen when using obj.related.add and related.related_reverse.add and which models should be tracked (both or only the ones explicitly registered) My opinion is that if I register RelatedModel.related to be tracked, then I should get a RelatedModel log entry when either side is updated. And if I want a OtherRelatedModel entry I would need to explicitly register OtherRelatedModel.related_reverse

I hope that makes sense?

[hramezani]

@alieh-rymasheuski Do you have time to check this?

[aleh-rymasheuski]

@rptmat57, I really appreciate that you paid the tests so much attention. Your arguments about the quality of the tests are valid, I'll try to improve them when I have time.

I would argue that m2m changes should create an entry for both sides, so if I want to track obj.related it should also track related.related (the reverse relationship).

Will it be achieved by explicitly registering both the forward and reverse relationships?

m2m_only_auditlog.register(ManyRelatedModel, m2m_fields={"related"})
m2m_only_auditlog.register(ManyRelatedOtherModel, m2m_fields={"related"})  # added this

I believe the user should opt in tracking the reverse relationship explicitly (though I'm not sure it works :smile:). In my own use case - which was the primary reason to adopt m2m logging - the tracking of the reverse relationship has no added value, which is why I would seek to disable it if it were automatic.

[rptmat57]

Let's use a real life situation with Django's example: Publications can have multiple Articles and Articles can have multiple Publications:

class Publication(models.Model):
    title = models.CharField(max_length=30)

class Article(models.Model):
    headline = models.CharField(max_length=100)
    publications = models.ManyToManyField(Publication)

So, if I have: m2m_only_auditlog.register(Article, m2m_fields={"publications"})

I would argue that I should get an Article log entry (this is important) stating that a Publication was added if I use either of the following commands:

1. article.publications.add(publication) normal relationship
2. publication.article_set.add(article) reverse relationship

And if I have: m2m_only_auditlog.register(Publication, m2m_fields={"article_set"})

Then I should have a Publication log entry stating that an Article was added if I use either of the exact same commands:

1. publication.article_set.add(article) this would be the normal relationship in the sense of what we are tracking
2. article.publications.add(publication) reverse relationship

I have not double checked the tests for foreign key relationships, but in my opinion it should do the same, meaning if an Article has a Reporter then if I register to track Article's reporter field then I should also get an Article log entry when reporter.article_set.add(article) is used. I would only get a Reporter log entry if I explicitly registered to track its article_set field.

Does that make sense?