state.clone doesn't copy Django models

[Claude Paroz]

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?

[Claude Paroz]

Test to reproduce the problem:

tests/migrations/test_state.py

@@ -381 +381 @@
             {},
             None,
         ))
+        project_state.apps  # Fill the apps cached property
         other_state = project_state.clone()
         self.assertEqual(project_state, project_state)
         self.assertEqual(project_state, other_state)
         self.assertEqual(project_state != project_state, False)
         self.assertEqual(project_state != other_state, False)
+        self.assertNotEqual(project_state.apps, other_state.apps)
+        self.assertNotEqual(
+            project_state.apps.get_model('migrations', 'tag'),
+            other_state.apps.get_model('migrations', 'tag')
+        )
 
         # Make a very small change (max_len 99) and see if that affects it
         project_state = ProjectState()

[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)?

[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.

[Collin Anderson]

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

[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.

[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.

[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)

[Marten Kenbeek]

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?

[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.

[Marten Kenbeek]

​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.

[Claude Paroz]

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!