Opened 6 years ago

Closed 2 months ago

#10414 closed Bug (fixed)

select_related ignores unrecognized fields

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

Download all attachments as: .zip

Change History (33)

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

  • Triage Stage changed from Unreviewed to Design decision needed

comment:3 Changed 5 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 5 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 5 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 4 years ago by trebor74hr

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

Changed 4 years ago by trebor74hr

select_related("invalid_field") patch

comment:7 Changed 4 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 4 years ago by trebor74hr

  • Has patch set

comment:9 Changed 4 years ago by trebor74hr

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

comment:10 Changed 4 years ago by trebor74hr

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

comment:11 Changed 4 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 4 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 4 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 4 years ago by SmileyChris

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

comment:15 Changed 4 years ago by trebor74hr

  • Cc trebor74hr@… added
  • Easy pickings unset

comment:16 Changed 3 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 20 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 16 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 16 months ago by timo (previous) (diff)

comment:19 Changed 16 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.

comment:20 Changed 7 months ago by nip3o

  • Owner changed from trebor74hr to nip3o

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

comment:21 Changed 7 months ago by nip3o

  • Patch needs improvement unset

comment:22 Changed 7 months ago by slurms

  • Triage Stage changed from Accepted to Ready for checkin

comment:23 Changed 7 months ago by loic84

  • Triage Stage changed from Ready for checkin to Accepted

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 7 months ago by timo

  • Patch needs improvement set

comment:25 Changed 5 months ago by nip3o

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

comment:26 Changed 5 months ago by charettes

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

comment:27 Changed 3 months ago by akaariai

  • 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 3 months ago by nip3o

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

comment:29 Changed 3 months ago by timgraham

  • Patch needs improvement set

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

comment:30 Changed 3 months ago by nip3o

  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin

comment:31 Changed 3 months ago by timgraham

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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

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

  • Resolution set to fixed
  • Status changed from assigned to closed

In 3daa9d60be6672841ceed5bb812b6fb25950dc16:

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

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