From e973ee6a9879969b8ae05bb7ff681172cc5386a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anssi=20K=C3=A4=C3=A4ri=C3=A4inen?= Date: Fri, 30 Aug 2013 09:41:07 +0300 Subject: [PATCH] Fixed #20988 -- Added model meta option select_on_save The option can be used to force pre 1.6 style SELECT on save behaviour. This is needed in case the database returns zero updated rows even if there is a matching row in the DB. One such case is PostgreSQL update trigger that returns NULL. Reviewed by Tim Graham. Refs #16649 --- django/db/models/base.py | 18 ++++++++--- django/db/models/options.py | 4 ++- docs/ref/models/instances.txt | 17 +++++++--- docs/ref/models/options.txt | 22 +++++++++++++ docs/releases/1.6.txt | 20 +++++++++--- tests/basic/models.py | 5 +++ tests/basic/tests.py | 60 ++++++++++++++++++++++++++++++++++- 7 files changed, 130 insertions(+), 16 deletions(-) diff --git a/django/db/models/base.py b/django/db/models/base.py index d63017c438..887381187d 100644 --- a/django/db/models/base.py +++ b/django/db/models/base.py @@ -667,7 +667,9 @@ class Model(six.with_metaclass(ModelBase)): base_qs = cls._base_manager.using(using) values = [(f, None, (getattr(self, f.attname) if raw else f.pre_save(self, False))) for f in non_pks] - updated = self._do_update(base_qs, using, pk_val, values, update_fields) + forced_update = update_fields or force_update + updated = self._do_update(base_qs, using, pk_val, values, update_fields, + forced_update) if force_update and not updated: raise DatabaseError("Forced update did not affect any rows.") if update_fields and not updated: @@ -691,21 +693,27 @@ class Model(six.with_metaclass(ModelBase)): setattr(self, meta.pk.attname, result) return updated - def _do_update(self, base_qs, using, pk_val, values, update_fields): + def _do_update(self, base_qs, using, pk_val, values, update_fields, forced_update): """ 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. """ + filtered = base_qs.filter(pk=pk_val) 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. - return update_fields is not None or base_qs.filter(pk=pk_val).exists() - else: - return base_qs.filter(pk=pk_val)._update(values) > 0 + return update_fields is not None or filtered.exists() + if self._meta.select_on_save and not forced_update: + if filtered.exists(): + filtered._update(values) + return True + else: + return False + return filtered._update(values) > 0 def _do_insert(self, manager, using, fields, update_pk, raw): """ diff --git a/django/db/models/options.py b/django/db/models/options.py index 14f73c301f..5221468493 100644 --- a/django/db/models/options.py +++ b/django/db/models/options.py @@ -22,7 +22,8 @@ DEFAULT_NAMES = ('verbose_name', 'verbose_name_plural', 'db_table', 'ordering', 'unique_together', 'permissions', 'get_latest_by', 'order_with_respect_to', 'app_label', 'db_tablespace', 'abstract', 'managed', 'proxy', 'swappable', 'auto_created', - 'index_together', 'app_cache', 'default_permissions') + 'index_together', 'app_cache', 'default_permissions', + 'select_on_save') @python_2_unicode_compatible class Options(object): @@ -35,6 +36,7 @@ class Options(object): self.ordering = [] self.unique_together = [] self.index_together = [] + self.select_on_save = False self.default_permissions = ('add', 'change', 'delete') self.permissions = [] self.object_name, self.app_label = None, app_label diff --git a/docs/ref/models/instances.txt b/docs/ref/models/instances.txt index da657a9a01..d195936964 100644 --- a/docs/ref/models/instances.txt +++ b/docs/ref/models/instances.txt @@ -305,16 +305,23 @@ follows this algorithm: * 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 primary-key value is unused. For more on this nuance, see `Explicitly specifying auto-primary-key values`_ above and `Forcing an INSERT or UPDATE`_ below. +.. versionchanged:: 1.6 + + Previously Django did a ``SELECT`` when the primary key attribute was set. + If the ``SELECT`` found a row, then Django did an ``UPDATE``, otherwise it + did an ``INSERT``. The old algorithm results in one more query in the + ``UPDATE`` case. There are some rare cases where the database doesn't + report that a row was updated even if the database contains a row for the + object's primary key value. An example is the PostgreSQL ``ON UPDATE`` + trigger which returns ``NULL``. In such cases it is possible to revert to the + old algorithm by setting the :attr:`~django.db.models.Options.select_on_save` + option to ``True``. + .. _ref-models-force-insert: Forcing an INSERT or UPDATE diff --git a/docs/ref/models/options.txt b/docs/ref/models/options.txt index baa24f63cb..7f5d193a89 100644 --- a/docs/ref/models/options.txt +++ b/docs/ref/models/options.txt @@ -256,6 +256,28 @@ Django quotes column and table names behind the scenes. If ``proxy = True``, a model which subclasses another model will be treated as a :ref:`proxy model `. +``select_on_save`` +------------------ + +.. attribute:: Options.select_on_save + + .. versionadded:: 1.6 + + Determines if Django will use the pre-1.6 + :meth:`django.db.models.Model.save()` algorithm. The old algorithm + uses ``SELECT`` to determine if there is an existing row to be updated. + The new algorith tries an ``UPDATE`` directly. In some rare cases the + ``UPDATE`` of an existing row isn't visible to Django. An example is the + PostgreSQL ``ON UPDATE`` trigger which returns ``NULL``. In such cases the + new algorithm will end up doing an ``INSERT`` even when a row exists in + the database. + + Usually there is no need to set this attribute. The default is + ``False``. + + See :meth:`django.db.models.Model.save()` for more about the old and + new saving algorithm. + ``unique_together`` ------------------- diff --git a/docs/releases/1.6.txt b/docs/releases/1.6.txt index 556edddda1..1e71e1db2e 100644 --- a/docs/releases/1.6.txt +++ b/docs/releases/1.6.txt @@ -138,6 +138,22 @@ A :djadmin:`check` management command was added, enabling you to verify if your current configuration (currently oriented at settings) is compatible with the current version of Django. +:meth:`Model.save() ` algorithm changed +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +The :meth:`Model.save() ` method now +tries to directly ``UPDATE`` the database if the instance has a primary +key value. Previously ``SELECT`` was performed to determine if ``UPDATE`` +or ``INSERT`` were needed. The new algorithm needs only one query for +updating an existing row while the old algorithm needed two. See +:meth:`Model.save() ` for more details. + +In some rare cases the database doesn't report that a matching row was +found when doing an ``UPDATE``. An example is the PostgreSQL ``ON UPDATE`` +trigger which returns ``NULL``. In such cases it is possible to set +:attr:`django.db.models.Options.select_on_save` flag to force saving to +use the old algorithm. + Minor features ~~~~~~~~~~~~~~ @@ -222,10 +238,6 @@ 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. - * The documentation contains a :doc:`deployment checklist `. diff --git a/tests/basic/models.py b/tests/basic/models.py index 1bffcb9cda..ee44c1658a 100644 --- a/tests/basic/models.py +++ b/tests/basic/models.py @@ -19,6 +19,11 @@ class Article(models.Model): def __str__(self): return self.headline +class ArticleSelectOnSave(Article): + class Meta: + proxy = True + select_on_save = True + @python_2_unicode_compatible class SelfRef(models.Model): selfref = models.ForeignKey('self', null=True, blank=True, diff --git a/tests/basic/tests.py b/tests/basic/tests.py index 611944902a..2204989823 100644 --- a/tests/basic/tests.py +++ b/tests/basic/tests.py @@ -5,6 +5,7 @@ import threading from django.core.exceptions import ObjectDoesNotExist, MultipleObjectsReturned from django.db import connections, DEFAULT_DB_ALIAS +from django.db import DatabaseError from django.db.models.fields import Field, FieldDoesNotExist from django.db.models.manager import BaseManager from django.db.models.query import QuerySet, EmptyQuerySet, ValuesListQuerySet, MAX_GET_RESULTS @@ -12,7 +13,7 @@ from django.test import TestCase, TransactionTestCase, skipIfDBFeature, skipUnle from django.utils import six from django.utils.translation import ugettext_lazy -from .models import Article, SelfRef +from .models import Article, SelfRef, ArticleSelectOnSave class ModelTest(TestCase): @@ -806,3 +807,60 @@ class ManagerTest(TestCase): sorted(BaseManager._get_queryset_methods(QuerySet).keys()), sorted(self.QUERYSET_PROXY_METHODS), ) + +class SelectOnSaveTests(TestCase): + def test_select_on_save(self): + a1 = Article.objects.create(pub_date=datetime.now()) + with self.assertNumQueries(1): + a1.save() + asos = ArticleSelectOnSave.objects.create(pub_date=datetime.now()) + with self.assertNumQueries(2): + asos.save() + with self.assertNumQueries(1): + asos.save(force_update=True) + Article.objects.all().delete() + with self.assertRaises(DatabaseError): + with self.assertNumQueries(1): + asos.save(force_update=True) + + def test_select_on_save_lying_update(self): + """ + Test that select_on_save works correctly if the database + doesn't return correct information about matched rows from + UPDATE. + """ + # Change the manager to not return "row matched" for update(). + # We are going to change the Article's _base_manager class + # dynamically. This is a bit of a hack, but it seems hard to + # test this properly otherwise. Article's manager, because + # proxy models use their parent model's _base_manager. + + orig_class = Article._base_manager.__class__ + + class FakeQuerySet(QuerySet): + # Make sure the _update method below is in fact called. + called = False + + def _update(self, *args, **kwargs): + FakeQuerySet.called = True + super(FakeQuerySet, self)._update(*args, **kwargs) + return 0 + + class FakeManager(orig_class): + def get_queryset(self): + return FakeQuerySet(self.model) + try: + Article._base_manager.__class__ = FakeManager + asos = ArticleSelectOnSave.objects.create(pub_date=datetime.now()) + with self.assertNumQueries(2): + asos.save() + self.assertTrue(FakeQuerySet.called) + # This is not wanted behaviour, but this is how Django has always + # behaved for databases that do not return correct information + # about matched rows for UPDATE. + with self.assertRaises(DatabaseError): + asos.save(force_update=True) + with self.assertRaises(DatabaseError): + asos.save(update_fields=['pub_date']) + finally: + Article._base_manager.__class__ = orig_class