Opened 9 years ago
Closed 2 years ago
#26099 closed Cleanup/optimization (needsinfo)
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: | Unreviewed |
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 (14)
comment:1 by , 9 years ago
Type: | Uncategorized → Bug |
---|
comment:2 by , 9 years ago
Version: | 1.9 → 1.8 |
---|
comment:3 by , 9 years ago
Component: | Uncategorized → Migrations |
---|---|
Summary: | Migrations name conflict → Add a warning about import conflicts when auto-generating migrations |
Triage Stage: | Unreviewed → Accepted |
Type: | Bug → Cleanup/optimization |
comment:4 by , 8 years ago
Cc: | added |
---|
comment:5 by , 8 years ago
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 by , 8 years ago
This doesn't change anything _by itself_, as an app can still be named django_settings
.
comment:7 by , 8 years ago
Cc: | 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 by , 8 years ago
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 by , 8 years ago
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 by , 8 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:11 by , 8 years ago
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:13 by , 8 years ago
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.
comment:14 by , 2 years ago
Resolution: | → needsinfo |
---|---|
Status: | assigned → closed |
Triage Stage: | Accepted → Unreviewed |
I'd want a consensus on the DevelopersMailingList on this change before proceeding.
Marking as needsinfo
as we should first reach a consensus about the approach.
It might be possible to automatically add some "Import x as y" shims as necessary.