Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reusable apps can not be shipped with migrations #12

Closed
sacha-senchuk opened this issue Oct 19, 2016 · 4 comments
Closed

Reusable apps can not be shipped with migrations #12

sacha-senchuk opened this issue Oct 19, 2016 · 4 comments

Comments

@sacha-senchuk
Copy link

sacha-senchuk commented Oct 19, 2016

Hello!

First of all, many thanks for this repo! I hope Django will ship this feature in its core in not such a distant future.

I think that migrations don't really work with this system (except for simple cases).

Consider 3 models with ForeignKey relationships, A -> B -> C (C depends on B which depends on A). If you swap model B for your own, you will run into a problem:

  • the reusable app needs your model B to migrate, because A uses B, which you redefined
  • your website needs model C to migrate, because B has a ForeignKey to C, but C is in the reusable app

If you generated the migrations file in the reusable app, you basically end up with a circular import.

django.db.migrations.exceptions.CircularDependencyError: other_app.0001_initial, reusable_app.0001_initial

You can actually see the problem with the following example:

models.py in the reusable app:

import swapper

from django.db import models

class BaseA(models.Model):
    class Meta:
        abstract = True

class A(BaseA):
    class Meta:
        swappable = swapper.swappable_setting('reusable_app', 'A')

class BaseB(models.Model):
    a = models.ForeignKey(swapper.get_model_name('reusable_app', 'A'), on_delete=models.CASCADE)

    class Meta:
        abstract = True

class B(BaseB):
    class Meta:
        swappable = swapper.swappable_setting('reusable_app', 'B')

class BaseC(models.Model):
    b = models.ForeignKey(swapper.get_model_name('reusable_app', 'B'), on_delete=models.CASCADE)

    class Meta:
        abstract = True

class C(BaseC):
    class Meta:
        swappable = swapper.swappable_setting('reusable_app', 'C')

models.py in the other app:

from django.db import models
from reusable_app.models import BaseB

class B(BaseB, models.Model):
    name = models.CharField("extra field for B", max_length=255)

First attempt: we generate migrations for the reusable app first

In this first case, your other app is not in INSTALLED_APPS, and you only make migrations for your reusable app, so that it can create a proper database schema when installed.

So you run:

$ ./manage.py makemigrations reusable_app
Migrations for 'reusable_app':
  reusable_app/migrations/0001_initial.py:
    - Create model A
    - Create model B
    - Create model C

You get a nice migrations file:

# -*- coding: utf-8 -*-
# Generated by Django 1.10.2 on 2016-10-19 08:21
from __future__ import unicode_literals

from django.conf import settings
from django.db import migrations, models
import django.db.models.deletion


class Migration(migrations.Migration):

    initial = True

    dependencies = [
    ]

    operations = [
        migrations.CreateModel(
            name='A',
            fields=[
                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
            ],
            options={
                'swappable': 'REUSABLE_APP_A_MODEL',
            },
        ),
        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, to=settings.REUSABLE_APP_A_MODEL)),
            ],
            options={
                'swappable': 'REUSABLE_APP_B_MODEL',
            },
        ),
        migrations.CreateModel(
            name='C',
            fields=[
                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
                ('b', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to=settings.REUSABLE_APP_B_MODEL)),
            ],
            options={
                'swappable': 'REUSABLE_APP_C_MODEL',
            },
        ),
    ]

You can, of course, modify it according to your documentation in order to avoid blindly reading configuration values from django.conf.settings which may not be defined:

# -*- coding: utf-8 -*-
from __future__ import unicode_literals

from django.db import migrations, models
import django.db.models.deletion
import swapper


class Migration(migrations.Migration):

    initial = True

    dependencies = [
    ]

    operations = [
        migrations.CreateModel(
            name='A',
            fields=[
                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
            ],
            options={
                'swappable': swapper.swappable_setting('reusable_app', 'A'),
            },
        ),
        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, to=swapper.get_model_name('reusable_app', 'A'))),
            ],
            options={
                'swappable': swapper.swappable_setting('reusable_app', 'B'),
            },
        ),
        migrations.CreateModel(
            name='C',
            fields=[
                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
                ('b', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to=swapper.get_model_name('reusable_app', 'B'))),
            ],
            options={
                'swappable': swapper.swappable_setting('reusable_app', 'C'),
            },
        ),
    ]

And this works fine:

$ ./manage.py migrate
Operations to perform:
  Apply all migrations: admin, auth, contenttypes, reusable_app, sessions
Running migrations:
  Applying reusable_app.0001_initial... OK

But now, lets say I want to use my other app that ships with its own version of the "B" model.

I add other_app to INSTALLED_APPS and specify REUSABLE_APP_B_MODEL = 'other_app.B'. Now it doesn't work anymore…

$ ./manage.py makemigrations other_app
Traceback (most recent call last):
  File "./manage.py", line 22, in <module>
    execute_from_command_line(sys.argv)
  File ".../django/core/management/__init__.py", line 367, in execute_from_command_line
    utility.execute()
  File ".../django/core/management/__init__.py", line 359, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File ".../django/core/management/base.py", line 294, in run_from_argv
    self.execute(*args, **cmd_options)
  File ".../django/core/management/base.py", line 345, in execute
    output = self.handle(*args, **options)
  File ".../django/core/management/commands/makemigrations.py", line 173, in handle
    migration_name=self.migration_name,
  File ".../django/db/migrations/autodetector.py", line 47, in changes
    changes = self._detect_changes(convert_apps, graph)
  File ".../django/db/migrations/autodetector.py", line 132, in _detect_changes
    self.old_apps = self.from_state.concrete_apps
  File ".../django/db/migrations/state.py", line 180, in concrete_apps
    self.apps = StateApps(self.real_apps, self.models, ignore_swappable=True)
  File ".../django/db/migrations/state.py", line 249, in __init__
    raise ValueError("\n".join(error.msg for error in errors))
ValueError: The field reusable_app.C.b was declared with a lazy reference to 'other_app.b', but app 'other_app' isn't installed.

Well, Django is lying in a way. Because other_app IS installed:

$ ./manage.py check
System check identified no issues (0 silenced).
$ ./manage.py shell
Python 3.5.1 (default, Jan 22 2016, 08:54:32) 
[GCC 4.2.1 Compatible Apple LLVM 7.0.2 (clang-700.1.81)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
(InteractiveConsole)
>>> import swapper
>>> swapper.load_model('reusable_app', 'B')
<class 'other_app.models.B'>

I guess that Django doesn't see the other_app being installed because it reads the reusable_app.0001_initial migration file, which doesn't depend on anything, so it loads it before loading the other_app, and see references to other_app that it cannot resolve.

I tried to add fill the dependencies parameter in reusable_app.0001_initial, I was hoping that it would trigger an import of the other_app, but it didn't change anything:

    dependencies = [
        swapper.dependency('reusable_app', 'A'),
        swapper.dependency('reusable_app', 'B'),
        swapper.dependency('reusable_app', 'C'),
    ]

Second attempt: generate both migration files at the same time

So since generating migrations for the reusable app first doesn't work, there is a workaround. To clean things up, I just deleted the migrations file from the reusable app.

Now the only way to make the whole thing work is to type the following:

$ ./manage.py makemigrations
Migrations for 'other_app':
  other_app/migrations/0001_initial.py:
    - Create model B
  other_app/migrations/0002_b_a.py:
    - Add field a to b
Migrations for 'reusable_app':
  reusable_app/migrations/0001_initial.py:
    - Create model A
    - Create model B
    - Create model C

When I apply these migrations, they are executed in the following order:

  • other_app.0001_initial
  • reusable_app.0001_initial
  • other_app.0002_b_a

I guess that Django splits migrations in order to resolve the circular dependencies issue.

In my real-life project, I have a much more complicated setup with 12 inter-dependent swappable models. I guess it can lead to even more complicated inter-depndencies…

@sheppard
Copy link
Collaborator

Thanks for the detailed report, I have come across similar issues. The short answer to the CircularDependencyError is to manually split the migrations in your reusable app into an initial migration that only creates model A, then a second migration that depends on model A and B and creates B and C.

But the lazy reference issue is a bit harder to solve. I think it may have to do with django/django#6359 which was released with Django 1.10. Does the same thing happen with Django 1.9? Either way, there are some underlying assumptions in Django that make it difficult to have automatically generated migrations work as expected when you have multiple swappable models. So, maybe the best short term approach really is to not ship any migrations in a reusable app, though that does seem a bit funny.

The long term approach is probably to submit a PR with some additional changes to Django - this and the other open tickets on this repo should be useful in identifying what those changes need to be.

@sacha-senchuk
Copy link
Author

Thanks for the answer! Perhaps will be looking into that if I happen to have a bit of more time :)

sheppard added a commit to powered-by-wq/vera that referenced this issue Oct 26, 2016
 * use identify pattern for Site/ReportStatus/Parameter base models
 * don't check in migrations (see openwisp/django-swappable-models#12)
marcoooo pushed a commit to lirmm/waves-core that referenced this issue Sep 29, 2017
…migration files from core, and force user to run makemigrations upon installing waves core module. Just in cas someone wants to override defaults Service or Submission model
@sheppard sheppard mentioned this issue Jul 23, 2019
@SilverGeri
Copy link

SilverGeri commented Aug 6, 2020

In your app you just create a basic modell just to have it in the db and create an initial migration without any swap and add this:

run_before = [
        ('reusable_app', '0001_initial'),
    ]

this way your model will exist by the time reusable app looks for swapped models and can create FK to it.
In your other migrations you can inherit from any base, that will be just adding fields to you existing model

@nemesifier
Copy link
Member

Closed because it's not a bug but rather a configuration issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants