Fix HistoricForeignKey when used together with prefetch_related()

[mr-mare]

Description

This fixes behaviour of HistoricForeignKey when used together with prefetch_related().

The following is sufficient to reproduce the bug:

• models:

from django.db import models
from simple_history.models import HistoricalRecords, HistoricForeignKey


class Poll(models.Model):
    question = models.CharField(max_length=200)

    history = HistoricalRecords()


class Choice(models.Model):
    poll = HistoricForeignKey(Poll, on_delete=models.CASCADE)
    choice = models.CharField(max_length=200)
    votes = models.IntegerField()

    history = HistoricalRecords()

• code:

from poll.models import Poll, Choice

why_poll = Poll.objects.create(question="why?")
how_poll = Poll.objects.create(question="how?")

Choice.objects.create(poll=why_poll, votes=0)
Choice.objects.create(poll=how_poll, votes=0)

for poll in Poll.objects.prefetch_related("choice_set").all():
    print(poll.choice_set.all())

# expected result:
# <QuerySet [<Choice: Choice object (1)>]>
# <QuerySet [<Choice: Choice object (2)>]>

# actual result:
# <QuerySet [<Choice: Choice object (1)>]>
# <QuerySet []>

Related Issue

#1152

Motivation and Context

Property related_manager_cls is defined in HistoricReverseManyToOneDescriptor. This property returns a function call of create_reverse_many_to_one_manager function and passes it HistoricRelationModelManager as an argument. However, create_reverse_many_to_one_manager defines RelatedManager, which subclasses the manager passed as an argument and returns this RelatedManager class.

When prefetch_related is used, get_prefetch_queryset method of RelatedManager is called. The problem lies in the fact that it bypasses RelatedManager.get_queryset method which does filtering to instance (_apply_rel_filters) by calling super().get_queryset(). But due to inheritance, it gets evaluated to HistoricRelationModelManager.get_queryset which does the same filtering as well.

This SQL is generated then (notice the "poll_choice"."poll_id" = 1 condition):

SELECT "poll_poll"."id", "poll_poll"."question" FROM "poll_poll"
SELECT "poll_choice"."id", "poll_choice"."poll_id", "poll_choice"."choice", "poll_choice"."votes" FROM "poll_choice" WHERE ("poll_choice"."poll_id" = 1 AND "poll_choice"."poll_id" IN (1, 2))

Removing _apply_rel_filters call from HistoricRelationModelManager.get_queryset solves this problem.

On the other hand, since create_reverse_many_to_one_manager returns RelatedManager, rather than HistoricRelationModelManager, the only way to access HistoricRelationModelManager.get_queryset is by calling super().get_queryset() from RelatedManager. This call can be found:

• in RelatedManager.get_prefetch_queryset where it is undesired,
• in RelatedManager.get_queryset where _apply_rel_filters is called right after super().get_queryset().

Therefore, I believe it is safe to remove it.

How Has This Been Tested?

Added new tests.

Screenshots (if appropriate):

Types of changes

• [x] Bug fix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• [x] I have run the pre-commit run command to format and lint.
• [ ] My change requires a change to the documentation.
• [ ] I have updated the documentation accordingly.
• [x] I have read the CONTRIBUTING document.
• [x] I have added tests to cover my changes.
• [ ] I have added my name and/or github handle to AUTHORS.rst
• [x] I have added my change to CHANGES.rst
• [x] All new and existing tests passed.

[codecov[bot]]

Codecov Report

Merging #1159 (bcf2086) into master (27b3dbf) will not change coverage. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1159   +/-   ##
=======================================
  Coverage   97.24%   97.24%           
=======================================
  Files          23       23           
  Lines        1234     1234           
  Branches      200      200           
=======================================
  Hits         1200     1200           
  Misses         16       16           
  Partials       18       18           

Impacted Files	Coverage Δ
simple_history/models.py	96.71% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[evanhaldane]

Plans to review/merge this?

(I'm in the process of integrating this package for the first time and I spent a few hours chasing around a bug until I realized it was caused by this issue.)

cc: @ddabble ?

[anyidea]

any progress?

[robertofd-nextmatter]

Any reason why this is not getting merged? We are also having the same issue

[mr-mare]

I'm happy to help. If there is anything I can do, please let me know.