Browse Source

Fixed #35060 -- Deprecated passing positional arguments to Model.save()/asave().

Salvo Polizzi 1 year ago
parent
commit
3915d4c70d

+ 3 - 0
django/contrib/auth/base_user.py

@@ -54,6 +54,9 @@ class AbstractBaseUser(models.Model):
     def __str__(self):
         return self.get_username()
 
+    # RemovedInDjango60Warning: When the deprecation ends, replace with:
+    # def save(self, **kwargs):
+    #   super().save(**kwargs)
     def save(self, *args, **kwargs):
         super().save(*args, **kwargs)
         if self._password is not None:

+ 61 - 2
django/db/models/base.py

@@ -49,6 +49,7 @@ from django.db.models.signals import (
     pre_save,
 )
 from django.db.models.utils import AltersData, make_model_tuple
+from django.utils.deprecation import RemovedInDjango60Warning
 from django.utils.encoding import force_str
 from django.utils.hashable import make_hashable
 from django.utils.text import capfirst, get_text_list
@@ -764,8 +765,17 @@ class Model(AltersData, metaclass=ModelBase):
             return getattr(self, field_name)
         return getattr(self, field.attname)
 
+    # RemovedInDjango60Warning: When the deprecation ends, replace with:
+    # def save(
+    #   self, *, force_insert=False, force_update=False, using=None, update_fields=None,
+    # ):
     def save(
-        self, force_insert=False, force_update=False, using=None, update_fields=None
+        self,
+        *args,
+        force_insert=False,
+        force_update=False,
+        using=None,
+        update_fields=None,
     ):
         """
         Save the current instance. Override this in a subclass if you want to
@@ -775,6 +785,26 @@ class Model(AltersData, metaclass=ModelBase):
         that the "save" must be an SQL insert or update (or equivalent for
         non-SQL backends), respectively. Normally, they should not be set.
         """
+        # RemovedInDjango60Warning.
+        if args:
+            warnings.warn(
+                "Passing positional arguments to save() is deprecated",
+                RemovedInDjango60Warning,
+                stacklevel=2,
+            )
+            for arg, attr in zip(
+                args, ["force_insert", "force_update", "using", "update_fields"]
+            ):
+                if arg:
+                    if attr == "force_insert":
+                        force_insert = arg
+                    elif attr == "force_update":
+                        force_update = arg
+                    elif attr == "using":
+                        using = arg
+                    else:
+                        update_fields = arg
+
         self._prepare_related_fields_for_save(operation_name="save")
 
         using = using or router.db_for_write(self.__class__, instance=self)
@@ -828,9 +858,38 @@ class Model(AltersData, metaclass=ModelBase):
 
     save.alters_data = True
 
+    # RemovedInDjango60Warning: When the deprecation ends, replace with:
+    # async def asave(
+    #   self, *, force_insert=False, force_update=False, using=None, update_fields=None,
+    # ):
     async def asave(
-        self, force_insert=False, force_update=False, using=None, update_fields=None
+        self,
+        *args,
+        force_insert=False,
+        force_update=False,
+        using=None,
+        update_fields=None,
     ):
+        # RemovedInDjango60Warning.
+        if args:
+            warnings.warn(
+                "Passing positional arguments to asave() is deprecated",
+                RemovedInDjango60Warning,
+                stacklevel=2,
+            )
+            for arg, attr in zip(
+                args, ["force_insert", "force_update", "using", "update_fields"]
+            ):
+                if arg:
+                    if attr == "force_insert":
+                        force_insert = arg
+                    elif attr == "force_update":
+                        force_update = arg
+                    elif attr == "using":
+                        using = arg
+                    else:
+                        update_fields = arg
+
         return await sync_to_async(self.save)(
             force_insert=force_insert,
             force_update=force_update,

+ 3 - 0
docs/internals/deprecation.txt

@@ -68,6 +68,9 @@ details on these changes.
 
 * The ``django.contrib.gis.geoip2.GeoIP2.open()`` method will be removed.
 
+* Support for passing positional arguments to ``Model.save()`` and
+  ``Model.asave()`` will be removed.
+
 .. _deprecation-removed-in-5.1:
 
 5.1

+ 8 - 4
docs/ref/models/instances.txt

@@ -116,7 +116,7 @@ are loaded from the database::
         return instance
 
 
-    def save(self, *args, **kwargs):
+    def save(self, **kwargs):
         # Check how the current values differ from ._loaded_values. For example,
         # prevent changing the creator_id of the model. (This example doesn't
         # support cases where 'creator_id' is deferred).
@@ -124,7 +124,7 @@ are loaded from the database::
             self.creator_id != self._loaded_values["creator_id"]
         ):
             raise ValueError("Updating the value of creator isn't allowed")
-        super().save(*args, **kwargs)
+        super().save(**kwargs)
 
 The example above shows a full ``from_db()`` implementation to clarify how that
 is done. In this case it would be possible to use a ``super()`` call in the
@@ -410,8 +410,8 @@ Saving objects
 
 To save an object back to the database, call ``save()``:
 
-.. method:: Model.save(force_insert=False, force_update=False, using=DEFAULT_DB_ALIAS, update_fields=None)
-.. method:: Model.asave(force_insert=False, force_update=False, using=DEFAULT_DB_ALIAS, update_fields=None)
+.. method:: Model.save(*, force_insert=False, force_update=False, using=DEFAULT_DB_ALIAS, update_fields=None)
+.. method:: Model.asave(*, force_insert=False, force_update=False, using=DEFAULT_DB_ALIAS, update_fields=None)
 
 *Asynchronous version*: ``asave()``
 
@@ -424,6 +424,10 @@ method. See :ref:`overriding-model-methods` for more details.
 
 The model save process also has some subtleties; see the sections below.
 
+.. deprecated:: 5.1
+
+    Support for positional arguments is deprecated.
+
 Auto-incrementing primary keys
 ------------------------------
 

+ 3 - 0
docs/releases/5.1.txt

@@ -331,6 +331,9 @@ Miscellaneous
 * The ``django.contrib.gis.geoip2.GeoIP2.open()`` method is deprecated. Use the
   :class:`~django.contrib.gis.geoip2.GeoIP2` constructor instead.
 
+* Passing positional arguments to :meth:`.Model.save` and :meth:`.Model.asave`
+  is deprecated in favor of keyword-only arguments.
+
 Features removed in 5.1
 =======================
 

+ 17 - 23
docs/topics/db/models.txt

@@ -868,9 +868,9 @@ to happen whenever you save an object. For example (see
         name = models.CharField(max_length=100)
         tagline = models.TextField()
 
-        def save(self, *args, **kwargs):
+        def save(self, **kwargs):
             do_something()
-            super().save(*args, **kwargs)  # Call the "real" save() method.
+            super().save(**kwargs)  # Call the "real" save() method.
             do_something_else()
 
 You can also prevent saving::
@@ -882,24 +882,23 @@ You can also prevent saving::
         name = models.CharField(max_length=100)
         tagline = models.TextField()
 
-        def save(self, *args, **kwargs):
+        def save(self, **kwargs):
             if self.name == "Yoko Ono's blog":
                 return  # Yoko shall never have her own blog!
             else:
-                super().save(*args, **kwargs)  # Call the "real" save() method.
+                super().save(**kwargs)  # Call the "real" save() method.
 
 It's important to remember to call the superclass method -- that's
-that ``super().save(*args, **kwargs)`` business -- to ensure
-that the object still gets saved into the database. If you forget to
-call the superclass method, the default behavior won't happen and the
-database won't get touched.
+that ``super().save(**kwargs)`` business -- to ensure that the object still
+gets saved into the database. If you forget to call the superclass method, the
+default behavior won't happen and the database won't get touched.
 
 It's also important that you pass through the arguments that can be
-passed to the model method -- that's what the ``*args, **kwargs`` bit
-does. Django will, from time to time, extend the capabilities of
-built-in model methods, adding new arguments. If you use ``*args,
-**kwargs`` in your method definitions, you are guaranteed that your
-code will automatically support those arguments when they are added.
+passed to the model method -- that's what the ``**kwargs`` bit does. Django
+will, from time to time, extend the capabilities of built-in model methods,
+adding new keyword arguments. If you use ``**kwargs`` in your method
+definitions, you are guaranteed that your code will automatically support those
+arguments when they are added.
 
 If you wish to update a field value in the :meth:`~Model.save` method, you may
 also want to have this field added to the ``update_fields`` keyword argument.
@@ -914,18 +913,13 @@ example::
         name = models.CharField(max_length=100)
         slug = models.TextField()
 
-        def save(
-            self, force_insert=False, force_update=False, using=None, update_fields=None
-        ):
+        def save(self, **kwargs):
             self.slug = slugify(self.name)
-            if update_fields is not None and "name" in update_fields:
+            if (
+                update_fields := kwargs.get("update_fields")
+            ) is not None and "name" in update_fields:
                 update_fields = {"slug"}.union(update_fields)
-            super().save(
-                force_insert=force_insert,
-                force_update=force_update,
-                using=using,
-                update_fields=update_fields,
-            )
+            super().save(**kwargs)
 
 See :ref:`ref-models-update-fields` for more details.
 

+ 46 - 0
tests/basic/tests.py

@@ -13,6 +13,8 @@ from django.test import (
     TransactionTestCase,
     skipUnlessDBFeature,
 )
+from django.test.utils import ignore_warnings
+from django.utils.deprecation import RemovedInDjango60Warning
 from django.utils.translation import gettext_lazy
 
 from .models import (
@@ -187,6 +189,50 @@ class ModelInstanceCreationTests(TestCase):
         with self.assertNumQueries(2):
             ChildPrimaryKeyWithDefault().save()
 
+    def test_save_deprecation(self):
+        a = Article(headline="original", pub_date=datetime(2014, 5, 16))
+        msg = "Passing positional arguments to save() is deprecated"
+        with self.assertWarnsMessage(RemovedInDjango60Warning, msg):
+            a.save(False, False, None, None)
+            self.assertEqual(Article.objects.count(), 1)
+
+    async def test_asave_deprecation(self):
+        a = Article(headline="original", pub_date=datetime(2014, 5, 16))
+        msg = "Passing positional arguments to asave() is deprecated"
+        with self.assertWarnsMessage(RemovedInDjango60Warning, msg):
+            await a.asave(False, False, None, None)
+            self.assertEqual(await Article.objects.acount(), 1)
+
+    @ignore_warnings(category=RemovedInDjango60Warning)
+    def test_save_positional_arguments(self):
+        a = Article.objects.create(headline="original", pub_date=datetime(2014, 5, 16))
+        a.headline = "changed"
+
+        a.save(False, False, None, ["pub_date"])
+        a.refresh_from_db()
+        self.assertEqual(a.headline, "original")
+
+        a.headline = "changed"
+        a.save(False, False, None, ["pub_date", "headline"])
+        a.refresh_from_db()
+        self.assertEqual(a.headline, "changed")
+
+    @ignore_warnings(category=RemovedInDjango60Warning)
+    async def test_asave_positional_arguments(self):
+        a = await Article.objects.acreate(
+            headline="original", pub_date=datetime(2014, 5, 16)
+        )
+        a.headline = "changed"
+
+        await a.asave(False, False, None, ["pub_date"])
+        await a.arefresh_from_db()
+        self.assertEqual(a.headline, "original")
+
+        a.headline = "changed"
+        await a.asave(False, False, None, ["pub_date", "headline"])
+        await a.arefresh_from_db()
+        self.assertEqual(a.headline, "changed")
+
 
 class ModelTest(TestCase):
     def test_objects_attribute_is_only_available_on_the_class_itself(self):

+ 1 - 1
tests/model_forms/models.py

@@ -463,7 +463,7 @@ class Photo(models.Model):
         self._savecount = 0
 
     def save(self, force_insert=False, force_update=False):
-        super().save(force_insert, force_update)
+        super().save(force_insert=force_insert, force_update=force_update)
         self._savecount += 1