Code

Opened 4 years ago

Last modified 2 years ago

#14368 new Bug

Reverse relation attribute for OneToOneField fails when set to None

Reported by: gsakkis Owned by: nobody
Component: Database layer (models, ORM) Version: 1.2
Severity: Normal Keywords: OneToOneField, bug
Cc: gsakkis, subsume, gruszczy Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
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 gruszczy 4 years ago.
ticket-14368.patch (4.4 KB) - added by gsakkis 4 years ago.
Sets NULL the related_field of the existing child object (if any)

Download all attachments as: .zip

Change History (19)

comment:1 Changed 4 years ago by gsakkis

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 4 years ago by subsume

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 Changed 4 years ago by subsume

  • Cc subsume added

comment:4 Changed 4 years ago by gsakkis

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 follow-up: Changed 4 years ago by subsume

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.

comment:6 in reply to: ↑ 5 Changed 4 years ago by gsakkis

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 Changed 4 years ago by subsume

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 Changed 4 years ago by gruszczy

  • Cc gruszczy 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.

Changed 4 years ago by gruszczy

comment:9 Changed 4 years ago by gsakkis

  • 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

Changed 4 years ago by gsakkis

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

comment:10 Changed 4 years ago by gsakkis

  • Patch needs improvement unset

comment:11 Changed 4 years ago by gruszczy

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 Changed 3 years ago by gsakkis

  • Triage Stage changed from Accepted to Ready for checkin

comment:13 Changed 3 years ago by julien

  • Severity set to Normal
  • Type set to Bug

comment:14 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

comment:11 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:12 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:13 Changed 2 years ago by akaariai

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.