Fixed #22350 -- Consistently serialize bytes and text in migrations. #2501

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

Member

Thanks to treyhunner for the report and Loïc for the initial patch.

Member
loic commented Apr 1, 2014

LGTM.

Member

Is an additional test needed for this?

I find it somewhat concerning that this bug was able to slip through. I would imagine the test suite should have failed against Python 3.3 due to broken migrations.

Member
loic commented Apr 1, 2014

It's pretty hard to test for this, you need to generate a migration with Python 2.7 then run it in Python 3.2 for the problem to show. Python 3.3 is fine since it supports the u'' prefix anyway.

Member

I don't like that this results in a b prefix showing up everywhere (notably for the model and most field names). There is also still a bug in the options keys which I fixed in the last commit in this branch.

Example file:

# encoding: utf8
from __future__ import unicode_literals

from django.db import models, migrations


class Migration(migrations.Migration):

    dependencies = [
    ]

    operations = [
        migrations.CreateModel(
            name=b'Email',
            fields=[
                ('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)),
                (b'from_email', models.TextField(verbose_name='from e-mail')),
                (b'recipients', models.TextField(verbose_name='recipients')),
                (b'subject', models.TextField(verbose_name='subject')),
                (b'body', models.TextField(verbose_name='body')),
                (b'ok', models.BooleanField(default=False, db_index=True, verbose_name='ok')),
                (b'date_sent', models.DateTimeField(auto_now_add=True, verbose_name='date sent', db_index=True)),
            ],
            options={
                'ordering': ('-date_sent',),
                'verbose_name': 'e-mail',
                'verbose_name_plural': 'e-mails',
            },
            bases=(models.Model,),
        ),
    ]
Member
loic commented Apr 1, 2014

@treyhunner, I suspect there will be a few more of these errors, I caught Promise with a quick survey, but there could be more.

Regarding b'' prefix, that's the only way to go as far as migrations are concerned, if we use bytes where we should be using str that has to be fixed in the rest of the codebase.

Member

@treyhunner thanks for spotting the dict key issue, I'll merge your changes and add tests.

The point of this ticket is to make sure migrations generated by Django are portable between Python versions. Since field names defaults to their associated attribute names which a str (byte on Py2 and unicode on Py3) the correct way of fixing this cosmetic issue would be to make sure Field.name is always an instance of six.text_type.

@charettes charettes referenced this pull request in charettes/django Apr 1, 2014
Closed

Migration additional fix #1

Member

@treyhunner, I added a assertion based on your initial approach for unicode prefix existence.

/cc @andrewgodwin I'd like to get your feedback before merging this. Did I overlook something?

Owner

No, this looks good to me. Good to see this being ironed out.

Member

@charettes using lexical analysis is much better than the hack test I suggested. I'm very glad there's a test for that now.

Member

Merged in 72d3889.

@charettes charettes closed this Apr 14, 2014
@charettes charettes deleted the charettes:ticket-22350-migration-string-literals branch Aug 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment