migrations created with python2 break with python3

[Brian May]

As an example, my model has:

@python_2_unicode_compatible                                                     
class Institute(models.Model):
    ...
    delegates = models.ManyToManyField(                                          
        Person, related_name='delegate_for',                                     
        blank=True, null=True, through='InstituteDelegate')
    ...

makemigrations running under Python2 turns this into:

migrations.AddField(
    model_name='institute',
    name='delegates',
    field=models.ManyToManyField(related_name=b'delegate_for', null=True, through='karaage.InstituteDelegate', to='karaage.Person', blank=True),
    preserve_default=True,
),

This works fine under Python2, where b'' == ''.

Under Python3 however, I get:

Traceback (most recent call last):
  File "./manage.py", line 7, in <module>
    management.execute_from_command_line()
  File "/usr/lib/python3/dist-packages/django/core/management/__init__.py", line 385, in execute_from_command_line
    utility.execute()
  File "/usr/lib/python3/dist-packages/django/core/management/__init__.py", line 377, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/usr/lib/python3/dist-packages/django/core/management/commands/test.py", line 50, in run_from_argv
    super(Command, self).run_from_argv(argv)
  File "/usr/lib/python3/dist-packages/django/core/management/base.py", line 288, in run_from_argv
    self.execute(*args, **options.__dict__)
  File "/usr/lib/python3/dist-packages/django/core/management/commands/test.py", line 71, in execute
    super(Command, self).execute(*args, **options)
  File "/usr/lib/python3/dist-packages/django/core/management/base.py", line 338, in execute
    output = self.handle(*args, **options)
  File "/usr/lib/python3/dist-packages/django/core/management/commands/test.py", line 88, in handle
    failures = test_runner.run_tests(test_labels)
  File "/usr/lib/python3/dist-packages/django/test/runner.py", line 147, in run_tests
    old_config = self.setup_databases()
  File "/usr/lib/python3/dist-packages/django/test/runner.py", line 109, in setup_databases
    return setup_databases(self.verbosity, self.interactive, **kwargs)
  File "/usr/lib/python3/dist-packages/django/test/runner.py", line 299, in setup_databases
    serialize=connection.settings_dict.get("TEST_SERIALIZE", True),
  File "/usr/lib/python3/dist-packages/django/db/backends/creation.py", line 374, in create_test_db
    test_flush=True,
  File "/usr/lib/python3/dist-packages/django/core/management/__init__.py", line 115, in call_command
    return klass.execute(*args, **defaults)
  File "/usr/lib/python3/dist-packages/django/core/management/base.py", line 338, in execute
    output = self.handle(*args, **options)
  File "/usr/lib/python3/dist-packages/django/core/management/commands/migrate.py", line 160, in handle
    executor.migrate(targets, plan, fake=options.get("fake", False))
  File "/usr/lib/python3/dist-packages/django/db/migrations/executor.py", line 63, in migrate
    self.apply_migration(migration, fake=fake)
  File "/usr/lib/python3/dist-packages/django/db/migrations/executor.py", line 91, in apply_migration
    if self.detect_soft_applied(migration):
  File "/usr/lib/python3/dist-packages/django/db/migrations/executor.py", line 135, in detect_soft_applied
    apps = project_state.render()
  File "/usr/lib/python3/dist-packages/django/db/migrations/state.py", line 67, in render
    model.render(self.apps)
  File "/usr/lib/python3/dist-packages/django/db/migrations/state.py", line 311, in render
    body,
  File "/usr/lib/python3/dist-packages/django/db/models/base.py", line 171, in __new__
    new_class.add_to_class(obj_name, obj)
  File "/usr/lib/python3/dist-packages/django/db/models/base.py", line 300, in add_to_class
    value.contribute_to_class(cls, name)
  File "/usr/lib/python3/dist-packages/django/db/models/fields/related.py", line 2239, in contribute_to_class
    super(ManyToManyField, self).contribute_to_class(cls, name)
  File "/usr/lib/python3/dist-packages/django/db/models/fields/related.py", line 269, in contribute_to_class
    'app_label': cls._meta.app_label.lower()
TypeError: unsupported operand type(s) for %: 'bytes' and 'dict'

If I run makemigrations under Python 3, it does the correct thing:

migrations.AddField(
    model_name='institute',
    name='delegates',
    field=models.ManyToManyField(related_name='delegate_for', null=True, through='karaage.InstituteDelegate', to='karaage.Person', blank=True),
    preserve_default=True,
),

If I look at a diff, it seems that just about everywhere a string (in the above example, through and to appear to be ok) was used it is incorrectly quoted in the migration as a byte string.

Apologies if this is already reported or documented somewhere, I looked but couldn't see anything.

I have a vague recollection this may not have been a problem in the RC1 release, not absolutely sure now.

[Brian May]

Urgh. Missed out on ticket 23456 by one :-(.

[Simon Charette]

Managed to reproduce with the latest 1.7 release.

I guess a fix similar to 5257b85ab8a4a86b24005e3ca8c542ede44b0687 (#23226) applied to field options should do here.

[Tim Graham]

I don't think it's a good idea to coerce field options. For example, you might have models.BinaryField(default=b""). How about instead documenting this issue and recommending adding unicode_literals as appropriate. For example, I did this for contrib/contenttypes/models.py when adding migrations there in eb8600a65673649ea15ed18d17127f741807ac8b.

[Brian May]

Replying to timgraham:

It seems unfortunate that this means altering the previously working source code, however I very much agree with your reasoning.

I would suggest that assertions be added to ensure that certain fields, e.g. 'related_name' are always unicode strings (u"" for Python2/3 or "" for Python3). i.e. ensure that broken code breaks for Python 2 as well as Python 3, and possibly with a more consistent and friendly message.

Obviously this would break code that appears to be fine under Python 2.

[Claude Paroz]

Sure, for most of the fields, the binary prefix makes no sense. I think that the unicode_literals recommendation and the conversion of some fields are no antagonistic solutions.

[Markus Holtermann]

I added a pull-request: ​https://github.com/django/django/pull/3276

[Tim Graham <timograham@…>]

In 45bd7b3bd9008941580c100b9fc7361e3ff3ff0d:

Fixed #23455 -- Forced related_name to be a unicode string during deconstruction.

[Tim Graham <timograham@…>]

In e8a08514de6b97de1fce13b6c6b24cf72d1d60d9:

[1.7.x] Fixed #23455 -- Forced related_name to be a unicode string during deconstruction.

Backport of 45bd7b3bd9 from master

[Carl Meyer]

I think this change should probably be reverted, and we should avoid any auto-conversion that introduces a difference between real models and migration models. See further discussion on #23982.

[Carl Meyer]

Thanks to feedback from charettes on ​https://github.com/django/django/pull/3718, I've realized that this needs to be fixed somehow, we can't only revert the existing fix - the error message is too obscure.

But I think a better fix would be for Django to just always convert related_name to text, not to do it specifically on deconstruction for migrations. Rationale:

1) In general, the goal should be as much as possible to maintain uniform behavior between Python 2 and Python 3.

2) With the current fix here, the situation is "on Python 2, related_name can be bytes or text; on Python 3 it must be text." Migrations generated on Python 2 and run on Python 3 expose this difference. So currently we work around it by ensuring that Python 2 stores related_name as text in migrations.

3) With my proposed fix, we maintain better consistency. Migrations always preserves bytes/text, and Django always (regardless of Python version) supports setting related_name as either bytes or text, and converts it to text internally (since it needs that for interpolation).

The effect on generated migrations in the end will be the same as the current fix; since related_name is converted to text, it will still be deconstructed as text. The most noticeable difference is that with my proposed fix, migrations generated on Django 1.7.0 under Python 2 won't need to be manually edited to work under Python 3, they'll just work.

Is this reasonable, or am I missing something?

[Simon Charette]

It seems reasonable to me.

I also think migrations should always preserve bytes/text and this shouldn't be special cased at the migration level but handled at the model one.

[Carl Meyer <carl@…>]

In 8aaf51f94c70e3cfcd2c75a0be1b6f55049d82d8:

Revert "Fixed #23455 -- Forced related_name to be a unicode string during deconstruction."

This reverts commit 45bd7b3bd9008941580c100b9fc7361e3ff3ff0d.

[Carl Meyer <carl@…>]

In c72eb80d114fb5d90bd21b5549e8abd0bbd17f99:

Fixed #23455 -- Accept either bytes or text for related_name, convert to text.

[Carl Meyer <carl@…>]

In f8b4cf4022aae460d7bbfb9510858baf37ae6c15:

[1.7.x] Revert "Fixed #23455 -- Forced related_name to be a unicode string during deconstruction."

This reverts commit 45bd7b3bd9008941580c100b9fc7361e3ff3ff0d.

This is a backport of 8aaf51f94c70e3cfcd2c75a0be1b6f55049d82d8 from master.

[Carl Meyer <carl@…>]

In 0a8b911582eee85ea6da0d40dd2c7b12de5a78b2:

[1.7.x] Fixed #23455 -- Accept either bytes or text for related_name, convert to text.

Backport of c72eb80d114fb5d90bd21b5549e8abd0bbd17f99 from master.