Opened 8 years ago

Closed 8 years ago

#27311 closed New feature (wontfix)

Support unpickled models (e.g. read from cache) in migrations

Reported by: Odero Owned by: nobody
Component: Migrations Version: dev
Severity: Normal Keywords: migrations, foreignkey, cache
Cc: Markus Holtermann Triage Stage: Someday/Maybe
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Markus Holtermann)

Trying to create the migration below which is failing. It seems like the same issue discussed and resolved in #24282.

def generate_invoices(apps, schema_editor):
    A = apps.get_model('app1', 'A')
    B = apps.get_model('app1', 'B')
    User = apps.get_model('accounts', 'User')

    a = A.objects.last()

    b = B(
        client=a.user,
        status=5
    )
    b.save()

Also tried

    b = B(
        client=User.objects.get(pk=a.user.pk),
        status=5
    )

In both cases I get the error:

Cannot assign "<User: Some User>": "B.client" must be a "User" instance.

Investigating the issue shows that it's due to the models being read from cache.

Change History (8)

comment:1 by Odero, 8 years ago

Description: modified (diff)

comment:2 by Odero, 8 years ago

Description: modified (diff)

comment:3 by Tim Graham, 8 years ago

Component: UncategorizedMigrations
Description: modified (diff)

I can't reproduce this. Could you provide a sample project and confirm your Django version?

comment:4 by Odero, 8 years ago

I discovered that it was a caching issue. I have caching enabled by default. It seems that whenever the item is fetched it creates and returns the cached version which is of the *real* type i.e. the one you'd get by doing

from app1.models import A

which is different from what you'd get by doing

apps.get_model('app1', 'A')

comment:5 by Tim Graham, 8 years ago

What does it mean to have caching enabled by default? Is it a third-party app?

comment:6 by Odero, 8 years ago

I meant automatic caching. I have set up django-cacheops to auto-manage the caching for me. BUT, even without cacheops, just manual caching also causes the error. Here's a contrived example to illustrate this effect and which fails when using cached data.

def generate_invoices(apps, schema_editor):
    A = apps.get_model('app1', 'A')
    B = apps.get_model('app1', 'B')
    User = apps.get_model('accounts', 'User')

    for item in A.objects.all():
        u = User.objects.get(pk=item.user.pk)

        # Simulate fetching cached data
        # start caching-segment
        cache.set('user', u)
        u = cache.get('user')
        # end caching-segment

        b = B(client=u, status=1)
        b.save()

If I remove those 2 lines in the "caching-segment", thereby eliminating caching, the migration runs successfully.

comment:7 by Tim Graham, 8 years ago

Cc: Markus Holtermann added

I guess it probably infeasible or impractical to try to allow that to work, but CCing Markus for a second opinion.

comment:8 by Markus Holtermann, 8 years ago

Description: modified (diff)
Keywords: cache added
Resolution: wontfix
Status: newclosed
Summary: Assigning ForeignKey fields in migrationsSupport unpickled models (e.g. read from cache) in migrations
Triage Stage: UnreviewedSomeday/Maybe
Type: BugNew feature
Version: 1.10master

This is not really feasible as the unpickling of models from cache uses the global app registry (https://github.com/django/django/blob/d5c0eb7d686ea89d6dbf787abc5bf18302c4e428/django/db/models/base.py#L1770). We would need to look up the models from the app registry at this state of the migration process. I have no idea how we could achieve that w/o passing apps around all the time.

I also think it's not a good idea to cache objects during the migration process. You can easily end up with objects from previous model states that are completely invalid at a later stage because too many things changed in between that wasn't taken care of for cached model instances.

I'm closing this as won't fix for the time being. If this is possible at some point we could re-evaluate the feature, but for now I doubt it's going to work out.

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