From a55ec380f1d63a6332dcf05fc2eb0be9caa0f8ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rapha=C3=ABl=20Barrois?= Date: Sat, 3 Oct 2020 12:04:02 +0200 Subject: [PATCH 1/7] Expose memleak in SQLAlchemyModelFactory. The code responsible for `get_or_create` places a copy of the passed-in params on the factory class itself; which is kept after the instance has been created. Those fields are part of the BuildStep context, and should be fetched from there instead. The issue also occurs in DjangoModelFactory. See #781 for details. --- tests/alchemyapp/models.py | 12 ++++++++++-- tests/test_alchemy.py | 33 +++++++++++++++++++++++++++++++++ tests/test_django.py | 27 +++++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 2 deletions(-) diff --git a/tests/alchemyapp/models.py b/tests/alchemyapp/models.py index 013a56ae..3a7e2994 100644 --- a/tests/alchemyapp/models.py +++ b/tests/alchemyapp/models.py @@ -3,9 +3,9 @@ """Helpers for testing SQLAlchemy apps.""" -from sqlalchemy import Column, Integer, Unicode, create_engine +from sqlalchemy import Column, ForeignKey, Integer, Unicode, create_engine from sqlalchemy.ext.declarative import declarative_base -from sqlalchemy.orm import scoped_session, sessionmaker +from sqlalchemy.orm import relationship, scoped_session, sessionmaker session = scoped_session(sessionmaker()) engine = create_engine('sqlite://') @@ -43,4 +43,12 @@ class NonIntegerPk(Base): id = Column(Unicode(20), primary_key=True) +class ForeignKeyModel(Base): + __tablename__ = 'ForeignKeyModelTable' + + id = Column(Integer(), primary_key=True) + standard_id = Column(Integer, ForeignKey("StandardModelTable.id")) + standard = relationship(StandardModel) + + Base.metadata.create_all(engine) diff --git a/tests/test_alchemy.py b/tests/test_alchemy.py index 2205ca1e..7dbac40d 100644 --- a/tests/test_alchemy.py +++ b/tests/test_alchemy.py @@ -2,7 +2,9 @@ """Tests for factory_boy/SQLAlchemy interactions.""" +import gc import unittest +import weakref from unittest import mock import sqlalchemy @@ -72,6 +74,14 @@ class Meta: text = factory.Sequence(lambda n: "text%s" % n) +class ForeignKeyModelFactory(SQLAlchemyModelFactory): + class Meta: + model = models.ForeignKeyModel + sqlalchemy_session = models.session + + standard = factory.SubFactory(StandardFactory) + + class SQLAlchemyPkSequenceTestCase(unittest.TestCase): def setUp(self): super().setUp() @@ -261,3 +271,26 @@ def test_build_does_not_raises_exception_when_no_session_was_set(self): inst1 = NoSessionFactory.build() self.assertEqual(inst0.id, 0) self.assertEqual(inst1.id, 1) + + +class SQLAlchemyMemoryTests(unittest.TestCase): + def test_no_memleak(self): + """A factory class shouldn't keep pointers to its provided parameters.""" + std = StandardFactory() + + # Get a weak reference to the Standard object + std_weak = weakref.ref(std) + ForeignKeyModelFactory(standard=std) + + # Drop references to the `std` object: + # - Commit the SQLAlchemy session, so no local ref has to be kept by + # SQLAlchemy + models.session.commit() + # - Delete the local instance + del std + + # Garbage collect; the instance should be removed + gc.collect() + + # Ensure the instance pointed to by the weak reference is no longer available. + self.assertIsNone(std_weak()) diff --git a/tests/test_django.py b/tests/test_django.py index 694b79ab..c5c5a5e9 100644 --- a/tests/test_django.py +++ b/tests/test_django.py @@ -2,9 +2,11 @@ """Tests for factory_boy/Django interactions.""" +import gc import io import os import unittest +import weakref from unittest import mock import django @@ -1022,3 +1024,28 @@ class Meta: # Our CustomManager will remove the 'arg=' argument, # invalid for the actual model. ObjFactory.create(arg='invalid') + + +class DjangoMemoryTests(django_test.TestCase): + def test_no_memleak(self): + """A factory class shouldn't keep refs to its parameters.""" + class PointerFactory(factory.django.DjangoModelFactory): + class Meta: + model = models.PointerModel + + target = models.PointedModel.objects.create(foo="42") + + # Get a weak reference to the Pointed object + target_weak = weakref.ref(target) + + PointerFactory(pointed=target, bar="bar") + + # Drop references to the `target` object + del target + + # Garbage collect; the instance should be removed + gc.collect() + + # Ensure the instance pointed to by the weak reference is no longer available. + self.assertIsNone(target_weak()) + From 11fe3687a419c5c7b727a676e5111d6b53d6fa0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rapha=C3=ABl=20Barrois?= Date: Sun, 11 Oct 2020 22:12:32 +0200 Subject: [PATCH 2/7] Add an optional normalization to class Meta fields This could be used to ensure that an entry is actually a list. --- factory/base.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/factory/base.py b/factory/base.py index bd00bf67..4c6b446c 100644 --- a/factory/base.py +++ b/factory/base.py @@ -109,18 +109,22 @@ class OptionDefault: checker: callable or None, an optional function used to detect invalid option values at declaration time """ - def __init__(self, name, value, inherit=False, checker=None): + def __init__( + self, name, default, inherit=False, checker=None, normalize=None): self.name = name - self.value = value + self.default = default self.inherit = inherit self.checker = checker + self.normalize = normalize def apply(self, meta, base_meta): - value = self.value + value = self.default if self.inherit and base_meta is not None: value = getattr(base_meta, self.name, value) if meta is not None: value = getattr(meta, self.name, value) + if self.normalize is not None: + value = self.normalize(value) if self.checker is not None: self.checker(meta, value) From 5d0b77f7bc3bcc2e53a581accefb65ec42114120 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rapha=C3=ABl=20Barrois?= Date: Sun, 11 Oct 2020 22:14:42 +0200 Subject: [PATCH 3/7] Provide support for migrating class Meta fields Through `def _get_renamed_options()`, a specific `FactoryOptions` subclass may now indicate that one of its fields should now use another name, and optionally another syntax. The warning message displays the new format to use when adjusting the option. --- factory/base.py | 89 ++++++++++++++++++++++++++++++++++++++++++++-- tests/test_base.py | 36 +++++++++++++++++++ 2 files changed, 123 insertions(+), 2 deletions(-) diff --git a/factory/base.py b/factory/base.py index 4c6b446c..49e7b996 100644 --- a/factory/base.py +++ b/factory/base.py @@ -3,6 +3,7 @@ import collections import logging +import warnings from . import builder, declarations, enums, errors, utils @@ -116,13 +117,44 @@ def __init__( self.inherit = inherit self.checker = checker self.normalize = normalize + self.aliases = {} + + def mark_renamed_from(self, alias): + self.aliases[alias.origin] = alias.transform def apply(self, meta, base_meta): value = self.default if self.inherit and base_meta is not None: value = getattr(base_meta, self.name, value) if meta is not None: - value = getattr(meta, self.name, value) + fields_found = set() + for alias, transform in self.aliases.items(): + if hasattr(meta, alias): + fields_found.add(alias) + origin = getattr(meta, alias) + value = transform(origin) + warning = "Option '{alias}' on {meta} has been renamed to '{name}'." + if origin != value: + warning += " Convert `{alias} = {origin}` into `{name} = {value}`." + warnings.warn( + warning.format( + alias=alias, + name=self.name, + meta=meta, + origin=origin, + value=value, + ), + DeprecationWarning, + stacklevel=5, + ) + if hasattr(meta, self.name): + value = getattr(meta, self.name) + fields_found.add(self.name) + if len(fields_found) > 1: + raise ValueError( + "More than one declaration found for %s: %s. Use only %s - others are obsolete." + % (self.name, ', '.join(sorted(fields_found)), self.name), + ) if self.normalize is not None: value = self.normalize(value) @@ -137,7 +169,34 @@ def __str__(self): self.name, self.value, self.inherit) +def _noop(value): + return value + + +class OptionRenamed: + """A renamed option. + + Provide the original option name, the target name + and an optional transformation to apply to the original + option value. + + If a user class contains a renamed option, a warning will be generated, + showing the expected new name and, if applicable, the transformation + to apply to the value. + """ + def __init__(self, origin, target, transform=_noop): + self.origin = origin + self.target = target + self.transform = transform + + def __str__(self): + return '%s(%r, %r, tranform=%r)' % ( + self.__class__.__name__, + self.origin, self.target, self.transform) + + class FactoryOptions: + def __init__(self): self.factory = None self.base_factory = None @@ -178,8 +237,30 @@ def is_model(meta, value): OptionDefault('inline_args', (), inherit=True), OptionDefault('exclude', (), inherit=True), OptionDefault('rename', {}, inherit=True), + OptionDefault( + 'lookup_groups', [], inherit=True, + normalize=lambda groups: [set(group) for group in groups], + ), ] + def _get_renamed_options(self): + """Retrieve a list of renamed options.""" + return [] + + def _prepare_options(self): + """Prepare the list of options + + - Compute the options; + - Register the renaming rules for relevant options. + """ + options = { + option.name: option + for option in self._build_default_options() + } + for move in self._get_renamed_options(): + options[move.target].mark_renamed_from(move) + return options.values() + def _fill_from_meta(self, meta, base_meta): # Exclude private/protected fields from the meta if meta is None: @@ -191,10 +272,14 @@ def _fill_from_meta(self, meta, base_meta): if not k.startswith('_') } - for option in self._build_default_options(): + for option in self._prepare_options(): assert not hasattr(self, option.name), "Can't override field %s." % option.name value = option.apply(meta, base_meta) meta_attrs.pop(option.name, None) + + # Remove renamed options too + for alias in option.aliases: + meta_attrs.pop(alias, None) setattr(self, option.name, value) if meta_attrs: diff --git a/tests/test_base.py b/tests/test_base.py index 4759bd1e..fa65c7a3 100644 --- a/tests/test_base.py +++ b/tests/test_base.py @@ -1,6 +1,7 @@ # Copyright: See the LICENSE file. import unittest +import warnings from factory import base, declarations, enums, errors @@ -215,6 +216,41 @@ class Meta: with self.assertRaises(TypeError): type("SecondFactory", (base.Factory,), {"Meta": Meta}) + def test_option_renaming(self): + class OtherOptions(base.FactoryOptions): + def _get_renamed_options(self): + return super()._get_renamed_options() + [ + base.OptionRenamed( + origin='alias', + target='rename', + transform=lambda pair: {pair[0]: pair[1]}, + ), + ] + + class OtherFactory(base.Factory): + _options_class = OtherOptions + + class NormalFactory(OtherFactory): + class Meta: + rename = { + 'foo': 'bar', + } + + self.assertEqual({'foo': 'bar'}, NormalFactory._meta.rename) + + with warnings.catch_warnings(record=True) as w: + warnings.filterwarnings("default", category=DeprecationWarning, module=r"tests\.test_base") + + class RenamedFactory(OtherFactory): + class Meta: + alias = ['foo', 'bar'] + + self.assertEqual(1, len(w)) + message = str(w[-1].message) + self.assertIn("RenamedFactory", message) + self.assertIn("alias", message) + self.assertIn("rename = {'foo': 'bar'}", message) + class DeclarationParsingTests(unittest.TestCase): def test_classmethod(self): From f292b7057f9c4d2ab26800a4f32eb0d77615d51c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rapha=C3=ABl=20Barrois?= Date: Mon, 12 Oct 2020 00:17:17 +0200 Subject: [PATCH 4/7] Add `unique_constraints` and associated _lookup() Declare unique constraints on field values through this new `class Meta` option. When building a new instance, the parameters will first be resolved for each unique constraint, and a dedicated lookup performed; an instance will only be created if each lookup failed. Declarations are resolved lazily: a non-constraint SubFactory will only be called if each previous constraint lookup failed. --- docs/changelog.rst | 8 +++- docs/internals.rst | 22 ++++++++++- docs/reference.rst | 91 ++++++++++++++++++++++++++++++++++++++++++++ factory/base.py | 42 ++++++++++++++++++-- factory/builder.py | 42 +++++++++++++------- tests/test_base.py | 1 + tests/test_using.py | 93 +++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 279 insertions(+), 20 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 7e84f9f6..6ae3cfd2 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -1,10 +1,14 @@ ChangeLog ========= -3.1.1 (unreleased) +3.2.0 (unreleased) ------------------ -- Nothing changed yet. +*New:* + + - Add a generic "get-or-create" mechanism through :attr:`~FactoryOptions.unique_constraints`. + + - Add support for composite unique constraints; addressing :issue:`241`. 3.1.0 (2020-10-02) diff --git a/docs/internals.rst b/docs/internals.rst index 6cbf3cec..9474353a 100644 --- a/docs/internals.rst +++ b/docs/internals.rst @@ -76,7 +76,27 @@ Instantiating, Step 2: Preparing values encountering a :class:`SubFactory`. -Instantiating, Step 3: Building the object +Instantiating, step 3: Lookup existing data (optional) +------------------------------------------------------ + +Once the ``StepBuilder`` and its ``Resolver`` are ready, the builder looks at the +:attr:`~Factory.Meta.unique_constraints`: + +1. It provides the call-time parameters to the :meth:`FactoryOptions.get_lookup_groups` + method; +2. That method computes groups of lookups to try: + + - Any call-time parameter that appear in any unique constraint will *always* be included; + - The first lookups are performed on the unique constraintss sharing the most parameters + with the call-time parameters + +3. For each group, the ``StepBuilder`` computes the value of the required parameters, + and performs a database lookup; +4. If any lookup returns an instance, it is used; otherwise, the remaining parameters + will be resolved by the ``Resolver``. + + +Instantiating, Step 4: Building the object ------------------------------------------ 1. The ``StepBuilder`` fetches the attributes computed by the ``Resolver``. diff --git a/docs/reference.rst b/docs/reference.rst index d255f6e0..27b05cf0 100644 --- a/docs/reference.rst +++ b/docs/reference.rst @@ -135,6 +135,86 @@ Meta options .. versionadded: 2.6.0 + .. attribute:: unique_constraints + + Some models have unicity constraints on parts of their fields. + Instead of failing to save an instance, factories may define + these unicity constraints here: + + .. code-block:: python + + class EmployeeFactory(factory.Factory): + class Meta: + model = Employee + unique_constraints = [ + ['email'], + ['access_card_id'], + ['company', 'employee_id'], + ] + + Each group is a list of fields which should be *collectively unique*; + in this example, the ``employee_id`` is unique within each company. + + When a new instance is required, the factory will start by resolving + the parameter used for each unique constraint, and attempt a lookup for this + combination of fields: + + .. code-block:: pycon + + >>> e1 = EmployeeFactory() + >>> e2 = EmployeeFactory(access_card_id=42) + lookup(email='john.doe@example.org') -> None; continue + lookup(access_card_id=42) -> e1; return + >>> e1 == e2 + True + + If the lookup succeeds, the instance is used as is; post-generation hooks + will still be called. + + .. note:: The :attr:`~FactoryOptions.unique_constraints` feature requires + a :meth:`Factory._lookup` definition on the factory class + or one of its parents. + + .. tip:: Group parameters are resolved lazily: in the above example, + the `company` declaration will only be evaluated if the `email` + and `access_card_id` lookup failed. + + This avoids polluting the database with unneeded objects. + + .. note:: + + **Lookup priority** + + Unique constraints have a specific interaction with call-time parameters: + if a parameter is explicitly set when calling the factory, and appears + in any unique constraint, it will be included in each lookup: it is likely + a unique identifier of some sort. + + Moreover, unique constraints containing more call-time parameters will be + tried first: they are more likely to succeed. + + With the above definition, the following will happen: + + .. code-block:: pycon + + >>> EmployeeFactory() + # 1. lookup(email=...) + # 2. lookup(access_card_id=...) + # 3. lookup(company=..., employee_id=...) + + >>> EmployeeFactory(access_card_id='42') + # 1. lookup(access_card_id=42) + # 2. lookup(access_card_id=42, email=...) + # 3. lookup(access_card_id=42, company=..., employee_id=...) + + >>> EmployeeFactory(access_card_id='42', name="John Doe") + # The `name=` is not included in lookups, since it doesn't appear + # in unique constraints. + # 1. lookup(access_card_id=42) + # 2. lookup(access_card_id=42, email=...) + # 3. lookup(access_card_id=42, company=..., employee_id=...) + + .. versionadded:: 3.2.0 .. attribute:: strategy @@ -319,6 +399,17 @@ Attributes and methods .. OHAI_VIM* + .. classmethod:: _lookup(cls, model_class, strategy, fields) + + Lookup an instance from the database, using the passed-in fields. + + This is required for the :attr:`~FactoryOptions.unique_constraints` feature. + + The ``strategy`` field can be used to disable lookups with specific + strategies - typically for :data:`BUILD_STRATEGY` and :data:`STUB_STRATEGY`. + + .. versionadded:: 3.2.0 + .. classmethod:: _after_postgeneration(cls, obj, create, results=None) :arg object obj: The object just generated diff --git a/factory/base.py b/factory/base.py index 49e7b996..8094abf7 100644 --- a/factory/base.py +++ b/factory/base.py @@ -238,7 +238,7 @@ def is_model(meta, value): OptionDefault('exclude', (), inherit=True), OptionDefault('rename', {}, inherit=True), OptionDefault( - 'lookup_groups', [], inherit=True, + 'unique_constraints', [], inherit=True, normalize=lambda groups: [set(group) for group in groups], ), ] @@ -371,6 +371,15 @@ def reset_sequence(self, value=None, force=False): value = self.counter_reference.factory._setup_next_sequence() self._counter.reset(value) + def apply_renames(self, mapping): + """Rename items based on self.renames. + + Mutates the passed-in mapping in place. + """ + for old_name, new_name in self.rename.items(): + if old_name in mapping: + mapping[new_name] = mapping.pop(old_name) + def prepare_arguments(self, attributes): """Convert an attributes dict to a (args, kwargs) tuple.""" kwargs = dict(attributes) @@ -384,9 +393,7 @@ def prepare_arguments(self, attributes): } # 3. Rename fields - for old_name, new_name in self.rename.items(): - if old_name in kwargs: - kwargs[new_name] = kwargs.pop(old_name) + self.apply_renames(kwargs) # 4. Extract inline args args = tuple( @@ -396,6 +403,26 @@ def prepare_arguments(self, attributes): return args, kwargs + def get_lookup_groups(self, params): + """Retrieve the list of lookup fields based on provided params.""" + # Extract call-time params that appear in any unique constraint + lookup_params = set(params) & set().union(*self.unique_constraints) + + # Sort constraint groups: start with those containing the most + # call-time params + by_distance = sorted( + self.unique_constraints, + key=lambda group: -len(group & set(params)), + ) + for group in by_distance: + yield group | lookup_params + + def lookup(self, fields, strategy): + kwargs = dict(fields) + self.apply_renames(kwargs) + model = self.get_model_class() + return self.factory._lookup(model, strategy, fields=kwargs) + def instantiate(self, step, args, kwargs): model = self.get_model_class() @@ -593,6 +620,13 @@ def _create(cls, model_class, *args, **kwargs): """ return model_class(*args, **kwargs) + @classmethod + def _lookup(cls, model_class, params): + raise NotImplementedError( + "Using `class Meta: unique_constraints` is not available on class %s: " + "no `def _lookup()` method has been provided." % cls, + ) + @classmethod def build(cls, **kwargs): """Build an instance of the associated class, with overriden attrs.""" diff --git a/factory/builder.py b/factory/builder.py index 09153f76..df1653ec 100644 --- a/factory/builder.py +++ b/factory/builder.py @@ -199,21 +199,22 @@ def parse_declarations(decls, base_pre=None, base_post=None): class BuildStep: - def __init__(self, builder, sequence, parent_step=None): + def __init__(self, *, builder, sequence, declarations, parent_step=None): self.builder = builder self.sequence = sequence self.attributes = {} + self.declarations = declarations self.parent_step = parent_step - self.stub = None - - def resolve(self, declarations): self.stub = Resolver( - declarations=declarations, + declarations=self.declarations, step=self, sequence=self.sequence, ) - for field_name in declarations: + def resolve(self, only=None): + only = only or self.declarations + + for field_name in only: self.attributes[field_name] = getattr(self.stub, field_name) @property @@ -272,16 +273,31 @@ def build(self, parent_step=None, force_sequence=None): builder=self, sequence=sequence, parent_step=parent_step, + declarations=pre, ) - step.resolve(pre) - args, kwargs = self.factory_meta.prepare_arguments(step.attributes) + instance = None - instance = self.factory_meta.instantiate( - step=step, - args=args, - kwargs=kwargs, - ) + for group in self.factory_meta.get_lookup_groups(self.extras.keys()): + step.resolve(only=group) + fields = { + field: step.attributes[field] + for field in group + } + instance = self.factory_meta.lookup(fields, strategy=self.strategy) + if instance is not None: + break + + if instance is None: + step.resolve() + + args, kwargs = self.factory_meta.prepare_arguments(step.attributes) + + instance = self.factory_meta.instantiate( + step=step, + args=args, + kwargs=kwargs, + ) postgen_results = {} for declaration_name in post.sorted(): diff --git a/tests/test_base.py b/tests/test_base.py index fa65c7a3..0f0571e1 100644 --- a/tests/test_base.py +++ b/tests/test_base.py @@ -109,6 +109,7 @@ class AbstractFactory(base.Factory): self.assertEqual((), AbstractFactory._meta.inline_args) self.assertEqual((), AbstractFactory._meta.exclude) self.assertEqual(enums.CREATE_STRATEGY, AbstractFactory._meta.strategy) + self.assertEqual([], AbstractFactory._meta.unique_constraints) # Non-declarative attributes self.assertEqual({}, AbstractFactory._meta.pre_declarations.as_dict()) diff --git a/tests/test_using.py b/tests/test_using.py index fe4a3ed5..4ab35590 100644 --- a/tests/test_using.py +++ b/tests/test_using.py @@ -32,6 +32,10 @@ def as_dict(self): five=self.five, ) + def __repr__(self): + return 'TestObject(one=%r, two=%r, three=%r, four=%r, five=%r)' % ( + self.one, self.two, self.three, self.four, self.five) + class Dummy: def __init__(self, **kwargs): @@ -1473,6 +1477,95 @@ def pricing(self, create, extracted, base_price=5, extra=0, **kwargs): self.assertEqual({5: p}, PRICES) +class LookupGroupTests(unittest.TestCase): + store = [] + + class LookupFactory(factory.Factory): + # No `class Meta: model=` here, to ensure + # each test's factory has an independent sequence counter. + + @classmethod + def _lookup(cls, model_class, strategy, fields): + for instance in LookupGroupTests.store: + if all( + getattr(instance, field) == value + for field, value in fields.items() + ): + return instance + + @classmethod + def _create(cls, model_class, *args, **kwargs): + instance = super()._create(model_class, *args, **kwargs) + LookupGroupTests.store.append(instance) + return instance + + def setUp(self): + super().setUp() + LookupGroupTests.store = [] + + def test_simple_lookup(self): + class SingleConstraintFactory(self.LookupFactory): + class Meta: + model = TestObject + unique_constraints = [ + ['one'], + ] + + one = factory.Sequence(lambda n: n) + two = factory.Sequence(lambda n: 2 * n) + + o1 = SingleConstraintFactory() + o2 = SingleConstraintFactory() + o3 = SingleConstraintFactory(one=0) + o4 = SingleConstraintFactory(two=0) + self.assertEqual(o1, o3) + self.assertEqual([o1, o2, o4], LookupGroupTests.store) + + def test_composite_lookup(self): + class CompositeConstraintFactory(self.LookupFactory): + class Meta: + model = TestObject + unique_constraints = [ + ['one', 'two'], + ] + + one = factory.Sequence(lambda n: n) + two = factory.Sequence(lambda n: 2 * n) + three = factory.Sequence(lambda n: 3 * n) + + o1 = CompositeConstraintFactory() # 0, 0 + o2 = CompositeConstraintFactory(one=0) # 0, 2 => no match + o3 = CompositeConstraintFactory(two=0) # 2, 0 => no match + o4 = CompositeConstraintFactory(one=0, two=0) # 0, 0 => lookup + self.assertEqual(o1, o4) + self.assertEqual([o1, o2, o3], LookupGroupTests.store) + + def test_multiple_lookups(self): + class MultipleConstraintFactory(self.LookupFactory): + class Meta: + model = TestObject + unique_constraints = [ + ['one', 'two'], + ['three'], + ] + + one = factory.Sequence(lambda n: n) + two = factory.Sequence(lambda n: 2 * n) + three = factory.Sequence(lambda n: 3 * n) + four = factory.Sequence(lambda n: 4 * n) + + o1 = MultipleConstraintFactory() # 0, 0, 0 + o2 = MultipleConstraintFactory(one=0) # 0, 2, 3 => no match + o3 = MultipleConstraintFactory(two=0) # 2, 0, 6 => no match + o4 = MultipleConstraintFactory(one=0, two=0) # 0, 0, 9 => match on one/two + o5 = MultipleConstraintFactory(three=4) # 4, 8, 4 => no match + o6 = MultipleConstraintFactory(three=3) # 10, 10, 6 => match on three + o7 = MultipleConstraintFactory(one=3, three=3) # 3, 12, 3 => no match (one+three) + self.assertEqual(o1, o4) + self.assertEqual(o2, o6) + self.assertEqual([o1, o2, o3, o5, o7], LookupGroupTests.store) + + class SubFactoryTestCase(unittest.TestCase): def test_sub_factory(self): class TestModel2(FakeModel): From 7bfe247df15b011c12dd6b9cf8f6f2b3ff74ddd3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rapha=C3=ABl=20Barrois?= Date: Mon, 12 Oct 2020 00:25:28 +0200 Subject: [PATCH 5/7] Replace *_get_or_create with unique_constraints. This change deprecates django_get_or_create and sqlalchemy_get_or_create; upgrade helpers are provided throught the option renaming helpers. Closes #241 The new `unique_constraints` feature is more powerful, but supports the exact same cases; a specific case has been added to opt out of lookups for `Factory.build()` and `Factory.stub()`. Examples are provided for Django and SQLAlchemy unique constraint declarations. Closes #69 With the implementation of `unique_constraints` - and its lazy declaration resolution algorithm, non-constrained declarations will only be resolved once lookups have failed; this avoids creating orphan objects. Closes #781 The call-time parameters are now explicitly included in the lookup algorithm; they no longer need to be kept attached to the factory class, which leaked instances. --- docs/changelog.rst | 28 ++++++++++++++- docs/orms.rst | 79 +++++++++++++++++++++++++++++++++++++++++++ docs/reference.rst | 25 ++++++++++++++ factory/alchemy.py | 67 ++++++++++-------------------------- factory/django.py | 65 ++++++++++------------------------- tests/test_alchemy.py | 20 ++++++++--- tests/test_django.py | 45 ++++++++++++++++++++---- tests/test_using.py | 54 ++++++++++++++++------------- 8 files changed, 253 insertions(+), 130 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 6ae3cfd2..2fb0d8d5 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -6,10 +6,36 @@ ChangeLog *New:* - - Add a generic "get-or-create" mechanism through :attr:`~FactoryOptions.unique_constraints`. + - Add a generic "get-or-create" mechanism through :attr:`~FactoryOptions.unique_constraints`; this replaces + :attr:`~django.DjangoOptions.django_get_or_create` and + :attr:`~alchemy.SQLAlchemyOptions.sqlalchemy_get_or_create`. + + Replace ``django_get_or_create = ['foo', 'bar']`` with + ``unique_constraints = [ ['foo'], ['bar'] ]``. + + - Only resolve non-constrained attributes once lookups have failed. This addresses + :issue:`69`. - Add support for composite unique constraints; addressing :issue:`241`. +*Bugfix:* + + - :issue:`781`: Don't leak memory by keeping references to call-time parameters + beyond the instance creation. + +*Deprecation:* + + - :attr:`~django.DjangoOptions.django_get_or_create` is deprecated; + :attr:`~FactoryOptions.unique_constraints` should be used instead. + - :attr:`~alchemy.SQLAlchemyOptions.sqlalchemy_get_or_create` is deprecated; + :attr:`~FactoryOptions.unique_constraints` should be used instead. + +The changes brought to :attr:`~django.DjangoOptions.django_get_or_create` +and :attr:`~alchemy.SQLAlchemyOptions.sqlalchemy_get_or_create` might alter the +number and details of SQL queries performed when these attributes were defined with +more than one field: there is now one query per group until one matching instance is +found. + 3.1.0 (2020-10-02) ------------------ diff --git a/docs/orms.rst b/docs/orms.rst index 4508d235..fa40b549 100644 --- a/docs/orms.rst +++ b/docs/orms.rst @@ -37,6 +37,8 @@ All factories for a Django :class:`~django.db.models.Model` should use the * The :attr:`~factory.FactoryOptions.model` attribute also supports the ``'app.Model'`` syntax * :func:`~factory.Factory.create()` uses :meth:`Model.objects.create() ` + * :func:`~factory.FactoryOptions.unique_constraints` uses a provided :meth:`Factory._lookup` + implementation, performin a :meth:`Model.objects.get() ` call * When using :class:`~factory.RelatedFactory` or :class:`~factory.PostGeneration` attributes, the base object will be :meth:`saved ` once all post-generation hooks have run. @@ -56,6 +58,43 @@ All factories for a Django :class:`~django.db.models.Model` should use the .. attribute:: django_get_or_create .. versionadded:: 2.4.0 + .. deprecated:: 3.2.0 + + Use :attr:`FactoryOptions.unique_constraints` instead. + + .. important:: + + Replace ``django_get_or_create = ['foo', 'bar']`` with ``unique_constraints = [ ['foo'], ['bar'] ]``. + + Whereas :attr:`~django_get_or_create` could only provide a list + of independantly unique columns, :attr:`~FactoryOptions.unique_constraints` + can support :attr:`~django.db.models.Options.unique_together`: + + .. code-block:: python + + class City(models.Model): + region = models.ForeignKey(Region) + name = models.TextField() + zipcode = models.TextField(unique=True) + + class Meta: + unique_together = [ + ('region', 'name'), + ] + + class CityFactory(factory.django.DjangoModelFactory): + class Meta: + model = models.City + unique_constraints = [ + ['zipcode'], + ['region', 'name'], + ] + + ... + + .. note:: The :meth:`django.DjangoModelFactory._lookup` method will only + perform database lookups when using the + :data:`~factory.CREATE_STRATEGY` strategy. Fields whose name are passed in this list will be used to perform a :meth:`Model.objects.get_or_create() ` @@ -347,6 +386,46 @@ To work, this class needs an `SQLAlchemy`_ session object affected to the :attr: .. attribute:: sqlalchemy_get_or_create .. versionadded:: 3.0.0 + .. deprecated:: 3.2.0 + + Use :attr:`FactoryOptions.unique_constraints` instead. + + .. important:: + + Replace ``sqlalchemy_get_or_create = ['foo', 'bar']`` with + ``unique_constraints = [ ['foo'], ['bar'] ]``. + + Whereas :attr:`~sqlalchemy_get_or_create` could only provide a list + of independantly unique columns, :attr:`~FactoryOptions.unique_constraints` + can support :class:`sqlalchemy.schema.UniqueConstraint`: + + .. code-block:: python + + class City(Base): + __tablename__ = 'cities' + + region_code = Column(Unicode(10)) + name = Column(Unicode(200)) + zipcode = Column(Unicode(10), unique=True) + + __table__args = ( + UniqueConstraint('region_code', 'name'), + ) + + + class CityFactory(factory.django.DjangoModelFactory): + class Meta: + model = models.City + unique_constraints = [ + ['zipcode'], + ['region_code', 'name'], + ] + + ... + + .. note:: The :meth:`alchemy.SQLAlchemyModelFactory._lookup` method will only + perform database lookups when using the + :data:`~factory.CREATE_STRATEGY` strategy. Fields whose name are passed in this list will be used to perform a :meth:`Model.query.one_or_none() ` diff --git a/docs/reference.rst b/docs/reference.rst index 27b05cf0..9225a7dd 100644 --- a/docs/reference.rst +++ b/docs/reference.rst @@ -175,6 +175,10 @@ Meta options a :meth:`Factory._lookup` definition on the factory class or one of its parents. + A native implementation is provided for + :class:`django.DjangoModelFactory` and + :class:`alchemy.SQLAlchemyModelFactory`. + .. tip:: Group parameters are resolved lazily: in the above example, the `company` declaration will only be evaluated if the `email` and `access_card_id` lookup failed. @@ -214,6 +218,27 @@ Meta options # 2. lookup(access_card_id=42, email=...) # 3. lookup(access_card_id=42, company=..., employee_id=...) + This feature *replaces* :attr:`django.DjangoOptions.django_get_or_create` + and :attr:`alchemy.SQLAlchemyOptions.sqlalchemy_get_or_create` options. + + .. warning:: :attr:`~django.DjangoOptions.django_get_or_create` and + :attr:`~alchemy.SQLAlchemyOptions.sqlalchemy_get_or_create` + where list of fields, understood as "a list of separately + unique columns"; whereas :attr:`~FactoryOptions.unique_constraints` + is a list of *lists of collectively unique columns*. + + Migrate as follow: + + .. code-block:: python + + class EmployeeFactory(factory.django.DjangoModelFactory): + class Meta: + django_get_or_create = ['email', 'employee_id'] + unique_constraints = [ + ['email'], + ['employee_id'], + ] + .. versionadded:: 3.2.0 .. attribute:: strategy diff --git a/factory/alchemy.py b/factory/alchemy.py index beccdccf..dfd545fe 100644 --- a/factory/alchemy.py +++ b/factory/alchemy.py @@ -1,9 +1,6 @@ # Copyright: See the LICENSE file. -from sqlalchemy.exc import IntegrityError -from sqlalchemy.orm.exc import NoResultFound - -from . import base, errors +from . import base, enums SESSION_PERSISTENCE_COMMIT = 'commit' SESSION_PERSISTENCE_FLUSH = 'flush' @@ -24,7 +21,6 @@ def _check_sqlalchemy_session_persistence(self, meta, value): def _build_default_options(self): return super()._build_default_options() + [ - base.OptionDefault('sqlalchemy_get_or_create', (), inherit=True), base.OptionDefault('sqlalchemy_session', None, inherit=True), base.OptionDefault( 'sqlalchemy_session_persistence', @@ -34,6 +30,16 @@ def _build_default_options(self): ), ] + def _get_renamed_options(self): + """Map an obsolete option to its new name.""" + return super()._get_renamed_options() + [ + base.OptionRenamed( + origin='sqlalchemy_get_or_create', + target='unique_constraints', + transform=lambda fields: [[field] for field in fields], + ), + ] + class SQLAlchemyModelFactory(base.Factory): """Factory for SQLAlchemy models. """ @@ -44,48 +50,13 @@ class Meta: abstract = True @classmethod - def _generate(cls, strategy, params): - # Original params are used in _get_or_create if it cannot build an - # object initially due to an IntegrityError being raised - cls._original_params = params - return super(SQLAlchemyModelFactory, cls)._generate(strategy, params) - - @classmethod - def _get_or_create(cls, model_class, session, *args, **kwargs): - key_fields = {} - for field in cls._meta.sqlalchemy_get_or_create: - if field not in kwargs: - raise errors.FactoryError( - "sqlalchemy_get_or_create - " - "Unable to find initialization value for '%s' in factory %s" % - (field, cls.__name__)) - key_fields[field] = kwargs.pop(field) - - obj = session.query(model_class).filter_by( - *args, **key_fields).one_or_none() - - if not obj: - try: - obj = cls._save(model_class, session, *args, **key_fields, **kwargs) - except IntegrityError as e: - session.rollback() - get_or_create_params = { - lookup: value - for lookup, value in cls._original_params.items() - if lookup in cls._meta.sqlalchemy_get_or_create - } - if get_or_create_params: - try: - obj = session.query(model_class).filter_by( - **get_or_create_params).one() - except NoResultFound: - # Original params are not a valid lookup and triggered a create(), - # that resulted in an IntegrityError. - raise e - else: - raise e - - return obj + def _lookup(cls, model_class, strategy, fields): + if strategy != enums.CREATE_STRATEGY: + return + session = cls._meta.sqlalchemy_session + if session is None: + raise RuntimeError("No session provided.") + return session.query(model_class).filter_by(**fields).one_or_none() @classmethod def _create(cls, model_class, *args, **kwargs): @@ -94,8 +65,6 @@ def _create(cls, model_class, *args, **kwargs): if session is None: raise RuntimeError("No session provided.") - if cls._meta.sqlalchemy_get_or_create: - return cls._get_or_create(model_class, session, *args, **kwargs) return cls._save(model_class, session, *args, **kwargs) @classmethod diff --git a/factory/django.py b/factory/django.py index 5405be71..6a816d3f 100644 --- a/factory/django.py +++ b/factory/django.py @@ -10,9 +10,9 @@ import os from django.core import files as django_files -from django.db import IntegrityError +from django.core.exceptions import ObjectDoesNotExist -from . import base, declarations, errors +from . import base, declarations, enums, errors logger = logging.getLogger('factory.generate') @@ -45,10 +45,19 @@ def _lazy_load_get_model(): class DjangoOptions(base.FactoryOptions): 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), ] + def _get_renamed_options(self): + """Map an obsolete option to its new name.""" + return super()._get_renamed_options() + [ + base.OptionRenamed( + origin='django_get_or_create', + target='unique_constraints', + transform=lambda fields: [[field] for field in fields], + ), + ] + def _get_counter_reference(self): counter_reference = super()._get_counter_reference() if (counter_reference == self.base_factory @@ -110,58 +119,20 @@ def _get_manager(cls, model_class): return manager @classmethod - def _generate(cls, strategy, params): - # Original params are used in _get_or_create if it cannot build an - # object initially due to an IntegrityError being raised - cls._original_params = params - return super()._generate(strategy, params) + def _lookup(cls, model_class, strategy, fields): + if strategy != enums.CREATE_STRATEGY: + return - @classmethod - def _get_or_create(cls, model_class, *args, **kwargs): - """Create an instance of the model through objects.get_or_create.""" manager = cls._get_manager(model_class) - assert 'defaults' not in cls._meta.django_get_or_create, ( - "'defaults' is a reserved keyword for get_or_create " - "(in %s._meta.django_get_or_create=%r)" - % (cls, cls._meta.django_get_or_create)) - - key_fields = {} - for field in cls._meta.django_get_or_create: - if field not in kwargs: - raise errors.FactoryError( - "django_get_or_create - " - "Unable to find initialization value for '%s' in factory %s" % - (field, cls.__name__)) - key_fields[field] = kwargs.pop(field) - key_fields['defaults'] = kwargs - try: - instance, _created = manager.get_or_create(*args, **key_fields) - except IntegrityError as e: - get_or_create_params = { - lookup: value - for lookup, value in cls._original_params.items() - if lookup in cls._meta.django_get_or_create - } - if get_or_create_params: - try: - instance = manager.get(**get_or_create_params) - except manager.model.DoesNotExist: - # Original params are not a valid lookup and triggered a create(), - # that resulted in an IntegrityError. Follow Django’s behavior. - raise e - else: - raise e - - return instance + return manager.get(**fields) + except ObjectDoesNotExist: + return None @classmethod def _create(cls, model_class, *args, **kwargs): """Create an instance of the model, and save it to the database.""" - if cls._meta.django_get_or_create: - return cls._get_or_create(model_class, *args, **kwargs) - manager = cls._get_manager(model_class) return manager.create(*args, **kwargs) diff --git a/tests/test_alchemy.py b/tests/test_alchemy.py index 7dbac40d..78079f5c 100644 --- a/tests/test_alchemy.py +++ b/tests/test_alchemy.py @@ -43,7 +43,9 @@ class Meta: class MultifieldModelFactory(SQLAlchemyModelFactory): class Meta: model = models.MultiFieldModel - sqlalchemy_get_or_create = ('slug',) + unique_constraints = [ + ['slug'], + ] sqlalchemy_session = models.session sqlalchemy_session_persistence = 'commit' @@ -54,7 +56,9 @@ class Meta: class WithGetOrCreateFieldFactory(SQLAlchemyModelFactory): class Meta: model = models.StandardModel - sqlalchemy_get_or_create = ('foo',) + unique_constraints = [ + ['foo'], + ] sqlalchemy_session = models.session sqlalchemy_session_persistence = 'commit' @@ -65,7 +69,10 @@ class Meta: class WithMultipleGetOrCreateFieldsFactory(SQLAlchemyModelFactory): class Meta: model = models.MultifieldUniqueModel - sqlalchemy_get_or_create = ("slug", "text",) + unique_constraints = [ + ['slug'], + ['text'], + ] sqlalchemy_session = models.session sqlalchemy_session_persistence = 'commit' @@ -129,7 +136,7 @@ def test_simple_call(self): self.assertEqual(obj1, obj2) def test_missing_arg(self): - with self.assertRaises(factory.FactoryError): + with self.assertRaises(AttributeError): MultifieldModelFactory() def test_raises_exception_when_existing_objs(self): @@ -153,6 +160,11 @@ def test_multicall(self): ["alt", "main"], ) + def test_no_lookup_on_build(self): + MultifieldModelFactory(slug='first', foo="First") + second = MultifieldModelFactory.build(slug='first') + self.assertNotEqual("First", second.foo) + class MultipleGetOrCreateFieldsTest(unittest.TestCase): def setUp(self): diff --git a/tests/test_django.py b/tests/test_django.py index c5c5a5e9..067311e8 100644 --- a/tests/test_django.py +++ b/tests/test_django.py @@ -61,7 +61,7 @@ class Meta: class StandardFactoryWithPKField(factory.django.DjangoModelFactory): class Meta: model = models.StandardModel - django_get_or_create = ('pk',) + unique_constraints = [['pk']] foo = factory.Sequence(lambda n: "foo%d" % n) pk = None @@ -78,7 +78,7 @@ class Meta: class MultifieldModelFactory(factory.django.DjangoModelFactory): class Meta: model = models.MultifieldModel - django_get_or_create = ['slug'] + unique_constraints = [['slug']] text = factory.Faker('text') @@ -135,7 +135,10 @@ class Meta: class WithMultipleGetOrCreateFieldsFactory(factory.django.DjangoModelFactory): class Meta: model = models.MultifieldUniqueModel - django_get_or_create = ("slug", "text",) + unique_constraints = [ + ['slug'], + ['text'], + ] slug = factory.Sequence(lambda n: "slug%s" % n) text = factory.Sequence(lambda n: "text%s" % n) @@ -215,7 +218,7 @@ def test_simple_call(self): ) def test_missing_arg(self): - with self.assertRaises(factory.FactoryError): + with self.assertRaises(AttributeError): MultifieldModelFactory() def test_multicall(self): @@ -234,9 +237,40 @@ def test_multicall(self): ["alt", "main"], ) + def test_no_lookup_on_build(self): + MultifieldModelFactory(slug='first', text="First") + second = MultifieldModelFactory.build(slug='first') + self.assertNotEqual("First", second.text) + + def test_no_side_effects(self): + class PointedFactory(factory.django.DjangoModelFactory): + class Meta: + model = models.PointedModel + foo = factory.Sequence(lambda n: 'foo%d' % n) + + class PointerGetOrCreate(factory.django.DjangoModelFactory): + class Meta: + model = models.PointerModel + unique_constraints = [ + ('bar',), + ] + bar = factory.Sequence(lambda n: 'bar%d' % n) + pointed = factory.SubFactory(PointedFactory) + + first = PointerGetOrCreate() + self.assertEqual('bar0', first.bar) + second = PointerGetOrCreate(bar='other') + self.assertNotEqual(first.pointed, second.pointed) + third = PointerGetOrCreate(bar='bar0') + self.assertEqual(third, first) + self.assertEqual( + [first.pointed, second.pointed], + list(models.PointedModel.objects.all()), + ) + class MultipleGetOrCreateFieldsTest(django_test.TestCase): - def test_one_defined(self): + def test_any_field(self): obj1 = WithMultipleGetOrCreateFieldsFactory() obj2 = WithMultipleGetOrCreateFieldsFactory(slug=obj1.slug) self.assertEqual(obj1, obj2) @@ -1048,4 +1082,3 @@ class Meta: # Ensure the instance pointed to by the weak reference is no longer available. self.assertIsNone(target_weak()) - diff --git a/tests/test_using.py b/tests/test_using.py index 4ab35590..d1eef444 100644 --- a/tests/test_using.py +++ b/tests/test_using.py @@ -62,18 +62,13 @@ def create(cls, **kwargs): return instance class FakeModelManager: - def get_or_create(self, **kwargs): - defaults = kwargs.pop('defaults', {}) - kwargs.update(defaults) - instance = FakeModel.create(**kwargs) - instance.id = 2 - instance._defaults = defaults - return instance, True + def get(self, **kwargs): + self.last_lookup = kwargs + return None def create(self, **kwargs): instance = FakeModel.create(**kwargs) instance.id = 2 - instance._defaults = None return instance def values_list(self, *args, **kwargs): @@ -2114,14 +2109,16 @@ def __init__(self, keys, instance): self.keys = keys self.instance = instance - def get_or_create(self, **kwargs): - defaults = kwargs.pop('defaults', {}) - if kwargs == self.keys: - return self.instance, False - kwargs.update(defaults) + def create(self, **kwargs): instance = FakeModel.create(**kwargs) instance.id = 2 - return instance, True + return instance + + def get(self, **kwargs): + if kwargs == self.keys: + return self.instance + else: + return None def using(self, db): return self @@ -2160,7 +2157,7 @@ class MyFakeModel(BetterFakeModel): class MyFakeModelFactory(factory.django.DjangoModelFactory): class Meta: model = MyFakeModel - django_get_or_create = ('x',) + unique_constraints = [['x']] x = 1 y = 4 z = 6 @@ -2182,7 +2179,11 @@ class MyFakeModel(BetterFakeModel): class MyFakeModelFactory(factory.django.DjangoModelFactory): class Meta: model = MyFakeModel - django_get_or_create = ('x', 'y', 'z') + unique_constraints = [ + ['x'], + ['y'], + ['z'], + ] x = 1 y = 4 z = 6 @@ -2204,7 +2205,7 @@ class MyFakeModel(BetterFakeModel): class MyFakeModelFactory(factory.django.DjangoModelFactory): class Meta: model = MyFakeModel - django_get_or_create = ('x',) + unique_constraints = [['x']] x = 1 y = 4 z = 6 @@ -2226,7 +2227,11 @@ class MyFakeModel(BetterFakeModel): class MyFakeModelFactory(factory.django.DjangoModelFactory): class Meta: model = MyFakeModel - django_get_or_create = ('x', 'y', 'z') + unique_constraints = [ + ['x'], + ['y'], + ['z'], + ] x = 1 y = 4 z = 6 @@ -2265,7 +2270,6 @@ class Meta: a = factory.Sequence(lambda n: 'foo_%s' % n) o = TestModelFactory() - self.assertEqual(None, o._defaults) self.assertEqual('foo_0', o.a) self.assertEqual(2, o.id) @@ -2273,7 +2277,9 @@ def test_get_or_create(self): class TestModelFactory(factory.django.DjangoModelFactory): class Meta: model = TestModel - django_get_or_create = ('a', 'b') + unique_constraints = [ + ('a', 'b'), + ] a = factory.Sequence(lambda n: 'foo_%s' % n) b = 2 @@ -2281,7 +2287,7 @@ class Meta: d = 4 o = TestModelFactory() - self.assertEqual({'c': 3, 'd': 4}, o._defaults) + self.assertEqual({'a': 'foo_0', 'b': 2}, TestModel.objects.last_lookup) self.assertEqual('foo_0', o.a) self.assertEqual(2, o.b) self.assertEqual(3, o.c) @@ -2293,7 +2299,9 @@ def test_full_get_or_create(self): class TestModelFactory(factory.django.DjangoModelFactory): class Meta: model = TestModel - django_get_or_create = ('a', 'b', 'c', 'd') + unique_constraints = [ + ('a', 'b', 'c', 'd'), + ] a = factory.Sequence(lambda n: 'foo_%s' % n) b = 2 @@ -2301,7 +2309,7 @@ class Meta: d = 4 o = TestModelFactory() - self.assertEqual({}, o._defaults) + self.assertEqual(dict(a='foo_0', b=2, c=3, d=4), TestModel.objects.last_lookup) self.assertEqual('foo_0', o.a) self.assertEqual(2, o.b) self.assertEqual(3, o.c) From 772a0678786ce70d6f3f6162a2c107866796aeb9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rapha=C3=ABl=20Barrois?= Date: Mon, 12 Oct 2020 00:31:07 +0200 Subject: [PATCH 6/7] docs: Use `.. rubric::` for minor sections. --- docs/reference.rst | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/reference.rst b/docs/reference.rst index 9225a7dd..372bf3ad 100644 --- a/docs/reference.rst +++ b/docs/reference.rst @@ -255,7 +255,7 @@ Attributes and methods .. class:: Factory - **Class-level attributes:** + .. rubric:: Class-level attributes: .. attribute:: Meta .. attribute:: _meta @@ -284,7 +284,7 @@ Attributes and methods the :class:`Factory`-building metaclass can use it instead. - **Base functions:** + .. rubric:: Base functions The :class:`Factory` class provides a few methods for getting objects; the usual way being to simply call the class: @@ -350,7 +350,7 @@ Attributes and methods according to :obj:`create`. - **Extension points:** + .. rubric:: Extension points A :class:`Factory` subclass may override a couple of class methods to adapt its behaviour: @@ -449,7 +449,7 @@ Attributes and methods values, for instance. - **Advanced functions:** + .. rubric:: Advanced functions: .. classmethod:: reset_sequence(cls, value=None, force=False) From 2a87857a659faa264ed6a0e1d3d28a72f7a5a75d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rapha=C3=ABl=20Barrois?= Date: Mon, 12 Oct 2020 00:31:33 +0200 Subject: [PATCH 7/7] Rename a StepBuilder instance to step_builder The variable is a StepBuilder, not a BuildStep... --- factory/base.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/factory/base.py b/factory/base.py index 8094abf7..69c42edd 100644 --- a/factory/base.py +++ b/factory/base.py @@ -576,8 +576,8 @@ def _generate(cls, strategy, params): "Ensure %(f)s.Meta.model is set and %(f)s.Meta.abstract " "is either not set or False." % dict(f=cls.__name__)) - step = builder.StepBuilder(cls._meta, params, strategy) - return step.build() + step_builder = builder.StepBuilder(cls._meta, params, strategy) + return step_builder.build() @classmethod def _after_postgeneration(cls, instance, create, results=None):