Opened 4 years ago

Closed 11 months ago

#14492 closed Bug (fixed)

Model proxy instance does not equal the respective model instance

Reported by: bruth Owned by: bruth
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 bruth 4 years ago.
django-14492.diff (1.3 KB) - added by Alex 4 years ago.
model_eq_14405.diff (1.8 KB) - added by bruth 4 years ago.

Download all attachments as: .zip

Change History (22)

Changed 4 years ago by bruth

comment:1 Changed 4 years ago by Alex

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Changed 4 years ago by Alex

comment:2 Changed 4 years ago by Alex

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

comment:3 Changed 4 years ago by DrMeers

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

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 (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 4 years ago by russellm

  • Triage Stage changed from Accepted to Design 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.

Changed 4 years ago by bruth

comment:7 Changed 4 years ago by bruth

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

comment:8 Changed 4 years ago by bruth

  • Status changed from new to assigned

comment:9 Changed 4 years ago by bruth

  • Component changed from Core framework to Database layer (models, ORM)
  • Triage Stage changed from Design decision needed to Ready 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 3 years ago by russellm

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

comment:11 Changed 3 years ago by lukeplant

  • Triage Stage changed from Ready for checkin to Design 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 3 years ago by julien

  • Severity set to Normal
  • Type set to Bug

comment:13 Changed 3 years ago by thepapermen

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

comment:14 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

comment:15 Changed 15 months ago by Alex

  • Triage Stage changed from Design decision needed to Accepted

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

comment:16 Changed 11 months ago by ryankask

Since this ticket has been accepted again, I've added a new patch:

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

Last edited 11 months ago by ryankask (previous) (diff)

comment:17 Changed 11 months ago by timo

  • Triage Stage changed from Accepted to Ready for checkin

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

comment:18 Changed 11 months ago by bruth

Great to see this finally ready to go in.

comment:19 Changed 11 months ago by akaariai

  • Resolution set to fixed
  • Status changed from assigned to closed

Add Comment

Modify Ticket

Change Properties
<Author field>
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'

E-mail address and user name can be saved in the Preferences.

Note: See TracTickets for help on using tickets.