Opened 6 years ago

Closed 2 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: copelco, 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 copelco 6 years ago.
11892.deferred_model_eq.diff (1.6 KB) - added by emulbreh 6 years ago.

Download all attachments as: .zip

Change History (15)

Changed 6 years ago by copelco

comment:1 Changed 6 years ago by copelco

  • Cc copelco 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 6 years ago by Alex

  • Has patch set
  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 6 years ago by emulbreh

  • 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 6 years ago by emulbreh

comment:4 Changed 6 years ago by emulbreh

  • Patch needs improvement unset

The patch uses the approach described in my previous comment.

comment:5 Changed 6 years ago by anonymous

  • Patch needs improvement set
  • Version changed from 1.1 to SVN

comment:6 Changed 4 years ago by julien

  • Severity set to Normal
  • Type set to Bug

comment:7 Changed 4 years ago by emulbreh

  • Easy pickings unset
  • UI/UX unset

related or duplicate: #16458

comment:8 follow-up: Changed 4 years ago by aaugustin

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

comment:9 in reply to: ↑ 8 Changed 4 years ago by akaariai

  • 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 4 years ago by kmtracey

#17466 reported this again.

comment:11 Changed 3 years ago by intgr

  • Cc marti@… added

comment:12 Changed 2 years ago by timo

  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin

Patch for #16458 is RFC.

comment:13 Changed 2 years ago by akaariai

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