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

[LukaszKorzybski]

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,

[LukaszKorzybski]

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

[Alex Gaynor]

Updated the syntax.

[Tobias McNulty]

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

[Tobias McNulty]

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

[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.

[b.leskes@…]

Extracted the suggest fix from previous patches

[b.leskes@…]

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).

[Malcolm Tredinnick]

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.

[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

[Aymeric Augustin]

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

[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.

[Karen Tracey]

In [17093]:

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