Opened 13 years ago

Closed 8 years ago

Last modified 8 years ago

#14368 closed Bug (fixed)

Reverse relation attribute for OneToOneField fails when set to None

Reported by: George Sakkis Owned by: nobody
Component: Database layer (models, ORM) Version: 1.2
Severity: Normal Keywords: OneToOneField, bug
Cc: George Sakkis, Yeago, Filip Gruszczyński 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

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

Attachments (2)

14368.patch (3.4 KB ) - added by Filip Gruszczyński 13 years ago.
ticket-14368.patch (4.4 KB ) - added by George Sakkis 13 years ago.
Sets NULL the related_field of the existing child object (if any)

Download all attachments as: .zip

Change History (23)

comment:1 by George Sakkis, 13 years ago

Triage Stage: UnreviewedAccepted

comment:2 by Yeago, 13 years ago

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.

comment:3 by Yeago, 13 years ago

Cc: Yeago added

comment:4 by George Sakkis, 13 years ago

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

comment:5 by Yeago, 13 years ago

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.

in reply to:  5 comment:6 by George Sakkis, 13 years ago

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 ?

comment:7 by Yeago, 13 years ago

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.

comment:8 by Filip Gruszczyński, 13 years ago

Cc: Filip Gruszczyński added

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.

by Filip Gruszczyński, 13 years ago

Attachment: 14368.patch added

comment:9 by George Sakkis, 13 years ago

Has patch: set
Patch needs improvement: set

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

by George Sakkis, 13 years ago

Attachment: ticket-14368.patch added

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

comment:10 by George Sakkis, 13 years ago

Patch needs improvement: unset

comment:11 by Filip Gruszczyński, 13 years ago

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.

comment:12 by George Sakkis, 13 years ago

Triage Stage: AcceptedReady for checkin

comment:13 by Julien Phalip, 13 years ago

Severity: Normal
Type: Bug

comment:14 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

comment:11 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:12 by Aymeric Augustin, 12 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:13 by Anssi Kääriäinen, 12 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

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

comment:14 by Tim Graham, 8 years ago

Patch needs improvement: unset

This still crashes as reported in the description.

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

comment:15 by Simon Charette, 8 years ago

Triage Stage: AcceptedReady for checkin

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

comment:16 by Tim Graham <timograham@…>, 8 years ago

Resolution: fixed
Status: newclosed

In 384ddbe:

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

comment:17 by Tim Graham <timograham@…>, 8 years ago

In b646fbe:

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

Backport of 384ddbec1b73a4636f234da3894fde8f8420bb63 from master

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