Reverse relation attribute for OneToOneField fails when set to None

[George Sakkis]

Initially reported by reopening #12420 (​http://code.djangoproject.com/ticket/12420#comment:10), moved to a new ticket after the assignee's suggestion.

Test case with the sample models used at #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'

The offending lines are ​http://code.djangoproject.com/browser/django/trunk/django/db/models/fields/related.py#L258 and ​http://code.djangoproject.com/browser/django/trunk/django/db/models/fields/related.py#L264. It's clearly a bug because the comment at ​http://code.djangoproject.com/browser/django/trunk/django/db/models/fields/related.py#L238 explicitly mentions "If null=True, we can assign null here".

[Yeago]

I'm not really sure this is a bug. What are you hoping to achieve by setting the accessor to be None?

If you have A.attribute which points to B, the information is still stored on B and so changes need to be made in the context of B.

[George Sakkis]

At the very least, I expect a well-thought out exception message that explains what happened and why, like those checked for a few lines above:

if value is None and self.related.field.null == False:
    raise ValueError('Cannot assign None: "%s.%s" does not allow null values.' %
 	                                (instance._meta.object_name, self.related.get_accessor_name()))
elif value is not None and not isinstance(value, self.related.model):
    raise ValueError('Cannot assign "%r": "%s.%s" must be a "%s" instance.' %
          (value, instance._meta.object_name,
 	   self.related.get_accessor_name(), self.related.opts.object_name))

The fact that there is no such message for this case means that it was missed, it's not a deliberate choice.

As for what it should happen, I think it should do the equivalent of what the ForeignRelatedObjectsDescriptor does for foreign keys: make A.link_to_B = None, B.link_to_A = None and B.save(), thus breaking the relation between A and B (we're always talking about a null=True OneToOneField).

[Yeago]

I disagree. Do you expect a message when you do: instance.queryset_set = SomeQueryset.objects.all()?

You're explicitly defining an accessor and expecting the ORM to know what you mean. In your example, soul isn't a field on Bob so stop treating it like it is one. person is a field on Soul.

[George Sakkis]

I'm baffled, your comment doesn't make any sense.

I disagree. Do you expect a message when you do: instance.queryset_set = SomeQueryset.objects.all()?

Even if setting to None is not allowed, are you seriously against an error message that explains why, when ten lines above it does exactly that for non-nullable fields? And I have no idea what the instance.queryset_set = SomeQueryset.objects.all() analogy is supposed to mean; it's perfectly valid code that does work, so I don't expect any message.

You're explicitly defining an accessor and expecting the ORM to know what you mean. In your example, soul isn't a field on Bob so stop treating it like it is one. person is a field on Soul.

So bob.soul = None is "treating it like a field" but bob.soul = Soul.objects.create() doesn't ?

[Yeago]

Wow. I just tried bob.soul = Soul.objects.create() and I must admit you're right. I've never seen that syntax and I'm quite shocked it exists. Unfortunate really.

[Filip Gruszczyński]

I have created a unit test for this and a patch, but I don't know, if it is any good. I have several concerns. The test looks like this:

        first = First.objects.create()
        second = Second.objects.create()
        first.second = second
        first.second = None
        self.assertRaises(Second.DoesNotExist, getattr, first, 'second')
        self.assert_(second.first is None)

and I believe this is an expected behaviour.

However, if we take the snippet from this ticket:

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

then setting bob.soul to None doesn't have any effect, because bob.soul is created by a Query, so without changing data in the database you can't really affect it. I will post my patch and if someone could point me, how it should work, I will be glad to change it the way it should work.

[George Sakkis]

According to the discussion on django-dev [1], updating the database is the right thing to do and is consistent with the behaviour of ForeignKey for the analogous operation.

Also the second assertion (self.assert_(second.first is None)) is impossible to enforce in general; at best you can do it for the existing cached related instance of first. For example the second_copy below will become stale, there is no way to magically update it:

        first = First.objects.create()
        second = Second.objects.create()
        first.second = second
        second.save()
        second_copy = Second.objects.get(pk=second.pk)
        self.assert_(second_copy.first is not None)
        first.second = None
        # this can be made to succeed
        self.assert_(second.first is None)
        # but this cannot
        self.assert_(second_copy.first is None)

I'm working on a patch too, will hopefully post it later today.

[1] ​http://groups.google.com/group/django-developers/tree/browse_frm/thread/634499444687556f#doc_f1a254e8bf4ad8d9

[George Sakkis]

Sets NULL the related_field of the existing child object (if any)

[Filip Gruszczyński]

Thanks for the comment. I'll download your patch and take a look at how it should have been done, so next time I can provide a better one.

[Jacob]

Milestone 1.3 deleted

[Aymeric Augustin]

Change UI/UX from NULL to False.

[Aymeric Augustin]

Change Easy pickings from NULL to False.

[Anssi Kääriäinen]

The patch does no longer apply, and thus this isn't ready for commit.

I don't like the idea of doing an implicit save when assigning to the o2o field. Explicit saves are better. I also wrote to django-developers about removing the implicit save on foreign key assignments: ​http://groups.google.com/group/django-developers/browse_thread/thread/4558af28adef5271

[Tim Graham]

This still crashes as reported in the description.

I made an updated ​PR which doesn't have the implicit save of the original patch.

[Simon Charette]

As noted on the ticket the changes and tests provided by Tim LGTM!

[Tim Graham <timograham@…>]

In 384ddbe:

Fixed #14368 -- Allowed setting a reverse OneToOne relation to None.

[Tim Graham <timograham@…>]

In b646fbe:

[1.9.x] Fixed #14368 -- Allowed setting a reverse OneToOne relation to None.

Backport of 384ddbec1b73a4636f234da3894fde8f8420bb63 from master