Opened 4 years ago

Closed 4 years ago

#24241 closed Bug (wontfix)

state.clone doesn't copy Django models

Reported by: Claude Paroz Owned by: Marten Kenbeek
Component: Migrations Version: 1.8alpha1
Severity: Release blocker Keywords:
Cc: sokandpal@…, Markus Holtermann Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

While debugging #24225, I noticed that StateApps.clone(), even if it calls copy.deepcopy(self.all_models), the all_models structure is only partially deecopied.

all_models looks like:

defaultdict(<class 'collections.OrderedDict'>, {'migrations': OrderedDict([('tag', <class 'Tag'>)])})

Unfortunately, the <class 'Tag'> class is identical in the original and the copy (and what's more important, its _meta content), which is a potential cause of difficult-to-debug bugs. Hints?

Change History (16)

comment:1 Changed 4 years ago by Claude Paroz

Test to reproduce the problem:

  • tests/migrations/test_state.py

    diff --git a/tests/migrations/test_state.py b/tests/migrations/test_state.py
    index a7dbdbe..af440b9 100644
    a b class StateTests(TestCase): 
    381381            {},
    382382            None,
    383383        ))
     384        project_state.apps  # Fill the apps cached property
    384385        other_state = project_state.clone()
    385386        self.assertEqual(project_state, project_state)
    386387        self.assertEqual(project_state, other_state)
    387388        self.assertEqual(project_state != project_state, False)
    388389        self.assertEqual(project_state != other_state, False)
     390        self.assertNotEqual(project_state.apps, other_state.apps)
     391        self.assertNotEqual(
     392            project_state.apps.get_model('migrations', 'tag'),
     393            other_state.apps.get_model('migrations', 'tag')
     394        )
    389395
    390396        # Make a very small change (max_len 99) and see if that affects it
    391397        project_state = ProjectState()
Last edited 4 years ago by Claude Paroz (previous) (diff)

comment:2 Changed 4 years ago by Andriy Sokolovskiy

ipdb> id(self.all_models['migrations']['tag'])
140327793908816
ipdb> id(clone.all_models['migrations']['tag'])
140327793908816
ipdb> other_state.apps.all_models['migrations']['tag']
<class 'Tag'>
ipdb> coopy = deepcopy(other_state.apps.all_models['migrations']['tag'])
ipdb> id(other_state.apps.all_models['migrations']['tag'])
140635957997264
ipdb> id(coopy)
140635957997264

Deep copying only applies to instance level attributes but not class level.
Maybe we should write own tool for deep copy to be able to copy class (not class instance)?

comment:3 Changed 4 years ago by Andriy Sokolovskiy

Cc: sokandpal@… added

comment:4 Changed 4 years ago by Andriy Sokolovskiy

I will try to do some research. But it requires to do some python magic on a model class

UPD:
Unfortunately, __deepcopy__ defines behavior for copy.deepcopy() for instances of class, so it should be resolved in another way.

Last edited 4 years ago by Andriy Sokolovskiy (previous) (diff)

comment:5 Changed 4 years ago by Collin Anderson

It might work to define __deepcopy__ on Model's __metaclass__.

comment:6 Changed 4 years ago by Anssi Kääriäinen

My experience from QuerySet deepcopy removal is that it is better to write a custom method for this. Deepcopy is slow and it is hard to see what exactly gets copied; and if you restrict it, then some other user of deepcopy doesn't actually get a true deepcopy.

comment:7 Changed 4 years ago by Markus Holtermann

Cc: Markus Holtermann added

comment:8 Changed 4 years ago by Markus Holtermann

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

comment:9 Changed 4 years ago by Andriy Sokolovskiy

I tried smth like this

        models_to_copy = self.all_models
        for apps in models_to_copy.values():
            for model in apps.values():
                model = type(model.__name__, (model,), {'__module__': model.__module__})

But, some models (e.g. ContentType) have __fake__ as module, so it is not present in sys.modules and model metaclass can not work.
And, am I going in the right way? I think we should have an util for this specific issue.

comment:10 Changed 4 years ago by Andriy Sokolovskiy

More reliable to copy class, but still no ideal

dict_copy = SomeDjangoModel.__dict__.copy()
del dict_copy[‘_meta’]
type(SomeDjangoModel.__name__, SomeDjangoModel.__bases__, dict_copy)

comment:11 Changed 4 years ago by Marten Kenbeek

Owner: changed from nobody to Marten Kenbeek
Status: newassigned

I've created a patch on github, though it either exposes or creates a bug (I would say expose) when copying a ProjectState that contains models without any managers, not even a default one. ModelState.from_model can't handle models without managers.

I'm quite new to this. Would it be better to submit this patch and create a new ticket for the other bug, or should I wait because this patch causes additional tests to fail?

comment:12 Changed 4 years ago by Markus Holtermann

Thanks knbk! The patch looks sensible at the first glance. Could you create if for the master branch (ie 1.9) and open a pull request.

Regarding the manager problem: could you open a new ticket with a failing test case that demonstrates the problem.

comment:13 Changed 4 years ago by Marten Kenbeek

Has patch: set
Version: 1.8alpha1master

Pull request

Note that I removed the explicit assignment to clone[app_label][model_name]. ModelBase.__new__ calls apps.register_model, in which this assignment is the very first line.

comment:14 Changed 4 years ago by Tim Graham

Version: master1.8alpha1

comment:15 Changed 4 years ago by Andriy Sokolovskiy

Has patch: unset

comment:16 Changed 4 years ago by Claude Paroz

Resolution: wontfix
Status: assignedclosed

After more thoughts, I think now that models not being copied is not necessarily a bug, but may be part of the optimization process. As my patch for #24225 shows, state_forwards methods have to take care of re-rendering models which are potentially changed by the migration operation. I think it's the price to pay for migration optimization.

Sorry for the time spent trying to create a patch!

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