Opened 13 years ago
Closed 13 years ago
#17851 closed Bug (fixed)
Db Field instances cannot be compared to other objects
Reported by: | Simon Charette | Owned by: | Simon Charette |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | field compare |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
`django.db.models.fields.Field.__cmp__` assumes that it's always compared to a another Field
instance thus raising an AttributeError
when the following code is executed:
from django.db.models.fields import Field Field() == None
It should make sure that other
is a Field
instance before accessing the creation_counter
property and if not return NotImplemented
.
I stumbled on this issue when trying to use a PickledObjectField
to store Field
instance. When calling full_clean
on the field it raised an AttributeError
because it was trying to compare it EMPTY_VALUES
: value in EMPTY_VALUES
.
I agree that this is NOT a common use case of the Field class however I think that Field
instances should be able to compare themselves to other objects without raising a confusing AttributeError
.
I'm attaching a patch that sticks with simple comparison (although we should REALLY move to advanced comparison) with tests.
Here's a chat log I had on #python concerning simple comparison:
charettes 13:31:23 What should an object that implements __cmp__ method should return if it can't compare to other. papna 13:31:49 charettes: 1) Don't write __cmp__ methods unless you're targetting mesozoic Python 2) NotImplemented charettes 13:32:41 papna: But the doc doesn't say that... it says only to use it for advanced comparaison charettes 13:32:58 papna: http://docs.python.org/reference/datamodel.html#object.__cmp__ papna 13:33:42 charettes: Why are you using __cmp__? charettes 13:34:42 papna: actually I'm writing a patch for a project that used this method... should I suggest to switch to adavanced comparaison? papna 13:36:43 charettes: IIRC __cmp__ still uses the paradigm that __eq__ and friends and __add__ and friends and plenty of other stuff does, but I might be wrong. It's not flexible, which is why it has been deprecated well over a decade. charettes 13:37:27 papna: aight ty for your time
I can adapt the current patch to use advanced comparison instead if you prefer.
Attachments (4)
Change History (15)
by , 13 years ago
Attachment: | fix-db-field-compare.diff added |
---|
comment:1 by , 13 years ago
Summary: | Db Field instances cannot be to other objects → Db Field instances cannot be compared to other objects |
---|
comment:2 by , 13 years ago
Triage Stage: | Unreviewed → Accepted |
---|
by , 13 years ago
Attachment: | fix-db-field-compare-with-advanced-comparison.diff added |
---|
comment:3 by , 13 years ago
Version: | 1.3 → 1.4 |
---|
comment:4 by , 13 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Note to the committer: transform the test comment in a docstring and replace __cmp__
by __lt__
in the same comment. Apart from that, looks good and tests pass.
by , 13 years ago
Attachment: | fix-db-field-compare-with-test-docstring.diff added |
---|
comment:6 by , 13 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
Maybe I am over-complicating this ... but I think we should define total ordering for the Field class. This could be done by adding total_ordering support in utils.functional (*), adding __eq__
for Field and then just decorating the Field class with @total_ordering. Defining total ordering is a PEP-8 recommendation.
(*) Total ordering is available in Python 2.7 from functools, for 2.6 it could be just added by copy-pasting the functools code to utils/functional.py, or by using this snippet: http://code.activestate.com/recipes/576685-total-ordering-class-decorator/.
It would be best to have the total_ordering support as a separate patch (if not separate ticket). The Field total_ordering support would then be an easy addition using charettes' patch.
comment:7 by , 13 years ago
Version: | 1.4 → master |
---|
I moved this to a pull request. Do you think this should be set to DDN?
by , 13 years ago
Attachment: | total_ordering.diff added |
---|
Added total_ordering decorator to utils.functionnal
comment:9 by , 13 years ago
A couple minor notes:
- A mention of where the code of "our" implementation of total_ordering comes from (Python 2.7 I guess?)
- A very simple test case for total_ordering? (A class that defines
__lt__
and__eq__
, uses @total_ordering, and then test two instances with <, >, <=, >=.)
It would be nice to get another opinion if this is overcomplicating... But IMO not, the decorator might find other uses, and following PEP-8 is always a good choice.
comment:10 by , 13 years ago
I don't want to misinterpret his comment but Alex said it looked fine with `@total_ordering` on github.
I updated the pull request with a comment explaining that the code is borrowed for python 2.7.3.
Are you sure a TestCase is needed? I can write it but I feel like it must be already tested if it's included in stdlib?
comment:11 by , 13 years ago
Patch needs improvement: | unset |
---|---|
Resolution: | → fixed |
Status: | new → closed |
Fixed in commit 5cbfb48b92cb26a335f9c8c0f79d3390290103e2
Added an improved patch that rely on
__lt__
instead of__cmp__
since this is really only used for sorting anyway. All tests pass on trunk.