Opened 6 years ago

Closed 6 years ago

Last modified 4 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 Changed 6 years ago by morshed.nader@…

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

comment:2 Changed 6 years ago by Simon Charette

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 Changed 6 years ago by Baptiste Mispelon

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 Changed 6 years ago by Simon Charette

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 6 years ago by Simon Charette (previous) (diff)

comment:5 Changed 6 years ago by anonymous

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 Changed 6 years ago by Andrew Godwin

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.

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

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 Changed 6 years ago by Simon Charette

Cc: Simon Charette added

comment:9 Changed 6 years ago by Collin Anderson

Cc: cmawebsite@… added

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

Resolution: fixed
Status: assignedclosed

In 5257b85ab8a4a86b24005e3ca8c542ede44b0687:

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

comment:11 Changed 6 years ago by Andrew Godwin <andrew@…>

In bbcd86e264077fa3796dc584667ebf51313f485c:

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

comment:12 Changed 6 years ago by Andrew Godwin

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 Changed 6 years ago by Tim Graham

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 Changed 6 years ago by Tim Graham <timograham@…>

In 57b60f9f93f4aa660c191c54ebb30ce44e54f236:

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

comment:15 Changed 6 years ago by Tim Graham <timograham@…>

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 Changed 4 years ago by conor

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 4 years ago by conor (previous) (diff)

comment:17 Changed 4 years ago by Tim Graham

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

comment:18 Changed 4 years ago by Damon Timm

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 Changed 4 years ago by Tim Graham

Editing migrations to fix this issue is safe.

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