Opened 8 years ago

Closed 7 years ago

#2936 closed defect (fixed)

[patch]override __hash__ method of django.db.base.Model

Reported by: favo@… Owned by: adrian
Component: Database layer (models, ORM) Version: master
Severity: normal Keywords:
Cc: favo@…, canburak@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Since django Model has override __eq__ method, should override __hash__
method too (objects comparing equal should also hash to the
same value).

Attachments (1)

add_hash_to_model.diff (529 bytes) - added by favo@… 8 years ago.
add hash to django.db.model.base

Download all attachments as: .zip

Change History (12)

Changed 8 years ago by favo@…

add hash to django.db.model.base

comment:1 Changed 8 years ago by anonymous

  • Summary changed from override __hash__ method of django.db.base.Model to [patch]override __hash__ method of django.db.base.Model

comment:2 Changed 8 years ago by jim-django@…

There's no need to stringify the primary key before hashing, is there?

comment:3 Changed 8 years ago by favo@…

hmm... I just remember some strange situation that I meet. anyway it seems no need.

comment:4 Changed 8 years ago by favo@…

  • Resolution set to invalid
  • Status changed from new to closed

I was totally wrong to open this ticket, since http://docs.python.org/ref/customization.html:

  If a class defines mutable objects and implements a __cmp__() or __eq__() method, 
it should not implement __hash__()

And django model is a mutable object. so we can't implement __hash__().

comment:5 Changed 8 years ago by Ned Batchelder <ned@…>

I think in this case the Python docs were mis-interpreted. The key
point to consider when writing a hash function is that two objects
that compare equal must hash equally, and the hash value cannot
change. Just because an object is mutable doesn't mean that these
rules will be violated. A mutable object may have a hash
that uses its mutable data as part of the hash implementation,
which would be unacceptable. But a mutable object can have a carefully
planned hash which only uses the immutable data in its calculation.

In particular, Django model instances will
usually simply use their id attribute as the hash, and so will be
perfectly acceptable as dictionary keys.

comment:6 Changed 8 years ago by vdupras@…

I agree with Ned here, I would expect len(set([Model.objects.get(pk=1), Model.objects.get(pk=1)])) to be 1, but it's 2. I'd be inclined to consider it as a bug.

comment:7 Changed 7 years ago by Jim Crossley <jcrossley@…>

  • Resolution invalid deleted
  • Status changed from closed to reopened

I concur. Using sets to filter duplicates is a common, efficient idiom. We should be able to do the same with model instances, mutable or not. The very fact that model equality is determined by their id alone makes the question of mutability irrelevant.

Consider a contrived example:

x = set(Foo.objects.all())
for f in Foo.objects.all():
  x.add(f)

As it now stands, you'll end up with two copies of each Foo model in the set, a clear violation of the "principle of least surprise", IMHO.

comment:8 Changed 7 years ago by mir

Please, could you send an email to django-developers? I think this has been rejected by the core developers, but your arguments are valid.

comment:9 Changed 7 years ago by mtredinnick

  • Triage Stage changed from Unreviewed to Accepted

It hasn't been rejected. There's no need for an email, it's a reasonable request. The original ticket burned and died based on mistaken assumptions (as Ned correctly notes) and just never got onto the radar.

comment:10 Changed 7 years ago by Can Burak Cilingir <canburak@…>

  • Cc canburak@… added

comment:11 Changed 7 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [7132]) Fixed #2936, #6500 -- Added a hash() method to Models (since we implement our own eq method).

Note: See TracTickets for help on using tickets.
Back to Top