Opened 3 years ago

Closed 3 years ago

#17851 closed Bug (fixed)

Db Field instances cannot be compared to other objects

Reported by: charettes Owned by: charettes
Component: Database layer (models, ORM) Version: master
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 charettes 3 years ago.
fix-db-field-compare-with-advanced-comparison.diff (2.2 KB) - added by charettes 3 years ago.
fix-db-field-compare-with-test-docstring.diff (2.3 KB) - added by charettes 3 years ago.
total_ordering.diff (2.0 KB) - added by charettes 3 years ago.
Added total_ordering decorator to utils.functionnal

Download all attachments as: .zip

Change History (15)

Changed 3 years ago by charettes

comment:1 Changed 3 years ago by charettes

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from Db Field instances cannot be to other objects to Db Field instances cannot be compared to other objects

comment:2 Changed 3 years ago by claudep

  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 3 years ago by charettes

  • Version changed from 1.3 to 1.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 Changed 3 years ago by claudep

  • Triage Stage changed from Accepted to 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.

Changed 3 years ago by charettes

comment:5 Changed 3 years ago by charettes

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

comment:6 Changed 3 years ago by akaariai

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to 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 Changed 3 years ago by charettes

  • Version changed from 1.4 to master

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

Last edited 3 years ago by charettes (previous) (diff)

Changed 3 years ago by charettes

Added total_ordering decorator to utils.functionnal

comment:8 Changed 3 years ago by charettes

Updated pull request to use total_ordering.

comment:9 Changed 3 years ago by akaariai

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 Changed 3 years ago by charettes

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 Changed 3 years ago by akaariai

  • Patch needs improvement unset
  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.
Back to Top