Self-Referential ForeignKeys broken using newforms form_for_instance with a formfield_callback

[danlarkin]

Problem arises when deepcopying a model that has a ForeignKey. I've attached a patch that adds a __deepcopy__ method to django.newforms.widgets.Widget that handles it gracefully.

[danlarkin]

patch against revision 6348

[danlarkin]

After Malcom asked me to write a test around this issue I realized it's a lot more complex than I had initially thought. "newforms-deepcopy.patch" does not solve the problem.

The error appears to happen only when using form_for_instance with the formfield_callback argument. I *think* writing a __deepcopy__ method for django.db.models.fields.Field solves the problem. I have written (and attached) three patches addressing this issue.

"db-models-fields-init.patch" adds a __deepcopy__ method to django.db.models.fields.Field
"newforms-forms.patch" fixes an issue I was having with AutoField not having a widget attribute, and raising an exception when it tried to access it
"tests-regressiontests-forms-models.patch" adds tests to display the issue. Without the above two patches the test fails, and with the patches it passes.

FYI, the patches are against trunk r6363

Additionally, the __deepcopy__ method might not be correct; it definitely needs someone familiar with the codebase to examine it and determine the implications. Works for me, though.

[jkocherhans]

This seems to affect ForeignKey and ManyToManyField, even if they aren't self-referential (at least on the newforms-admin branch). Also, danlarkin, could you put everything that's needed into a single patch? I'm having a hard time figuring out which one(s) to apply.

[Malcolm Tredinnick]

I'm travelling for work, so might not be able to get to this for a couple of days, so here are some notes from a quick read of the patches when danlarkin pointed me at them:

• View the __deepcopy__ implementation with some suspicion. If there's anything in the Field class that is a complex structure, such as a list or a dictionary, we need to make copies of those attributes directly, since copy.copy() only makes a new reference to them, not a new copy.
• My initial reaction was to wonder if we needed __deepcopy__ on the Field class or possibly just on something like django.db.models.field.related.Relation. It's worth spending a few minutes trying to put it in the right place in order to be clear. Basic rule is that if a class if going to have dynamic methods created by its own __init__ or other methods, it's a fair chance it will need a manual __deepcopy__ written.

[danlarkin]

@jkocherhans: I exercised bad form when uploaded the patches, I should have been a little more clear with naming them etc.

In the interest of not polluting this ticket with yet more attachments, though, I'll just tell you which patches to apply. db-models-fields-init.patch and newforms-forms.patch fix the problem (at a cursory level, anyway). tests-regressiontests-forms-models.patch adds a regression test to show off the problem (and fix). newforms-deepcopy.patch is wrong and useless (I wish I could delete attachments!).

@mtredinnick: You are exactly right about __deepcopy__. I don't have the level of familiarity with django necessary to know if/how many complex structures might end up inside a Field object, so someone else will have to take over on that front.

[jkocherhans]

Note to self, or anyone else for that matter: This appears to have been caused by [6156]. Specifically, ​here. Changing that line back to use copy instead of deepcopy makes the symptoms go away, but almost certainly doesn't address the problem. Still looking deeper at why that was done in the first place, and what the real solution should be.

[jkocherhans]

I'm fairly sure the problem I'm seeing, and the one danlarkin is reporting are different now. I've tracked mine down to django.contrib.admin.widgets.RelatedFieldWrapper which has a reference to an AdminSite instance among other things, and deepcopy just goes crazy from there trying to deepcopy all kinds of stuff. RelatedFieldWrapper should probably be immutable, so the naive fix is pretty simple.

However, other people writing custom fields and widgets are going to run into this stuff eventually. I think Widget is probably the right place to fix this though. By default, just do a copy, and let individual widgets override __deepcopy__ if they need to do something else.

@danlarkin: I have no idea why a form field would have a direct reference to a db field (the one I saw was eventually getting there, but that was about 5 levels deep). I'm wondering if the problem you're running into needs to be stopped somewhere higher up the call chain.

And, if I'm talking out of my ass, I'm sure Malcolm will put me in my place when he gets back. I'm not particularly confident in this discussion.

[James Bennett]

#5583 was a duplicate.

[jkocherhans]

(In [6415]) newforms-admin: Fixed a deepcopy bug in RelatedFieldWidgetWrapper. This should probably be addressed in the base Widget class in trunk, but I'm not going to make that call. Refs #5505.

[danlarkin]

jkocherhans' commit fixes the way I originally observed the deepcopy problem.

[Maciej Wiśniowski]

One more important thing is python version you're using.

I've found that under python2.4 copy.deepcopy fails with lambda functions. Same thing works with python 2.5.

Python2.4:

Python 2.4.4 (#2, Apr 12 2007, 21:22:23)
[GCC 4.1.2 (Ubuntu 4.1.2-0ubuntu4)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import copy
>>> class Tester(object):
...     pass
...
>>> a = Tester()
>>> a.func = lambda p: p + 1
>>>
>>> b = copy.deepcopy(a, {})
Traceback (most recent call last):
  File "<stdin>", line 1, in ?
  File "/usr/lib/python2.4/copy.py", line 204, in deepcopy
    y = _reconstruct(x, rv, 1, memo)
  File "/usr/lib/python2.4/copy.py", line 351, in _reconstruct
    state = deepcopy(state, memo)
  File "/usr/lib/python2.4/copy.py", line 174, in deepcopy
    y = copier(x, memo)
  File "/usr/lib/python2.4/copy.py", line 268, in _deepcopy_dict
    y[deepcopy(key, memo)] = deepcopy(value, memo)
  File "/usr/lib/python2.4/copy.py", line 204, in deepcopy
    y = _reconstruct(x, rv, 1, memo)
  File "/usr/lib/python2.4/copy.py", line 336, in _reconstruct
    y = callable(*args)
  File "/usr/lib/python2.4/copy_reg.py", line 92, in __newobj__
    return cls.__new__(cls, *args)
TypeError: function() takes at least 2 arguments (0 given)
>>> 

Python2.5:

Python 2.5.1 (r251:54863, May  2 2007, 16:27:44)
[GCC 4.1.2 (Ubuntu 4.1.2-0ubuntu4)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import copy
>>> class Tester(object):
...     pass
...
>>> a = Tester()
>>> a.func=lambda p: p + 1
>>> b = copy.deepcopy(a, {})
>>> b.func
<function <lambda> at 0x2aedfa332aa0>
>>>

Maybe there should be a kind of try: except: fallback for using copy instead of deepcopy?

[Malcolm Tredinnick]

I need to test some things on an earlier Python version (2.5 is a bit too robust and makes replicating the problem difficult), but here are a few notes:

1. The test case is a bit broken. The formfield_callback doesn't return the right thing (a form field), so that is triggering some other problems. It should be something similar to the default in newforms/models.py, otherwise we are testing something that isn't intended to work.
2. newforms-forms.patch is only needed because of the problem in the previous comment. Fixing the callback in the test to return the right sort of thing removes the issue. All form fields should have a widget attached, so that patch is fixing the wrong place; fortunately, we can omit it.
3. Joseph Kocherhan's feelings are pretty much correct about how Widget.__deepcopy__ should behave. I have a patch that mostly fixes that which I need to test against an earlier Python version that I have to build on my laptop.

With reference to the previous comment, we're not going to put in alternate behaviour for Python 2.4 and 2.5. That would make the results inconsistent (dependent on the Python version). We should always have the same behaviour, regardless of Python version. That's what compatibility is all about.

[Malcolm Tredinnick]

(In [6450]) Added a deepcopy() method to the Widget class in order to avoid a number of easy-to-trigger problems when copying Widget subclasses. Subclasses which are intended to have extra mutable fields should override this method. Refs #5505.

[Malcolm Tredinnick]

Corrected version of Dan Larkin's test

[Malcolm Tredinnick]

I must admit to being a bit confused now. Using a version of Dan's test that meets the required interface (attached), I cannot see any problem when I am using python 2.3, 2.4 or 2.5. It also doesn't make a difference whether or not I include the change in [6450].

I'm going to close this now, since I think [6450] probably helps to fix the meta-issue in any case. If somebody comes up with a reliable way to reproduce whatever the problem is that people were seeing, please reopen and attach instructions (or preferably a test case).

[lukas@…]

Works for me now.