Skip to content

Commit

Permalink
Remove automagic pk-based sequence setup
Browse files Browse the repository at this point in the history
Related to issues #78, #92, #103, #111, #153, #170

The default value of all sequences is now 0; the automagic
``_setup_next_sequence`` behavior of Django/SQLAlchemy has been removed.

This feature's only goal was to allow the following scenario:

1. Run a Python script that uses MyFactory.create() a couple of times
   (with a unique field based on the sequence counter)
2. Run the same Python script a second time

Without the magical ``_setup_next_sequence``, the Sequence counter would be set
to 0 at the beginning of each script run, so both runs would generate objects
with the same values for the unique field ; thus conflicting and crashing.

The above behavior having only a very limited use and bringing various
issues (hitting the database on ``build()``, problems with non-integer
or composite primary key columns, ...), it has been removed.

It could still be emulated through custom ``_setup_next_sequence``
methods, or by calling ``MyFactory.reset_sequence()``.
  • Loading branch information
rbarrois committed Nov 16, 2014
1 parent 827af8f commit 13d310f
Show file tree
Hide file tree
Showing 7 changed files with 50 additions and 74 deletions.
13 changes: 12 additions & 1 deletion docs/changelog.rst
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,17 @@ ChangeLog
=========


.. _v2.5.0:

2.5.0 (master)
--------------

*Deprecation:*

- Remove deprecated features from :ref:`v2.4.0`
- Remove the auto-magical sequence setup (based on the latest primary key value in the database) for Django and SQLAlchemy;
this relates to issues :issue:`170`, :issue:`153`, :issue:`111`, :issue:`103`, :issue:`92`, :issue:`78`.

.. _v2.4.1:

2.4.1 (2014-06-23)
Expand All @@ -19,7 +30,7 @@ ChangeLog
*New:*

- Add support for :attr:`factory.fuzzy.FuzzyInteger.step`, thanks to `ilya-pirogov <https://github.com/ilya-pirogov>`_ (:issue:`120`)
- Add :meth:`~factory.django.mute_signals` decorator to temporarily disable some signals, thanks to `ilya-pirogov <https://github.com>`_ (:issue:`122`)
- Add :meth:`~factory.django.mute_signals` decorator to temporarily disable some signals, thanks to `ilya-pirogov <https://github.com/ilya-pirogov>`_ (:issue:`122`)
- Add :class:`~factory.fuzzy.FuzzyFloat` (:issue:`124`)
- Declare target model and other non-declaration fields in a ``class Meta`` section.

Expand Down
2 changes: 0 additions & 2 deletions docs/orms.rst
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ 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() <django.db.models.query.QuerySet.create>`
* :func:`~factory.Factory._setup_next_sequence()` selects the next unused primary key value
* When using :class:`~factory.RelatedFactory` or :class:`~factory.PostGeneration`
attributes, the base object will be :meth:`saved <django.db.models.Model.save>`
once all post-generation hooks have run.
Expand Down Expand Up @@ -284,7 +283,6 @@ To work, this class needs an `SQLAlchemy`_ session object affected to the :attr:
This class provides the following features:

* :func:`~factory.Factory.create()` uses :meth:`sqlalchemy.orm.session.Session.add`
* :func:`~factory.Factory._setup_next_sequence()` selects the next unused primary key value

.. attribute:: FACTORY_SESSION

Expand Down
12 changes: 0 additions & 12 deletions factory/alchemy.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,18 +43,6 @@ class Meta:
'FACTORY_SESSION': 'sqlalchemy_session',
})

@classmethod
def _setup_next_sequence(cls, *args, **kwargs):
"""Compute the next available PK, based on the 'pk' database field."""
session = cls._meta.sqlalchemy_session
model = cls._meta.model
pk = getattr(model, model.__mapper__.primary_key[0].name)

This comment has been minimized.

Copy link
@lazytype

lazytype Jan 5, 2015

The import of max from sqlalchemy.sql.functions in this file is no longer necessary after this deletion :)

max_pk = session.query(max(pk)).one()[0]
if isinstance(max_pk, int):
return max_pk + 1 if max_pk else 1
else:
return 1

@classmethod
def _create(cls, model_class, *args, **kwargs):
"""Create an instance of the model, and save it to the database."""
Expand Down
15 changes: 0 additions & 15 deletions factory/django.py
Original file line number Diff line number Diff line change
Expand Up @@ -109,21 +109,6 @@ def _get_manager(cls, model_class):
except AttributeError:
return model_class.objects

@classmethod
def _setup_next_sequence(cls):
"""Compute the next available PK, based on the 'pk' database field."""

model = cls._get_model_class() # pylint: disable=E1101
manager = cls._get_manager(model)

try:
return 1 + manager.values_list('pk', flat=True
).order_by('-pk')[0]
except (IndexError, TypeError):
# IndexError: No instance exist yet
# TypeError: pk isn't an integer type
return 1

@classmethod
def _get_or_create(cls, model_class, *args, **kwargs):
"""Create an instance of the model through objects.get_or_create."""
Expand Down
22 changes: 11 additions & 11 deletions tests/test_alchemy.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,18 +88,18 @@ def test_pk_creation(self):

StandardFactory.reset_sequence()
std2 = StandardFactory.create()
self.assertEqual('foo2', std2.foo)
self.assertEqual(2, std2.id)
self.assertEqual('foo0', std2.foo)
self.assertEqual(0, std2.id)

def test_pk_force_value(self):
std1 = StandardFactory.create(id=10)
self.assertEqual('foo1', std1.foo) # sequence was set before pk
self.assertEqual('foo1', std1.foo) # sequence and pk are unrelated
self.assertEqual(10, std1.id)

StandardFactory.reset_sequence()
std2 = StandardFactory.create()
self.assertEqual('foo11', std2.foo)
self.assertEqual(11, std2.id)
self.assertEqual('foo0', std2.foo) # Sequence doesn't care about pk
self.assertEqual(0, std2.id)


@unittest.skipIf(sqlalchemy is None, "SQLalchemy not installed.")
Expand All @@ -111,27 +111,27 @@ def setUp(self):

def test_first(self):
nonint = NonIntegerPkFactory.build()
self.assertEqual('foo1', nonint.id)
self.assertEqual('foo0', nonint.id)

def test_many(self):
nonint1 = NonIntegerPkFactory.build()
nonint2 = NonIntegerPkFactory.build()

self.assertEqual('foo1', nonint1.id)
self.assertEqual('foo2', nonint2.id)
self.assertEqual('foo0', nonint1.id)
self.assertEqual('foo1', nonint2.id)

def test_creation(self):
nonint1 = NonIntegerPkFactory.create()
self.assertEqual('foo1', nonint1.id)
self.assertEqual('foo0', nonint1.id)

NonIntegerPkFactory.reset_sequence()
nonint2 = NonIntegerPkFactory.build()
self.assertEqual('foo1', nonint2.id)
self.assertEqual('foo0', nonint2.id)

def test_force_pk(self):
nonint1 = NonIntegerPkFactory.create(id='foo10')
self.assertEqual('foo10', nonint1.id)

NonIntegerPkFactory.reset_sequence()
nonint2 = NonIntegerPkFactory.create()
self.assertEqual('foo1', nonint2.id)
self.assertEqual('foo0', nonint2.id)
40 changes: 20 additions & 20 deletions tests/test_django.py
Original file line number Diff line number Diff line change
Expand Up @@ -174,32 +174,32 @@ def setUp(self):

def test_pk_first(self):
std = StandardFactory.build()
self.assertEqual('foo1', std.foo)
self.assertEqual('foo0', std.foo)

def test_pk_many(self):
std1 = StandardFactory.build()
std2 = StandardFactory.build()
self.assertEqual('foo1', std1.foo)
self.assertEqual('foo2', std2.foo)
self.assertEqual('foo0', std1.foo)
self.assertEqual('foo1', std2.foo)

def test_pk_creation(self):
std1 = StandardFactory.create()
self.assertEqual('foo1', std1.foo)
self.assertEqual('foo0', std1.foo)
self.assertEqual(1, std1.pk)

StandardFactory.reset_sequence()
std2 = StandardFactory.create()
self.assertEqual('foo2', std2.foo)
self.assertEqual('foo0', std2.foo)
self.assertEqual(2, std2.pk)

def test_pk_force_value(self):
std1 = StandardFactory.create(pk=10)
self.assertEqual('foo1', std1.foo) # sequence was set before pk
self.assertEqual('foo0', std1.foo) # sequence is unrelated to pk
self.assertEqual(10, std1.pk)

StandardFactory.reset_sequence()
std2 = StandardFactory.create()
self.assertEqual('foo11', std2.foo)
self.assertEqual('foo0', std2.foo)
self.assertEqual(11, std2.pk)


Expand All @@ -212,12 +212,12 @@ def setUp(self):
def test_no_pk(self):
std = StandardFactoryWithPKField()
self.assertIsNotNone(std.pk)
self.assertEqual('foo1', std.foo)
self.assertEqual('foo0', std.foo)

def test_force_pk(self):
std = StandardFactoryWithPKField(pk=42)
self.assertIsNotNone(std.pk)
self.assertEqual('foo1', std.foo)
self.assertEqual('foo0', std.foo)

def test_reuse_pk(self):
std1 = StandardFactoryWithPKField(foo='bar')
Expand Down Expand Up @@ -286,9 +286,9 @@ class Meta:
self.assertEqual(models.StandardModel, e1.__class__)
self.assertEqual(models.StandardSon, e2.__class__)
self.assertEqual(models.StandardModel, e3.__class__)
self.assertEqual(1, e1.foo)
self.assertEqual(2, e2.foo)
self.assertEqual(3, e3.foo)
self.assertEqual(0, e1.foo)
self.assertEqual(1, e2.foo)
self.assertEqual(2, e3.foo)


@unittest.skipIf(django is None, "Django not installed.")
Expand All @@ -299,23 +299,23 @@ def setUp(self):

def test_first(self):
nonint = NonIntegerPkFactory.build()
self.assertEqual('foo1', nonint.foo)
self.assertEqual('foo0', nonint.foo)

def test_many(self):
nonint1 = NonIntegerPkFactory.build()
nonint2 = NonIntegerPkFactory.build()

self.assertEqual('foo1', nonint1.foo)
self.assertEqual('foo2', nonint2.foo)
self.assertEqual('foo0', nonint1.foo)
self.assertEqual('foo1', nonint2.foo)

def test_creation(self):
nonint1 = NonIntegerPkFactory.create()
self.assertEqual('foo1', nonint1.foo)
self.assertEqual('foo1', nonint1.pk)
self.assertEqual('foo0', nonint1.foo)
self.assertEqual('foo0', nonint1.pk)

NonIntegerPkFactory.reset_sequence()
nonint2 = NonIntegerPkFactory.build()
self.assertEqual('foo1', nonint2.foo)
self.assertEqual('foo0', nonint2.foo)

def test_force_pk(self):
nonint1 = NonIntegerPkFactory.create(pk='foo10')
Expand All @@ -324,8 +324,8 @@ def test_force_pk(self):

NonIntegerPkFactory.reset_sequence()
nonint2 = NonIntegerPkFactory.create()
self.assertEqual('foo1', nonint2.foo)
self.assertEqual('foo1', nonint2.pk)
self.assertEqual('foo0', nonint2.foo)
self.assertEqual('foo0', nonint2.pk)


@unittest.skipIf(django is None, "Django not installed.")
Expand Down
20 changes: 7 additions & 13 deletions tests/test_using.py
Original file line number Diff line number Diff line change
Expand Up @@ -1490,12 +1490,6 @@ def get_or_create(self, **kwargs):
instance.id = 2
return instance, True

def values_list(self, *args, **kwargs):
return self

def order_by(self, *args, **kwargs):
return [1]


class BetterFakeModel(object):
@classmethod
Expand Down Expand Up @@ -1618,14 +1612,14 @@ class Meta:
o1 = TestModelFactory()
o2 = TestModelFactory()

self.assertEqual('foo_2', o1.a)
self.assertEqual('foo_3', o2.a)
self.assertEqual('foo_0', o1.a)
self.assertEqual('foo_1', o2.a)

o3 = TestModelFactory.build()
o4 = TestModelFactory.build()

self.assertEqual('foo_4', o3.a)
self.assertEqual('foo_5', o4.a)
self.assertEqual('foo_2', o3.a)
self.assertEqual('foo_3', o4.a)

def test_no_get_or_create(self):
class TestModelFactory(factory.django.DjangoModelFactory):
Expand All @@ -1636,7 +1630,7 @@ class Meta:

o = TestModelFactory()
self.assertEqual(None, o._defaults)
self.assertEqual('foo_2', o.a)
self.assertEqual('foo_0', o.a)
self.assertEqual(2, o.id)

def test_get_or_create(self):
Expand All @@ -1652,7 +1646,7 @@ class Meta:

o = TestModelFactory()
self.assertEqual({'c': 3, 'd': 4}, o._defaults)
self.assertEqual('foo_2', o.a)
self.assertEqual('foo_0', o.a)
self.assertEqual(2, o.b)
self.assertEqual(3, o.c)
self.assertEqual(4, o.d)
Expand All @@ -1672,7 +1666,7 @@ class Meta:

o = TestModelFactory()
self.assertEqual({}, o._defaults)
self.assertEqual('foo_2', o.a)
self.assertEqual('foo_0', o.a)
self.assertEqual(2, o.b)
self.assertEqual(3, o.c)
self.assertEqual(4, o.d)
Expand Down

0 comments on commit 13d310f

Please sign in to comment.