Opened 22 months ago

Last modified 9 months ago

#29177 new Bug

Unmanaged models with ForeignKeys do not get those fields serialized into their migration state when CreateModel happens.

Reported by: Keryn Knight Owned by: nobody
Component: Migrations Version: master
Severity: Normal Keywords:
Cc: Keryn Knight, Daniel Naab Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by Keryn Knight)

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='B',
    fields=[
        ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
    ],
    options={
        'db_table': 'test_b',
        'managed': False,
    },
),
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.

Change History (10)

comment:1 Changed 22 months ago by Keryn Knight

Cc: Keryn Knight added

comment:2 Changed 22 months ago by Keryn Knight

Description: modified (diff)

comment:3 Changed 22 months ago by Simon Charette

Triage Stage: UnreviewedAccepted

Seems like a legitimate bug.

Do you think you could submit a PR with the removed lines and a test in test_autodetector.py for it?

comment:4 Changed 22 months ago by Keryn Knight

Has patch: set
Needs documentation: set
Patch needs improvement: set

comment:5 Changed 19 months ago 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.

comment:6 Changed 16 months ago by Sven R. Kunze

Bug is also present on 1.11.

comment:7 Changed 15 months ago 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 models in the migration to 'managed': False
4) Switch models back to managed = False

Last edited 15 months ago by Daniel Naab (previous) (diff)

comment:8 Changed 15 months ago by Daniel Naab

Cc: Daniel Naab added

comment:9 Changed 9 months ago 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.

comment:10 Changed 9 months ago 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 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 documentation warns about this pattern.

I still believe this is a legitimate bug.

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