Re: [Django] #12708: Django raises DoesNotExist when consulting an empty ForeignKey field
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: nobody
Type: Uncategorized | Status: closed
Component: Database layer | Version: master
(models, ORM) | Resolution: invalid
Severity: Normal | Triage Stage:
Keywords: ForeignKey | Unreviewed
DoesNotExist | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by Luis):
* ui_ux: => 0
* type: => Uncategorized
* severity: => Normal
* easy: => 0
Comment:
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).
--
Ticket URL: <https://code.djangoproject.com/ticket/12708#comment:2>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: nobody
Type: Uncategorized | Status: closed
Component: Database layer | Version: master
(models, ORM) | Resolution: invalid
Severity: Normal | Triage Stage:
Keywords: ForeignKey | Unreviewed
DoesNotExist | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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*.
--
Ticket URL: <https://code.djangoproject.com/ticket/12708#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: nobody
Type: Uncategorized | Status: closed
Component: Database layer | Version: master
(models, ORM) | Resolution: invalid
Severity: Normal | Triage Stage:
Keywords: ForeignKey | Unreviewed
DoesNotExist | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Luis):
What BW incompatibilities could arise? (Beware: this question is really
important and not to take as so lightweight).
--
Ticket URL: <https://code.djangoproject.com/ticket/12708#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: nobody
Type: Uncategorized | Status: closed
Component: Database layer | Version: master
(models, ORM) | Resolution: invalid
Severity: Normal | Triage Stage:
Keywords: ForeignKey | Unreviewed
DoesNotExist | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/12708#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: nobody
Type: Uncategorized | Status: closed
Component: Database layer | Version: master
(models, ORM) | Resolution: invalid
Severity: Normal | Triage Stage:
Keywords: ForeignKey | Unreviewed
DoesNotExist | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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).
--
Ticket URL: <https://code.djangoproject.com/ticket/12708#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: nobody
Cleanup/optimization | Version: master
Component: Database layer | Resolution: invalid
(models, ORM) | Triage Stage:
Severity: Normal | Unreviewed
Keywords: ForeignKey | Needs documentation: 0
DoesNotExist | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by simon29):
* cc: simon@… (added)
* type: Uncategorized => Cleanup/optimization
Comment:
@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.
--
Ticket URL: <https://code.djangoproject.com/ticket/12708#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
(models, ORM) |
Keywords: ForeignKey | Triage Stage:
DoesNotExist | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by marfire):
* cc: k@… (added)
Comment:
Since the resolution to #26179 allows setting a non-nullable `ForeignKey`
attribute to `None`, the current behavior seems even more inconsistent. I
recommend re-opening this ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/12708#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: invalid
Keywords: ForeignKey | Triage Stage:
DoesNotExist | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timgraham):
To reopen the ticket, we'd need to a proposal for how to proceed regarding
the backwards-incompatibilities. The place for that discussion is the
DevelopersMailingList. Feel free to start a thread and link to it from
here.
--
Ticket URL: <https://code.djangoproject.com/ticket/12708#comment:9>