Opened 6 weeks ago

Closed 3 weeks ago

Last modified 3 weeks ago

#28918 closed Bug (fixed)

Model.refresh_from_db() should use _base_manager instead of the default_manager

Reported by: Sjoerd Job Postmus Owned by: nobody
Component: Database layer (models, ORM) Version: 2.0
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When using refresh_from_db, currently the _default_manager is used.

Based upon the documentation at https://docs.djangoproject.com/en/2.0/topics/db/managers/#default-managers , it seems that there's a requirement for _base_manager to not filter out results, while for _default_manager there's just a preference for this to be the case.

I think the stronger "guarantee" provided by _base_manager is more appropriate in this case.

The following patch both adds a test and fixes the issue.

diff --git a/django/db/models/base.py b/django/db/models/base.py
index 27ca63fd22..0813bedcb0 100644
--- a/django/db/models/base.py
+++ b/django/db/models/base.py
@@ -591,7 +591,7 @@ class Model(metaclass=ModelBase):
                     'are not allowed in fields.' % LOOKUP_SEP)

         db = using if using is not None else self._state.db
-        db_instance_qs = self.__class__._default_manager.using(db).filter(pk=self.pk)
+        db_instance_qs = self.__class__._base_manager.using(db).filter(pk=self.pk)

         # Use provided fields, if not set then reload all non-deferred fields.
         deferred_fields = self.get_deferred_fields()
diff --git a/tests/custom_managers/tests.py b/tests/custom_managers/tests.py
index ee2ac1d552..61b1a07933 100644
--- a/tests/custom_managers/tests.py
+++ b/tests/custom_managers/tests.py
@@ -643,3 +643,13 @@ class CustomManagersRegressTestCase(TestCase):
         qs_custom = Person.custom_init_queryset_manager.all()
         qs_default = Person.objects.all()
         self.assertQuerysetEqual(qs_custom, qs_default)
+
+    def test_refresh_from_db_when_default_manager_filters(self):
+        """
+        obj.refresh_from_db() should also fetch object if default manager
+        hides it.
+        """
+        book = Book._base_manager.create(is_published=False)
+        Book._base_manager.filter(pk=book.pk).update(title='Hi')
+        book.refresh_from_db()
+        self.assertEqual(book.title, 'Hi')

Change History (5)

comment:1 Changed 5 weeks ago by Tim Graham

Has patch: set
Summary: refresh_from_db should use _base_managerModel.refresh_from_db() should use _base_manager instead of the default_manager
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

Will you provide a pull request?

comment:2 Changed 3 weeks ago by Tim Graham

Triage Stage: AcceptedReady for checkin

PR from the patch in the ticket description.

comment:3 Changed 3 weeks ago by GitHub <noreply@…>

Resolution: fixed
Status: newclosed

In d065c926:

Fixed #28918 -- Fixed Model.refresh_from_db() for instances hidden by the default manager.

comment:4 Changed 3 weeks ago by Sjoerd Job Postmus

Sorry for not providing a PR myself. I was on a 2-week holiday. Thank you for handling it.

Should I create a PR directly next time when I find an issue instead of attaching a patch?

comment:5 Changed 3 weeks ago by Tim Graham

Yes, that's fine.

Note: See TracTickets for help on using tickets.
Back to Top