Opened 5 years ago

Closed 5 years ago

#30691 closed Bug (fixed)

Change uuid field to FK does not create dependency

Reported by: Dart Owned by: Dart
Component: Migrations Version: dev
Severity: Normal Keywords: UUID, FK
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Simon Charette)

Hi! I am new in django community, so please help me, because i really dont know is it really "bug".

I have a django project named "testproject" and two apps: testapp1, testapp2.

It will be simpler to understand, with this example:

# TestApp1(models.py):

class App1(models.Model):
    id = models.UUIDField(primary_key=True, unique=True, default=uuid.uuid4, editable=False, verbose_name=_('identifier'))
    text = models.CharField(max_length=100, verbose_name=_('text'))
    another_app = models.UUIDField(null=True, blank=True, verbose_name=_('another app'))

# TestApp2(models.py):

class App2(models.Model):
    id = models.UUIDField(primary_key=True, unique=True, default=uuid.uuid4, editable=False, verbose_name=_('identifier'))
    text = models.CharField(max_length=100, verbose_name=_('text'))

First model named "App1" has UUID field named "another_app":

  • another_app = models.UUIDField(null=True, blank=True, verbose_name=_('another app'))

After some time i change field from UUID to FK, like this:

  • another_app = models.ForeignKey(App2, null=True, blank=True, on_delete=models.SET_NULL, verbose_name=_('another app'))

And as result i create new migration, but Migration class was unexpected result, because it does not create any "dependencies" for App2, because of FK.

I think the correct solution will be create dependency for App2.

This project use django version 2.2 and postgresql. Attach archive with sources. Project contains small test, after running him, you will get exception like this: ValueError: Related model 'testapp2.App2' cannot be resolved.

So is it problem in django or maybe i dont understand something ?

Here is my post in django users:
https://groups.google.com/forum/#!searchin/django-users/Django$20bug$3A$20change$20uuid$20field$20to$20FK$20does$20not$20create$20dependency%7Csort:date/django-users/-h9LZxFomLU/yz-NLi1cDgAJ

Regards, Viktor Lomakin

Attachments (1)

testproject.zip (29.9 KB ) - added by Dart 5 years ago.
Django project which show the bug

Download all attachments as: .zip

Change History (12)

by Dart, 5 years ago

Attachment: testproject.zip added

Django project which show the bug

comment:1 by Carlton Gibson, 5 years ago

Component: UncategorizedMigrations
Triage Stage: UnreviewedAccepted
Version: 2.2master

There's a lot of history in here, so it may turn out this is a duplicate, but I'll accept this for now, as there's certainly a bug.

makemigrations generats this:

    dependencies = [
        ('testapp1', '0002_app1_another_app'),
    ]

    operations = [
        migrations.AlterField(
            model_name='app1',
            name='another_app',
            field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, to='testapp2.App2', verbose_name='another app'),
        ),
    ]

But that gives the ValueError as you say.

Adjusting dependencies allows the test to pass:

    dependencies = [
        ('testapp1', '0002_app1_another_app'),
        ('testapp2', '0001_initial'),
    ]

Thus, AlterField should pick this up.

(Note for later: SQLite raised will required disabling constraint checking, ref #30023.)

Version 0, edited 5 years ago by Carlton Gibson (next)

comment:2 by Dart, 5 years ago

Owner: changed from nobody to Dart
Status: newassigned

comment:3 by Simon Charette, 5 years ago

Description: modified (diff)

That's probably a bug in django.db.migrations.autodetector.MigrationAutodetector.generate_altered_fields's call to add_operation which doesn't pass any dependencies when new_field.is_relation and not old_field.is_relation. The dependencies can be retrieved by using self._get_dependencies_for_foreign_key(new_field).

We want to depend on the latest migration of the referenced app instead of the initial one though as the model we're referencing might have been altered since then but that's something the aforementioned method should handle.

Tests should be added to django/tests/migrations/test_autodetector.py.

comment:4 by Simon Charette, 5 years ago

Hello Dart,

Are you still planing to work on this? Do you need any assistance submitting a patch?

comment:5 by Dmitrij Strelnikov, 5 years ago

I'm pretty sure your db change interfere with migration best practice.
You are changing field type, which have to be done in two steps.
And your migrations are not telling django what should be done with data in uuid field contains when migrate.

in reply to:  4 comment:6 by Dart, 5 years ago

Replying to Simon Charette:

Hello Dart,

Are you still planing to work on this? Do you need any assistance submitting a patch?

Yes i'm work with this patch and planning to send this. Yes, it will be good, if you help me.

in reply to:  4 comment:7 by Dart, 5 years ago

Replying to Simon Charette:

Hello Dart,

Are you still planing to work on this? Do you need any assistance submitting a patch?

Hi! Today i will send a patch. I found a problem and resolve it.

Regards

comment:8 by Dart, 5 years ago

Hi! Send pull request, please check it. And please can you help me where to put docs.

comment:10 by Simon Charette, 5 years ago

Triage Stage: AcceptedReady for checkin

Hey Dart, thanks for submitting a PR!

Since this is a bug fix it shouldn't not require any docs. I've marked the patch RFC since only trivial changes need to be made before the patch is merged which is something committers can take care of.

comment:11 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In 5931d2e9:

Fixed #30691 -- Made migrations autodetector find dependencies for foreign keys altering.

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