From 6b4834952dcce0db5cbc1534635c00ff8573a6d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anssi=20K=C3=A4=C3=A4ri=C3=A4inen?= Date: Thu, 29 Nov 2012 12:10:31 +0200 Subject: [PATCH] Fixed #16649 -- Refactored save_base logic Model.save() will use UPDATE - if not updated - INSERT instead of SELECT - if found UPDATE else INSERT. This should save a query when updating, but will cost a little when inserting model with PK set. Also fixed #17341 -- made sure .save() commits transactions only after the whole model has been saved. This wasn't the case in model inheritance situations. The save_base implementation was refactored into multiple methods. A typical chain for inherited save is: save_base() _save_parents(self) for each parent: _save_parents(parent) _save_table(parent) _save_table(self) --- django/db/models/base.py | 216 ++++++++++++++------------- docs/ref/models/instances.txt | 13 +- docs/releases/1.6.txt | 4 + tests/basic/tests.py | 31 +++- tests/model_inheritance/tests.py | 2 +- tests/transactions_regress/models.py | 2 + tests/transactions_regress/tests.py | 22 ++- 7 files changed, 178 insertions(+), 112 deletions(-) diff --git a/django/db/models/base.py b/django/db/models/base.py index ab0e42d461..f3e3b76dd7 100644 --- a/django/db/models/base.py +++ b/django/db/models/base.py @@ -545,125 +545,139 @@ class Model(six.with_metaclass(ModelBase)): force_update=force_update, update_fields=update_fields) save.alters_data = True - def save_base(self, raw=False, cls=None, origin=None, force_insert=False, + def save_base(self, raw=False, force_insert=False, force_update=False, using=None, update_fields=None): """ - Does the heavy-lifting involved in saving. Subclasses shouldn't need to - override this method. It's separate from save() in order to hide the - need for overrides of save() to pass around internal-only parameters - ('raw', 'cls', and 'origin'). + Handles the parts of saving which should be done only once per save, + yet need to be done in raw saves, too. This includes some sanity + checks and signal sending. + + The 'raw' argument is telling save_base not to save any parent + models and not to do any changes to the values before save. This + is used by fixture loading. """ using = using or router.db_for_write(self.__class__, instance=self) assert not (force_insert and (force_update or update_fields)) assert update_fields is None or len(update_fields) > 0 - if cls is None: - cls = self.__class__ - meta = cls._meta - if not meta.proxy: - origin = cls - else: - meta = cls._meta - - if origin and not meta.auto_created: + cls = origin = self.__class__ + # Skip proxies, but keep the origin as the proxy model. + if cls._meta.proxy: + cls = cls._meta.concrete_model + meta = cls._meta + if not meta.auto_created: signals.pre_save.send(sender=origin, instance=self, raw=raw, using=using, update_fields=update_fields) - - # If we are in a raw save, save the object exactly as presented. - # That means that we don't try to be smart about saving attributes - # that might have come from the parent class - we just save the - # 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 - else: - org = None - for parent, field in meta.parents.items(): - # At this point, parent's primary key field may be unknown - # (for example, from administration form which doesn't fill - # this field). If so, fill it. - if field and getattr(self, parent._meta.pk.attname) is None and getattr(self, field.attname) is not None: - setattr(self, parent._meta.pk.attname, getattr(self, field.attname)) - - self.save_base(cls=parent, origin=org, using=using, - update_fields=update_fields) - - if field: - setattr(self, field.attname, self._get_pk_val(parent._meta)) - # Since we didn't have an instance of the parent handy, we - # set attname directly, bypassing the descriptor. - # Invalidate the related object cache, in case it's been - # accidentally populated. A fresh instance will be - # re-built from the database if necessary. - cache_name = field.get_cache_name() - if hasattr(self, cache_name): - delattr(self, cache_name) - - if meta.proxy: - return - - if not meta.proxy: - non_pks = [f for f in meta.local_fields if not f.primary_key] - - if update_fields: - non_pks = [f for f in non_pks if f.name in update_fields or f.attname in update_fields] - - with transaction.commit_on_success_unless_managed(using=using): - # First, try an UPDATE. If that doesn't update anything, do an INSERT. - pk_val = self._get_pk_val(meta) - pk_set = pk_val is not None - record_exists = True - manager = cls._base_manager - if pk_set: - # Determine if we should do an update (pk already exists, forced update, - # no force_insert) - if ((force_update or update_fields) or (not force_insert and - manager.using(using).filter(pk=pk_val).exists())): - if force_update or non_pks: - values = [(f, None, (raw and getattr(self, f.attname) or f.pre_save(self, False))) for f in non_pks] - if values: - rows = manager.using(using).filter(pk=pk_val)._update(values) - if force_update and not rows: - raise DatabaseError("Forced update did not affect any rows.") - if update_fields and not rows: - raise DatabaseError("Save with update_fields did not affect any rows.") - else: - record_exists = False - if not pk_set or not record_exists: - if meta.order_with_respect_to: - # If this is a model with an order_with_respect_to - # autopopulate the _order field - field = meta.order_with_respect_to - order_value = manager.using(using).filter(**{field.name: getattr(self, field.attname)}).count() - self._order = order_value - - fields = meta.local_fields - if not pk_set: - if force_update or update_fields: - raise ValueError("Cannot force an update in save() with no primary key.") - fields = [f for f in fields if not isinstance(f, AutoField)] - - record_exists = False - - update_pk = bool(meta.has_auto_field and not pk_set) - result = manager._insert([self], fields=fields, return_id=update_pk, using=using, raw=raw) - - if update_pk: - setattr(self, meta.pk.attname, result) - + with transaction.commit_on_success_unless_managed(using=using, savepoint=False): + if not raw: + self._save_parents(cls, using, update_fields) + updated = self._save_table(raw, cls, force_insert, force_update, using, update_fields) # Store the database on which the object was saved self._state.db = using # Once saved, this is no longer a to-be-added instance. self._state.adding = False # Signal that the save is complete - if origin and not meta.auto_created: - signals.post_save.send(sender=origin, instance=self, created=(not record_exists), + if not meta.auto_created: + signals.post_save.send(sender=origin, instance=self, created=(not updated), update_fields=update_fields, raw=raw, using=using) save_base.alters_data = True + def _save_parents(self, cls, using, update_fields): + """ + Saves all the parents of cls using values from self. + """ + meta = cls._meta + for parent, field in meta.parents.items(): + # Make sure the link fields are synced between parent and self. + if (field and getattr(self, parent._meta.pk.attname) is None + and getattr(self, field.attname) is not None): + setattr(self, parent._meta.pk.attname, getattr(self, field.attname)) + self._save_parents(cls=parent, using=using, update_fields=update_fields) + self._save_table(cls=parent, using=using, update_fields=update_fields) + # Set the parent's PK value to self. + if field: + setattr(self, field.attname, self._get_pk_val(parent._meta)) + # Since we didn't have an instance of the parent handy set + # attname directly, bypassing the descriptor. Invalidate + # the related object cache, in case it's been accidentally + # populated. A fresh instance will be re-built from the + # database if necessary. + cache_name = field.get_cache_name() + if hasattr(self, cache_name): + delattr(self, cache_name) + + def _save_table(self, raw=False, cls=None, force_insert=False, + force_update=False, using=None, update_fields=None): + """ + Does the heavy-lifting involved in saving. Updates or inserts the data + for a single table. + """ + meta = cls._meta + non_pks = [f for f in meta.local_fields if not f.primary_key] + + if update_fields: + non_pks = [f for f in non_pks + if f.name in update_fields or f.attname in update_fields] + + pk_val = self._get_pk_val(meta) + pk_set = pk_val is not None + if not pk_set and (force_update or update_fields): + raise ValueError("Cannot force an update in save() with no primary key.") + updated = False + # If possible, try an UPDATE. If that doesn't update anything, do an INSERT. + if pk_set and not force_insert: + base_qs = cls._base_manager.using(using) + values = [(f, None, (raw and getattr(self, f.attname) or f.pre_save(self, False))) + for f in non_pks] + if not values: + # We can end up here when saving a model in inheritance chain where + # update_fields doesn't target any field in current model. In that + # case we just say the update succeeded. Another case ending up here + # is a model with just PK - in that case check that the PK still + # exists. + updated = update_fields is not None or base_qs.filter(pk=pk_val).exists() + else: + updated = self._do_update(base_qs, using, pk_val, values) + if force_update and not updated: + raise DatabaseError("Forced update did not affect any rows.") + if update_fields and not updated: + raise DatabaseError("Save with update_fields did not affect any rows.") + if not updated: + if meta.order_with_respect_to: + # If this is a model with an order_with_respect_to + # autopopulate the _order field + field = meta.order_with_respect_to + order_value = cls._base_manager.using(using).filter( + **{field.name: getattr(self, field.attname)}).count() + self._order = order_value + + fields = meta.local_fields + if not pk_set: + fields = [f for f in fields if not isinstance(f, AutoField)] + + update_pk = bool(meta.has_auto_field and not pk_set) + result = self._do_insert(cls._base_manager, using, fields, update_pk, raw) + if update_pk: + setattr(self, meta.pk.attname, result) + return updated + + def _do_update(self, base_qs, using, pk_val, values): + """ + This method will try to update the model. If the model was updated (in + the sense that an update query was done and a matching row was found + from the DB) the method will return True. + """ + return base_qs.filter(pk=pk_val)._update(values) > 0 + + def _do_insert(self, manager, using, fields, update_pk, raw): + """ + Do an INSERT. If update_pk is defined then this method should return + the new pk for the model. + """ + return manager._insert([self], fields=fields, return_id=update_pk, + using=using, raw=raw) + def delete(self, using=None): using = using or router.db_for_write(self.__class__, instance=self) assert self._get_pk_val() is not None, "%s object can't be deleted because its %s attribute is set to None." % (self._meta.object_name, self._meta.pk.attname) diff --git a/docs/ref/models/instances.txt b/docs/ref/models/instances.txt index 92071b8d3f..9f583c42ac 100644 --- a/docs/ref/models/instances.txt +++ b/docs/ref/models/instances.txt @@ -292,12 +292,13 @@ follows this algorithm: * If the object's primary key attribute is set to a value that evaluates to ``True`` (i.e., a value other than ``None`` or the empty string), Django - executes a ``SELECT`` query to determine whether a record with the given - primary key already exists. -* If the record with the given primary key does already exist, Django - executes an ``UPDATE`` query. -* If the object's primary key attribute is *not* set, or if it's set but a - record doesn't exist, Django executes an ``INSERT``. + executes an ``UPDATE``. +* If the object's primary key attribute is *not* set or if the ``UPDATE`` + didn't update anything, Django executes an ``INSERT``. + +.. versionchanged:: 1.6 + Previously Django used ``SELECT`` - if not found ``INSERT`` else ``UPDATE`` + algorithm. The old algorithm resulted in one more query in ``UPDATE`` case. The one gotcha here is that you should be careful not to specify a primary-key value explicitly when saving new objects, if you cannot guarantee the diff --git a/docs/releases/1.6.txt b/docs/releases/1.6.txt index 30c3cc5d2c..a44545ddf3 100644 --- a/docs/releases/1.6.txt +++ b/docs/releases/1.6.txt @@ -150,6 +150,10 @@ Minor features * Generic :class:`~django.contrib.gis.db.models.GeometryField` is now editable with the OpenLayers widget in the admin. +* The :meth:`Model.save() ` will do + ``UPDATE`` - if not updated - ``INSERT`` instead of ``SELECT`` - if not + found ``INSERT`` else ``UPDATE`` in case the model's primary key is set. + Backwards incompatible changes in 1.6 ===================================== diff --git a/tests/basic/tests.py b/tests/basic/tests.py index 2de87a225f..a8005baca7 100644 --- a/tests/basic/tests.py +++ b/tests/basic/tests.py @@ -1,11 +1,13 @@ from __future__ import absolute_import, unicode_literals from datetime import datetime +import threading from django.core.exceptions import ObjectDoesNotExist, MultipleObjectsReturned, FieldError +from django.db import connections, DEFAULT_DB_ALIAS from django.db.models.fields import Field, FieldDoesNotExist from django.db.models.query import QuerySet, EmptyQuerySet, ValuesListQuerySet -from django.test import TestCase, skipIfDBFeature, skipUnlessDBFeature +from django.test import TestCase, TransactionTestCase, skipIfDBFeature, skipUnlessDBFeature from django.utils import six from django.utils.translation import ugettext_lazy @@ -690,4 +692,29 @@ class ModelTest(TestCase): def test_invalid_qs_list(self): qs = Article.objects.order_by('invalid_column') self.assertRaises(FieldError, list, qs) - self.assertRaises(FieldError, list, qs) \ No newline at end of file + self.assertRaises(FieldError, list, qs) + +class ConcurrentSaveTests(TransactionTestCase): + @skipUnlessDBFeature('test_db_allows_multiple_connections') + def test_concurrent_delete_with_save(self): + """ + Test fetching, deleting and finally saving an object - we should get + an insert in this case. + """ + a = Article.objects.create(headline='foo', pub_date=datetime.now()) + exceptions = [] + def deleter(): + try: + # Do not delete a directly - doing so alters its state. + Article.objects.filter(pk=a.pk).delete() + connections[DEFAULT_DB_ALIAS].commit_unless_managed() + except Exception as e: + exceptions.append(e) + finally: + connections[DEFAULT_DB_ALIAS].close() + self.assertEqual(len(exceptions), 0) + t = threading.Thread(target=deleter) + t.start() + t.join() + a.save() + self.assertEqual(Article.objects.get(pk=a.pk).headline, 'foo') diff --git a/tests/model_inheritance/tests.py b/tests/model_inheritance/tests.py index 62f521c07f..dc40d2d2e0 100644 --- a/tests/model_inheritance/tests.py +++ b/tests/model_inheritance/tests.py @@ -294,7 +294,7 @@ class ModelInheritanceTests(TestCase): rating=4, chef=c ) - with self.assertNumQueries(6): + with self.assertNumQueries(3): ir.save() def test_update_parent_filtering(self): diff --git a/tests/transactions_regress/models.py b/tests/transactions_regress/models.py index a4b576c3ca..e09e81d93d 100644 --- a/tests/transactions_regress/models.py +++ b/tests/transactions_regress/models.py @@ -4,6 +4,8 @@ from django.db import models class Mod(models.Model): fld = models.IntegerField() +class SubMod(Mod): + cnt = models.IntegerField(unique=True) class M2mA(models.Model): others = models.ManyToManyField('M2mB') diff --git a/tests/transactions_regress/tests.py b/tests/transactions_regress/tests.py index 142c09d3cf..e320f76169 100644 --- a/tests/transactions_regress/tests.py +++ b/tests/transactions_regress/tests.py @@ -1,6 +1,7 @@ from __future__ import absolute_import -from django.db import connection, connections, transaction, DEFAULT_DB_ALIAS, DatabaseError +from django.db import (connection, connections, transaction, DEFAULT_DB_ALIAS, DatabaseError, + IntegrityError) from django.db.transaction import commit_on_success, commit_manually, TransactionManagementError from django.test import TransactionTestCase, skipUnlessDBFeature from django.test.utils import override_settings @@ -8,8 +9,25 @@ from django.utils.unittest import skipIf, skipUnless from transactions.tests import IgnorePendingDeprecationWarningsMixin -from .models import Mod, M2mA, M2mB +from .models import Mod, M2mA, M2mB, SubMod +class ModelInheritanceTests(TransactionTestCase): + def test_save(self): + # First, create a SubMod, then try to save another with conflicting + # cnt field. The problem was that transactions were committed after + # every parent save when not in managed transaction. As the cnt + # conflict is in the second model, we can check if the first save + # was committed or not. + SubMod(fld=1, cnt=1).save() + # We should have committed the transaction for the above - assert this. + connection.rollback() + self.assertEqual(SubMod.objects.count(), 1) + try: + SubMod(fld=2, cnt=1).save() + except IntegrityError: + connection.rollback() + self.assertEqual(SubMod.objects.count(), 1) + self.assertEqual(Mod.objects.count(), 1) class TestTransactionClosing(IgnorePendingDeprecationWarningsMixin, TransactionTestCase): """