Opened 10 years ago

Closed 10 years ago

Last modified 9 years ago

#23226 closed Bug (fixed)

Migrations generated by python2 break when using python3 to migrate

Reported by: morshed.nader@… Owned by: Andrew Godwin
Component: Migrations Version: 1.7-rc-2
Severity: Release blocker Keywords:
Cc: Simon Charette, cmawebsite@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

It seems that the use of byte strings in the files generated by python2 manage.py makemigrations <module> cause migrations to break when running python3 manage.py migrate. This seems to be caused by the use of byte strings in the model field names, as replacing all instances of b'' with '' fixes the issue.

I have created a simple demo project that will cause an error to be throw during migrations. This includes migrations already generated by Python2 + Django-1.7rc2 + makemigrations:

https://github.com/naderm/django-migrations-test

Which throws the exception:

$ python3 manage.py migrate

...

django.db.models.fields.FieldDoesNotExist: ModelTest has no field named b'season'

$ python2 manage.py migrate
Operations to perform:
  Apply all migrations: admin, contenttypes, migratemodule, auth, sessions
Running migrations:
  Applying migratemodule.0001_initial... OK
  Applying sessions.0001_initial... OK

The case here uses a unique_together constraint that causes the exception to be thrown, but I have also seen the same error generated by the use of foreign keys and many to many relations.

Change History (19)

comment:1 by morshed.nader@…, 10 years ago

I should add that the exact version of python used were 2.7.7 and 3.4.1.

comment:2 by Simon Charette, 10 years ago

Triage Stage: UnreviewedAccepted

I'm getting the following traceback on Python 3.3.1 with 1.7r2 and master.

Operations to perform:
  Apply all migrations: migratemodule, admin, auth, contenttypes, sessions
Running migrations:
  Applying contenttypes.0001_initial... OK
  Applying auth.0001_initial... OK
  Applying admin.0001_initial... OK
  Applying migratemodule.0001_initial...Traceback (most recent call last):
  File "/home/simon/workspace/django/django/db/models/options.py", line 414, in get_field_by_name
    return self._name_map[name]
AttributeError: 'Options' object has no attribute '_name_map'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/simon/workspace/django/django/db/models/options.py", line 417, in get_field_by_name
    return cache[name]
KeyError: b'season'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "./manage.py", line 11, in <module>
    execute_from_command_line(sys.argv)
  File "/home/simon/workspace/django/django/core/management/__init__.py", line 385, in execute_from_command_line
    utility.execute()
  File "/home/simon/workspace/django/django/core/management/__init__.py", line 377, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/home/simon/workspace/django/django/core/management/base.py", line 288, in run_from_argv
    self.execute(*args, **options.__dict__)
  File "/home/simon/workspace/django/django/core/management/base.py", line 337, in execute
    output = self.handle(*args, **options)
  File "/home/simon/workspace/django/django/core/management/commands/migrate.py", line 160, in handle
    executor.migrate(targets, plan, fake=options.get("fake", False))
  File "/home/simon/workspace/django/django/db/migrations/executor.py", line 63, in migrate
    self.apply_migration(migration, fake=fake)
  File "/home/simon/workspace/django/django/db/migrations/executor.py", line 97, in apply_migration
    migration.apply(project_state, schema_editor)
  File "/home/simon/workspace/django/django/db/migrations/migration.py", line 107, in apply
    operation.database_forwards(self.app_label, schema_editor, project_state, new_state)
  File "/home/simon/workspace/django/django/db/migrations/operations/models.py", line 241, in database_forwards
    getattr(new_model._meta, self.option_name, set()),
  File "/home/simon/workspace/django/django/db/backends/sqlite3/schema.py", line 188, in alter_unique_together
    self._remake_table(model, override_uniques=new_unique_together)
  File "/home/simon/workspace/django/django/db/backends/sqlite3/schema.py", line 121, in _remake_table
    self.create_model(temp_model)
  File "/home/simon/workspace/django/django/db/backends/schema.py", line 261, in create_model
    columns = [model._meta.get_field_by_name(field)[0].column for field in fields]
  File "/home/simon/workspace/django/django/db/backends/schema.py", line 261, in <listcomp>
    columns = [model._meta.get_field_by_name(field)[0].column for field in fields]
  File "/home/simon/workspace/django/django/db/models/options.py", line 420, in get_field_by_name
    % (self.object_name, name))
django.db.models.fields.FieldDoesNotExist: ModelTest has no field named b'season'

Thanks to PEP3134 it looks like it might be related to something else.

comment:3 by Baptiste Mispelon, 10 years ago

I can reproduce with this simple model:

from django.db import models

class Foo(models.Model):
    foo = models.IntegerField()
    bar = models.IntegerField()

    class Meta:
        unique_together = ('foo', 'bar')

Running python2 manage.py makemigrations && python3 manage.py migrate yields the traceback from the above comment.

comment:4 by Simon Charette, 10 years ago

I guess we didn't hit this before because most of the code bases that intent to support both Python 2 and Python 3 use unicode literals (from __future__ import unicode_literals) which would have made the unique_together fields unicode instances on Python 2 that would have been serialized without the problematic b prefix.

Can you confirm your migration was generated on Python 2 from a model file without the from __future__ import unicode_literals import?

I'm unsure if we should document this as a caveat of make sure that all string defined at the model module level are always converted to six.text_type. That includes some of the model field options and many of the Model Meta options.

Last edited 10 years ago by Simon Charette (previous) (diff)

comment:5 by anonymous, 10 years ago

I can confirm that unicode_literals were indeed not imported, as well as that adding that statement fixes the migrations generated by python2 manage.py makemigrations.

comment:6 by Andrew Godwin, 10 years ago

Owner: changed from nobody to Andrew Godwin
Status: newassigned

I'm in two minds as to whether we should just force-convert everything to Unicode in migration files (which obviously is technically changing data, and might break with strings intended for humans to read), or document that you should be using unicode for your models files.

There's a potential third outcome here where we fix the state stuff to convert all model names/field names to unicode, which would solve most immediate problems. I'll investigate.

in reply to:  6 comment:7 by Simon Charette, 10 years ago

Replying to andrewgodwin:

I'm in two minds as to whether we should just force-convert everything to Unicode in migration files (which obviously is technically changing data, and might break with strings intended for humans to read), or document that you should be using unicode for your models files.

There's a potential third outcome here where we fix the state stuff to convert all model names/field names to unicode, which would solve most immediate problems. I'll investigate.

The thing is that it's hard to which part needs fixing to solve the immediate problem. Take order_with_respect_to for example:

# models.py
from django.db import models

class A(models.Model): pass

class B(models.Model):
    a = models.ForeignKey(A)

    class Meta:
        order_with_respect_to = 'a'
# migrations/0001_initial.py (generated on Py2)
from __future__ import unicode_literals

from django.db import models, migrations


class Migration(migrations.Migration):

    dependencies = [ 
    ]   

    operations = [ 
        migrations.CreateModel(
            name='A',
            fields=[
                ('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)),
            ],
            options={
            },
            bases=(models.Model,),
        ),
        migrations.CreateModel(
            name='B',
            fields=[
                ('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)),
                ('a', models.ForeignKey(to='app.A')),
            ],
            options={
            },
            bases=(models.Model,),
        ),
        migrations.AlterOrderWithRespectTo(
            name='b',
            order_with_respect_to=b'a',
        ),
    ]

It would also fail with a similar exception when migration on Python 3

Operations to perform:
  Apply all migrations: auth, app, sessions, contenttypes, migratemodule, admin
Running migrations:
  Applying app.0001_initial...Traceback (most recent call last):
  File "./manage.py", line 11, in <module>
    execute_from_command_line(sys.argv)
  File "/home/simon/workspace/django/django/core/management/__init__.py", line 330, in execute_from_command_line
    utility.execute()
  File "/home/simon/workspace/django/django/core/management/__init__.py", line 322, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/home/simon/workspace/django/django/core/management/base.py", line 363, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/home/simon/workspace/django/django/core/management/base.py", line 413, in execute
    output = self.handle(*args, **options)
  File "/home/simon/workspace/django/django/core/management/commands/migrate.py", line 160, in handle
    executor.migrate(targets, plan, fake=options.get("fake", False))
  File "/home/simon/workspace/django/django/db/migrations/executor.py", line 63, in migrate
    self.apply_migration(migration, fake=fake)
  File "/home/simon/workspace/django/django/db/migrations/executor.py", line 91, in apply_migration
    if self.detect_soft_applied(migration):
  File "/home/simon/workspace/django/django/db/migrations/executor.py", line 135, in detect_soft_applied
    apps = project_state.render()
  File "/home/simon/workspace/django/django/db/migrations/state.py", line 67, in render
    model.render(self.apps)
  File "/home/simon/workspace/django/django/db/migrations/state.py", line 292, in render
    body,
  File "/home/simon/workspace/django/django/db/models/base.py", line 287, in __new__
    new_class._prepare()
  File "/home/simon/workspace/django/django/db/models/base.py", line 312, in _prepare
    opts._prepare(cls)
  File "/home/simon/workspace/django/django/db/models/options.py", line 170, in _prepare
    self.order_with_respect_to = self.get_field(self.order_with_respect_to)
  File "/home/simon/workspace/django/django/db/models/options.py", line 388, in get_field
    raise FieldDoesNotExist('%s has no field named %r' % (self.object_name, name))
django.db.models.fields.FieldDoesNotExist: B has no field named b'a'

I think that all field and model options referring to field names somehow should be converted to six.text_type. Idem with dict based options (such as limit_choices_to) that might be passed as **kwargs or string based ones that might end up as a kwarg name since callable(**{b'key': 'value'}) isn't allowed on Python 3 (While both bytes and unicode kwarg is allowed on Python 2).

Since those criteria covers most of the existing field and model options I suggest we convert all of them (during deconstruction) instead of trying to guess every possible places where this option might be referenced.

This alternative might also help mitigate issues users on Python 2 creating migrations and then later trying to move to Python 3 would encounter.

comment:8 by Simon Charette, 10 years ago

Cc: Simon Charette added

comment:9 by Collin Anderson, 10 years ago

Cc: cmawebsite@… added

comment:10 by Andrew Godwin <andrew@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

In 5257b85ab8a4a86b24005e3ca8c542ede44b0687:

Fixed #23226: Model options appearing as bytes type in migrations

comment:11 by Andrew Godwin <andrew@…>, 10 years ago

In bbcd86e264077fa3796dc584667ebf51313f485c:

[1.7.x] Fixed #23226: Model options appearing as bytes type in migrations

comment:12 by Andrew Godwin, 10 years ago

So, I have opted to fix this by making everything in the model options state a text type when creating it from live models; this fixes the problem for me for every model option I can think of (I've got a big file with them all set to various things)

If someone can replicate the issue _in a crashing manner_ then please re-open; I'm fine with leaving things like choices= set to bytes types, as this is actually what the models file says and doing any conversion there could lead to issues.

comment:13 by Tim Graham, 10 years ago

The tests fail to run on Python 2:

  File "/home/tim/code/django/django/db/migrations/state.py", line 259, in force_text_recursive
    return six.text_type(value)
UnicodeDecodeError: 'ascii' codec can't decode byte 0xc3 in position 0: ordinal not in range(128)

The failing string is from tests/model_regress/models.py: verbose_name = "\xc3\x85ngstr\xc3\xb6m's Articles". This file doesn't have from __future__ import unicode_literals, but adding it fixes the crash. Is it the correct fix or is the change for this ticket going to cause compatibility problems?

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

In 57b60f9f93f4aa660c191c54ebb30ce44e54f236:

Added a missing unicode_literals that caused a test failure after refs #23226.

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

In 796030590a851b1e75fee9172661abf0d6390cc3:

[1.7.x] Added a missing unicode_literals that caused a test failure after refs #23226.

Backport of 57b60f9f93 from master

comment:16 by conor, 9 years ago

Sorry if I'm a little confused about this. Was it supposed to be fixed? I'm getting it now on 1.9.6, trying to port from 2 -> 3:

  File "/usr/local/lib/python3.4/dist-packages/django/db/models/fields/related.py", line 594, in foreign_related_fields
    return tuple(rhs_field for lhs_field, rhs_field in self.related_fields if rhs_field)
  File "/usr/local/lib/python3.4/dist-packages/django/db/models/fields/related.py", line 581, in related_fields
    self._related_fields = self.resolve_related_fields()
  File "/usr/local/lib/python3.4/dist-packages/django/db/models/fields/related.py", line 574, in resolve_related_fields
    else self.remote_field.model._meta.get_field(to_field_name))
  File "/usr/local/lib/python3.4/dist-packages/django/db/models/options.py", line 582, in get_field
    raise FieldDoesNotExist('%s has no field named %r' % (self.object_name, field_name))
django.core.exceptions.FieldDoesNotExist: Subject has no field named b'name'

Is there some step I'm missing (I've added unicode literals to the models.py file)? I can't see anything in the porting docs.

Just to add: removing the existing migrations and making a new one allowed me to migrate just fine.

Last edited 9 years ago by conor (previous) (diff)

comment:17 by Tim Graham, 9 years ago

You need to add from __future__ import unicode_literals to your models.py before you generate the migration.

comment:18 by Damon Timm, 9 years ago

Related to this - I have an older project with a migration that was created and uses the b'string value' approach; now, when running python manage.py makemigrations a new migration is generated that only removes the b and just leaves it as string value.

Initial:

('object_pk', models.PositiveIntegerField(verbose_name=b'object ID'))

New migration:

migrations.AlterField(
    model_name='mymodel',
    name='object_pk',
    field=models.PositiveIntegerField(verbose_name='object ID'),
),

My question is: is it necessary to create this migration just to change from bytes to str ? Or can I safely just edit my _initial_ migration and remove the b ?

Thanks

comment:19 by Tim Graham, 9 years ago

Editing migrations to fix this issue is safe.

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