select_related ignores unrecognized fields

[Robert Lujo]

This doesn't report any problems:

>>> models.ReportField.objects.select_related("i can type here", "whatever i want", 
                                              "and won't get error or warning at least", "it is ignored!")[0]
<ReportField: 61.Report Field XXX>

I think that select_related in "fields mode" should check if values are ok (<model.fk_fieldname>, <model.fk_fieldname><depmodel1.fk_fieldname>, etc.).

[Malcolm Tredinnick]

I'm pretty sure there's a good reason it's doing this (at least on the implementation level). Have to try and remember what it is. Otherwise we should add some error reporting, providing it doens't slow things down too much.

[Collin Anderson]

I've ran into this issue before. Typos and other errors are getting silently ignored. Not very django-like.

[Russell Keith-Magee]

This is either going to be trivial or impossible to fix. Either way, we should address this for 1.2

[James Bennett]

While I agree it'd be nice to have error reporting when you mistype a field name there, I don't see it as critical for 1.2 to get out the door.

[Robert Lujo]

select_related("invalid_field") patch

[Robert Lujo]

python runtests.py --settings=test_sqlite select_related

.............

---

Ran 13 tests in 0.625s

Checking all tests isn't possible at my place.

[Malcolm Tredinnick]

I found some old (handwritten) notes I had around this item from back in 2007. Introducing select_related() functionality for specific fields was a measurable performance impact (lots more introspection of models to construct the queryset). Avoiding yet another check against the list of available fields sped this up slightly. Being fast for correct code is (at least was) more important than catching every possible way somebody can fumble-finger something. Execution speed on the Python side unfortunately slowed down further since those days, so this might now disappear into the noise.

[Robert Lujo]

@mtredeinnick I don't think I understand you well, so sorry if I got something wrong. If you claim that this patch will decrease performance, IMHO this should not be the case. Allocation of one 'set' with several string values and making 'set.difference' should not make a big performance impact. On the other hand, I think that silent ignoring of 'bad' field names is a BUG and therefore should be corrected.

[Collin Anderson]

I would be fine with not checking this until _after_ the query happens. We could keep track of which select_related fields are used, and then error if there are fields that were not used. It seems to me this would not affect performance.

[Aymeric Augustin]

I've seen this bite people (including myself) many times. I don't think silently dropping invalid code is acceptable.

Given that the only concern was about performance, and dates back to 5 years ago, I'm going to accept this ticket.

A quick benchmark to prove that the patch doesn't incur a performance hit would be appreciated.

[Tim Graham]

I don't think the patch is correct as it causes several tests to fail, including some in select_related_onetoone and known_related_objects.

[anonymous]

Can anyone explain in plain simple words why fixing this issue could cause some sort of performance problems? When you call select_related() without any parameters django knows the structure of tables to be joined. Is it that big performance problem to parse the string 'table__nexttable__blabla' and check it against this structure?

This issue can be annoying, when you use select_related to avoid bombarding the DB with additional selects, and you make a small typo then your code WILL actually bombard the db with these unnecessary selects and you won't even know about it.

[Tim Graham]

I think the lack of a patch that works is the main reason this isn't fixed. After we have that we can evaluate any performance problems, but like you (and I'm not an expert in the ORM), I feel they are likely to be minor.

[Niclas Olofsson]

I will try to adopt this since there has been no activity for a while.

[Niclas Olofsson]

New patch.

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

[loic84]

This still allows using select_related on fields other than relations (e.g. a Charfield), IMO this needs a little more work.

[Niclas Olofsson]

I have forgotten about this for quite a while, but the patch was updated to support using non-relational fields some time ago (including tests for this case, of course). I just updated the pull-request to the latest version of master, so it should be ready to merge if all tests pass at the CI server. Can I trig a CI build myself or can only core developers do that?

The patch is still at ​https://github.com/django/django/pull/2986

[Simon Charette]

Only people on the white list can trigger builds. I just did for your PR.

[Anssi Kääriäinen]

I'll mark patch needs improvement as there needs to be tests for the following cases:

• .select_related('tags') where tags is a GenericRelation
• .select_related('content_object') where content_object is a GenericForeignKey()
• .select_related('m2m_field') where m2m_field is a ManyToManyField
• .select_related('reverse_foreign_key') where reverse_foreign_key is a non-unique reverse side of a foreign key

These are easy additions to write to the test suite, so I'll mark this as easy pickings (the whole patch isn't easy, but the needed additions to the current patch are).

[Niclas Olofsson]

The PR is updated with tests for the mentioned cases. I have never worked with generic relations previously, but I hope that I managed to implement the tests as intended anyway.

[Tim Graham]

You can bump the ticket to "ready for checkin" after addressing my comments, thanks.

[Tim Graham]

There are some new comments on the pull request and also some test failures.

[Tim Graham <timograham@…>]

In 3daa9d60be6672841ceed5bb812b6fb25950dc16:

Fixed #10414 -- Made select_related() fail on invalid field names.