Code

Opened 6 years ago

Closed 6 years ago

#7904 closed (fixed)

Manager 'get' overloading now fails / Accessing single related (parent) objects should not bypass manager

Reported by: magneto Owned by: nobody
Component: Core (Other) Version: master
Severity: Keywords:
Cc: Maniac@… Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The fix to Issue #7666 caused Manage overloading for "get" to fail (rather it now fails to use the overloaded method)

from django.db import models 

class M1Manager(models.Manager):
   def get(self, *args, **kwargs):
       """ over load 'get' to allow for more fined tuned actions """
       #in this trivial example, all gets return none
       return None

class M1(models.Model):
    col1 = models.CharField(max_length = 30)
    objects = M1Manager()
	
    __unicode__(self):
         return col1
	 
class M2(models.Model):
    m1 = models.ForeignKey(M1)
    col2 = models.CharField()



my_m1 = M1.objects.create(col1="m1_obj")
my_m2 = M2.objects.create(m1 = my_m1, col2="my_m2")

find_m2 = M2.objects.get(col2 = "my_m2")
#prints "m1_obj" and should print None
print find_m2.m1

Attachments (2)

use_for_related_fields.diff (934 bytes) - added by jbronn 6 years ago.
Patch that looks for use_for_related_fields attribute on managers.
get_full_query_set.diff (2.5 KB) - added by jbronn 6 years ago.
Patch from Ivan Sagalaev that offers a different approach.

Download all attachments as: .zip

Change History (8)

Changed 6 years ago by jbronn

Patch that looks for use_for_related_fields attribute on managers.

comment:1 Changed 6 years ago by magneto

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

I take it that little option would go into the Manager

class M1Manager(models.Manager):
   use_for_related_fields = True
   def get(self, *args, **kwargs):
       """ over load 'get' to allow for more fined tuned actions """
       #in this trivial example, all gets return none
       return None

in any case, that does seem to work, but there's still something fishy about the fact that normal object inheritance does not seem to be obeyed

comment:2 Changed 6 years ago by jbronn

  • Summary changed from Re: #7666 -- Manager 'get' overloading now failes to Re: #7666 -- Manager 'get' overloading now fails

comment:3 Changed 6 years ago by jbronn

  • Summary changed from Re: #7666 -- Manager 'get' overloading now fails to Manager 'get' overloading now fails / Accessing single related (parent) objects should not bypass manager

comment:4 Changed 6 years ago by jbronn

Ivan opened up #8119 which I closed as a duplicate of this ticket. He had another approach, and I moved the patch and comments to this ticket:

This is a spun-of from http://code.djangoproject.com/ticket/7666#comment:7 with the relevant discussion here: http://groups.google.com/group/django-developers/browse_frm/thread/a2326c32f8860fb0/

The solution is to have a separate queryset factory on a manager get_full_query_set that can be overridden but is required to return all records. Old get_query_set normally just calls get_full_query_set and can be overridden in a usual way. This part is backwards compatible. Also managers acquire a new method get_full which works like get but explicitly calls get_full_query_set. This method is used to access single related (parent) objects. This will require users who was overriding get to override get_full instead if they wish to ensure that default filtering doesn't get in the way. This is as backwards incompatible as was after #7666 that has broken overriding of get but provides a new API to overcome this.

Changed 6 years ago by jbronn

Patch from Ivan Sagalaev that offers a different approach.

comment:5 Changed 6 years ago by isagalaev

  • Cc Maniac@… added
  • Has patch set

Incidentally, I was also thinking about setting some state to a Manager before calling get. But I've decided against it because:

  • when the flag is set permanently to True it binds get to behave only in a single fashion, but the point of #7666 is that it should have different behavior depending on if it is called for parent relation or directly with Model.objects.get
  • switching the flag dynamically creates a possibility to forget to do it and, worse, introduces a transient state into process which will break things if Manager.get calls other functions that also indirectly call Manager.get counting on different state
  • a stack of states might fix previous issue but this solution is really overcomplicated and ugly

This is why I've just made an explicit full_get that can be called separately from get. And two queryset factories are needed because queryset creation in general may be customized in many different ways but filtering is the only type of customization that we care about when accessing parent objects. Thus splitting it into a separate point.

comment:6 Changed 6 years ago by jacob

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

(In [8212]) Fixed #7904: added support for a "use_for_related_fields" property on managers. If True, the manager will be used for related object lookups instead of the "bare" QuerySet introduced bu [8107]. Patch from Justin Bronn.

Add Comment

Modify Ticket

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


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

 
Note: See TracTickets for help on using tickets.