Closed AndrewIngram closed 11 years ago
This part of the code hasn't meaningfully changed since 1.3.0, where it was:
@classmethod
def _setup_next_sequence(cls):
"""Compute the next available PK, based on the 'pk' database field."""
try:
return 1 + cls._associated_class._default_manager.values_list('pk', flat=True
).order_by('-pk')[0]
except IndexError:
return 1
The v2.0.0 changed the default type of n
in factory.Sequence
from str to int, which might lead to the problem you're seeing.
If your pk is non-numeric, the supported way of handling this case is to override the _setup_next_sequence
method:
class FooFactory(factory.DjangoModelFactory):
FACTORY_FOR = models.Foo
@classmethod
def _setup_next_sequence(cls):
return 1
I ran into this problem too. Many of my Django models have non-integer PKs. Eventually I just monkey-patched it to catch the additional exception TypeError
:
# Patch DjangoModelFactory to avoid TypeError if PK isn't an IntegerField
@classmethod
def _setup_next_sequence(cls):
"""Compute the next available PK, based on the 'pk' database field."""
model = cls._associated_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):
return 1
factory.DjangoModelFactory._setup_next_sequence = _setup_next_sequence
del _setup_next_sequence
However now that I think more about it, the code could be a bit safer by separating the exception handling. Something like this (untested):
@classmethod
def _setup_next_sequence(cls):
"""Compute the next available PK, based on the 'pk' database field."""
model = cls._associated_class # pylint: disable=E1101
manager = cls._get_manager(model)
try:
last_key = manager.values_list('pk', flat=True
).order_by('-pk')[0]
except IndexError:
return 1
if isinstance(last_key, int):
return last_key + 1
return 1
@agriffis That's indeed a great idea, and should work much better for all uses. Could you make that into a pull request?
Fixed in 83461f0ff1772f7d29936c11fa6ecf49c22ef1d8.
Why did factory.DjangoModelFactory._setup_next_sequence
got deleted?
What do you mean, got deleted? It is still on master: https://github.com/FactoryBoy/factory_boy/blob/ec4211a90cf6a6b58690d2fc7bfa2f911076e28e/factory/base.py#L436-L443
Yeah, it is in the base class, but the DjangoModelFactory
does not have an overridden version. In my case I needed the following:
from factory import django
class DjangoModelFactory(django.DjangoModelFactory):
@classmethod
def _setup_next_sequence(cls):
model = cls._meta.model
manager = cls._get_manager(model)
try:
return 1 + manager.values_list('pk', flat=True).order_by('-pk')[0]
except IndexError:
return 0
except TypeError:
return 1 + manager.count()
I still don’t understand the issue. DjangoModelFactory inherits from base.Factory
, which inherits from base.BaseFactory
that defines _setup_next_sequence
.
Python 3.9.1 (default, Feb 6 2021, 06:49:13)
[GCC 10.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from factory.django import DjangoModelFactory
>>> DjangoModelFactory._setup_next_sequence
<bound method BaseFactory._setup_next_sequence of <class 'factory.django.DjangoModelFactory'>>
It should be possible to override it in subclasses. A quick edit in the test suite confirms that.
diff --git a/tests/test_django.py b/tests/test_django.py
index ad68610..e7285e7 100644
--- a/tests/test_django.py
+++ b/tests/test_django.py
@@ -49,7 +49,13 @@ class StandardFactory(factory.django.DjangoModelFactory):
foo = factory.Sequence(lambda n: "foo%d" % n)
-class StandardFactoryWithPKField(factory.django.DjangoModelFactory):
+class DjangoModelFactory(factory.django.DjangoModelFactory):
+ @classmethod
+ def _setup_next_sequence(cls):
+ breakpoint()
+ return 12
+
+class StandardFactoryWithPKField(DjangoModelFactory):
class Meta:
model = models.StandardModel
django_get_or_create = ('pk',)
Issue is when you have already some data in the database and use factories to insert fixtures, then the ids and keys need to not to start with the 0 included.
Thanks for precising the use case.
The django factory is calling _setup_next_sequence
based on my experiment in https://github.com/FactoryBoy/factory_boy/issues/57#issuecomment-778211935. I don’t understand the issue you are facing. Can you provide a minimal code sample that produces the issue?
class Entry(models.Model):
pass
class EntryFactory(factory.django.DjangoModelFactory):
class Meta:
model = 'Entry'
Entry(pk=0).save()
EntryFactory.create() # error due to not clean database
This does not call _setup_next_sequence
?
Actually, even the provided example does not fail for me, here’s the patch I’m using:
diff --git a/tests/test_django.py b/tests/test_django.py
index ad68610..97f083e 100644
--- a/tests/test_django.py
+++ b/tests/test_django.py
@@ -1015,3 +1015,15 @@ class DjangoCustomManagerTestCase(django_test.TestCase):
# Our CustomManager will remove the 'arg=' argument,
# invalid for the actual model.
ObjFactory.create(arg='invalid')
+
+ def test_57(self):
+ class MyFactory(factory.django.DjangoModelFactory):
+ class Meta:
+ model = models.StandardModel
+
+ foo = factory.Sequence(lambda n: "foo%d" % n)
+
+ standard = models.StandardModel.objects.create(pk=0, foo="4")
+ created_by_factory = MyFactory.create()
+ self.assertEqual(standard.pk, 0)
+ self.assertEqual(created_by_factory.pk, 1)
Sorry for bad example.
I would like the following test to pass.
https://github.com/wieczorek1990/factory_boy/commit/8f57e69fcb5088bfe2675a28fc9a7c64d844298a
diff --git a/tests/test_django.py b/tests/test_django.py
index 48b9432..e60ea74 100644
--- a/tests/test_django.py
+++ b/tests/test_django.py
@@ -373,6 +373,14 @@ class DjangoNonIntegerPkTestCase(django_test.TestCase):
self.assertEqual('foo0', nonint2.foo)
self.assertEqual('foo0', nonint2.pk)
+ def test_setup_next_sequence(self):
+ nonint1 = models.NonIntegerPk.objects.create(pk='foo0')
+ self.assertEqual('foo0', nonint1.foo)
+ self.assertEqual('foo0', nonint1.pk)
+ nonint2 = NonIntegerPkFactory.create()
+ self.assertEqual('foo1', nonint2.foo)
+ self.assertEqual('foo1', nonint2.pk)
+
class DjangoAbstractBaseSequenceTestCase(django_test.TestCase):
def test_auto_sequence_son(self):
Currently it fails with IntegrityError
:
python3 -m unittest tests.test_django.DjangoNonIntegerPkTestCase.test_setup_next_sequence
E
======================================================================
ERROR: test_setup_next_sequence (tests.test_django.DjangoNonIntegerPkTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/usr/local/lib/python3.9/site-packages/django/db/backends/utils.py", line 84, in _execute
return self.cursor.execute(sql, params)
File "/usr/local/lib/python3.9/site-packages/django/db/backends/sqlite3/base.py", line 413, in execute
return Database.Cursor.execute(self, query, params)
sqlite3.IntegrityError: UNIQUE constraint failed: djapp_nonintegerpk.foo
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "/Users/luke/Software/factory_boy/tests/test_django.py", line 380, in test_setup_next_sequence
nonint2 = NonIntegerPkFactory.create()
File "/Users/luke/Software/factory_boy/factory/base.py", line 528, in create
return cls._generate(enums.CREATE_STRATEGY, kwargs)
File "/Users/luke/Software/factory_boy/factory/django.py", line 120, in _generate
return super()._generate(strategy, params)
File "/Users/luke/Software/factory_boy/factory/base.py", line 465, in _generate
return step.build()
File "/Users/luke/Software/factory_boy/factory/builder.py", line 266, in build
instance = self.factory_meta.instantiate(
File "/Users/luke/Software/factory_boy/factory/base.py", line 317, in instantiate
return self.factory._create(model, *args, **kwargs)
File "/Users/luke/Software/factory_boy/factory/django.py", line 169, in _create
return manager.create(*args, **kwargs)
File "/usr/local/lib/python3.9/site-packages/django/db/models/manager.py", line 85, in manager_method
return getattr(self.get_queryset(), name)(*args, **kwargs)
File "/usr/local/lib/python3.9/site-packages/django/db/models/query.py", line 447, in create
obj.save(force_insert=True, using=self.db)
File "/usr/local/lib/python3.9/site-packages/django/db/models/base.py", line 753, in save
self.save_base(using=using, force_insert=force_insert,
File "/usr/local/lib/python3.9/site-packages/django/db/models/base.py", line 790, in save_base
updated = self._save_table(
File "/usr/local/lib/python3.9/site-packages/django/db/models/base.py", line 895, in _save_table
results = self._do_insert(cls._base_manager, using, fields, returning_fields, raw)
File "/usr/local/lib/python3.9/site-packages/django/db/models/base.py", line 933, in _do_insert
return manager._insert(
File "/usr/local/lib/python3.9/site-packages/django/db/models/manager.py", line 85, in manager_method
return getattr(self.get_queryset(), name)(*args, **kwargs)
File "/usr/local/lib/python3.9/site-packages/django/db/models/query.py", line 1254, in _insert
return query.get_compiler(using=using).execute_sql(returning_fields)
File "/usr/local/lib/python3.9/site-packages/django/db/models/sql/compiler.py", line 1397, in execute_sql
cursor.execute(sql, params)
File "/usr/local/lib/python3.9/site-packages/django/db/backends/utils.py", line 66, in execute
return self._execute_with_wrappers(sql, params, many=False, executor=self._execute)
File "/usr/local/lib/python3.9/site-packages/django/db/backends/utils.py", line 75, in _execute_with_wrappers
return executor(sql, params, many, context)
File "/usr/local/lib/python3.9/site-packages/django/db/backends/utils.py", line 84, in _execute
return self.cursor.execute(sql, params)
File "/usr/local/lib/python3.9/site-packages/django/db/utils.py", line 90, in __exit__
raise dj_exc_value.with_traceback(traceback) from exc_value
File "/usr/local/lib/python3.9/site-packages/django/db/backends/utils.py", line 84, in _execute
return self.cursor.execute(sql, params)
File "/usr/local/lib/python3.9/site-packages/django/db/backends/sqlite3/base.py", line 413, in execute
return Database.Cursor.execute(self, query, params)
django.db.utils.IntegrityError: UNIQUE constraint failed: djapp_nonintegerpk.foo
----------------------------------------------------------------------
Ran 1 test in 0.096s
FAILED (errors=1)
Basically the starting number of the sequence should be setup according to the database state.
Do you think it makes any sense?
No :)
The goal of _setup_next_sequence()
is simply to initialize the sequence counter, used in factory.Sequence()
and factory.LazyAttributeSequence()
. It holds no relation whatsoever with any database primary key — it has no knowledge thereof.
If you wish to have it start at a given value, why don't you use Factory.reset_sequence()
?
Another option is to override the _setup_next_sequence()
function in your factory to fetch a viable next ID from your database.
However, I strongly suggest letting your database generate the PK itself — otherwise, it is your job to define how your factory can choose values that are guaranteed not to conflict with those already existing in the DB.
In your example, what happens if some part of the code under test creates an entry with PK "foo2" after the factory was called? There is no way for the factory to automagically guess that it has to move on to foo3
...
Thanks. After all I think like this is specific to the use case and everybody should write his own code.
Offending code is here:
This problem didn't exist in factory_boy 1.3. My field that was using a non-integer PK is using a sequence, which worked fine previously:
I haven't dug into the code enough to know much about the changes that caused this problem, but ultimately I'd like to be able to use the sequence for the field again.