Opened 22 months ago

Last modified 10 months ago

#26099 assigned Cleanup/optimization

Add a warning about import conflicts when auto-generating migrations

Reported by: LucidComplex Owned by: Alexey Kotlyarov
Component: Migrations Version: 1.8
Severity: Normal Keywords:
Cc: a@…, desecho@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Django's automatic migration creation may suffer from a name conflict without any warning.

Creating a migration for a model having a relation with "django.contrib.auth.models.User" will cause Django to automatically import "django.conf.settings" in migrations.

This is all fine and good, unless you have an app unfortunately named "settings". Django will not fire a warning about this name conflict.

Change History (13)

comment:1 Changed 22 months ago by LucidComplex

Type: UncategorizedBug

comment:2 Changed 22 months ago by LucidComplex

Version: 1.91.8

comment:3 Changed 22 months ago by Tim Graham

Component: UncategorizedMigrations
Summary: Migrations name conflictAdd a warning about import conflicts when auto-generating migrations
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

It might be possible to automatically add some "Import x as y" shims as necessary.

comment:4 Changed 16 months ago by Alexey Kotlyarov

Cc: a@… added

comment:5 Changed 11 months ago by Anton Samarchyan

So we want to have something like this if the app name is settings?

from __future__ import unicode_literals

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


class Migration(migrations.Migration):

    initial = True

    dependencies = [
        migrations.swappable_dependency(django_settings.AUTH_USER_MODEL),
    ]

    operations = [
        migrations.CreateModel(
            name='Test',
            fields=[
                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
                ('user', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, to=django_settings.AUTH_USER_MODEL)),
            ],
        ),
    ]

comment:6 Changed 11 months ago by Alexey Kotlyarov

This doesn't change anything _by itself_, as an app can still be named django_settings.

comment:7 Changed 11 months ago by Anton Samarchyan

Cc: desecho@… added

What if first it's going to check if there is an app called settings, then if there is it's going to choose the name django_settings and check if the app exists again (with the name django_settings), if it does, it's going to name it django_settings_settings for example and check again, etc. Does it sound good?

comment:8 Changed 11 months ago by Tim Graham

There could be more import conflicts than just one for the name "settings" so the approach should be more general. I'm not sure whether or not solving this is feasible and/or practical.

comment:9 Changed 11 months ago by Alexey Kotlyarov

The needed parts of Django can be imported without from, so they stay inside django namespace:

import django.conf.settings
import django.db.migrations
import django.db.models

comment:10 Changed 11 months ago by Alexey Kotlyarov

Owner: changed from nobody to Alexey Kotlyarov
Status: newassigned

comment:11 Changed 11 months ago by Alexey Kotlyarov

While making changes, I found that the migration tests don't check the imports inside the migration files properly, and it was possible for them to pass even though the resulting migrations wouldn't have run on their own: https://github.com/django/django/commit/7bd4c3e75f3d9f9bbd43c2d4a39b786f2d75137d

I have a commit that fixes the imports, but I need to figure out how to make a test for the bad application name.

comment:12 Changed 11 months ago by Alexey Kotlyarov

Has patch: set

comment:13 Changed 10 months ago by Tim Graham

Patch needs improvement: set

I'm not sure if that's a developer friendly change considering that migrations can be authored and edited by hand. Defaulting to absolute paths everywhere makes migration files more verbose and perhaps less readable. I'd want a consensus on the DevelopersMailingList on this change before proceeding.

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