Opened 9 years ago

Closed 8 years ago

#25569 closed New feature (wontfix)

Add a friendly error report when using select_related() on a reverse relation

Reported by: Shai Berger Owned by: Vincent Perez
Component: Database layer (models, ORM) Version: 1.8
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (19)

comment:1 by Aymeric Augustin, 9 years ago

+1 to backporting to 1.8.

comment:2 by Tim Graham, 9 years ago

Needs documentation: unset
Needs tests: unset
Triage Stage: UnreviewedAccepted

comment:3 by Vincent Perez, 8 years ago

Owner: changed from nobody to Vincent Perez
Status: newassigned

comment:4 by Vincent Perez, 8 years ago

Has patch: set

I have opened a pull request for this:

https://github.com/django/django/pull/7487/

comment:5 by Josh Harwood, 8 years ago

Triage Stage: AcceptedReady for checkin

comment:6 by Vincent Perez, 8 years ago

In fact this still a WIP. I will make a new comment once my PR is updated.

comment:7 by Vincent Perez, 8 years ago

Triage Stage: Ready for checkinAccepted

comment:8 by Tim Graham, 8 years ago

Needs tests: set
Patch needs improvement: set

comment:9 by Vincent Perez, 8 years ago

Version 0, edited 8 years ago by Vincent Perez (next)

comment:10 by Alexandre Hajjar, 8 years ago

Needs tests: unset
Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

PR looks fine to me.

comment:11 by Tim Graham, 8 years ago

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.

comment:12 by Vincent Perez, 8 years ago

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.


comment:13 by Tim Graham, 8 years ago

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.

comment:14 by Vincent Perez, 8 years ago

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 :)

comment:15 by Tim Graham, 8 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

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.

comment:16 by Vincent Perez, 8 years ago

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'

Please give me your feedback, thanks.

Last edited 8 years ago by Vincent Perez (previous) (diff)

comment:17 by Vincent Perez, 8 years ago

Triage Stage: AcceptedReady for checkin

comment:18 by Tim Graham, 8 years ago

Patch needs improvement: unset
Triage Stage: Ready for checkinAccepted

The "Needs review" status is "Accepted + Has patch", not "Ready for checkin".

comment:19 by Shai Berger, 8 years ago

Resolution: wontfix
Status: assignedclosed

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.

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