Opened 6 years ago

Closed 3 years ago

#14492 closed Bug (fixed)

Model proxy instance does not equal the respective model instance

Reported by: Byron Ruth Owned by: Byron Ruth
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: thepapermen Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Attachments (3)

model_eq_14233.diff (1.0 KB) - added by Byron Ruth 6 years ago.
django-14492.diff (1.3 KB) - added by Alex Gaynor 6 years ago.
model_eq_14405.diff (1.8 KB) - added by Byron Ruth 6 years ago.

Download all attachments as: .zip

Change History (22)

Changed 6 years ago by Byron Ruth

Attachment: model_eq_14233.diff added

comment:1 Changed 6 years ago by Alex Gaynor

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted

Changed 6 years ago by Alex Gaynor

Attachment: django-14492.diff added

comment:2 Changed 6 years ago by Alex Gaynor

I need to think about the semantics of this a bit more.

comment:3 Changed 6 years ago by Simon Meers

Yes, it is difficult to say whether they necessarily *should* be equal. They are two distinct objects with different properties, and it is possible to compare them explicitly if necessary. It depends on what you mean by *equal*...

comment:4 Changed 6 years ago by Byron Ruth

It might help to explain how I noticed this in the first place. I wrote an auth backend that sets request.user to a proxy instance of User. When I went to compare entry.author (for example) to request.user it failed. I agree that they certainly two different objects, but in this context i think it is appropriate to have them equal each other. The reality is that even though having an explicit method to do this comparison would be semantically correct, the reason for comparison is with respect to the underlying data the instance represents.

Alex, I like your idea except that for any models (like User) that are from third party apps, I don't want to have to implement the method on them.

comment:5 Changed 6 years ago by Russell Keith-Magee

Triage Stage: AcceptedDesign decision needed

As DrMeers suggests, the real issue here is "What is a Proxy", and "what is equality". Is a Proxy the same object, or is it a different object? #11154 addresses a similar conceptual issue, as related to permissions.

Although the decision on #11154 was logged by Malcolm, I was with him when he made it, and we discussed it at the time, and I agree with the reasoning. The same reasoning applies here, IMHO.

That said, I'm interested in hearing community debate on this issue (on django-dev, please, not on the ticket). I'm willing to be convinced that I'm wrong or in the minority opinion.

comment:6 Changed 6 years ago by Byron Ruth

Changed 6 years ago by Byron Ruth

Attachment: model_eq_14405.diff added

comment:7 Changed 6 years ago by Byron Ruth

Latest patch attached, includes Alex's tests. Checks to ensure the object being compared against is an instance of Model.

comment:8 Changed 6 years ago by Byron Ruth

Status: newassigned

comment:9 Changed 6 years ago by Byron Ruth

Component: Core frameworkDatabase layer (models, ORM)
Triage Stage: Design decision neededReady for checkin

Since this is a relatively minor patch, I would like to see if it can get into 1.3. There has not been anymore discussion as to why this shouldn't be added. I think per what Russell and Malcolm has stated this is a valid implementation.

comment:10 Changed 6 years ago by Russell Keith-Magee

#15054 is another presentation of the same underlying issue (what is a proxy).

comment:11 Changed 6 years ago by Luke Plant

Triage Stage: Ready for checkinDesign decision needed

If I read those other discussions correctly, there is no consensus that this is the correct design decision, so it cannot be 'Ready for checkin'.

The closest we have to consensus is on #15054, which has a valid use case for proxy models not being treated as equal (with a loose definition of 'equal') to the parent model, which goes the other way from this. So I'm knocking it back to 'design decision needed'.

comment:12 Changed 5 years ago by Julien Phalip

Severity: Normal
Type: Bug

comment:13 Changed 5 years ago by thepapermen

Cc: thepapermen added
Easy pickings: unset
UI/UX: unset

comment:14 Changed 5 years ago by Jacob

milestone: 1.3

Milestone 1.3 deleted

comment:15 Changed 3 years ago by Alex Gaynor

Triage Stage: Design decision neededAccepted

I'm not sure this solution is correct, but the current situation definitely is not, marking this as accepted.

comment:16 Changed 3 years ago by Ryan Kaskel

Since this ticket has been accepted again, I've added a new patch: https://github.com/django/django/pull/1480/files

It would be great to get some feedback on the approach and how to handle backwards compatibility.

Last edited 3 years ago by Ryan Kaskel (previous) (diff)

comment:17 Changed 3 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

Actually, there's already another pull request open which addresses this ticket.

comment:18 Changed 3 years ago by Byron Ruth

Great to see this finally ready to go in.

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

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