[Django] #29177: Unmanaged models with ForeignKeys do not get those fields serialized into their migration state when CreateModel happens.
Django
their migration state when CreateModel happens.
----------------------------------------+------------------------
Reporter: Keryn Knight | 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 |
----------------------------------------+------------------------
Given models like the following, where `B` is unmanaged, and `C` is a
normal concrete `Model`:
{{{
from django.db import models
class A(models.Model):
class Meta:
db_table = "test_a"
class B(models.Model):
a = models.ForeignKey(A, related_name="+", on_delete=models.CASCADE)
class Meta:
managed = False
db_table = "test_b"
class C(models.Model):
a = models.ForeignKey(A, related_name="+", on_delete=models.CASCADE)
class Meta:
db_table = "test_c"
}}}
when `python manage.py makemigrations <appname>` is called, `B` ends up
looking like:
{{{
migrations.CreateModel(
name='B',
fields=[
('id', models.AutoField(auto_created=True, primary_key=True,
serialize=False, verbose_name='ID')),
],
options={
'db_table': 'test_b',
'managed': False,
},
)
}}}
whilst `C` correctly gets:
{{{
migrations.CreateModel(
name='C',
fields=[
('id', models.AutoField(auto_created=True, primary_key=True,
serialize=False, verbose_name='ID')),
('a',
models.ForeignKey(on_delete=django.db.models.deletion.CASCADE,
related_name='+', to='whee.A')),
],
options={
'db_table': 'test_c',
},
)
}}}
Because `B` doesn't have the `a` attribute, any subsequent data migrations
which would like to query on it, like so:
{{{
def forwards(apps, schema_editor):
B = apps.get_model('whee', 'B')
B.objects.filter(a=1)
}}}
will crash.
With the assistance of Markus on IRC:
{{{
MarkusH: do you want to try what's gonna happen when you just drop the if-
condition in
https://github.com/django/django/blob/75527c0f8360ae3555fcf67ce19b4cb910b69b9d/django/db/migrations/autodetector.py#L570-L572
?
}}}
commenting out the lines
{{{
if not model_opts.managed:
continue
}}}
allows the autodetector to generate the following:
{{{
migrations.CreateModel(
name='A',
fields=[
('id', models.AutoField(auto_created=True, primary_key=True,
serialize=False, verbose_name='ID')),
],
options={
'db_table': 'test_a',
},
),
migrations.AddField(
model_name='b',
name='a',
field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE,
related_name='+', to='whee.A'),
)
}}}
and subsequently the data migration can access `B.a`
If `A` itself is `managed=False` the migration generated is instead:
{{{
migrations.CreateModel(
name='B',
fields=[
('id', models.AutoField(auto_created=True, primary_key=True,
serialize=False, verbose_name='ID')),
('a',
models.ForeignKey(on_delete=django.db.models.deletion.CASCADE,
related_name='+', to='whee.A')),
],
options={
'db_table': 'test_b',
'managed': False,
},
)
}}}
Running the test suite via `./runtests.py` as of commit
75527c0f8360ae3555fcf67ce19b4cb910b69b9d doesn't seem to cause any
failures, with those lines commented out.
Markus also pointed out commit
https://github.com/django/django/commit/215aa4f53b6bbd07d5c1eecfa94e7fcd00da813e
which references #23415
I noticed this "issue" while bringing a project up from 1.9, so it's not a
recent problem.
--
Ticket URL: <https://code.djangoproject.com/ticket/29177>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
their migration state when CreateModel happens.
Reporter: Keryn Knight | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Keryn Knight):
* cc: Keryn Knight (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/29177#comment:1>
Django
their migration state when CreateModel happens.
Reporter: Keryn Knight | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
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 Keryn Knight:
Old description:
> if-condition in
New description:
{{{
from django.db import models
--
--
Ticket URL: <https://code.djangoproject.com/ticket/29177#comment:2>
Django
their migration state when CreateModel happens.
Reporter: Keryn Knight | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
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 Simon Charette):
* stage: Unreviewed => Accepted
Comment:
Seems like a legitimate bug.
Do you think you could submit a PR with the removed lines and a test in
[https://github.com/django/django/blob/master/tests/migrations/test_autodetector.py
test_autodetector.py] for it?
--
Ticket URL: <https://code.djangoproject.com/ticket/29177#comment:3>
Django
their migration state when CreateModel happens.
Reporter: Keryn Knight | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Keryn Knight):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
* needs_docs: 0 => 1
Comment:
WIP PR: https://github.com/django/django/pull/9741
--
Ticket URL: <https://code.djangoproject.com/ticket/29177#comment:4>
Django
their migration state when CreateModel happens.
Reporter: Keryn Knight | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by YBull):
Using version 2.0.5, I just encountered this bug and when trying to work
around it, discovered that it's a little more expansive than the example
model with A, B, C above shows. In my case, I had used the 'inspectdb'
command to generate models for 12 legacy tables (all with managed =
False). The inspectdb command did a reasonably good job and included all
the ForeignKey fields it should have. But even after I commented out the
lines in autodetector.py to work around this bug, I discovered that the
'makemigrations' command still has another related problem with Fkeys on
unmanaged models. Specifically, whenever a new unmanaged model is
referenced by another model's Fkeys, you must run 'makemigrations' for the
referenced table BEFORE adding the model(s) containing the reference. So
in my case, even though the models.py was valid and complete at the
beginning, I had to temporarily remove all the new dependent models to
carefully generate a sequence of six separate migrations to get all my 12
legacy tables in with the correct FKey declarations in the migration
scripts.
Being new to Django and the code base, I wanted to report this here, but
I'm not sure whether the fix for this aspect is actually a separate
related bug.
--
Ticket URL: <https://code.djangoproject.com/ticket/29177#comment:5>
Django
their migration state when CreateModel happens.
Reporter: Keryn Knight | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Sven R. Kunze):
Bug is also present on 1.11.
--
Ticket URL: <https://code.djangoproject.com/ticket/29177#comment:6>
Django
their migration state when CreateModel happens.
Reporter: Keryn Knight | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Daniel Naab):
I hit this bug also, and worked around by:
1) Making the models `managed = True`
2) Running `manage.py makemigrations` for those models
3) Manually switched all `'managed': True` columns in the migration to
`'managed': False`
4) Switch models back to `managed = False`
--
Ticket URL: <https://code.djangoproject.com/ticket/29177#comment:7>
Django
their migration state when CreateModel happens.
Reporter: Keryn Knight | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Daniel Naab):
* cc: Daniel Naab (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/29177#comment:8>
Django
their migration state when CreateModel happens.
Reporter: Keryn Knight | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Sergey Yurchenko):
Unmanaged models should not work in migrations by their nature.
You can make direct import of this model in migrations and use it.
I think it is expected behaviour not a bug.
--
Ticket URL: <https://code.djangoproject.com/ticket/29177#comment:9>
Django
their migration state when CreateModel happens.
Reporter: Keryn Knight | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
Sergey,
> Unmanaged models should not work in migrations by their nature.
Is there a reason for that? How would the migration framework know how to
generate references from managed models to unmanaged ones then (e.g. a
foreign key)? The migration framework has supported managed model
references for a while and even
[https://docs.djangoproject.com/en/2.1/howto/writing-migrations/#changing-
an-unmanaged-model-to-managed documents how to move an unmanaged model to
a managed one].
> You can make direct import of this model in migrations and use it.
Direct model imports in migrations will break over time if the model
definition happen to change and won't work when mixed up with
state/migration models. The
[https://docs.djangoproject.com/en/2.1/topics/migrations/#historical-
models documentation warns about this pattern].
I still believe this is a legitimate bug.
--
Ticket URL: <https://code.djangoproject.com/ticket/29177#comment:10>
Django
their migration state when CreateModel happens.
Reporter: Keryn Knight | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Fabio Sangiovanni):
* cc: Fabio Sangiovanni (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/29177#comment:11>
Django
their migration state when CreateModel happens.
Reporter: Keryn Knight | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Ponytech):
Replying to [comment:7 Daniel Naab]:
> I hit this bug also, and worked around by:
>
> 1) Making the models `managed = True`
> 2) Running `manage.py makemigrations` for those models
> 3) Manually switched all `'managed': True` models in the migration to
`'managed': False`
> 4) Switch models back to `managed = False`
Same bug here and this workaround saved my day.
Thanks a lot!
--
Ticket URL: <https://code.djangoproject.com/ticket/29177#comment:12>
Django
their migration state when CreateModel happens.
Reporter: Keryn Knight | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by João Paulo Limberger - JOTAPE - Shoo):
Thanks! This workaround save my day Too!!!
> Replying to [comment:7 Daniel Naab]:
> > I hit this bug also, and worked around by:
> >
> > 1) Making the models `managed = True`
> > 2) Running `manage.py makemigrations` for those models
> > 3) Manually switched all `'managed': True` models in the migration to
`'managed': False`
> > 4) Switch models back to `managed = False`
>
> Same bug here and this workaround saved my day.
> Thanks a lot!
--
Ticket URL: <https://code.djangoproject.com/ticket/29177#comment:13>
Django
their migration state when CreateModel happens.
Reporter: Keryn Knight | Owner: Michal Mládek
Type: Bug | Status: assigned
Component: Migrations | Version: dev
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Changes (by Michal Mládek):
* owner: nobody => Michal Mládek
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/29177#comment:14>
Django
their migration state when CreateModel happens.
Reporter: Keryn Knight | Owner: Michal Mládek
Type: Bug | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Comment (by Michal Mládek):
Currently, it seems there is a lot of questions about indended behavior of
unmanaged models and related migrations. I will do the research tommorow
and report the results here. Maybe if I will be lucky, I will add some
recommendations.
--
Ticket URL: <https://code.djangoproject.com/ticket/29177#comment:15>
Django
their migration state when CreateModel happens.
Reporter: Keryn Knight | Owner: Michal Mládek
Type: Bug | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Changes (by Michal Mládek):
Django
their migration state when CreateModel happens.
Reporter: Keryn Knight | Owner: Michal Mládek
Type: Bug | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Changes (by Michal Mládek):
Django
their migration state when CreateModel happens.
Reporter: Keryn Knight | Owner: Michal Mládek
Type: Bug | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Changes (by Michal Mládek):
There is an django project **dj_bug_29177** contains code to trigger the
bug reported here.
Django
their migration state when CreateModel happens.
Reporter: Keryn Knight | Owner: Michal Mládek
Type: Bug | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Comment (by Michal Mládek):
called **foo** with models and migrations written in such a way that the
reported bug is triggered. Additionally, I’ve added code that generalizes
the issue - **this bug occurs for any column added to model B**. The error
disappears if I **uncomment lines 19 and 20** in migration **foo 0001**. I
confirm that the issue is not related to the SQL generated by the **python
manage.py migrate** command, but rather to the **python manage.py
makemigrations** command. The bug affects any column used in an unmanaged
model, except for id. The **solution is** to fix the Django project code
so that **all fields defined on unmanaged models are included in
automatically generated migrations**. I will try to check today whether
the ticket already contains a test; if not, I’ll attempt to write one.
--
Ticket URL: <https://code.djangoproject.com/ticket/29177#comment:16>
Django
their migration state when CreateModel happens.
Reporter: Keryn Knight | Owner: Michal Mládek
Type: Bug | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Comment (by Michal Mládek):
Finally, there is problem only with FK. I made a mistake while I was
testing unmanaged models migrations.
> I have created a Django project named **dj_bug_29177**. It contains an
app called **foo** with models and migrations written in such a way that
the reported bug is triggered. Additionally, I’ve added code that
generalizes the issue - **this bug occurs for any column added to model
B**. The error disappears if I **uncomment lines 19 and 20** in migration
**foo 0001**. I confirm that the issue is not related to the SQL generated
by the **python manage.py migrate** command, but rather to the **python
manage.py makemigrations** command. The bug affects any column used in an
unmanaged model, except for id. The **solution is** to fix the Django
project code so that **all fields defined on unmanaged models are included
in automatically generated migrations**. I will try to check today whether
it is described [https://docs.djangoproject.com/en/dev/intro/contributing
/#writing-some-tests-for-your-ticket here] it must be regression test.
--
Ticket URL: <https://code.djangoproject.com/ticket/29177#comment:17>
Django
their migration state when CreateModel happens.
Reporter: Keryn Knight | Owner: Michal Mládek
Type: Bug | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Changes (by Fabio Sangiovanni):
* cc: Fabio Sangiovanni (removed)
--
Ticket URL: <https://code.djangoproject.com/ticket/29177#comment:18>
Django
their migration state when CreateModel happens.
Reporter: Keryn Knight | Owner: Michal Mládek
Type: Bug | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Changes (by Michal Mládek):
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/29177#comment:19>
Django
their migration state when CreateModel happens.
Reporter: Keryn Knight | Owner: Michal Mládek
Type: Bug | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Changes (by Michal Mládek):
Comment:
I decided to add unit tests to cover the new functionality of Autodetector
which has no unit tests still.
--
Ticket URL: <https://code.djangoproject.com/ticket/29177#comment:20>
Django
their migration state when CreateModel happens.
Reporter: Keryn Knight | Owner: Michal Mládek
Type: Bug | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Changes (by Michal Mládek):
--
Ticket URL: <https://code.djangoproject.com/ticket/29177#comment:21>
Django
their migration state when CreateModel happens.
Reporter: Keryn Knight | Owner: Michal Mládek
Type: Bug | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Changes (by Jacob Walls):
* needs_better_patch: 0 => 1
[https://github.com/django/django/pull/19452 PR]
There is a test failure. When you've got it patched up, please unset
"Patch needs improvement", thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/29177#comment:22>
Django
their migration state when CreateModel happens.
------------------------------+-----------------------------------------
Reporter: Keryn Knight | Owner: Michal Mládek
Type: Bug | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
------------------------------+-----------------------------------------
* needs_better_patch: 1 => 0
I am sorry, I had misunderstand some things in CI/CD pipeline ... But now
it should work fine.
Replying to [comment:22 Jacob Walls]:
>
> There is a test failure. When you've got it patched up, please unset
"Patch needs improvement", thanks!
--