Opened 15 years ago

Closed 11 years ago

#11892 closed Bug (fixed)

Model __eq__ is not symmetric.

Reported by: pmcnerthney@… Owned by: nobody
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Colin Copeland, anssi.kaariainen@…, marti@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The Model test for equal implemented in eq is not symmetric. A non-deferred instance compares equal to a deferred instance, but a deferred instance compares unequal to a non-deferred instance. This is due to the following implementation of eq:

    def __eq__(self, other):
        return isinstance(other, self.__class__ and self._get_pk_val() == other._get_pk_val()

This needs to be changed to:

    def __eq__(self, other):
        return (isinstance(other, self.__class__) or isinstance(self, other.__class__)) and self._get_pk_val() == other._get_pk_val()

Which allows for complete symmetric of comparisons.

Attachments (2)

11892.diff (1.2 KB ) - added by Colin Copeland 14 years ago.
11892.deferred_model_eq.diff (1.6 KB ) - added by Johannes Dollinger 14 years ago.

Download all attachments as: .zip

Change History (15)

by Colin Copeland, 14 years ago

Attachment: 11892.diff added

comment:1 by Colin Copeland, 14 years ago

Cc: Colin Copeland added

Uploaded a patch with pmcnerthney's fix and an accompanying test.

comment:2 by Alex Gaynor, 14 years ago

Has patch: set
Triage Stage: UnreviewedAccepted

comment:3 by Johannes Dollinger, 14 years ago

Patch needs improvement: set

This patch also changes how __eq__ behaves for inherited models. Given:

class Base(models.Model): pass
class Sub(Base): pass

With the patch sub.base == sub, while currently sub.base != sub.

It also doen't handle the case where both instances have different deferred fields: qs.defer('f1)[0] != qs.defer('f2')[0].

Something like the following could work, but it seems the special-casing should rather happen in deferred_class_factory.

def __eq__(self, other):
    other_model = other.__class__
    if getattr(other_model, '_deferred', False):
        other_model = other_model.mro()[1]

    self_model = self.__class__
    if getattr(self_model, '_deferred', False):
        self_model = self_model.mro()[1]

    return issubclass(other_model, self_model) and self._get_pk_val() == other._get_pk_val()

by Johannes Dollinger, 14 years ago

comment:4 by Johannes Dollinger, 14 years ago

Patch needs improvement: unset

The patch uses the approach described in my previous comment.

comment:5 by anonymous, 14 years ago

Patch needs improvement: set
Version: 1.1SVN

comment:6 by Julien Phalip, 13 years ago

Severity: Normal
Type: Bug

comment:7 by Johannes Dollinger, 13 years ago

Easy pickings: unset
UI/UX: unset

related or duplicate: #16458

comment:8 by Aymeric Augustin, 13 years ago

#16458 is about inheritance, this one is about deferred instances. They're independent issues with similar symptoms.

in reply to:  8 comment:9 by Anssi Kääriäinen, 13 years ago

Cc: anssi.kaariainen@… added

Replying to aaugustin:

#16458 is about inheritance, this one is about deferred instances. They're independent issues with similar symptoms.

There is a patch in #16458 which solves this ticket.

comment:10 by Karen Tracey, 12 years ago

#17466 reported this again.

comment:11 by Marti Raudsepp, 11 years ago

Cc: marti@… added

comment:12 by Tim Graham, 11 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

Patch for #16458 is RFC.

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

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