#23555: QuerySet.first suppresses internal IndexError exceptions
-------------------------------------+-------------------------------------
Reporter: artemrizhov | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: QuerySet first | Unreviewed
IndexError | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by artemrizhov):
Sorry for my English, my explanation is probably not too clear. But if you
see the pull request and the test that I've added to the branch, you
probably will understand the problem. There is also a lot of comments on
the test case code.
Let me try to explain one more time.
'''You are right.''' There is no reason why `__getitem__` shouldn't raise
an `IndexError`. This is correct and normal behaviour. I'm talking about
another case.
'''`IndexError` from `QuerySet.__getitem__` is not always what you think,
because it may be produced by nested calls, not by this method itself.'''
Let's remember that QuerySet is a lazy request to DB. So when you write
`qs[i]`, it makes actual query to the DB, retrieves a list of items and
returns i-th element. `IndexError` maybe raised by some another method
that is called from `__getitem__`, especially by `__iter__` and any nested
call. For example by the iterator implementation, or by database adaptor.
For example if internal list of db connections is empty, but the adaptor
tries to get one from the list. Such abnormal situation should not be
treated as "no such element in the queryset". `IndexError` in nested calls
may mean anything, but probably not what you mean. If any nested call
produces `IndexError`, the caller (`_getitem__`, `__iter__` and another
nested methods) is responsible to handle this right way, because it is
aware of the source of error. Your outer code can't handle such error just
because it's not aware of the source.
Hope this is clear now. If you are in doubt, please review the test in the
pull request.
So what is the problem?
You can use try/except if you access i-th element of a list. Because you
are sure `IndexError` means "no such element" in this certain list. But
you should not rely on try/except for `IndexError` when accessing i-th
element of QuerySet. You should not. But the current implementation of
`first()` method does this. It uses "standard" try/except in the case
where it is not safe:
https://github.com/django/django/blob/e9103402c0fa873aea58a6a11dba510cd308cb84/django/db/models/query.py#L515
{{{
try:
return qs[0]
except IndexError:
return None
}}}
This would work if `qs` as plain `list`. But `qs` is `QuerySet` with all
it's magic. It's `__getitem__` is not simple and rely on successful
execution of a lot of nested calls. If any of those calls produces
unexpected exception (either `IndexError` or another one), this should not
be treated as "empty query set", and should not be suppressed. The outer
code should receive this exception as a singal that something goes wrong.
But `first()` method suppresses such errors.
Hope you understand the problem now. If no then please review the pull
request, especially my tests, and the comments on the PR.
Thanks for the attention.
--
Ticket URL: <https://code.djangoproject.com/ticket/23555#comment:4>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/069.9d678ae3d130080a983048feacf9661a%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
