[Django] #25569: Add a friendly error report when using select_related() on a reverse relation
Django
relation
-------------------------------------+-------------------------------------
Reporter: shaib | Owner: nobody
Type: New | Status: new
feature |
Component: Database | Version: 1.8
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 1 | Needs tests: 1
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Since 1.8, if you refer to a non-existent field in `select_related()`, you
get an error where the code used to pass silently. One potentially common
source for such errors is attempts to use `select_related()` on reverse
relations -- up to 1.7, this would have seemed to work, with 1.8 it
breaks.
I think it would be friendlier to users upgrading from (<=) 1.7 if the
error report called this situation out explicitly, rather than referring
to reverse relations as "non-existent".
Not sure if this merits a backport to 1.8.x -- on the one hand, it doesn't
really fall under the criteria, on the other hand, I think it would be
very useful and many people will be coming over to 1.8 and stay there, and
the validation of `select_related()` itself is a new feature in 1.8.
--
Ticket URL: <https://code.djangoproject.com/ticket/25569>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
relation
-------------------------------------+-------------------------------------
Reporter: shaib | Owner: nobody
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by aaugustin):
+1 to backporting to 1.8.
--
Ticket URL: <https://code.djangoproject.com/ticket/25569#comment:1>
Django
relation
-------------------------------------+-------------------------------------
Reporter: shaib | Owner: nobody
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_docs: 1 => 0
* needs_tests: 1 => 0
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/25569#comment:2>
Django
relation
-------------------------------------+-------------------------------------
| Perez
Type: New feature | Status: assigned
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => assigned
* owner: nobody => Vincent Perez
--
Ticket URL: <https://code.djangoproject.com/ticket/25569#comment:3>
Django
relation
-------------------------------------+-------------------------------------
| Perez
Type: New feature | Status: assigned
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
I have opened a pull request for this:
https://github.com/django/django/pull/7487/
--
Ticket URL: <https://code.djangoproject.com/ticket/25569#comment:4>
Django
relation
-------------------------------------+-------------------------------------
| Perez
Type: New feature | Status: assigned
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/25569#comment:5>
Django
relation
-------------------------------------+-------------------------------------
| Perez
Type: New feature | Status: assigned
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Vincent Perez):
In fact this still a WIP. I will make a new comment once my PR is updated.
--
Ticket URL: <https://code.djangoproject.com/ticket/25569#comment:6>
Django
relation
-------------------------------------+-------------------------------------
| Perez
Type: New feature | Status: assigned
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Ready for checkin => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/25569#comment:7>
Django
relation
-------------------------------------+-------------------------------------
| Perez
Type: New feature | Status: assigned
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/25569#comment:8>
Django
relation
-------------------------------------+-------------------------------------
| Perez
Type: New feature | Status: assigned
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Vincent Perez):
The PR is updated: https://github.com/django/django/pull/7487/files
--
Ticket URL: <https://code.djangoproject.com/ticket/25569#comment:9>
Django
relation
-------------------------------------+-------------------------------------
| Perez
Type: New feature | Status: assigned
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
* stage: Accepted => Ready for checkin
Comment:
PR looks fine to me.
--
Ticket URL: <https://code.djangoproject.com/ticket/25569#comment:10>
Django
relation
-------------------------------------+-------------------------------------
| Perez
Type: New feature | Status: assigned
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
The patch turned out a lot more complicated than I thought it would be,
and I'm not sure if it's worth it. In the original report, Shai said the
error message refers to fields as "non-existent" but as far as I see, the
error message is something like:
FieldError: Invalid field name(s) given in select_related: 'choice_set'.
Choices are: (none)".
Since the choices are there, I think it's already clear whether there's a
typo or if the field isn't supported by `select_related()`. With the
patch, the error turns into:
FieldError: Invalid field name(s) given in select_related: 'choice_set'
(reason: reverse relational field). Choices are: (none).
We have similar error messages elsewhere and this might set a precedent
for doing more work to "improve" them similarly. Other opinions welcome.
--
Ticket URL: <https://code.djangoproject.com/ticket/25569#comment:11>
Django
relation
-------------------------------------+-------------------------------------
| Perez
Type: New feature | Status: assigned
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Vincent Perez):
A simpler alternative could be to simply adjust the error message if there
is at least one reverse relation amongst the incorrect fields. In that
case, the error message would be:
>FieldError: Invalid field name(s) given in select_related: 'choice_set'.
Choices are: (none)". Reverse relational fields are not allowed in
select_related.
This way, the patch will indeed be much simpler .
But the message won't be more friendly for generic relation and generic
FK, which are also forbidden. Is it a big deal? If the answer to this last
question is yes, I could adjust the previous logic and simply do: if there
is at least one reverse relation, generic FK or generic relation amongst
the incorrect fields, then I display the message:
>FieldError: Invalid field name(s) given in select_related: 'choice_set'.
Choices are: (none)". Reverse relational fields, Generic relations and
generic foreign keys are not allowed in select_related.
--
Ticket URL: <https://code.djangoproject.com/ticket/25569#comment:12>
Django
relation
-------------------------------------+-------------------------------------
| Perez
Type: New feature | Status: assigned
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
My preference is to close the ticket as wontfix. Do you find the existing
error message confusing or unclear? I think it's clear that anything that
doesn't appear in the message's choices isn't allowed, regardless of the
reason.
--
Ticket URL: <https://code.djangoproject.com/ticket/25569#comment:13>
Django
relation
-------------------------------------+-------------------------------------
| Perez
Type: New feature | Status: assigned
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Vincent Perez):
I think the message in its current state is confusing, specially when you
are upgrading from a Django version prior to 1.8, where the select_related
used to fail silently. IMHO, the fix as I suggested in my previous
comment is simple enough to be worth it.
Btw, Aymeric Augustin found this fix worthy enough to even consider
backporting it in 1.8 (see comment above), so that makes at least 3 of us
thinking this fix is worth it :)
--
Ticket URL: <https://code.djangoproject.com/ticket/25569#comment:14>
Django
relation
-------------------------------------+-------------------------------------
| Perez
Type: New feature | Status: assigned
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
I don't know, the ticket's description didn't include the actual message
so it seems to was tough to evaluate the ticket. We're not going to
backport a fix at this point.
If "Invalid field name(s) given in select_related:" isn't specific enough,
how about something like "Non-single value relation given in
select_related:".
Anyway, I'll take another look at the PR however you'd like to amend it.
--
Ticket URL: <https://code.djangoproject.com/ticket/25569#comment:15>
Django
relation
-------------------------------------+-------------------------------------
| Perez
Type: New feature | Status: assigned
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
-------------------------------------+-------------------------------------
Comment (by Vincent Perez):
I've opened a new simplied PR:
https://github.com/django/django/pull/7622/files
If there at least one relational fields amongst the fields given to
select_related, the message becomes:
FieldError: Invalid field name(s) given in select_related: 'choice_set'.
Choices are: (none)". Reverse relations cannot be used in select_related'
--
Ticket URL: <https://code.djangoproject.com/ticket/25569#comment:16>
Django
relation
-------------------------------------+-------------------------------------
| Perez
Type: New feature | Status: assigned
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/25569#comment:17>
Django
relation
-------------------------------------+-------------------------------------
| Perez
Type: New feature | Status: assigned
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* stage: Ready for checkin => Accepted
Comment:
The "Needs review" status is "Accepted + Has patch", not "Ready for
checkin".
--
Ticket URL: <https://code.djangoproject.com/ticket/25569#comment:18>
Django
relation
-------------------------------------+-------------------------------------
| Perez
Component: Database layer | Version: 1.8
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => wontfix
Comment:
Hi Vincent,
Thanks for your efforts, and sorry for your time, but I agree with Tim:
This ticket would have been nice to add early in the 1.8 lifecycle, but
it's become much less valuable now. Most of the people who wanted to
upgrade from pre-1.8 have already done so. Your latest suggestion is
indeed simple, but IMO doesn't really add much value, and without the
"help upgrade to 1.8" motive, indeed sets a bad precedent for adding
unnecessary details to an error message.
--
Ticket URL: <https://code.djangoproject.com/ticket/25569#comment:19>