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

@@ -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()
+            return update_fields is not None or filtered.exists()
-        else:
+        if self._meta.select_on_save and not forced_update:
-            return base_qs.filter(pk=pk_val)._update(values) > 0
+            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):
         """

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

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

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 <proxy-models>`.
 
+``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``
 -------------------
 

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() <django.db.models.Model.save()>` algorithm changed
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The :meth:`Model.save() <django.db.models.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() <django.db.models.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() <django.db.models.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
   </howto/deployment/checklist>`.
 

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,

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