keepdb migrations break choices as generators

[David Szotten]

So, not got to the bottom of this yet, but wanted to get a ticket in, in case we want to block the release on this

To reproduce:

# models.py
from django.db import models


def gen():
    for x in 'abc':
        yield (x, x)

class MyModel(models.Model):
    foo = models.CharField(choices=gen(), max_length=1)


# settings.py
DATABASES = {
    'default': {
        'ENGINE': 'django.db.backends.sqlite3',
        'NAME': os.path.join(BASE_DIR, 'db.sqlite3'),
        'TEST': {
            # make sure we use a db we can keep between tests
            'NAME': os.path.join(BASE_DIR, 'test_db.sqlite3'),
        },
    }
}

# tests.py

from django.test import TestCase

from .models import MyModel


class Test(TestCase):
    def test_choices(self):
        field = MyModel._meta.get_field('foo')
        expected = [('a', 'a'), ('b', 'b'), ('c', 'c')]
        self.assertEqual(
            list(field.choices),
            expected,
        )


# shell (note; need to run twice; only happens when re-using db)
$ ./manage.py test -v2 -k
$ ./manage.py test -v2 -k

======================================================================
FAIL: test_choices (app.tests.Test)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/david/dev/django_bugs/choices_generator/app/tests.py", line 12, in test_choices
    expected,
AssertionError: Lists differ: [] != [('a', 'a'), ('b', 'b'), ('c',...

Second list contains 3 additional elements.
First extra element 0:
('a', 'a')

- []
+ [('a', 'a'), ('b', 'b'), ('c', 'c')]

interestingly, this only breaks when verbosity is 2, i think because this is the trigger:
​https://github.com/django/django/blob/a52cd407b86a51e1badf6771e590361e24fd7155/django/core/management/commands/migrate.py#L177

[Josh Smeaton]

I'm not so sure that django really supports the use of generators for choices. The docs do say "An iterable (e.g., a list or tuple) consisting itself of iterables of exactly two items (e.g. [(A, B), (A, B) ...])" which a generator is, but I can't see the value in having it as a generator in the first place as it's going to be consumed in its entirety fairly quickly in the load process.

Does this even work on a live website? If yes, then we should probably fix it. But I doubt we'd mark it as a release blocker due to the necessary combinations of actually triggering the problem (the impact is small).

[Josh Smeaton]

https://code.djangoproject.com/ticket/3490 was specifically added to support the use of generators with choices, so we probably should find and fix the problem. Though I'm unsure whether or not this should be a blocker.

[David Szotten]

not sure what you mean by "Does this even work on a live website?" I have a generator providing choices in an app i've had in production for a good few years now (though probably using a generator mostly by accident). I noticed this issue a few days ago as i was trying out the new 1.8 beta and --keepdb to speed up my test suite.

the patch updates code that was definitely put in place to support generic iterators, and as you point out, the docs also talk about iterables.

not sure of the correct protocol, so the pr is raised against 1.8.x, not master (i might be going crazy, but every other time i look i can't find the pr listed on the ticket, so here's a direct link: ​https://github.com/django/django/pull/4326 )

[Tim Graham <timograham@…>]

In b4a56ed:

Refs #24483 -- Added a test for deconstruction of Field.choices

[Tim Graham <timograham@…>]

In 247251c:

[1.8.x] Refs #24483 -- Added a test for deconstruction of Field.choices

Backport of b4a56ed4f55502239cb11b57f0fa75baa0a97640 from master

[Tim Graham <timograham@…>]

In 80e3444:

Fixed #24483 -- Prevented keepdb from breaking with generator choices.

If Field.choices is provided as an iterator, consume it in init instead
of using itertools.tee (which ends up holding everything in memory
anyway). Fixes a bug where deconstruct() was consuming the iterator but
bypassing the call to tee.