Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #18306 -- Defer and update_fields interaction. #213

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 21 additions & 1 deletion django/db/models/base.py
Expand Up @@ -448,6 +448,7 @@ def save(self, force_insert=False, force_update=False, using=None,
that the "save" must be an SQL insert or update (or equivalent for
non-SQL backends), respectively. Normally, they should not be set.
"""
using = using or router.db_for_write(self.__class__, instance=self)
if force_insert and (force_update or update_fields):
raise ValueError("Cannot force both insert and updating in model saving.")

Expand Down Expand Up @@ -475,6 +476,22 @@ def save(self, force_insert=False, force_update=False, using=None,
"model or are m2m fields: %s"
% ', '.join(non_model_fields))

# If saving to the same database, and this model is deferred, then
# automatically do a "update_fields" save on the loaded fields.
elif not force_insert and self._deferred and using == self._state.db:
field_names = set([])
for field in self._meta.fields:
if not field.primary_key and not hasattr(field, 'through'):
field_names.add(field.attname)
deferred_fields = [
f.attname for f in self._meta.fields
if f.attname not in self.__dict__ and
isinstance(self.__class__.__dict__[f.attname], DeferredAttribute)]

loaded_fields = field_names.difference(deferred_fields)
if loaded_fields:
update_fields = frozenset(loaded_fields)

self.save_base(using=using, force_insert=force_insert,
force_update=force_update, update_fields=update_fields)
save.alters_data = True
Expand All @@ -487,7 +504,9 @@ def save_base(self, raw=False, cls=None, origin=None, force_insert=False,
need for overrides of save() to pass around internal-only parameters
('raw', 'cls', and 'origin').
"""
using = using or router.db_for_write(self.__class__, instance=self)
if raw:
using = using or router.db_for_write(self.__class__, instance=self)
assert using is not None
assert not (force_insert and (force_update or update_fields))
assert update_fields is None or len(update_fields) > 0
if cls is None:
Expand All @@ -508,6 +527,7 @@ def save_base(self, raw=False, cls=None, origin=None, force_insert=False,
# attributes we have been given to the class we have been given.
# We also go through this process to defer the save of proxy objects
# to their actual underlying model.

if not raw or meta.proxy:
if meta.proxy:
org = cls
Expand Down
1 change: 1 addition & 0 deletions django/db/models/query.py
Expand Up @@ -271,6 +271,7 @@ def iterator(self):
aggregate_start = index_start + len(load_fields or self.model._meta.fields)

skip = None

if load_fields and not fill_cache:
# Some fields have been deferred, so we have to initialise
# via keyword arguments.
Expand Down
2 changes: 1 addition & 1 deletion django/db/models/query_utils.py
Expand Up @@ -114,7 +114,7 @@ def __set__(self, instance, value):

def _check_parent_chain(self, instance, name):
"""
Check if the field value can be fetched from a parent field already
Check if the field value can be fetched from a parent field already
loaded in the instance. This can be done if the to-be fetched
field is a primary key field.
"""
Expand Down
6 changes: 6 additions & 0 deletions docs/ref/models/instances.txt
Expand Up @@ -386,6 +386,12 @@ perform an update on all fields.

Specifying ``update_fields`` will force an update.

When saving a model fetched through deferred model loading
(:meth:`~Model.only()` or :meth:`~Model.defer()`) only the fields loaded from
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These links are wrong, the methods are on QuerySet, not Model.

the DB will get updated. In effect there is an automatic ``update_fields`` in
this case. If you assign or change any deferred field value, these fields will
be added to the updated fields.

Deleting objects
================

Expand Down
16 changes: 16 additions & 0 deletions docs/ref/models/querysets.txt
Expand Up @@ -1110,6 +1110,14 @@ one, doing so will result in an error.
reader, is slightly faster and consumes a little less memory in the Python
process.

.. versionchanged:: 1.5

.. note::

When calling :meth:`~Model.save()` for instances with deferred fields,
only the loaded fields will be saved. See :meth:`~Model.save()` for more
details.


only
~~~~
Expand Down Expand Up @@ -1154,6 +1162,14 @@ All of the cautions in the note for the :meth:`defer` documentation apply to
options. Also note that using :meth:`only` and omitting a field requested
using :meth:`select_related` is an error as well.

.. versionchanged:: 1.5

.. note::

When calling :meth:`~Model.save()` for instances with deferred fields,
only the loaded fields will be saved. See :meth:`~Model.save()` for more
details.

using
~~~~~

Expand Down
4 changes: 4 additions & 0 deletions docs/releases/1.5.txt
Expand Up @@ -42,6 +42,10 @@ keyword argument ``update_fields``. By using this argument it is possible to
save only a select list of model's fields. This can be useful for performance
reasons or when trying to avoid overwriting concurrent changes.

Deferred instances (those loaded by .only() or .defer()) will automatically
save just the loaded fields. If any field is set manually after load, that
field will also get updated on save.

See the :meth:`Model.save() <django.db.models.Model.save()>` documentation for
more details.

Expand Down
1 change: 1 addition & 0 deletions tests/modeltests/update_only_fields/models.py
Expand Up @@ -13,6 +13,7 @@ class Account(models.Model):
class Person(models.Model):
name = models.CharField(max_length=20)
gender = models.CharField(max_length=1, choices=GENDER_CHOICES)
pid = models.IntegerField(null=True, default=None)

def __unicode__(self):
return self.name
Expand Down
101 changes: 101 additions & 0 deletions tests/modeltests/update_only_fields/tests.py
Expand Up @@ -18,6 +18,107 @@ def test_update_fields_basic(self):
self.assertEqual(s.gender, 'F')
self.assertEqual(s.name, 'Ian')

def test_update_fields_deferred(self):
s = Person.objects.create(name='Sara', gender='F', pid=22)
self.assertEqual(s.gender, 'F')

s1 = Person.objects.defer("gender", "pid").get(pk=s.pk)
s1.name = "Emily"
s1.gender = "M"

with self.assertNumQueries(1):
s1.save()

s2 = Person.objects.get(pk=s1.pk)
self.assertEqual(s2.name, "Emily")
self.assertEqual(s2.gender, "M")

def test_update_fields_only_1(self):
s = Person.objects.create(name='Sara', gender='F')
self.assertEqual(s.gender, 'F')

s1 = Person.objects.only('name').get(pk=s.pk)
s1.name = "Emily"
s1.gender = "M"

with self.assertNumQueries(1):
s1.save()

s2 = Person.objects.get(pk=s1.pk)
self.assertEqual(s2.name, "Emily")
self.assertEqual(s2.gender, "M")

def test_update_fields_only_2(self):
s = Person.objects.create(name='Sara', gender='F', pid=22)
self.assertEqual(s.gender, 'F')

s1 = Person.objects.only('name').get(pk=s.pk)
s1.name = "Emily"
s1.gender = "M"

with self.assertNumQueries(2):
s1.save(update_fields=['pid'])

s2 = Person.objects.get(pk=s1.pk)
self.assertEqual(s2.name, "Sara")
self.assertEqual(s2.gender, "F")

def test_update_fields_only_repeated(self):
s = Person.objects.create(name='Sara', gender='F')
self.assertEqual(s.gender, 'F')

s1 = Person.objects.only('name').get(pk=s.pk)
s1.gender = 'M'
with self.assertNumQueries(1):
s1.save()
# Test that the deferred class does not remember that gender was
# set, instead the instace should remember this.
s1 = Person.objects.only('name').get(pk=s.pk)
with self.assertNumQueries(1):
s1.save()

def test_update_fields_inheritance_defer(self):
profile_boss = Profile.objects.create(name='Boss', salary=3000)
e1 = Employee.objects.create(name='Sara', gender='F',
employee_num=1, profile=profile_boss)
e1 = Employee.objects.only('name').get(pk=e1.pk)
e1.name = 'Linda'
with self.assertNumQueries(1):
e1.save()
self.assertEqual(Employee.objects.get(pk=e1.pk).name,
'Linda')

def test_update_fields_fk_defer(self):
profile_boss = Profile.objects.create(name='Boss', salary=3000)
profile_receptionist = Profile.objects.create(name='Receptionist', salary=1000)
e1 = Employee.objects.create(name='Sara', gender='F',
employee_num=1, profile=profile_boss)
e1 = Employee.objects.only('profile').get(pk=e1.pk)
e1.profile = profile_receptionist
with self.assertNumQueries(1):
e1.save()
self.assertEqual(Employee.objects.get(pk=e1.pk).profile, profile_receptionist)
e1.profile_id = profile_boss.pk
with self.assertNumQueries(1):
e1.save()
self.assertEqual(Employee.objects.get(pk=e1.pk).profile, profile_boss)

def test_select_related_only_interaction(self):
profile_boss = Profile.objects.create(name='Boss', salary=3000)
e1 = Employee.objects.create(name='Sara', gender='F',
employee_num=1, profile=profile_boss)
e1 = Employee.objects.only('profile__salary').select_related('profile').get(pk=e1.pk)
profile_boss.name = 'Clerk'
profile_boss.salary = 1000
profile_boss.save()
# The loaded salary of 3000 gets saved, the name of 'Clerk' isn't
# overwritten.
with self.assertNumQueries(1):
e1.profile.save()
reloaded_profile = Profile.objects.get(pk=profile_boss.pk)
self.assertEqual(reloaded_profile.name, profile_boss.name)
self.assertEqual(reloaded_profile.salary, 3000)

def test_update_fields_m2m(self):
profile_boss = Profile.objects.create(name='Boss', salary=3000)
e1 = Employee.objects.create(name='Sara', gender='F',
Expand Down
15 changes: 15 additions & 0 deletions tests/regressiontests/multiple_database/tests.py
Expand Up @@ -1545,6 +1545,21 @@ def test_subquery(self):
# If you evaluate the query, it should work, running on 'other'
self.assertEqual(list(qs.values_list('title', flat=True)), ['Dive into Python'])

def test_deferred_models(self):
mark_def = Person.objects.using('default').create(name="Mark Pilgrim")
mark_other = Person.objects.using('other').create(name="Mark Pilgrim")
orig_b = Book.objects.using('other').create(title="Dive into Python",
published=datetime.date(2009, 5, 4),
editor=mark_other)
b = Book.objects.using('other').only('title').get(pk=orig_b.pk)
self.assertEqual(b.published, datetime.date(2009, 5, 4))
b = Book.objects.using('other').only('title').get(pk=orig_b.pk)
b.editor = mark_def
b.save(using='default')
self.assertEqual(Book.objects.using('default').get(pk=b.pk).published,
datetime.date(2009, 5, 4))


class AuthTestCase(TestCase):
multi_db = True

Expand Down