Opened 5 years ago

Closed 5 years ago

Last modified 7 months ago

#12708 closed Cleanup/optimization (invalid)

Django raises DoesNotExist when consulting an empty ForeignKey field

Reported by: lsaffre Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: ForeignKey DoesNotExist
Cc: simon@… Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

"""

Django raises DoesNotExist when consulting an empty ForeignKey field

We create an Order without a Journal::
  
  >>> o = Order()
  >>> print o.journal
  None
  >>> o.save()
 
This works only because `Order.journal` has `null=True`.
In fact I want the `save()` method to complain if `journal` is empty,
so I remove `null=True` from the field definition.

  >>> o = Order2()
  >>> print o.journal
  None
  
The above line raises a DoesNotExist exception. Django should raise 
an exception only when I try to save the instance, not already when 
I want to see the value of `journal`!

"""

from django.db import models
   
class Journal(models.Model):
    pass

class Order(models.Model):
    journal = models.ForeignKey(Journal,null=True)

class Order2(models.Model):
    journal = models.ForeignKey(Journal)

Change History (7)

comment:1 Changed 5 years ago by lukeplant

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed

I think the behaviour is consistent. If the journal_id attribute was set to an id which did not exist in the database, then you would get the same DoesNotExist behaviour. It is also symmetric with the behaviour of trying to set o.journal, which results in an exception.

To put simply, None is never the value of non-nullable foreign key object - you cannot set or get None as the value. If you want the foreign key value, rather than the object, use journal_id, not journal.

I've persuaded myself that this behaviour is intended and good, and changing it could be a significant backwards incompatibility anyway, so I'm closing INVALID.

Cheers.

comment:2 Changed 13 months ago by Luis

  • Easy pickings unset
  • Severity set to Normal
  • Type set to Uncategorized
  • UI/UX unset

It shouldn't be considered invalid, even worse to be considered as a "consistent behavior". Reason: this only happens with related fields, and not with normal fields.

If I have:

class B(Model):

name = CharField(max_length=30) #example

class A(Model):

#neither of these fields allow null
count = PositiveSmallIntegerField()
ref_a = ForeignKey(B)

And issue:

a = A()
...
a.count #will return
a.ref_a #will raise exception

That does not let me traverse model properties without asking for forgiveness everytime. Why throwing DoesNotExist error and not throwing a ValueError or AttributeError for count property? Either trigger an error (ValueError, AttributeError, whatever) in non-related properties OR (xor) stop throwing errors when a FK has a null value (and does not accept it).

comment:3 Changed 13 months ago by lsaffre

Thanks, Luis.
Luke, Django would be better if the value of a non-nullable foreign key object was allowed be None. The nullability of a field should become significant only when we try to save it. Forcing us to use journal_id here (not journal) is odd. This behaviour might have been intended at some time, and I agree that changing it might cause some backwards incompatibility, but that does not make and odd behaviour *good*.

comment:4 Changed 13 months ago by Luis

What BW incompatibilities could arise? (Beware: this question is really important and not to take as so lightweight).

comment:5 Changed 13 months ago by orshansk@…

Incompatibilities where you check if an order has a journal by trying to access it and checking for an exception. Unless people were proactive (think paranoid) and also checked for None, even though it was never allowed to be None, such a change would plainly break any and all such checks.

comment:6 Changed 13 months ago by lsaffre

Thanks for the example of a possible incompatibility. Yes of course, such code will break. The suggested change is not backward-compatible.
No it was not "proactive (think paranoid) to check for None" in a non-nullable field of an unsaved instance. There are even people who say that the opposite is true: expecting an exception for an expectable condition is considered bad style. The essence of this ticket is to (1) admit that the current behaviour is wrong (e.g. by agreeing to the statement "Django should raise an exception only when I try to save the instance, not already when I want to see the value of journal!"). Luis understood this and illustrated it beautifully. And only then we can start to (2) collect ideas about how to change it. We will never get ideas about (2) as long as we don't admit (1).

comment:7 Changed 7 months ago by simon29

  • Cc simon@… added
  • Type changed from Uncategorized to Cleanup/optimization

@lukeplant, perhaps this is something that should be cleaned up and flagged as backward incompatible in the next major release? We ought to be able to check the value of a field without raising an artificial error, or resorting to workarounds.

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