Opened 5 years ago

Closed 4 years ago

#13640 closed Bug (fixed)

add_filter in django/db/models/ sql/query.py causes exception when model have 'evaluate' attribute

Reported by: LukaszKorzybski Owned by: tobias
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: net147 Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description (last modified by Alex)

I was migrating some django project recently from django 1.0.4 to 1.2.
In Django 1.2/1.1 I found that if model have 'evaluate' attribute then
one will get exception in admin edit page for that model if the page
contains inline forms with related models:

Exception Value:  'Shipper' object has no attribute 'prepare'
Exception Location:     .../django/db/models/sql/expressions.py in
__init__, line 12

It is caused by the fact that add_filter function in django/db/models/sql/query.py does such a check:

...
1005.  elif hasattr(value, 'evaluate'):
1006.      # If value is a query expression, evaluate it
1007.      value = SQLEvaluator(value, self) ...
1008.      having_clause = value.contains_aggregate
...

The problem is that "value" in this case is Shipper model which in
have "evaluate" method so it is recognized as query expression here.

Greetings,

Attachments (3)

13640.diff (1.9 KB) - added by tobias 5 years ago.
regression test with fix that uses isinstance instead of hasattr()
13640_ducktyping.diff (1.9 KB) - added by tobias 5 years ago.
alternate patch that uses duck typing to determine if value is an expression object
13640_fix_only.diff (981 bytes) - added by b.leskes@… 4 years ago.
Extracted the suggest fix from previous patches

Download all attachments as: .zip

Change History (19)

comment:1 Changed 5 years ago by LukaszKorzybski

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

Ups I forgot to do some formatting of the text, sorry for that :/

comment:2 Changed 5 years ago by Alex

  • Description modified (diff)

Updated the syntax.

comment:3 Changed 5 years ago by tobias

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

comment:4 Changed 5 years ago by tobias

  • Triage Stage changed from Unreviewed to Accepted

Changed 5 years ago by tobias

regression test with fix that uses isinstance instead of hasattr()

Changed 5 years ago by tobias

alternate patch that uses duck typing to determine if value is an expression object

comment:5 Changed 5 years ago by tobias

I don't know enough about the ORM to know which of these makes the most sense. All tests pass with each patch.

comment:6 Changed 4 years ago by julien

  • Severity set to Normal
  • Type set to Bug

comment:7 Changed 4 years ago by net147

  • Cc net147 added
  • Easy pickings unset

comment:8 Changed 4 years ago by net147

  • Easy pickings set

Changed 4 years ago by b.leskes@…

Extracted the suggest fix from previous patches

comment:9 Changed 4 years ago by b.leskes@…

  • Has patch set
  • Triage Stage changed from Accepted to Ready for checkin
  • UI/UX unset
  • Version changed from 1.2 to SVN

The bug was verified against trunk and the suggested solution is approved by Alex. I extracted the fix from the original patch and it is ready for checkin to trunk. There is no need to add the test as it tests a functionality that has removed (the hasattr test is not used anymore).

comment:10 Changed 4 years ago by mtredinnick

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

I don't understand the claim that this doesn't need a test. Either it currently works properly, or the change is needed. If the change is needed, it requires a test to ensure we don't break it again in the future.

comment:11 Changed 4 years ago by tobias

Updated patch with test here: https://github.com/django/django/pull/75

And the corresponding diff: https://github.com/django/django/pull/75.diff

comment:12 Changed 4 years ago by tobias

  • Triage Stage changed from Accepted to Ready for checkin

comment:13 Changed 4 years ago by tobias

  • Needs tests unset
  • Patch needs improvement unset

comment:14 Changed 4 years ago by aaugustin

  • Triage Stage changed from Ready for checkin to Accepted

You shouldn't mark your own patches as "ready for checkin"; a review by someone else is required.

comment:15 Changed 4 years ago by tobias

Sorry about that, it's been so long since I wrote the original patch I honestly had forgotten doing it, and it seemed like a pretty trivial fix to me. I'll make sure I get someone else to review it next time.

comment:16 Changed 4 years ago by kmtracey

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

In [17093]:

Fixed #13640: Avoid generating an exception when a model has an attribute named 'evaluate'. Thanks LukaszKorzybski and tobias.

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