[Django] #27768: makemigrations uses unnecessary AddField for ForeignKey depending on model name

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

[Django] #27768: makemigrations uses unnecessary AddField for ForeignKey depending on model name

Django
#27768: makemigrations uses unnecessary AddField for ForeignKey depending on model
name
--------------------------------------+------------------------
               Reporter:  Ed Morley   |          Owner:  nobody
                   Type:  Bug         |         Status:  new
              Component:  Migrations  |        Version:  master
               Severity:  Normal      |       Keywords:
           Triage Stage:  Unreviewed  |      Has patch:  0
    Needs documentation:  0           |    Needs tests:  0
Patch needs improvement:  0           |  Easy pickings:  0
                  UI/UX:  0           |
--------------------------------------+------------------------
 Using Django master, for this model `makemigrations` generates an
 inefficient migrations file, which uses a combination of `CreateModel` and
 `AddField`, rather than just using a single `CreateModel` operation.

 {{{#!python
 class Aaa(models.Model):
     pass

 class Foo(models.Model):
     fk = models.ForeignKey(Aaa,
 on_delete=django.db.models.deletion.CASCADE)
 }}}

 Resultant migration operations generated by `./manage.py makemigrations`:
 {{{#!python
     operations = [
         migrations.CreateModel(
             name='Foo',
             fields=[
                 ('id', models.AutoField(auto_created=True,
 primary_key=True, serialize=False, verbose_name='ID')),
             ],
         ),
         migrations.CreateModel(
             name='Zzz',
             fields=[
                 ('id', models.AutoField(auto_created=True,
 primary_key=True, serialize=False, verbose_name='ID')),
             ],
         ),
         migrations.AddField(
             model_name='foo',
             name='fk',
 field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE,
 to='testapp.Zzz'),
         ),
     ]
 }}}

 However if the `Zzz` model was instead called `Aaa`, then the migration
 file is correctly optimised (ie: the `ForeignKey` is defined within the
 `CreateModel` operation):
 {{{#!python
     operations = [
         migrations.CreateModel(
             name='Aaa',
             fields=[
                 ('id', models.AutoField(auto_created=True,
 primary_key=True, serialize=False, verbose_name='ID')),
             ],
         ),
         migrations.CreateModel(
             name='Foo',
             fields=[
                 ('id', models.AutoField(auto_created=True,
 primary_key=True, serialize=False, verbose_name='ID')),
                 ('fk',
 models.ForeignKey(on_delete=django.db.models.deletion.CASCADE,
 to='testapp.Aaa')),
             ],
         ),
     ]
 }}}

 Ideally the optimizer would either just use the order as defined in
 models.py (which would at least allow for users to order them sensibly),
 or else intelligently sort the models such that those with no (or fewer)
 foreign keys are listed in `operations` first, thereby reducing the number
 of cases where the FK has to be added in a separate `AddField` operation.

--
Ticket URL: <https://code.djangoproject.com/ticket/27768>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/051.1f9139f1bebfbc0f01740769ae87cde9%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #27768: makemigrations uses unnecessary AddField for ForeignKey depending on model name

Django
#27768: makemigrations uses unnecessary AddField for ForeignKey depending on model
name
----------------------------+--------------------------------------
     Reporter:  Ed Morley   |                    Owner:  nobody
         Type:  Bug         |                   Status:  new
    Component:  Migrations  |                  Version:  master
     Severity:  Normal      |               Resolution:
     Keywords:              |             Triage Stage:  Unreviewed
    Has patch:  0           |      Needs documentation:  0
  Needs tests:  0           |  Patch needs improvement:  0
Easy pickings:  0           |                    UI/UX:  0
----------------------------+--------------------------------------

Comment (by Ed Morley):

 I guess there are two parts to this:

 1) When performing a `makemigrations` to generate the initial migrations
 file, if the order that the models were declared in models.py was
 preserved, then the resultant operations would (a) likely require less
 optimization in the first place (since unless people use string references
 to other models, they will be declared in the correct order in the file),
 and (b) users could at least control the order.

 2) Ideally the migrations optimizer would reorder `CreateModel`s (if no
 other dependencies between them) rather than refuse to optimize across
 them.

 ie for (2),
 [https://github.com/django/django/blob/e5c2e43cc832028a974399af07a1c3ba6afa2467/tests/migrations/test_optimizer.py#L313-L329
 this existing test]:

 {{{#!python
     def test_create_model_add_field_not_through_fk(self):
         """
         AddField should NOT optimize into CreateModel if it's an FK to a
 model
         that's between them.
         """
         self.assertOptimizesTo(
             [
                 migrations.CreateModel("Foo", [("name",
 models.CharField(max_length=255))]),
                 migrations.CreateModel("Link", [("url",
 models.TextField())]),
                 migrations.AddField("Foo", "link",
 models.ForeignKey("migrations.Link", models.CASCADE)),
             ],
             [
                 migrations.CreateModel("Foo", [("name",
 models.CharField(max_length=255))]),
                 migrations.CreateModel("Link", [("url",
 models.TextField())]),
                 migrations.AddField("Foo", "link",
 models.ForeignKey("migrations.Link", models.CASCADE)),
             ],
         )
 }}}

 Should actually be changed to:

 {{{#!python
         self.assertOptimizesTo(
             [
                 migrations.CreateModel("Foo", [("name",
 models.CharField(max_length=255))]),
                 migrations.CreateModel("Link", [("url",
 models.TextField())]),
                 migrations.AddField("Foo", "link",
 models.ForeignKey("migrations.Link", models.CASCADE)),
             ],
             [
                 migrations.CreateModel("Link", [("url",
 models.TextField())]),
                 migrations.CreateModel("Foo", [("name",
 models.CharField(max_length=255)), ("link",
 models.ForeignKey("migrations.Link", models.CASCADE)]),
             ],
         )
 }}}

--
Ticket URL: <https://code.djangoproject.com/ticket/27768#comment:1>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/066.ce425fb4496070e5c1f36aa89a77ba01%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #27768: makemigrations uses unnecessary AddField for ForeignKey depending on model name

Django
In reply to this post by Django
#27768: makemigrations uses unnecessary AddField for ForeignKey depending on model
name
----------------------------+--------------------------------------
     Reporter:  Ed Morley   |                    Owner:  nobody
         Type:  Bug         |                   Status:  new
    Component:  Migrations  |                  Version:  master
     Severity:  Normal      |               Resolution:
     Keywords:              |             Triage Stage:  Unreviewed
    Has patch:  0           |      Needs documentation:  0
  Needs tests:  0           |  Patch needs improvement:  0
Easy pickings:  0           |                    UI/UX:  0
----------------------------+--------------------------------------
Description changed by Ed Morley:

Old description:

> Using Django master, for this model `makemigrations` generates an
> inefficient migrations file, which uses a combination of `CreateModel`
> and `AddField`, rather than just using a single `CreateModel` operation.
>
> {{{#!python
> class Aaa(models.Model):
>     pass
>
> class Foo(models.Model):
>     fk = models.ForeignKey(Aaa,
> on_delete=django.db.models.deletion.CASCADE)
> }}}
>
> Resultant migration operations generated by `./manage.py makemigrations`:
> {{{#!python
>     operations = [
>         migrations.CreateModel(
>             name='Foo',
>             fields=[
>                 ('id', models.AutoField(auto_created=True,
> primary_key=True, serialize=False, verbose_name='ID')),
>             ],
>         ),
>         migrations.CreateModel(
>             name='Zzz',
>             fields=[
>                 ('id', models.AutoField(auto_created=True,
> primary_key=True, serialize=False, verbose_name='ID')),
>             ],
>         ),
>         migrations.AddField(
>             model_name='foo',
>             name='fk',
> field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE,
> to='testapp.Zzz'),
>         ),
>     ]
> }}}
>
> However if the `Zzz` model was instead called `Aaa`, then the migration
> file is correctly optimised (ie: the `ForeignKey` is defined within the
> `CreateModel` operation):
> {{{#!python
>     operations = [
>         migrations.CreateModel(
>             name='Aaa',
>             fields=[
>                 ('id', models.AutoField(auto_created=True,
> primary_key=True, serialize=False, verbose_name='ID')),
>             ],
>         ),
>         migrations.CreateModel(
>             name='Foo',
>             fields=[
>                 ('id', models.AutoField(auto_created=True,
> primary_key=True, serialize=False, verbose_name='ID')),
>                 ('fk',
> models.ForeignKey(on_delete=django.db.models.deletion.CASCADE,
> to='testapp.Aaa')),
>             ],
>         ),
>     ]
> }}}
>
> Ideally the optimizer would either just use the order as defined in
> models.py (which would at least allow for users to order them sensibly),
> or else intelligently sort the models such that those with no (or fewer)
> foreign keys are listed in `operations` first, thereby reducing the
> number of cases where the FK has to be added in a separate `AddField`
> operation.
New description:

 Using Django master, for this model `makemigrations` generates an
 inefficient migrations file, which uses a combination of `CreateModel` and
 `AddField`, rather than just using a single `CreateModel` operation.

 {{{#!python
 class Zzz(models.Model):
     pass

 class Foo(models.Model):
     fk = models.ForeignKey(Zzz,
 on_delete=django.db.models.deletion.CASCADE)
 }}}

 Resultant migration operations generated by `./manage.py makemigrations`:
 {{{#!python
     operations = [
         migrations.CreateModel(
             name='Foo',
             fields=[
                 ('id', models.AutoField(auto_created=True,
 primary_key=True, serialize=False, verbose_name='ID')),
             ],
         ),
         migrations.CreateModel(
             name='Zzz',
             fields=[
                 ('id', models.AutoField(auto_created=True,
 primary_key=True, serialize=False, verbose_name='ID')),
             ],
         ),
         migrations.AddField(
             model_name='foo',
             name='fk',
 field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE,
 to='testapp.Zzz'),
         ),
     ]
 }}}

 However if the `Zzz` model was instead called `Aaa`, then the migration
 file is correctly optimised (ie: the `ForeignKey` is defined within the
 `CreateModel` operation):
 {{{#!python
     operations = [
         migrations.CreateModel(
             name='Aaa',
             fields=[
                 ('id', models.AutoField(auto_created=True,
 primary_key=True, serialize=False, verbose_name='ID')),
             ],
         ),
         migrations.CreateModel(
             name='Foo',
             fields=[
                 ('id', models.AutoField(auto_created=True,
 primary_key=True, serialize=False, verbose_name='ID')),
                 ('fk',
 models.ForeignKey(on_delete=django.db.models.deletion.CASCADE,
 to='testapp.Aaa')),
             ],
         ),
     ]
 }}}

 Ideally the optimizer would either just use the order as defined in
 models.py (which would at least allow for users to order them sensibly),
 or else intelligently sort the models such that those with no (or fewer)
 foreign keys are listed in `operations` first, thereby reducing the number
 of cases where the FK has to be added in a separate `AddField` operation.

--

--
Ticket URL: <https://code.djangoproject.com/ticket/27768#comment:2>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/066.e24e84dc3b8bbf625e3a61e9d6d0c77c%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #27768: makemigrations uses unnecessary AddField for ForeignKey depending on model name

Django
In reply to this post by Django
#27768: makemigrations uses unnecessary AddField for ForeignKey depending on model
name
--------------------------------------+------------------------------------
     Reporter:  Ed Morley             |                    Owner:  nobody
         Type:  Cleanup/optimization  |                   Status:  new
    Component:  Migrations            |                  Version:  master
     Severity:  Normal                |               Resolution:
     Keywords:                        |             Triage Stage:  Accepted
    Has patch:  0                     |      Needs documentation:  0
  Needs tests:  0                     |  Patch needs improvement:  0
Easy pickings:  0                     |                    UI/UX:  0
--------------------------------------+------------------------------------
Changes (by Tim Graham):

 * type:  Bug => Cleanup/optimization
 * stage:  Unreviewed => Accepted


--
Ticket URL: <https://code.djangoproject.com/ticket/27768#comment:3>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/066.d980f875dd816cc341f91fc79856f03c%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #27768: makemigrations uses unnecessary AddField for ForeignKey depending on model name

Django
In reply to this post by Django
#27768: makemigrations uses unnecessary AddField for ForeignKey depending on model
name
-------------------------------------+-------------------------------------
     Reporter:  Ed Morley            |                    Owner:  Ed Morley
         Type:                       |                   Status:  assigned
  Cleanup/optimization               |
    Component:  Migrations           |                  Version:  master
     Severity:  Normal               |               Resolution:
     Keywords:                       |             Triage Stage:  Accepted
    Has patch:  1                    |      Needs documentation:  0
  Needs tests:  0                    |  Patch needs improvement:  0
Easy pickings:  0                    |                    UI/UX:  0
-------------------------------------+-------------------------------------
Changes (by Ed Morley):

 * status:  new => assigned
 * owner:  nobody => Ed Morley
 * has_patch:  0 => 1


Comment:

 PR opened.

 I've added additional testcases for the edge-cases where reordering will
 cause issues (eg inherited models, circular FKs etc) but may be missing
 some, so open to suggestions :-)

--
Ticket URL: <https://code.djangoproject.com/ticket/27768#comment:4>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/066.b2c960b4cdcaef3054af0f16f97ff45f%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #27768: makemigrations uses unnecessary AddField for ForeignKey depending on model name

Django
In reply to this post by Django
#27768: makemigrations uses unnecessary AddField for ForeignKey depending on model
name
-------------------------------------+-------------------------------------
     Reporter:  Ed Morley            |                    Owner:  Ed Morley
         Type:                       |                   Status:  assigned
  Cleanup/optimization               |
    Component:  Migrations           |                  Version:  master
     Severity:  Normal               |               Resolution:
     Keywords:                       |             Triage Stage:  Accepted
    Has patch:  1                    |      Needs documentation:  0
  Needs tests:  0                    |  Patch needs improvement:  0
Easy pickings:  0                    |                    UI/UX:  0
-------------------------------------+-------------------------------------
Changes (by Simon Charette):

 * cc: Simon Charette (added)


--
Ticket URL: <https://code.djangoproject.com/ticket/27768#comment:5>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/066.e665463dd784bcaa899e4d9ae2e5f0c0%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #27768: makemigrations uses unnecessary AddField for ForeignKey depending on model name

Django
In reply to this post by Django
#27768: makemigrations uses unnecessary AddField for ForeignKey depending on model
name
-------------------------------------+-------------------------------------
     Reporter:  Ed Morley            |                    Owner:  Ed Morley
         Type:                       |                   Status:  assigned
  Cleanup/optimization               |
    Component:  Migrations           |                  Version:  master
     Severity:  Normal               |               Resolution:
     Keywords:                       |             Triage Stage:  Accepted
    Has patch:  1                    |      Needs documentation:  0
  Needs tests:  0                    |  Patch needs improvement:  0
Easy pickings:  0                    |                    UI/UX:  0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

 [https://github.com/django/django/pull/7999 Adjusted PR]

--
Ticket URL: <https://code.djangoproject.com/ticket/27768#comment:6>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/066.34f9f06824d40db3c9da73becee59afa%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #27768: makemigrations uses unnecessary AddField for ForeignKey depending on model name

Django
In reply to this post by Django
#27768: makemigrations uses unnecessary AddField for ForeignKey depending on model
name
-------------------------------------+-------------------------------------
     Reporter:  Ed Morley            |                    Owner:  Ed Morley
         Type:                       |                   Status:  assigned
  Cleanup/optimization               |
    Component:  Migrations           |                  Version:  master
     Severity:  Normal               |               Resolution:
     Keywords:                       |             Triage Stage:  Accepted
    Has patch:  1                    |      Needs documentation:  0
  Needs tests:  0                    |  Patch needs improvement:  1
Easy pickings:  0                    |                    UI/UX:  0
-------------------------------------+-------------------------------------
Changes (by Tim Graham):

 * needs_better_patch:  0 => 1


--
Ticket URL: <https://code.djangoproject.com/ticket/27768#comment:7>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/066.8030ef791d8447688483b4aeb25652d1%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #27768: makemigrations uses unnecessary AddField for ForeignKey depending on model name

Django
In reply to this post by Django
#27768: makemigrations uses unnecessary AddField for ForeignKey depending on model
name
-------------------------------------+-------------------------------------
     Reporter:  Ed Morley            |                    Owner:  Ed Morley
         Type:                       |                   Status:  closed
  Cleanup/optimization               |
    Component:  Migrations           |                  Version:  master
     Severity:  Normal               |               Resolution:  fixed
     Keywords:                       |             Triage Stage:  Accepted
    Has patch:  1                    |      Needs documentation:  0
  Needs tests:  0                    |  Patch needs improvement:  1
Easy pickings:  0                    |                    UI/UX:  0
-------------------------------------+-------------------------------------
Changes (by Tim Graham <timograham@…>):

 * status:  assigned => closed
 * resolution:   => fixed


Comment:

 In [changeset:"a97845a823c86b1d6e65af70fbce64e6e747e081" a97845a8]:
 {{{
 #!CommitTicketReference repository=""
 revision="a97845a823c86b1d6e65af70fbce64e6e747e081"
 Fixed #27768 -- Allowed migration optimization of CreateModel order.

 Thanks Ed Morley from Mozilla for the tests.
 }}}

--
Ticket URL: <https://code.djangoproject.com/ticket/27768#comment:9>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/066.1d929df152cf9b4a6cd221e572d9ea3b%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #27768: makemigrations uses unnecessary AddField for ForeignKey depending on model name

Django
In reply to this post by Django
#27768: makemigrations uses unnecessary AddField for ForeignKey depending on model
name
-------------------------------------+-------------------------------------
     Reporter:  Ed Morley            |                    Owner:  Ed Morley
         Type:                       |                   Status:  assigned
  Cleanup/optimization               |
    Component:  Migrations           |                  Version:  master
     Severity:  Normal               |               Resolution:
     Keywords:                       |             Triage Stage:  Accepted
    Has patch:  1                    |      Needs documentation:  0
  Needs tests:  0                    |  Patch needs improvement:  1
Easy pickings:  0                    |                    UI/UX:  0
-------------------------------------+-------------------------------------

Comment (by Tim Graham <timograham@…>):

 In [changeset:"d3a935f01f6cb36d21b09d81c5f022a3a5116ab0" d3a935f0]:
 {{{
 #!CommitTicketReference repository=""
 revision="d3a935f01f6cb36d21b09d81c5f022a3a5116ab0"
 Refs #27768 -- Reversed order of optimized and in-between operations.

 Operations can only be optimized through if they don't reference any of
 the
 state the operation they are compared against defines or alters, so it's
 safe to reverse the order.
 }}}

--
Ticket URL: <https://code.djangoproject.com/ticket/27768#comment:8>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/066.d387b1e70be9ff02d7a8b9d4d04d12d2%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.