From 880d9e7029993702d4849110c9a9ee63c1754a07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Freitag?= Date: Fri, 4 Dec 2020 11:28:03 +0100 Subject: [PATCH] Deprecation plan This change is sure to cause disruption for users. Smooth the transition with an option. --- docs/changelog.rst | 16 ++++++++++++++++ docs/orms.rst | 8 ++++++++ factory/django.py | 19 +++++++++++++++++++ tests/test_django.py | 33 +++++++++++++++++++++++++++++++++ 4 files changed, 76 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index 6f62e814..b48ab94d 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -35,6 +35,22 @@ retain the previous behavior. :func:`~factory.use_strategy()` will be removed in the next major version. + - :class:`factory.django.DjangoModelFactory._after_postgeneration` raises a :class:`DeprecationWarning` when + calling :meth:`~django.db.models.Model.save`. :meth:`~django.db.models.Model.save` will no longer be called in + the next major version of Factory Boy. + + Inspect your :class:`~factory.django.DjangoModelFactory` subclasses: + + - If the :meth:`~django.db.models.Model.save` call is not needed after :class:`~factory.PostGeneration`, set + :attr:`~factory.django.DjangoOptions.skip_postgeneration_save` to ``True``. + - Otherwise, the instance has been modified by :class:`~factory.PostGeneration` hooks and needs to be + :meth:`~django.db.models.Model.save`\ d. Either: + + - call :meth:`django.db.models.Model.save` in the :class:`~factory.PostGeneration` hook that modifies the + instance, or + - override :class:`~factory.django.DjangoModelFactory._after_postgeneration` to + :meth:`~django.db.models.Model.save` the instance. + 3.1.1 (unreleased) ------------------ diff --git a/docs/orms.rst b/docs/orms.rst index 5fe219dd..9397e4ac 100644 --- a/docs/orms.rst +++ b/docs/orms.rst @@ -109,6 +109,14 @@ All factories for a Django :class:`~django.db.models.Model` should use the >>> john.email # The email value was not updated "john@example.com" + .. attribute:: skip_postgeneration_save + + Transitional option to prevent + :meth:`~factory.django.DjangoModelFactory._after_postgeneration` from + issuing a duplicate call to :meth:`~django.db.models.Model.save` on the + created instance when :class:`factory.PostGeneration` hooks return a + value. + Extra fields """""""""""" diff --git a/factory/django.py b/factory/django.py index 3e28e735..54b648cb 100644 --- a/factory/django.py +++ b/factory/django.py @@ -8,6 +8,7 @@ import io import logging import os +import warnings from django.contrib.auth.hashers import make_password from django.core import files as django_files @@ -48,6 +49,7 @@ def _build_default_options(self): return super()._build_default_options() + [ base.OptionDefault('django_get_or_create', (), inherit=True), base.OptionDefault('database', DEFAULT_DB_ALIAS, inherit=True), + base.OptionDefault('skip_postgeneration_save', False, inherit=True), ] def _get_counter_reference(self): @@ -166,6 +168,23 @@ def _create(cls, model_class, *args, **kwargs): manager = cls._get_manager(model_class) return manager.create(*args, **kwargs) + # DEPRECATED. Remove this override with the next major release. + @classmethod + def _after_postgeneration(cls, instance, create, results=None): + """Save again the instance if creating and at least one hook ran.""" + if create and results and not cls._meta.skip_postgeneration_save: + warnings.warn( + f"{cls.__name__}._after_postgeneration will stop saving the instance " + "after postgeneration hooks in the next major release.\n" + "If the save call is extraneous, set skip_postgeneration_save=True " + f"in the {cls.__name__}.Meta.\n" + "To keep saving the instance, move the save call to your " + "postgeneration hooks or override _after_postgeneration.", + DeprecationWarning, + ) + # Some post-generation hooks ran, and may have modified us. + instance.save() + class Password(declarations.Transformer): def __init__(self, password, *args, **kwargs): diff --git a/tests/test_django.py b/tests/test_django.py index e9f41f04..65d52530 100644 --- a/tests/test_django.py +++ b/tests/test_django.py @@ -425,6 +425,9 @@ class PointedRelatedFactory(PointedFactory): factory_related_name='pointed', ) + class Meta: + skip_postgeneration_save = True + class PointerExtraFactory(PointerFactory): pointed__foo = 'extra_new_foo' @@ -441,6 +444,9 @@ class Params: ) ) + class Meta: + skip_postgeneration_save = True + cls.PointedFactory = PointedFactory cls.PointerFactory = PointerFactory cls.PointedRelatedFactory = PointedRelatedFactory @@ -935,6 +941,7 @@ def test_class_decorator_with_subfactory(self): class WithSignalsDecoratedFactory(factory.django.DjangoModelFactory): class Meta: model = models.WithSignals + skip_postgeneration_save = True @factory.post_generation def post(obj, create, extracted, **kwargs): @@ -1021,6 +1028,7 @@ def test_class_decorator_with_muted_related_factory(self): class UndecoratedFactory(factory.django.DjangoModelFactory): class Meta: model = models.PointerModel + skip_postgeneration_save = True pointed = factory.RelatedFactory(self.WithSignalsDecoratedFactory) UndecoratedFactory() @@ -1041,3 +1049,28 @@ class Meta: # Our CustomManager will remove the 'arg=' argument, # invalid for the actual model. ObjFactory.create(arg='invalid') + + +class DjangoModelFactoryDuplicateSaveDeprecationTest(django_test.TestCase): + class StandardFactoryWithPost(StandardFactory): + @factory.post_generation + def post_action(obj, create, extracted, **kwargs): + return 3 + + def test_create_warning(self): + with self.assertWarns(DeprecationWarning) as cm: + self.StandardFactoryWithPost.create() + + [msg] = cm.warning.args + self.assertEqual( + msg, + "StandardFactoryWithPost._after_postgeneration will stop saving the " + "instance after postgeneration hooks in the next major release.\n" + "If the save call is extraneous, set skip_postgeneration_save=True in the " + "StandardFactoryWithPost.Meta.\n" + "To keep saving the instance, move the save call to your postgeneration " + "hooks or override _after_postgeneration.", + ) + + def test_build_no_warning(self): + self.StandardFactoryWithPost.build()