Opened 14 years ago

Closed 12 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 McNulty
Component: Database layer (models, ORM) Version: dev
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 Gaynor)

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 McNulty 14 years ago.
regression test with fix that uses isinstance instead of hasattr()
13640_ducktyping.diff (1.9 KB ) - added by Tobias McNulty 14 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@… 13 years ago.
Extracted the suggest fix from previous patches

Download all attachments as: .zip

Change History (19)

comment:1 by LukaszKorzybski, 14 years ago

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

comment:2 by Alex Gaynor, 14 years ago

Description: modified (diff)

Updated the syntax.

comment:3 by Tobias McNulty, 14 years ago

Owner: changed from nobody to Tobias McNulty
Status: newassigned

comment:4 by Tobias McNulty, 14 years ago

Triage Stage: UnreviewedAccepted

by Tobias McNulty, 14 years ago

Attachment: 13640.diff added

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

by Tobias McNulty, 14 years ago

Attachment: 13640_ducktyping.diff added

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

comment:5 by Tobias McNulty, 14 years ago

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 by Julien Phalip, 13 years ago

Severity: Normal
Type: Bug

comment:7 by net147, 13 years ago

Cc: net147 added
Easy pickings: unset

comment:8 by net147, 13 years ago

Easy pickings: set

by b.leskes@…, 13 years ago

Attachment: 13640_fix_only.diff added

Extracted the suggest fix from previous patches

comment:9 by b.leskes@…, 13 years ago

Has patch: set
Triage Stage: AcceptedReady for checkin
UI/UX: unset
Version: 1.2SVN

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 by Malcolm Tredinnick, 13 years ago

Needs tests: set
Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

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 by Tobias McNulty, 12 years ago

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 by Tobias McNulty, 12 years ago

Triage Stage: AcceptedReady for checkin

comment:13 by Tobias McNulty, 12 years ago

Needs tests: unset
Patch needs improvement: unset

comment:14 by Aymeric Augustin, 12 years ago

Triage Stage: Ready for checkinAccepted

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

comment:15 by Tobias McNulty, 12 years ago

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 by Karen Tracey, 12 years ago

Resolution: fixed
Status: assignedclosed

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