#7666: Default managers should not restrict access to single related objects
Joseph Kocherhans
what I believe is a bug. The code is a lot easier to understand than
my english, so I'll let it speak for itself. To be clear, I'm just
talking about changing the behavior of
ReverseSingleRelatedObjectDescriptor, not
SingleRelatedObjectDescriptor.
I couldn't find any related tickets, so if nothing else I hope this
ticket helps people find the reasoning even if we just close it.
Here's the example:
class SourceManager(models.Manager):
def get_query_set(self):
return super(SourceManager, self).get_query_set().filter(is_public=True)
class Source(models.Model):
is_public = models.BooleanField()
objects = SourceManager()
class Item(models.Model):
source = models.ForeignKey(Source)
>>> public_source = Source.objects.create(is_public=True)
>>> public_item = Item.objects.create(source=public_source)
>>> private_source = Source.objects.create(is_public=False)
>>> private_item = Item.objects.create(source=private_source)
# SUCCESS
>>> public_item.source
<Source: Source object>
# FAIL: Raises DoesNotExist when it quite clearly *does* exist. I
don't think the default manager should have any say here)
>>> private_item.source
<Source: Source object>
Joseph
Adrian Holovaty
> I've filed #7666 [1] which has a test I wrote that fails to illustrate
> what I believe is a bug. The code is a lot easier to understand than
> my english, so I'll let it speak for itself.
This is certainly a bug. The main question is how to fix it.
Currently, ReverseSingleRelatedObjectDescriptor uses _default_manager,
which is the problem (line 239 in django/db/models/related/fields.py).
One clean solution would be to give each model a _pristine_manager
attribute, which would be *guaranteed* to be a simple Manager()
instance -- i.e., not a custom one. Then we could use
_pristine_manager here, in this case.
This would add a tiny amount of overhead to model creation, because
that extra Manager() instance would need to be created, but the extra
overhead would only take effect for models that use a custom manager.
Another, less generic, solution could be to change the
ReverseSingleRelatedObjectDescriptor code so that it creates a
Manager() object right then and there. But that transfers the overhead
to runtime (query time) and penalizes *everybody* instead of just the
people who have created custom managers.
Other solutions?
Adrian
--
Adrian Holovaty
holovaty.com | everyblock.com | djangoproject.com
Ivan Sagalaev
> This is certainly a bug. The main question is how to fix it.
Oh, it's a big can of worms, actually. I was once pondering on this and
found some tricky cases.
1) Managers are not just restrict querysets by filtering. They can be
used for example to produce custom queryset classes with new properties,
which in turn produce instances with new properties. Then if you use
some "default" queryset for accessing a parent it'll lack the behavior
of other instances of its class.
2) There's a problem with filtering managers not only on the way up
(from child to parent) but on the way down also. If you want to delete a
parent it will try to delete its children beforehand. But if the default
manager of children won't give them all DB won't let the object be deleted.
> Currently, ReverseSingleRelatedObjectDescriptor uses _default_manager,
> which is the problem (line 239 in django/db/models/related/fields.py).
> One clean solution would be to give each model a _pristine_manager
> attribute, which would be *guaranteed* to be a simple Manager()
> instance
Yes I think there should be some pristine manager but it should not be
automatically a simple Manager() instance. Instead it should be a public
attribute "all_objects" that can be set by user but should contain a
manager giving all objects. By default it will of course be just
Manager() and be equal to "objects".
James Bennett
> Currently, ReverseSingleRelatedObjectDescriptor uses _default_manager,
> which is the problem (line 239 in django/db/models/related/fields.py).
> One clean solution would be to give each model a _pristine_manager
> attribute, which would be *guaranteed* to be a simple Manager()
> instance -- i.e., not a custom one. Then we could use
> _pristine_manager here, in this case.
Why bother with the overhead of creating and maintaining an entire
Manager instance, when what's really wanted is a pristine QuerySet?
That can be obtained by the simple expedient of
qs = QuerySet(whatever_model_this_should_be)
and doesn't come with any baggage.
--
"Bureaucrat Conrad, you are technically correct -- the best kind of correct."
Nick Lane
> > Currently, ReverseSingleRelatedObjectDescriptor uses _default_manager,
> > which is the problem (line 239 in django/db/models/related/fields.py).
> > One clean solution would be to give each model a _pristine_manager
> > attribute, which would be *guaranteed* to be a simple Manager()
> > instance -- i.e., not a custom one. Then we could use
> > _pristine_manager here, in this case.
>
> Why bother with the overhead of creating and maintaining an entire
> Manager instance, when what's really wanted is a pristine QuerySet?
> That can be obtained by the simple expedient of
>
> qs = QuerySet(whatever_model_this_should_be)
>
> and doesn't come with any baggage.
suggestion of a pristine QuerySet.
Justin Bronn
> suggestion of a pristine QuerySet.
for those who are using custom managers to properly generate SQL for
non-standard columns, e.g., geometries. The patch committed in r8011
has the unintended side-effect of breaking related geographic queries
for MySQL and Oracle because it bypasses the default manager
(GeoManager) and thus does not know how to properly select geometry
columns.
One possible solution would be look for the presence of an attribute
on the default manager so that it used instead of QuerySet.
Jacob Kaplan-Moss
> Using a "pristine" QuerySet is a bad assumption to make, especially
> for those who are using custom managers to properly generate SQL for
> non-standard columns, e.g., geometries. The patch committed in r8011
> has the unintended side-effect of breaking related geographic queries
> for MySQL and Oracle because it bypasses the default manager
> (GeoManager) and thus does not know how to properly select geometry
> columns.
Nuts -- I didn't think of that. Sorry!
> One possible solution would be look for the presence of an attribute
> on the default manager so that it used instead of QuerySet.
Hrm, so django/trunk/django/db/models/fields/related.py:240 becomes::
rel_obj = self.field.rel.to._default_manager.get_pristine_queryset().get(**params)
(assuming an appropriate get_pristine_queryset() funtion)?
Jacob
magneto
but i reopened #7666 for a reason mentioned in the ticket (i hope i
stepped on no ones toes by doing that)
the comment reads as follows
"""
this change, effectively kills "get" overloading in Managers
i can think of a million reasons way this is necessary,
caching these 'gets' in some local/thread/memcached world
'special gets' (deleted flags and permissions)
database sharding, etc, etc
i hope y'all re think this before Versions 1, so i'm reopening this as
since the 'QuerySet?' approach you took has no ability to be
overloaded
"""
bo
On Jul 22, 9:37 am, "Jacob Kaplan-Moss" <jacob.kaplanm...@gmail.com>
wrote: