Opened 7 years ago

Closed 3 years ago

#11892 closed Bug (fixed)

Model __eq__ is not symmetric.

Reported by: pmcnerthney@… Owned by: nobody
Component: Database layer (models, ORM) Version: master
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 7 years ago.
11892.deferred_model_eq.diff (1.6 KB) - added by Johannes Dollinger 7 years ago.

Download all attachments as: .zip

Change History (15)

Changed 7 years ago by Colin Copeland

Attachment: 11892.diff added

comment:1 Changed 7 years ago by Colin Copeland

Cc: Colin Copeland added
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

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

comment:2 Changed 7 years ago by Alex Gaynor

Has patch: set
Triage Stage: UnreviewedAccepted

comment:3 Changed 7 years ago by Johannes Dollinger

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()

Changed 7 years ago by Johannes Dollinger

comment:4 Changed 7 years ago by Johannes Dollinger

Patch needs improvement: unset

The patch uses the approach described in my previous comment.

comment:5 Changed 7 years ago by anonymous

Patch needs improvement: set
Version: 1.1SVN

comment:6 Changed 5 years ago by Julien Phalip

Severity: Normal
Type: Bug

comment:7 Changed 5 years ago by Johannes Dollinger

Easy pickings: unset
UI/UX: unset

related or duplicate: #16458

comment:8 Changed 5 years ago by Aymeric Augustin

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

comment:9 in reply to:  8 Changed 5 years ago by Anssi Kääriäinen

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 Changed 5 years ago by Karen Tracey

#17466 reported this again.

comment:11 Changed 4 years ago by Marti

Cc: marti@… added

comment:12 Changed 3 years ago by Tim Graham

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

Patch for #16458 is RFC.

comment:13 Changed 3 years ago by Anssi Kääriäinen

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