Code

Opened 5 years ago

Closed 4 years ago

Last modified 3 years ago

#11764 closed (fixed)

Bugfix: typo with avoid.update(..) in db/models/sql/compiler.py:SQLCompiler:fill_related_selections

Reported by: aurelio Owned by: ramiro
Component: Database layer (models, ORM) Version: master
Severity: Keywords:
Cc: django@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

There looks to be a bug due to a typo in db/models/sql/query.py .

The issue became apparent when it prevented the display of a ForeignKey field in list_display, when there is another ForeignKey to the base class of the model with the following error displayed:

Caught an exception while rendering: update() takes exactly one argument (2 given)

Original Traceback (most recent call last):
  File "/tippit/django-projects/django/Django-1.1/django/template/debug.py", line 71, in render_node
    result = node.render(context)
  File "/tippit/django-projects/django/Django-1.1/django/template/__init__.py", line 936, in render
    dict = func(*args)
  File "/tippit/django-projects/django/Django-1.1/django/contrib/admin/templatetags/admin_list.py", line 253, in result_list
    'results': list(results(cl))}
  File "/tippit/django-projects/django/Django-1.1/django/contrib/admin/templatetags/admin_list.py", line 247, in results
    for res in cl.result_list:
  File "/tippit/django-projects/django/Django-1.1/django/db/models/query.py", line 106, in _result_iter
    self._fill_cache()
  File "/tippit/django-projects/django/Django-1.1/django/db/models/query.py", line 692, in _fill_cache
    self._result_cache.append(self._iter.next())
  File "/tippit/django-projects/django/Django-1.1/django/db/models/query.py", line 238, in iterator
    for row in self.query.results_iter():
  File "/tippit/django-projects/django/Django-1.1/django/db/models/sql/query.py", line 287, in results_iter
    for rows in self.execute_sql(MULTI):
  File "/tippit/django-projects/django/Django-1.1/django/db/models/sql/query.py", line 2360, in execute_sql
    sql, params = self.as_sql()
  File "/tippit/django-projects/django/Django-1.1/django/db/models/sql/query.py", line 395, in as_sql
    self.pre_sql_setup()
  File "/tippit/django-projects/django/Django-1.1/django/db/models/sql/query.py", line 589, in pre_sql_setup
    self.fill_related_selections()
  File "/tippit/django-projects/django/Django-1.1/django/db/models/sql/query.py", line 1401, in fill_related_selections
    ())
TypeError: update() takes exactly one argument (2 given)

A patch has been attached applied against changeset 11475.

Attachments (4)

patch_against_11475.diff (771 bytes) - added by aurelio 5 years ago.
11764.diff (1.6 KB) - added by ramiro 5 years ago.
Same code patch plus regression test
11764.2.diff (1.7 KB) - added by wogan 4 years ago.
Updated patch against rev12438
11764.patch (1.8 KB) - added by wogan 4 years ago.
Updated patch against rev13018

Download all attachments as: .zip

Change History (13)

Changed 5 years ago by aurelio

Changed 5 years ago by ramiro

Same code patch plus regression test

comment:1 Changed 5 years ago by ramiro

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to ramiro
  • Patch needs improvement unset
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Accepted

Finally got around to reproduce this corner case thing by tracing and copying the treatment the queryset gets in the admin app to show a change list for the relevant model (there is no need to create model instances at all). Critical to make the bug show itself are the select_related() call and iterating over the QS (forcing its evaluation with list() doesn't trigger it).

comment:2 Changed 5 years ago by Alex

  • Triage Stage changed from Accepted to Ready for checkin

Changed 4 years ago by wogan

Updated patch against rev12438

comment:3 follow-up: Changed 4 years ago by wogan

  • Cc django@… added
  • Summary changed from Bugfix: typo with avoid.update(..) in db/models/sql/query.py:BaseQuery:fill_related_selections to Bugfix: typo with avoid.update(..) in db/models/sql/compiler.py:SQLCompiler:fill_related_selections

Updated patch as the typo had moved from [source:django/trunk/django/db/models/sql/query.py query.py] to [source:django/trunk/django/db/models/sql/compiler.py compiler.py].

comment:4 in reply to: ↑ 3 Changed 4 years ago by ramiro

Replying to wogan:

Updated patch as the typo had moved from [source:django/trunk/django/db/models/sql/query.py query.py] to [source:django/trunk/django/db/models/sql/compiler.py compiler.py].

Thanks for updating the patch. I had spotted the code move and implemented a simliar updating locally. Problem is that the regression test case I had added now pass with and without the fix applied. I ran the full Django test suite with code coverage analysis turned on and it shows that exact code path isn't exercised anymore.

This means we need to update the test because the core committers won't get this change in without a regression test. But I don't know if it's possible at all.

Sorry for not updating the ticket, I only made a comment about this on the #django-dev IRC channel.

comment:5 Changed 4 years ago by Alex

  • Triage Stage changed from Ready for checkin to Accepted

Moving off of RFC since ramiro notes that we can longer reproduce this :/

comment:6 Changed 4 years ago by russellm

  • milestone changed from 1.2 to 1.3

Not critical for 1.2

Changed 4 years ago by wogan

Updated patch against rev13018

comment:7 Changed 4 years ago by wogan

  • milestone changed from 1.3 to 1.2
  • Triage Stage changed from Accepted to Ready for checkin

Updated the patch, made a minor change to the test.

I could successfully run the test (failing appropriately when the patch is not applied).

Is it possible to get this in for 1.2?

comment:8 Changed 4 years ago by russellm

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

(In [13019]) Fixed #11764 -- Added a missing set of parentheses in a call calculating the select_related tables. Thanks to aurelio for the report and original patch, and wogan for the updated patch.

comment:9 Changed 3 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


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

 
Note: See TracTickets for help on using tickets.