Opened 10 years ago

Closed 10 years ago

#24483 closed Bug (fixed)

keepdb migrations break choices as generators

Reported by: David Szotten Owned by: Tim Graham <timograham@…>
Component: Migrations Version: 1.8beta2
Severity: Normal Keywords: migrations test keepdb
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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

Change History (10)

comment:1 by David Szotten, 10 years ago

Has patch: set
Owner: changed from nobody to David Szotten
Status: newassigned
Type: UncategorizedBug

comment:2 by Josh Smeaton, 10 years ago

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

comment:3 by Josh Smeaton, 10 years ago

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

Last edited 10 years ago by Tim Graham (previous) (diff)

comment:4 by David Szotten, 10 years ago

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 )

comment:5 by Tim Graham, 10 years ago

Owner: changed from David Szotten to Tim Graham
Triage Stage: UnreviewedAccepted

comment:6 by Tim Graham <timograham@…>, 10 years ago

In b4a56ed:

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

comment:7 by Tim Graham <timograham@…>, 10 years ago

In 247251c:

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

Backport of b4a56ed4f55502239cb11b57f0fa75baa0a97640 from master

comment:8 by Tim Graham, 10 years ago

Owner: Tim Graham removed
Patch needs improvement: set
Status: assignednew

comment:9 by Tim Graham, 10 years ago

Patch needs improvement: unset

comment:10 by Tim Graham <timograham@…>, 10 years ago

Owner: set to Tim Graham <timograham@…>
Resolution: fixed
Status: newclosed

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.

Note: See TracTickets for help on using tickets.
Back to Top