Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#25563 closed Bug (fixed)

Deferred models should be cached in their proxied model app

Reported by: Andriy Sokolovskiy Owned by: Simon Charette
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: me@… 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

Consider simple model and custom manager:

from django.db import models


class CustomManager(models.Manager):
    use_in_migrations = True

    def get_queryset(self, *args, **kwargs):
        return super(CustomManager, self).get_queryset(
            *args, **kwargs
        ).defer('test')


class ModelA(models.Model):
    test = models.CharField(blank=True, max_length=123)
    objects = CustomManager()

Also there is a simple test:

from django.test import TestCase
from .models import ModelA


class Test(TestCase):
    def test_defer_bug(self):
        ModelA.objects.create(test='test')
        self.assertIsInstance(ModelA.objects.last(), ModelA)

With initial migration test is passing well. But, when adding a datamigration like this (where test is evaluating a queryset, which is using custom manager with .defer() inside):

# -*- coding: utf-8 -*-
from __future__ import unicode_literals

from django.db import models, migrations


def test_forwards(apps, schema_editor):
    ModelA = apps.get_model('blankapp', 'ModelA')
    list(ModelA.objects.all())


def noop(apps, schema_editor):
    return


class Migration(migrations.Migration):

    dependencies = [
        ('blankapp', '0001_initial'),
    ]

    operations = [
        migrations.RunPython(test_forwards, noop)
    ]

test is failing like:

======================================================================
FAIL: test_defer_bug (blankapp.tests.Test)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/projpath/blank/blankapp/tests.py", line 8, in test_defer_bug
    self.assertIsInstance(ModelA.objects.last(), ModelA)
AssertionError: <ModelA_Deferred_test: ModelA_Deferred_test object> is not an instance of <class 'blankapp.models.ModelA'>

This is causing tests failures, when, for example, test contains FK assignment (there is isinstance check).

Test if failing from
https://github.com/django/django/commit/aa5ef0d4fc67a95ac2a5103810d0c87d8c547bac

Change History (11)

comment:1 Changed 4 years ago by Andriy Sokolovskiy

Summary: Datamigration causing incorrect behavior when custom manager is using .defer()Datamigration causing incorrect tests behavior when custom manager is using .defer()

comment:2 Changed 4 years ago by Tim Graham

Summary: Datamigration causing incorrect tests behavior when custom manager is using .defer()Data migration causes incorrect tests behavior when custom manager is using .defer()
Triage Stage: UnreviewedAccepted

That's a strange one. Could it be caused by models caching in django.apps.Apps?

comment:3 Changed 4 years ago by Simon Charette

Component: MigrationsDatabase layer (models, ORM)
Owner: changed from nobody to Simon Charette
Status: newassigned
Version: 1.8master

The issue lies in django.db.models.query_utils.deferred_class_factory, it should be using the apps of the provided model instead of the global one.

I will submit a patch in a few minutes.

comment:4 Changed 4 years ago by Simon Charette

Summary: Data migration causes incorrect tests behavior when custom manager is using .defer()Deferred models should be cached in their proxied model app

comment:5 Changed 4 years ago by Andriy Sokolovskiy

Will it will be backported to 1.8? (You have changed version to master)

comment:6 Changed 4 years ago by Simon Charette

Has patch: set

Submited a patch here. It would be great if you could test it Andriy.

Will it will be backported to 1.8? (You have changed version to master)

I guess it should be backported in 1.8, what do you think Tim?

comment:7 Changed 4 years ago by Andriy Sokolovskiy

Thanks for a patch, Simon.
I applied it to django1.8 and tests from my project passed fine.

comment:8 Changed 4 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

Looks okay to backport, just add release notes please.

comment:9 Changed 4 years ago by Simon Charette <charette.s@…>

Resolution: fixed
Status: assignedclosed

In 3db3ab71:

Fixed #25563 -- Cached deferred models in their proxied model's _meta.apps.

Thanks to Andriy Sokolovskiy for the report and Tim Graham for the review.

comment:10 Changed 4 years ago by Simon Charette <charette.s@…>

In 522b0bc:

[1.9.x] Fixed #25563 -- Cached deferred models in their proxied model's _meta.apps.

Thanks to Andriy Sokolovskiy for the report and Tim Graham for the review.

Backport of 3db3ab71e97d34260057a6f51d4b2f72da30dc8d from master

comment:11 Changed 4 years ago by Simon Charette <charette.s@…>

In 7196262:

[1.8.x] Fixed #25563 -- Cached deferred models in their proxied model's _meta.apps.

Thanks to Andriy Sokolovskiy for the report and Tim Graham for the review.

Backport of 3db3ab71e97d34260057a6f51d4b2f72da30dc8d from master

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