Opened 14 years ago

Closed 11 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: dev
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

Description

Attachments (3)

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

Download all attachments as: .zip

Change History (22)

by Byron Ruth, 14 years ago

Attachment: model_eq_14233.diff added

comment:1 by Alex Gaynor, 14 years ago

Triage Stage: UnreviewedAccepted

by Alex Gaynor, 14 years ago

Attachment: django-14492.diff added

comment:2 by Alex Gaynor, 14 years ago

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

comment:3 by Simon Meers, 14 years ago

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 by Byron Ruth, 14 years ago

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 by Russell Keith-Magee, 14 years ago

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.

by Byron Ruth, 14 years ago

Attachment: model_eq_14405.diff added

comment:7 by Byron Ruth, 14 years ago

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

comment:8 by Byron Ruth, 14 years ago

Status: newassigned

comment:9 by Byron Ruth, 14 years ago

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 by Russell Keith-Magee, 14 years ago

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

comment:11 by Luke Plant, 14 years ago

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 by Julien Phalip, 13 years ago

Severity: Normal
Type: Bug

comment:13 by thepapermen, 13 years ago

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

comment:14 by Jacob, 13 years ago

milestone: 1.3

Milestone 1.3 deleted

comment:15 by Alex Gaynor, 12 years ago

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 by Ryan Kaskel, 11 years ago

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 11 years ago by Ryan Kaskel (previous) (diff)

comment:17 by Tim Graham, 11 years ago

Triage Stage: AcceptedReady for checkin

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

comment:18 by Byron Ruth, 11 years ago

Great to see this finally ready to go in.

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

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