Opened 6 years ago

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

Download all attachments as: .zip

Change History (19)

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

Description: modified (diff)

Updated the syntax.

comment:3 Changed 6 years ago by Tobias McNulty

Owner: changed from nobody to Tobias McNulty
Status: newassigned

comment:4 Changed 6 years ago by Tobias McNulty

Triage Stage: UnreviewedAccepted

Changed 6 years ago by Tobias McNulty

Attachment: 13640.diff added

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

Changed 6 years ago by Tobias McNulty

Attachment: 13640_ducktyping.diff added

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

comment:5 Changed 6 years ago by Tobias McNulty

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

Severity: Normal
Type: Bug

comment:7 Changed 5 years ago by net147

Cc: net147 added
Easy pickings: unset

comment:8 Changed 5 years ago by net147

Easy pickings: set

Changed 5 years ago by b.leskes@…

Attachment: 13640_fix_only.diff added

Extracted the suggest fix from previous patches

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

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 Changed 5 years ago by Malcolm Tredinnick

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 Changed 5 years ago by Tobias McNulty

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

Triage Stage: AcceptedReady for checkin

comment:13 Changed 5 years ago by Tobias McNulty

Needs tests: unset
Patch needs improvement: unset

comment:14 Changed 5 years ago by Aymeric Augustin

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 Changed 5 years ago by Tobias McNulty

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

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