From 3915d4c70d0d7673abe675525b58117a5099afd3 Mon Sep 17 00:00:00 2001 From: Salvo Polizzi Date: Sun, 31 Dec 2023 10:07:13 +0100 Subject: [PATCH] Fixed #35060 -- Deprecated passing positional arguments to Model.save()/asave(). --- django/contrib/auth/base_user.py | 3 ++ django/db/models/base.py | 63 +++++++++++++++++++++++++++++++- docs/internals/deprecation.txt | 3 ++ docs/ref/models/instances.txt | 12 ++++-- docs/releases/5.1.txt | 3 ++ docs/topics/db/models.txt | 40 +++++++++----------- tests/basic/tests.py | 46 +++++++++++++++++++++++ tests/model_forms/models.py | 2 +- 8 files changed, 142 insertions(+), 30 deletions(-) diff --git a/django/contrib/auth/base_user.py b/django/contrib/auth/base_user.py index aa8e9f8a84..ccfe19fcc1 100644 --- a/django/contrib/auth/base_user.py +++ b/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: diff --git a/django/db/models/base.py b/django/db/models/base.py index 4a9150bf37..46ac762ccd 100644 --- a/django/db/models/base.py +++ b/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, diff --git a/docs/internals/deprecation.txt b/docs/internals/deprecation.txt index a1b00c364a..07e0f46856 100644 --- a/docs/internals/deprecation.txt +++ b/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 diff --git a/docs/ref/models/instances.txt b/docs/ref/models/instances.txt index 81f9bfb433..45af7f244f 100644 --- a/docs/ref/models/instances.txt +++ b/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 ------------------------------ diff --git a/docs/releases/5.1.txt b/docs/releases/5.1.txt index b825e9be4f..539ff566a3 100644 --- a/docs/releases/5.1.txt +++ b/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 ======================= diff --git a/docs/topics/db/models.txt b/docs/topics/db/models.txt index b419185bbc..244e9bbb16 100644 --- a/docs/topics/db/models.txt +++ b/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. diff --git a/tests/basic/tests.py b/tests/basic/tests.py index ad82cffe8c..990549edfc 100644 --- a/tests/basic/tests.py +++ b/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): diff --git a/tests/model_forms/models.py b/tests/model_forms/models.py index b6da15f48a..c28461d862 100644 --- a/tests/model_forms/models.py +++ b/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