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)

fix-db-field-compare.diff (2.2 KB ) - added by Simon Charette 13 years ago.
fix-db-field-compare-with-advanced-comparison.diff (2.2 KB ) - added by Simon Charette 13 years ago.
fix-db-field-compare-with-test-docstring.diff (2.3 KB ) - added by Simon Charette 13 years ago.
total_ordering.diff (2.0 KB ) - added by Simon Charette 13 years ago.
Added total_ordering decorator to utils.functionnal

Download all attachments as: .zip

Change History (15)

by Simon Charette, 13 years ago

Attachment: fix-db-field-compare.diff added

comment:1 by Simon Charette, 13 years ago

Summary: Db Field instances cannot be to other objectsDb Field instances cannot be compared to other objects

comment:2 by Claude Paroz, 13 years ago

Triage Stage: UnreviewedAccepted

comment:3 by Simon Charette, 13 years ago

Version: 1.31.4

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.

comment:4 by Claude Paroz, 13 years ago

Triage Stage: AcceptedReady 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 Simon Charette, 13 years ago

comment:5 by Simon Charette, 13 years ago

I'm no committer but I updated the match according to your note :)

comment:6 by Anssi Kääriäinen, 13 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

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 Simon Charette, 13 years ago

Version: 1.4master

I moved this to a pull request. Do you think this should be set to DDN?

Last edited 13 years ago by Simon Charette (previous) (diff)

by Simon Charette, 13 years ago

Attachment: total_ordering.diff added

Added total_ordering decorator to utils.functionnal

comment:8 by Simon Charette, 13 years ago

Updated pull request to use total_ordering.

comment:9 by Anssi Kääriäinen, 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 Simon Charette, 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 Anssi Kääriäinen, 13 years ago

Patch needs improvement: unset
Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top