Fix HistoricForeignKey reverse prefetch_related (rebases #1159 and addresses review)#1604
Open
tomhampshire wants to merge 3 commits into
Open
Fix HistoricForeignKey reverse prefetch_related (rebases #1159 and addresses review)#1604tomhampshire wants to merge 3 commits into
tomhampshire wants to merge 3 commits into
Conversation
This was referenced Apr 29, 2026
With HistoricForeignKey prefetch_related doesn't return all related objects in some situations
#1152
Open
The outer RelatedManager (constructed by create_reverse_many_to_one_manager) already returns from self.instance._prefetched_objects_cache when a prefetch cache exists, so HistoricRelationModelManager.get_queryset() does not need to check the cache itself. Removing it also keeps the prefetch-build path clean: get_prefetch_queryset calls super().get_queryset(), which would otherwise consult the per-instance cache when building a prefetch for many instances at once. Suggested by @ddabble in review of django-commons#1159.
Adds TestHistoricSubParticipantToHistoricParticipant - a third-level
historic model with a HistoricForeignKey to a model that itself has a
HistoricForeignKey - and a regression test exercising bare nested
prefetch_related ("a__b") across two HistoricForeignKey reverses, as
described in the SubSub example of django-commons#1152. The test fails on master
(the second parent's children come back as []) and passes with the
fix in place.
Also lists the original PR author and the rebase author in AUTHORS.rst.
Addresses django-commons#1152 review feedback from @ddabble.
fdd9f37 to
ca25ed1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Rebases @mr-mare's PR #1159 onto current master, resolves the merge conflict in
simple_history/tests/tests/test_models.py(which arose from theHistoricOneToOneFieldTestclass added on master), and addresses the two review points raised by @ddabble in September 2024.The branch contains three commits:
Fix HistoricForeignKey when used together with prefetch_related()(authored by @mr-mare, cherry-picked from Fix HistoricForeignKey when used together with prefetch_related() #1159) - removes the spurious_apply_rel_filters()call insideHistoricRelationModelManager.get_queryset()that produced aWHERE pk = X AND pk IN (...)clause for prefetch queries, masking all but the first parent's related objects. Includes the original three single-level prefetch tests.Remove redundant prefetched-objects cache lookup in reverse manager- addresses the first review point. The outerRelatedManagerproduced bycreate_reverse_many_to_one_manageralready short-circuits viaself.instance._prefetched_objects_cachebefore delegating tosuper().get_queryset(), so the same lookup insideHistoricRelationModelManager.get_queryset()was redundant. Removing it also keeps the prefetch-build path clean:RelatedManager.get_prefetch_querysets()callssuper().get_queryset()which would otherwise consult the per-instance cache while building a prefetch for many instances at once.Add nested-prefetch regression test and credit contributors- addresses the second review point with a new test for nestedprefetch_related()across twoHistoricForeignKeylevels (theSubSubcase from the description of With HistoricForeignKey prefetch_related doesn't return all related objects in some situations #1152). Adds aTestHistoricSubParticipantToHistoricParticipantmodel with aHistoricForeignKeytoTestHistoricParticipanToHistoricOrganization. The new test exercises both the bareprefetch_related(\"a__b\")form and the equivalent fully-explicitPrefetch()form. The bare form fails on master with[] != [\"p2-sub1\", \"p2-sub2\"]and passes with the fix in place. Also adds @mr-mare and @tomhampshire toAUTHORS.rst.Related Issue
Fixes #1152. Rebases and supersedes #1159 (with the original commit and authorship preserved).
Motivation and Context
Without this fix,
prefetch_related()across aHistoricForeignKeyreverse relation silently drops every parent's related objects except the first. The buggy code path is triggered by the natural Django idiom (prefetch_related(\"a__b\")) and produces no exception, no warning, no log line, just emptyRelatedManager.all()returns and quietly wrong results downstream. We hit this in production this week, three years after #1152 was filed.How Has This Been Tested?
Locally on Python 3.11 / Django 5.2.13 / SQLite using
python runtests.py:simple_history.testspass (was 340 + 2 new prefetch suites; now 342 + 1 new nested test = 343).simple_history.registry_testspass.HistoricForeignKeyTest(7 tests including the new nested test) all pass.AssertionError: Lists differ: [] != [\"p2-sub1\", \"p2-sub2\"]expected from the bug.pre-commit runclean across all changed files (bandit, black, codespell, flake8, isort, pyupgrade, django-upgrade, ...).Also verified end-to-end against a real downstream codebase that uses
HistoricForeignKeyacross multiple levels: a 4-parentprefetch_related(\"a__b\")correctly returned the expected child counts for every parent with the patchedsimple_historyinstalled; with vanilla 3.8.0, only the first parent's children came back.Types of changes
Checklist:
pre-commit runcommand to format and lint.AUTHORS.rstCHANGES.rst