Opened 3 years ago

Closed 3 years ago

#32689 closed Bug (invalid)

Infinite AlterField Migrations due to default callable object missmatch

Reported by: Samuel Bishop Owned by: nobody
Component: Migrations Version: 3.2
Severity: Normal Keywords: infinite, makemigrations, migrations, callable, default, field
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Samuel Bishop)

makemigration will generate infinite migrations if the callable path is miss-matched such that you end up with two discreet callable objects (I'm still trying to identify where exactly the migration framework is generating this missmatch, I can see the difference stepping through MigrationAutodetector.generate_altered_fields in the debugger but I'm not sure if the root cause is inside generate_altered_fields, another method on MigrationAutodetector or somewhere else entirely...) then the same migration is generated every time you run the command which results in no actual change to the database.

In my case it's from a custom field library being used as a primary key.

I was importing from ulid.api import new as new_ulid and setting default=new_ulid, but unless I swap to import ulid.api.api and set default=ulid.api.api.Api.new I just get a new identical migration altering the field's default value to ulid.api.api.Api.new every time i run makemigrations

I would have expected either a clear warning detailing why my choice of callable object for the default= will cause issues, or I would have expected makemigrations to not mutate my callable like this.

Root cause: Turns out this is a serialisation fault. The migration serialiser doesn't throw any kind of error or warning and just mutates the callable instead of preventing me from using an invalid default callable that the migration framework wont be able to serialise. A simple wrapper function added to the custom field library solves the matter entirely, however the underlying issue of the migration framework behaviour is worth looking at improving.

Change History (5)

comment:1 by Samuel Bishop, 3 years ago

Description: modified (diff)

comment:2 by Samuel Bishop, 3 years ago

Description: modified (diff)

comment:3 by Mariusz Felisiak, 3 years ago

The migration serialiser doesn't throw any kind of error or warning and just mutates the callable instead of preventing me from using an invalid default callable that the migration framework wont be able to serialise. A simple wrapper function added to the custom field library solves the matter entirely, however the underlying issue of the migration framework behaviour is worth looking at improving.

Can you add more details about this callable (an example)? or maybe a sample project.

comment:4 by Pablo Prieto, 3 years ago

I was able to "reproduce" using sqlite as a DB backend with the following model:

from ulid.api import new as new_ulid
from django.db import models


# Create your models here.
class SampleModel(models.Model):

    ulid = models.CharField(
        max_length=255,
        null=False,
        default=new_ulid
    )

    def __str__(self):
        return str(self.ulid)

It's important to mention that the created migration is useless. It cannot be applied due to an exception calling new_ulid: TypeError: new() missing 1 required positional argument: 'self'.

Might be related to the fact that new() is an instance method: https://github.com/ahawker/ulid/blob/96bdb1daad7ce96f6db8c91ac0410b66d2e1c4c1/ulid/api/api.py#L51

If you create a proxy function, the migration is created and applied correctly:

from ulid.api import new as new_ulid
from django.db import models


def ulid_proxy():
    return new_ulid()


# Create your models here.
class SampleModel(models.Model):

    ulid = models.CharField(
        max_length=255,
        null=False,
        default=ulid_proxy
    )

    def __str__(self):
        return str(self.ulid)

So I'm not sure if this is a bug related on how migrations are created or if this is has something to do with how new() is implemented in ulid.api

comment:5 by Mariusz Felisiak, 3 years ago

Resolution: invalid
Status: newclosed

I don't think there is anything we can improve in Django. new is a different object each time it's imported, so Django properly detects this as change.

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