[Django] #24241: state.clone doesn't copy Django models
Django
--------------------------------------+---------------------------
Reporter: claudep | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.8alpha1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
--------------------------------------+---------------------------
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/24241>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: claudep | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.8alpha1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by claudep):
Test to reproduce the problem:
{{{
#!diff
diff --git a/tests/migrations/test_state.py
b/tests/migrations/test_state.py
index a7dbdbe..af440b9 100644
--- a/tests/migrations/test_state.py
+++ b/tests/migrations/test_state.py
@@ -381,11 +381,18 @@ class StateTests(TestCase):
{},
None,
))
+ project_state.apps # Fill the apps cached property
+ import pdb; pdb.set_trace()
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()
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24241#comment:1>
Django
Reporter: claudep | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.8alpha1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by coldmind):
{{{
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)?
--
Ticket URL: <https://code.djangoproject.com/ticket/24241#comment:2>
Django
Reporter: claudep | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.8alpha1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by coldmind):
* cc: sokandpal@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/24241#comment:3>
Django
Reporter: claudep | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.8alpha1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by coldmind):
I will try to do some research. But it requires to do some python magic on
a model class
--
Ticket URL: <https://code.djangoproject.com/ticket/24241#comment:4>
Django
Reporter: claudep | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.8alpha1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by collinanderson):
It might work to define `__deepcopy__` on Model's `__metaclass__`.
--
Ticket URL: <https://code.djangoproject.com/ticket/24241#comment:5>
Django
Reporter: claudep | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.8alpha1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by akaariai):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24241#comment:6>
Django
Reporter: claudep | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.8alpha1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by MarkusH):
* cc: MarkusH (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/24241#comment:7>
Django
Reporter: claudep | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.8alpha1
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by MarkusH):
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/24241#comment:8>
Django
Reporter: claudep | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.8alpha1
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by coldmind):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24241#comment:9>
Django
Reporter: claudep | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.8alpha1
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by coldmind):
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)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24241#comment:10>
Django
Reporter: claudep | Owner: knbk
Type: Bug | Status: assigned
Component: Migrations | Version: 1.8alpha1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by knbk):
* owner: nobody => knbk
* status: new => assigned
Comment:
I've created a patch [https://github.com/knbk/django/tree/ticket_24241_1_8
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/24241#comment:11>
Django
Reporter: claudep | Owner: knbk
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by MarkusH):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24241#comment:12>
Django
Reporter: claudep | Owner: knbk
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by knbk):
* has_patch: 0 => 1
* version: 1.8alpha1 => master
Comment:
[https://github.com/django/django/pull/4049 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24241#comment:13>
Django
Reporter: claudep | Owner: knbk
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* version: master => 1.8alpha1
--
Ticket URL: <https://code.djangoproject.com/ticket/24241#comment:14>
Django
Reporter: claudep | Owner: knbk
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by coldmind):
* has_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/24241#comment:15>
Django
Reporter: claudep | Owner: knbk
Component: Migrations | Version: 1.8alpha1
Severity: Release blocker | Resolution: wontfix
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* status: assigned => closed
* resolution: => wontfix
Comment:
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!
--
Ticket URL: <https://code.djangoproject.com/ticket/24241#comment:16>