Code

Opened 5 years ago

Last modified 5 months ago

#10414 assigned Bug

select_related ignores unrecognized fields

Reported by: trebor74hr Owned by: trebor74hr
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 trebor74hr 3 years ago.
select_related("invalid_field") patch

Download all attachments as: .zip

Change History (20)

comment:1 Changed 5 years ago by mtredinnick

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 5 years ago by jacob

  • Triage Stage changed from Unreviewed to Design decision needed

comment:3 Changed 4 years ago by CollinAnderson

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

comment:4 Changed 4 years ago by russellm

  • milestone set to 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 4 years ago by ubernostrum

  • milestone changed from 1.2 to 1.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 3 years ago by trebor74hr

  • Owner changed from nobody to trebor74hr
  • Status changed from new to assigned

Changed 3 years ago by trebor74hr

select_related("invalid_field") patch

comment:7 Changed 3 years ago by trebor74hr

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 3 years ago by trebor74hr

  • Has patch set

comment:9 Changed 3 years ago by trebor74hr

  • Triage Stage changed from Design decision needed to Ready for checkin

comment:10 Changed 3 years ago by trebor74hr

  • Triage Stage changed from Ready for checkin to Design decision needed

comment:11 Changed 3 years ago by mtredinnick

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 3 years ago by trebor74hr

@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 3 years ago by CollinAnderson

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 3 years ago by SmileyChris

  • milestone 1.3 deleted
  • Severity set to Normal
  • Type set to Bug

comment:15 Changed 3 years ago by trebor74hr

  • Cc trebor74hr@… added
  • Easy pickings unset

comment:16 Changed 2 years ago by aaugustin

  • Triage Stage changed from Design decision needed to Accepted
  • 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 9 months ago by timo

  • 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 5 months 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 5 months ago by timo (previous) (diff)

comment:19 Changed 5 months ago by timo

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as assigned
The owner will be changed from trebor74hr to anonymous. Next status will be 'assigned'
The ticket will be disowned. Next status will be 'new'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.