Opened 13 years ago

Closed 13 years ago

Last modified 12 years ago

#12420 closed (fixed)

"OneToOneField doesn't allow assignment of None"

Reported by: Kieran Brownlees Owned by: Andrii Kurinnyi
Component: Database layer (models, ORM) Version: dev
Severity: Keywords:
Cc: Ryan Nowakowski, mortas.11@…, George Sakkis 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

In http://code.djangoproject.com/browser/django/trunk/django/forms/models.py (R:11874)
line 53 it states:

53 	        # OneToOneField doesn't allow assignment of None. Guard
against that
54 	        # instead of allowing it and throwing an error.
55 	        if isinstance(f, models.OneToOneField) and cleaned_data
[f.name] is None:
56 	            continue

Further reading of the function SingleRelatedObjectDescriptor which
seems to implement most of the OTOF functionality actually states:

http://code.djangoproject.com/browser/django/trunk/django/db/models/fields/related.py (R:11878)

207 	        # If null=True, we can assign null here, but otherwise
the value needs
208 	        # to be an instance of the related class.
209 	        if value is None and self.related.field.null == False:

It seems that when the code in models.py was written it was by someone who simply had null=False set, which meant that the OTOF (correctly) could not have a None value.

I found the issue when trying to set a OTOF as null in an admin form it says "saved successfully" but never changes the value.

Attachments (6)

t12420.diff (741 bytes) - added by Andrii Kurinnyi 13 years ago.
t12420_1.diff (3.0 KB) - added by Andrii Kurinnyi 13 years ago.
Better patch with tests
t12420_2.diff (3.0 KB) - added by Andrii Kurinnyi 13 years ago.
Changed name of the test method to be more descriptive
t12420_3.diff (3.0 KB) - added by Andrii Kurinnyi 13 years ago.
Added assert for form.is_valid()
t12420_4.diff (3.7 KB) - added by Andrii Kurinnyi 13 years ago.
t12420_5.diff (3.7 KB) - added by Andrii Kurinnyi 13 years ago.
Updated patch to r12403

Download all attachments as: .zip

Change History (22)

comment:1 Changed 13 years ago by Ryan Nowakowski

Cc: Ryan Nowakowski added

comment:2 Changed 13 years ago by Althalus

Cc: mortas.11@… added
Triage Stage: UnreviewedAccepted

comment:3 Changed 13 years ago by Althalus

Version: 1.1SVN

I've just run into the issue. Doesn't work properly in SVN too.

Changed 13 years ago by Andrii Kurinnyi

Attachment: t12420.diff added

comment:4 Changed 13 years ago by Alex Gaynor

Please change

f.null == False

to

not f.null

Also, needs tests.

comment:5 Changed 13 years ago by Alex Gaynor

Has patch: set
milestone: 1.2
Needs tests: set
Patch needs improvement: set

comment:6 Changed 13 years ago by Andrii Kurinnyi

Owner: changed from nobody to Andrii Kurinnyi

Changed 13 years ago by Andrii Kurinnyi

Attachment: t12420_1.diff added

Better patch with tests

Changed 13 years ago by Andrii Kurinnyi

Attachment: t12420_2.diff added

Changed name of the test method to be more descriptive

Changed 13 years ago by Andrii Kurinnyi

Attachment: t12420_3.diff added

Added assert for form.is_valid()

Changed 13 years ago by Andrii Kurinnyi

Attachment: t12420_4.diff added

comment:7 Changed 13 years ago by Alex Gaynor

Needs tests: unset
Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

Changed 13 years ago by Andrii Kurinnyi

Attachment: t12420_5.diff added

Updated patch to r12403

comment:8 Changed 13 years ago by jkocherhans

Resolution: fixed
Status: newclosed

(In [12545]) Fixed #12420. Now that OneToOneField allows assignment of None, stop guarding against it in ModelForms. Thanks, andrewsk.

comment:9 Changed 13 years ago by jkocherhans

In case anyone wonders, I didn't backport this because the tests fail on 1.1.X. I'm guessing that the change to OneToOneField that made this possible on trunk isn't on that branch.

comment:10 Changed 13 years ago by George Sakkis

Resolution: fixed
Status: closedreopened

comment:11 Changed 13 years ago by George Sakkis

Cc: George Sakkis added

comment:12 Changed 13 years ago by Andrii Kurinnyi

@gsakkis Is it related to #14043? Any test cases to trigger the error?

comment:13 Changed 13 years ago by George Sakkis

I don't think it's related, this dies fast and hard; I'm surprised it hasn't been noticed before. Using the models of #14043:

In [6]: bob = Person.objects.create(age=34)
In [7]: bobs_soul = Soul.objects.create(person=bob)
In [8]: bob.soul == bobs_soul
Out[8]: True
In [9]: bob.soul = None
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)

--> 258         setattr(value, self.related.field.attname, getattr(instance, self.related.field.rel.get_related_field().attname))
    259 
    260         # Since we already know what the related object is, seed the related


AttributeError: 'NoneType' object has no attribute 'person_id'


comment:14 Changed 13 years ago by Andrii Kurinnyi

Resolution: fixed
Status: reopenedclosed

@gsakkis - bob.soul from your example is a reverse relation.
Person table doesn't actually have soul_id. So, the way Django deals with it,
if you assign an instance of Soul (i.e. bobs_soul) to bob.soul, it modifies bobs_soul.person_id,
so it will be equal to bob.id (line 258). But, then you still need to save "bobs_soul" object, not
"bob". Which might be a bit counter intuitive, because it looks like we are trying to modify "bob"
object.
Now in case when we are trying to assign None, to bob.soul, it fails to modify assigned value, obviously,
because it is not an instance of a Soul.
If our intention is to severe the relation between objects, we can speculate about two approaches here:

  1. Check for the fact that the value is None, and don't do anything. But in this case assignment of None wouldn't change anything after saving any of our object to db. (So we didn't reach our goal)
  2. Retrieve previous value of "bobs_soul" and modify it's place_id to be None. The problem here is that we will be creating new instance of "bobs_soul", then modifying it, but it wouldn't be available outside of the descriptor. So, there is no way to save this newly created instance. (Unless we will be in fact modifying object from related object's cache, and this object is still referenced by user. Like in your example, you have bobs_soul created before, so with the second approach user will be able to do bobs_soul.save() after bob.soul=None, and it will break the relation)

Both of these approaches doesn't allow us to break the relation between objects. So, maybe the best way is not to allow assignment
on the reverse relation attribute for OneToOneField, or do not allow assignment of None, with proper error message, telling that
an assignment should be performed on the other end.

Anyways. The original intention of this ticket was to fix the bug which didn't allow to assign None to OneToOneFields in model forms.
In my opinion, the issue with reverse relation attribute should be moved to a separate ticket.

comment:15 Changed 13 years ago by George Sakkis

Fair enough, opened a new ticket at #14368.

comment:16 Changed 12 years ago by Jacob

milestone: 1.2

Milestone 1.2 deleted

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