Opened 8 years ago

Closed 2 years ago

#10414 closed Bug (fixed)

select_related ignores unrecognized fields

Reported by: Robert Lujo Owned by: Niclas Olofsson
Component: Database layer (models, ORM) Version:
Severity: Normal Keywords: select_related
Cc: trebor74hr@…, timograham@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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

Attachments (1)

10414_selectRelatedInvalidFields.diff (3.4 KB) - added by Robert Lujo 6 years ago.
select_related("invalid_field") patch

Download all attachments as: .zip

Change History (33)

comment:1 Changed 8 years ago by 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.

comment:2 Changed 8 years ago by Jacob

Triage Stage: UnreviewedDesign decision needed

comment:3 Changed 7 years ago by Collin Anderson

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

comment:4 Changed 7 years ago by Russell Keith-Magee

milestone: 1.2

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

comment:5 Changed 7 years ago by James Bennett

milestone: 1.21.3

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.

comment:6 Changed 6 years ago by Robert Lujo

Owner: changed from nobody to Robert Lujo
Status: newassigned

Changed 6 years ago by Robert Lujo

select_related("invalid_field") patch

comment:7 Changed 6 years ago by 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.

comment:8 Changed 6 years ago by Robert Lujo

Has patch: set

comment:9 Changed 6 years ago by Robert Lujo

Triage Stage: Design decision neededReady for checkin

comment:10 Changed 6 years ago by Robert Lujo

Triage Stage: Ready for checkinDesign decision needed

comment:11 Changed 6 years ago by 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.

comment:12 Changed 6 years ago by 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.

comment:13 Changed 6 years ago by 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.

comment:14 Changed 6 years ago by Chris Beaven

milestone: 1.3
Severity: Normal
Type: Bug

comment:15 Changed 6 years ago by Robert Lujo

Cc: trebor74hr@… added
Easy pickings: unset

comment:16 Changed 5 years ago by Aymeric Augustin

Triage Stage: Design decision neededAccepted
UI/UX: unset

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.

comment:17 Changed 3 years ago by Tim Graham

Cc: timograham@… added
Patch needs improvement: set

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.

comment:18 Changed 3 years ago by 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.

Last edited 3 years ago by Tim Graham (previous) (diff)

comment:19 Changed 3 years ago by 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.

comment:20 Changed 2 years ago by Niclas Olofsson

Owner: changed from Robert Lujo to Niclas Olofsson

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

comment:21 Changed 2 years ago by Niclas Olofsson

Patch needs improvement: unset

comment:22 Changed 2 years ago by Nick Sandford

Triage Stage: AcceptedReady for checkin

comment:23 Changed 2 years ago by loic84

Triage Stage: Ready for checkinAccepted

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

comment:24 Changed 2 years ago by Tim Graham

Patch needs improvement: set

comment:25 Changed 2 years ago by Niclas Olofsson

Patch needs improvement: unset

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

Last edited 2 years ago by Niclas Olofsson (previous) (diff)

comment:26 Changed 2 years ago by Simon Charette

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

comment:27 Changed 2 years ago by Anssi Kääriäinen

Easy pickings: set
Needs tests: set
Patch needs improvement: set

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

comment:28 Changed 2 years ago by Niclas Olofsson

Easy pickings: unset
Needs tests: unset
Patch needs improvement: unset

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.

Last edited 2 years ago by Niclas Olofsson (previous) (diff)

comment:29 Changed 2 years ago by Tim Graham

Patch needs improvement: set

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

comment:30 Changed 2 years ago by Niclas Olofsson

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:31 Changed 2 years ago by Tim Graham

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

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

comment:32 Changed 2 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 3daa9d60be6672841ceed5bb812b6fb25950dc16:

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

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