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

[Shai Berger]

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.

[Aymeric Augustin]

+1 to backporting to 1.8.

[Vincent Perez]

I have opened a pull request for this:

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

[Vincent Perez]

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

[Vincent Perez]

The PR is updated: ​https://github.com/django/django/pull/7487/files

In fact I went a little further than the initial topic. Indeed, I thought it would be also nice to have a specific messages for other failure reasons, such as many to many relations and generic relations / Foreign Key

[Alexandre Hajjar]

PR looks fine to me.

[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.

[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.

[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.

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

[Tim Graham]

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.

[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'

Please give me your feedback, thanks.

[Tim Graham]

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

[Shai Berger]

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.