I am trying to understand why Luke closed my ticket #12801
(http://code.djangoproject.com/ticket/12801).
Luke, don't get me wrong. Thank you for having taken the time to decide
upon this and my other ticket #12708. I agreed with you for marking
#12708 as invalid, because I didn't understand the real problem when I
wrote it, so the formulations in that ticket are rather confusing.
But #12801 now shows so clearly an odd behaviour which should at least
be documented if it cannot be fixed. Marking #12801 as a duplicate of an
invalid ticket means to me that I should not even *try* to write a
patch. Is that really what you want to say?
I'd agree if you say that this is not an urgent problem. But shouldn't
we mark it then as "Someday/Maybe"?
Luc
Yes, that is what I meant to say. I'll defend my opinion here, and
let others weigh in.
The issue is what should happen with the following model:
>>> class Foo(Model)
>>> a_date = DateTimeField()
>>> bar = ForeignKey(Bar)
>>>
>>> f = Foo()
>>> f.bar
The current behaviour is that a 'DoesNotExist' exception is thrown,
because f.bar_id = None, and the field is not nullable. This is
different from other uninitialised fields, which return None e.g.
>>> assert f.a_date is None
Luc wants the behaviour to be the same i.e. no DoesNotExist exception
for an uninitialised ForeignKey field. My response (in a more
expanded form here than on the ticket):
1. ForeignKey fields are different from simple values, in that they
cause database lookups (the only logical exception being nullable
foreign keys with a PK of None), so it's reasonable for them to behave
differently.
2. When that lookup occurs, whenever the PK value is not in the
database, you get an exception. So if the PK value was 123456 and
that did not exist, you would get an exception. So this behaviour is
consistent with other values of Foo.bar_id, and changing it would
break that consistency.
3. You currently get an exception if you attempt to set f.bar = None.
So, 'None' is never the value of a non-nullable foreign key field.
The proposed change would break that symmetry - you would not expect
the 2nd line of the following code to throw an exception if the first
did not:
>>> assert f.bar is None
>>> f.bar = None
If we changed Django to accept the first line, surely we also need to
change it to accept the second (which would be a bad idea, I think
we'd agree).
4. There is an easy work-around if you are allergic to exceptions,
which is to check the PK value (f.bar_id in the above case).
Thanks,
Luke
--
"Yes, wearily I sit here, pain and misery my only companions. And
vast intelligence of course. And infinite sorrow. And..." (Marvin
the paranoid android)
Luke Plant || http://lukeplant.me.uk/
Luke, I disagree with your explanations. Django behaves oddly.
Model.save() is the place where empty non-nullable fields cause an
exception. There is no reason for ForeignKey to behave differently.
ForeignKey fields are different in that they cause database lookups when
they are not None, and that they can cause exceptions in some special
situations, for example
- asking them to save before the foreign instance has been saved
- database integrity errors (invalid non-None pk)
That said, it is clear that I don't know Django well enough to decide
whether it is feasible/necessary to fix this odd behaviour.
Luc
> Luke, I disagree with your explanations. Django behaves oddly.
>
> Model.save() is the place where empty non-nullable fields cause an
> exception.
> There is no reason for ForeignKey to behave differently.
> ForeignKey fields are different in that they cause database lookups
> when they are not None, and that they can cause exceptions in some
> special situations, for example
> - asking them to save before the foreign instance has been saved
> - database integrity errors (invalid non-None pk)
>
> That said, it is clear that I don't know Django well enough to
> decide whether it is feasible/necessary to fix this odd behaviour.
It is easy to fix - a few lines in
ReverseSingleRelatedObjectDescriptor - because the behaviour there is
not accidental, but quite deliberate. The behaviour is only odd from
one angle, and it is not very strange, because ForeignKey fields *are*
a fundamentally different type of field. The corresponding 'normal'
field is the '_id' field - this is a dumb value.
I think you are missing the fact that this behaviour occurs *whenever*
the FK val is an invalid value, not just with creating new instances.
You are therefore asking either for special treatment of instances
that are not saved to the database, or for it to be broken in general.
Consider this code:
>>> o = SomeModel.objects.get(id=1)
>>> o.fk_field_id = None
>>> print o.fk_field
Are you are proposing that the above should print 'None' even when
fk_field is non-nullable, rather than a DoesNotExist? 'o.fk_field' is
a shortcut for a DB lookup of 'o.fk_field_id', and clearly the result
of that lookup is *not* None/NULL. The query returns no results, and,
just like every other case when a query returns no results and we
expect at least one, you get a DoesNotExist exception. (In this case
we skip doing the actual database query as an optimisation, but the
same logic applies).
On 8.02.2010 0:38, Luke Plant wrote:
> It is easy to fix - a few lines in
> ReverseSingleRelatedObjectDescriptor - because the behaviour there is
> not accidental, but quite deliberate.
I admit that I am surprised to hear this.
> The behaviour is only odd from
> one angle, and it is not very strange, because ForeignKey fields *are*
> a fundamentally different type of field. The corresponding 'normal'
> field is the '_id' field - this is a dumb value.
Sorry, but I still don't understand the angle from which this behaviour
is *not* odd. And the corresponding '_id' field is added 'behind the
scenes', so using it should be avoided as much as possible.
> I think you are missing the fact that this behaviour occurs *whenever*
> the FK val is an invalid value, not just with creating new instances.
No, I considered this. I wrote already the two possible causes of
exceptions that I can imagine.
> You are therefore asking either for special treatment of instances
> that are not saved to the database, or for it to be broken in general.
That's no special treatment. I'm asking to have the same behaviour for
nullable and non-nullable ForeignKey fields (until save()).
Having 'None' in a non-nullable field is a normal case of an instance
containing invalid data. Imagine an application that runs a complex
series of handlers on an instance that decide how to set some fields
depending of the (valid or invalid) values contained in some other
fields. You cannot ask user code to not even look at invalid data. I'm
not allergic to exceptions, but raising an exception when I ask for the
content of a field is not appropriate behaviour.
> Consider this code:
>
>>>> o = SomeModel.objects.get(id=1)
>>>> o.fk_field_id = None
>>>> print o.fk_field
>
> Are you are proposing that the above should print 'None' even when
> fk_field is non-nullable, rather than a DoesNotExist?
Yes. Though I'd say "o.fk_field = None" instead of "o.fk_field_id =
None", because the '_id' field should remain behind the scenes. Anyway
both should have the same effect.
> 'o.fk_field' is
> a shortcut for a DB lookup of 'o.fk_field_id', and clearly the result
> of that lookup is *not* None/NULL. The query returns no results, and,
> just like every other case when a query returns no results and we
> expect at least one, you get a DoesNotExist exception. (In this case
> we skip doing the actual database query as an optimisation, but the
> same logic applies).
'o.fk_field' is an attribute of a Model instance, and Django should
never refuse to show what it contains, even if that value is illegal.
Okay, it's obvious that Luke's and Luc's understandings of that problem
are quite different. We both explained our points of views. The next
step would be to hear other people's opinions. I personally won't insist
further, especially since Luke knows Django better than me. May I
suggest again to mark this ticket to something different than "duplicate
of an invalid ticket"?
Luc
[snip]
Okay, it's obvious that Luke's and Luc's understandings of that problem
are quite different. We both explained our points of views. The next
step would be to hear other people's opinions.
I personally won't insist
further, especially since Luke knows Django better than me. May I
suggest again to mark this ticket to something different than "duplicate
of an invalid ticket"?
"Closed" as "Invalid", as I understand it, means that this ticket is not
worth to get more consideration and that further comments are not welcome.
"Open" with a triage stage of "Someday/Maybe" would help people who
stumble on this behaviour to find our discussion. Especially if we add a
link to this discussion thread.
Luc
"Closed" as "Invalid", as I understand it, means that this ticket is not
worth to get more consideration and that further comments are not welcome.
"Open" with a triage stage of "Someday/Maybe" would help people who
stumble on this behaviour to find our discussion. Especially if we add a
link to this discussion thread.
On 8 fev, 20:31, Luc Saffre <luc.saf...@gmx.net> wrote:
> You cannot ask user code to not even look at invalid data. I'm
> not allergic to exceptions, but raising an exception when I ask for the
> content of a field is not appropriate behaviour.
>
> Luc
Raising the exception *is* appropriate behaviour, because when you
access `a_model.b_related`, and `b_related` is a ForeignKey, you're
not really accessing a value, but doing a lookup. This is consistent
with both the ER and ORM model, where FK's are pointers, not values.
And a FK pointing to a non-existant row *is* an exception. It means
broken data.
If you want the content of the *field*, what you really should check
is `a_model.b_related_id is None`, which is checking if a pointer was
set. Still, it doesn't guarantee that this pointer leads to anywhere
valid. That's why dealing with the exception is a common idiom, "fail
early, fail loudly", and particularly useful in this case to maintain
DB consistency.
I hope it helps in understanding the rationale behind the behaviour
and why Luke marked as invalid.
Luc
> --
> You received this message because you are subscribed to the Google
> Groups "Django developers" group.
> To post to this group, send email to django-d...@googlegroups.com.
> To unsubscribe from this group, send email to
> django-develop...@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/django-developers?hl=en.
As I said earlier, I don't want to bother the community with my
stubbornness or whatever it may be, so feel free to STOP READING HERE if
there is danger that this topic wastes your time.
But to be honest, I must unfortunately say that I disagree also with
your explanations and that they (for me) confirmed my opinion.
The expression `a_model.b_related` (where `a_model` is an instance of a
Model subclass that defines a field `b_related`) must always yield a
value in the Pythonic meaning (that is, it may yield None which is also
a value in Python).
This same expression should raise an exception only if the field name is
wrongly spelled, or if an error occured when constructing the related
instance to be yielded (e.g. a failed DB lookup in the case of a FK
field other than None).
A FK containing None (which is already now possible for nullable FK
fields) will never cause a DB lookup.
Django guarantees that the value of a FK field will always be either
None or an instance of the related Model.
Django never guaratees that every instance of a Model exists as a row in
the database.
Django's current behaviour is not correct because it forces me to access
non-nullable FK fields differently than nullable ones. "In Python,
throwing exceptions for expected outcomes is considered very bad form"
[1]. Django should raise an exception only if I try to save an instance
with invalid data (for example None in a non-nullable FK field), but not
when I try to access any data, may it be valid or not
[1]
http://groups.google.com/group/comp.lang.python/browse_thread/thread/7d6191ecba652daf
Luc
And yes, I can live with it. It's just a pity that others will stumble
on it just because it isn't documented.
Luc
The exception doesn't come from the fact you're accessing an
attribute. It comes from the related lookup, Django is just doing the
right thing and letting the DoesNotExist exception flow up. It's
better if you see things this way.
Also, the DoesNotExist exception is far from undocumented. Thru the
docs and tutorials you see the idiom on how to handle it quite a few
times. [1]
It also makes perfect sense to handle nullable FK's different than non-
nullable ones. One have a constraint imposed, the other not. Returning
None to a non-nullable FK, *this* would be what I call unexpected
behaviour.
Remember also that this is not a pure OO world. If we were seeing
things as just relations between classes, I would agree with you. But
we're dealing with a cumbersome thing that is the database, and
there's always a mismatch between the ER and OO models. [2]
Anyway, there's certainly nothing wrong or strange on throwing
exceptions on accessing attributes in Python (or in any dynamic
language, I would say), as any attribute on Python can be, in fact, a
method (@property). I'm surprised how opposing you feel on this, my
feeling is that you come from a different background, working with
languages that enforce exception handling in a special way (Java, C#).
But it's really getting into the field of personal taste now.
[1] http://docs.djangoproject.com/en/1.1/ref/models/querysets/#id5
[2] http://www.sbql.pl/Topics/ImpedanceMismatch.html
I'm puzzled how no Django core developer seems to understand it. I hope
that this is just because they are not following this thread because
there is admittedly more urgent work to do right now.
With the danger of repeating myself I explain once more:
Example models:
class Owner(models.Model):
pass
class Animal(models.Model): # nullable owner
owner = models.ForeignKey(Owner,blank=True,null=True)
class Thing(models.Model): # non-nullable owner
owner = models.ForeignKey(Owner)
There should never be a related lookup when the fk_id is None. For
nullable FK Django behaves correctly:
>>> a = Animal()
>>> print a.owner
None
The same doesn't work for non-nullable FK:
>>> t = Thing()
>>> print t.owner
Traceback (most recent call last):
...
DoesNotExist
This is whay I say that Django treats non-nullable FK in a special way.
It is as if Django thinks "Since Thing.owner may not be None, I can do
the lookup without even testing for a None value".
The correct exception is risen when you try to save it:
>>> t.save()
Traceback (most recent call last):
...
IntegrityError: 20100212_thing.owner_id may not be NULL
How can you not understand that the DoesNotExist exception above is
risen too early? It is a bug!
Luc
The correct exception is risen when you try to save it:
>>> t.save()
Traceback (most recent call last):
...
IntegrityError: 20100212_thing.owner_id may not be NULL
How can you not understand that the DoesNotExist exception above is
risen too early? It is a bug!
No, it really isn't *certainly* wrong. It's *arguably* wrong, and I
join my colleagues Luke and Karen in arguing that it isn't wrong at
all.
> I'm puzzled how no Django core developer seems to understand it. I hope
> that this is just because they are not following this thread because
> there is admittedly more urgent work to do right now.
>
> With the danger of repeating myself I explain once more:
>
> Example models:
>
> class Owner(models.Model):
> pass
>
> class Animal(models.Model): # nullable owner
> owner = models.ForeignKey(Owner,blank=True,null=True)
>
> class Thing(models.Model): # non-nullable owner
> owner = models.ForeignKey(Owner)
>
> There should never be a related lookup when the fk_id is None. For
> nullable FK Django behaves correctly:
>
> >>> a = Animal()
> >>> print a.owner
> None
Yes. The owner hasn't been specified. Therefore, the field falls back
to it's default value -- the owner is None.
> The same doesn't work for non-nullable FK:
>
> >>> t = Thing()
> >>> print t.owner
> Traceback (most recent call last):
> ...
> DoesNotExist
Yes. The owner hasn't been specified. The owner *must* be specified,
so any attempt to access it is an error - the owner Does Not Exist.
> This is whay I say that Django treats non-nullable FK in a special way.
Yes. Personally, I see the nullable case as the special case, but it
doesn't really matter which way you look at it -- the two behave
differently because they *are* different.
> It is as if Django thinks "Since Thing.owner may not be None, I can do
> the lookup without even testing for a None value".
Exactly. There's no point looking for a None value, because the field
*can't have a value of None*.
> The correct exception is risen when you try to save it:
>
> >>> t.save()
> Traceback (most recent call last):
> ...
> IntegrityError: 20100212_thing.owner_id may not be NULL
>
> How can you not understand that the DoesNotExist exception above is
> risen too early? It is a bug!
How can you *not* understand that this is a difference of opinion?
Three Django core developers (Luke, Karen and myself) have
independently told you that they disagree that this is "certainly
wrong".
In one last attempt to explain why Django works the way it does: it
may help to think of the problem in these terms. Thing.owner *isn't* a
field on Thing. It may be defined there, but what it is defining is a
relationship with the Owner table. Thing.owner is a descriptor that
substitutes for a query on the "Owner" table.
When you ask for Thing.owner, you're really saying "Give me the Owner
with a relationship to Thing X". If Thing X hasn't defined a
relationship with an owner, then the Owner.DoesNotExist.
In the case of the Animal, a legitimate answer to the question "Give
me the owner with a relationship to Animal X' is "no owner" - hence,
None is a legal return value.
Hopefully that clarifies why Django works the way it does. However,
even if, hypothetically, we were to accept that Django's current
behavior is in error, and your interpretation is the only correct
interpretation, there is an enormous issue of practicality. This
behavior was introduced as part of the 'magic-removal' branch, which
is 4 years old. The current behavior closely mirrors the analogous
behavior that preceded it. The current behavior has been a formal,
documented part of the 0.96, 0.96, 1.0, and 1.1 releases. There are
quite literally *thousands* of projects in production that rely on
Django's behavior being predictable and consistent between versions -
even if that behavior isn't "correct".
Changing this behavior would be a *huge* undertaking. If we were
talking about some esoteric corner of the aggregation API, we might be
able to entertain making a backwards-incompatible change -- but you're
talking about changing a fundamental aspect of the way foreign keys
work, and have always worked, and have been documented to work.
Django has been used by many people, over a long period of time. The
criticism of the API you are raising is *not* a regular feature of
complaints on Django-users, so there is very little reason to believe
that it is causing significant practical difficulties. Although the
change would be a minor code modification, it would be *very*
complicated to change it in practice, given Django's API backwards
compatibility guarantee. And as a result, we're *not* going to change
it.
Yours,
Russ Magee %-)
owner = models.ForeignKey(Owner)
specifies a contract. The contract says that when you access this
attribute on an instance, it will only return an instance of an Owner
object or raise an DoesNotExist exception.
Specifying a field like this:
owner = models.ForeignKey(Owner,blank=True,null=True)
specifies a very different contract. This contract says that accessing
this attribute will return either an instance of an Owner object, or
None if no owner has been specified.
Both contracts are fully described in the documentation and fulfilled
by the ORM, so clearly this is no bug. Both flavours are available for
you to use, you simply have to decide what sort of contract you want
that attribute to have - if you expect instance.owner to return None,
then clearly you should be using a nullable foreign key.
Cheers
Tom
No. It should raise no exception at all. The expression should yield
None. It should not be forbidden for an instance to contain invalid data
*until* you try to save it.
> If so, why would accessing a value on an unsaved instance (which by
> definition isn't in the database yet) be an IntegrityError rather than a
> DoesNotExist as a result of a lookup?
>
> It's clearly a design decision. You are free to disagree with that
> decision, but it's not a bug - it's behaving as designed (and documented).
If this is a topic that has been discussed earlier, then it would be
good to find pointers to this discussion. I didn't find any.
If at least one core developer would say "maybe Luc is right", then Luke
and Karen would maybe agree to reopen the ticket and mark it "needs
design decision".
And please: I continue to post here because I hope that my explanations
can help to understand the problem and to make Django better. I don't
want to offend anybody, I am not trying to "win" this "battle", I have
no personal interest in having this ticket reopened. If some day I
understand that *I* was wrong, then I will apologize for the noise I made.
Luc
Yes, it does. Thank you, Russel.
> However,
> even if, hypothetically, we were to accept that Django's current
> behavior is in error, (...) there is an enormous issue of practicality.
> (...)
> Although the
> change would be a minor code modification, it would be *very*
> complicated to change it in practice, given Django's API backwards
> compatibility guarantee. And as a result, we're *not* going to change
> it.
Yes, this is a good reason to not change Django's behaviour,
independently of whether it is "wrong" or not.
Luc
Wow! This formulation is really clear. Thank you, Tom.
My summary: Django's behaviour is un-pythonic and not the most elegant
one, but it is possible to get used to it, even to rely on it, and it
certainly won't change.
I didn't find such a good explanation in the docs, though. IMHO it
should be here:
http://docs.djangoproject.com/en/dev/topics/db/queries/#forward
I suggest to add a paragraph about the surprising DoesNotExist there.
Luc