Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DjangoModelFactory's "_setup_next_sequence" assumes that pk is an integer #57

Closed
AndrewIngram opened this issue Apr 22, 2013 · 16 comments
Closed

Comments

@AndrewIngram
Copy link

Offending code is here:

    @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:
            return 1

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:

code = factory.Sequence(lambda n: str(n).zfill(3))

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.

@rbarrois
Copy link
Member

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

@agriffis
Copy link

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

@rbarrois
Copy link
Member

@agriffis That's indeed a great idea, and should work much better for all uses.
Could you make that into a pull request?

@rbarrois
Copy link
Member

rbarrois commented Jun 9, 2013

Fixed in 83461f0.

@rbarrois rbarrois closed this as completed Jun 9, 2013
rbarrois added a commit that referenced this issue Jun 9, 2013
@wieczorek1990
Copy link

wieczorek1990 commented Feb 12, 2021

Why did factory.DjangoModelFactory._setup_next_sequence got deleted?

@francoisfreitag
Copy link
Member

What do you mean, got deleted? It is still on master:

factory_boy/factory/base.py

Lines 436 to 443 in ec4211a

@classmethod
def _setup_next_sequence(cls):
"""Set up an initial sequence value for Sequence attributes.
Returns:
int: the first available ID to use for instances of this factory.
"""
return 0

@wieczorek1990
Copy link

wieczorek1990 commented Feb 12, 2021

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()

@francoisfreitag
Copy link
Member

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',)

@wieczorek1990
Copy link

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.

@francoisfreitag
Copy link
Member

Thanks for precising the use case.
The django factory is calling _setup_next_sequence based on my experiment in #57 (comment). I don’t understand the issue you are facing. Can you provide a minimal code sample that produces the issue?

@wieczorek1990
Copy link

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

@francoisfreitag
Copy link
Member

This does not call _setup_next_sequence?

@francoisfreitag
Copy link
Member

francoisfreitag commented Feb 16, 2021

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)

@wieczorek1990
Copy link

Sorry for bad example.

I would like the following test to pass.

wieczorek1990@8f57e69

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?

@rbarrois
Copy link
Member

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...

@wieczorek1990
Copy link

Thanks. After all I think like this is specific to the use case and everybody should write his own code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants