Opened 16 years ago

Closed 16 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: dev
Severity: Keywords:
Cc: Maniac@… Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

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 16 years ago.
Patch that looks for use_for_related_fields attribute on managers.
get_full_query_set.diff (2.5 KB ) - added by jbronn 16 years ago.
Patch from Ivan Sagalaev that offers a different approach.

Download all attachments as: .zip

Change History (8)

by jbronn, 16 years ago

Attachment: use_for_related_fields.diff added

Patch that looks for use_for_related_fields attribute on managers.

comment:1 by magneto, 16 years ago

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 by jbronn, 16 years ago

Summary: Re: #7666 -- Manager 'get' overloading now failesRe: #7666 -- Manager 'get' overloading now fails

comment:3 by jbronn, 16 years ago

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

comment:4 by jbronn, 16 years ago

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.

by jbronn, 16 years ago

Attachment: get_full_query_set.diff added

Patch from Ivan Sagalaev that offers a different approach.

comment:5 by Ivan Sagalaev, 16 years ago

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 by Jacob, 16 years ago

Resolution: fixed
Status: newclosed

(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.

Note: See TracTickets for help on using tickets.
Back to Top