Opened 15 years ago

Closed 10 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 14 years ago.
select_related("invalid_field") patch

Download all attachments as: .zip

Change History (33)

comment:1 by Malcolm Tredinnick, 15 years ago

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 by Jacob, 15 years ago

Triage Stage: UnreviewedDesign decision needed

comment:3 by Collin Anderson, 15 years ago

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

comment:4 by Russell Keith-Magee, 15 years ago

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 by James Bennett, 14 years ago

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 by Robert Lujo, 14 years ago

Owner: changed from nobody to Robert Lujo
Status: newassigned

by Robert Lujo, 14 years ago

select_related("invalid_field") patch

comment:7 by Robert Lujo, 14 years ago

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 by Robert Lujo, 14 years ago

Has patch: set

comment:9 by Robert Lujo, 14 years ago

Triage Stage: Design decision neededReady for checkin

comment:10 by Robert Lujo, 14 years ago

Triage Stage: Ready for checkinDesign decision needed

comment:11 by Malcolm Tredinnick, 14 years ago

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 by Robert Lujo, 14 years ago

@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 by Collin Anderson, 14 years ago

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 by Chris Beaven, 13 years ago

milestone: 1.3
Severity: Normal
Type: Bug

comment:15 by Robert Lujo, 13 years ago

Cc: trebor74hr@… added
Easy pickings: unset

comment:16 by Aymeric Augustin, 13 years ago

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 by Tim Graham, 11 years ago

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 by anonymous, 11 years ago

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 11 years ago by Tim Graham (previous) (diff)

comment:19 by Tim Graham, 11 years ago

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 by Niclas Olofsson, 10 years ago

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 by Niclas Olofsson, 10 years ago

Patch needs improvement: unset

comment:22 by Nick Sandford, 10 years ago

Triage Stage: AcceptedReady for checkin

comment:23 by loic84, 10 years ago

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 by Tim Graham, 10 years ago

Patch needs improvement: set

comment:25 by Niclas Olofsson, 10 years ago

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 10 years ago by Niclas Olofsson (previous) (diff)

comment:26 by Simon Charette, 10 years ago

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

comment:27 by Anssi Kääriäinen, 10 years ago

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 by Niclas Olofsson, 10 years ago

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 10 years ago by Niclas Olofsson (previous) (diff)

comment:29 by Tim Graham, 10 years ago

Patch needs improvement: set

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

comment:30 by Niclas Olofsson, 10 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:31 by Tim Graham, 10 years ago

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 by Tim Graham <timograham@…>, 10 years ago

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