Overriding `get_query_set()` on default manager requires care

[scott@…]

I had a model where objects was a custom manager that filtered so only models marked "active" were returned. It looks like:

class ClubManager(models.Manager):
    def get_query_set(self):
        return super(ClubManager, self).get_query_set().filter(active=True)

class Club(models.Model):
    ...
    active = models.BooleanField(default=False)
    
    objects = ClubManager()
    ...

It used to work fine, but I think the queryset-refactor merge has changed that. I had a unit test like:

    club = Club.objects.create(name='Drunken Clam')
    ...
    club.active = True
    club.save()

It started to fail on the call to save() because of a duplicate primary key. It was trying to insert the same record twice.

The culprit is in django.db.models.base.py Model class save_base() method (from line 305):

        manager = cls._default_manager
        if pk_set:
            # Determine whether a record with the primary key already exists.
            if manager.filter(pk=pk_val).extra(select={'a': 1}).values('a').order_by():

This is SVN r7547.

On the call to create() the record is inserted in the db. On the subsequent call to save() it checks the default manager for the pk. The default manager filters out the recently inserted model because it has active=False. A record with the pk is not found, so it tries to insert it again and gets a pk conflict.

Not sure if you consider this a bug. If not, it seems like a new constraint that the default manager must not filter the queryset.

Thanks.

Scott

[James Bennett]

You've always been able to get yourself into this sort of problem by overriding get_query_set(0. The solution is to be careful when overriding get_query_set(), which the docs remind you about.

And your ticket title is misleading; you most certainly can filter in the default manager, you just have to pay attention to what you're doing when you do it. I'm going to change it to avoid confusing people who find this ticket later.

[scott@…]

Thanks for the clarification. If I'm understanding correctly, it is not possible to call save() on a model which is not returned by the default manager. That seems pretty restrictive to me and is certainly a new restriction.

Adding a second (i.e. non-default) manager which returns all objects means I can retrieve any object, but I still can't modify and save it.

Previously, I could have the default manager filter out objects with active=False and have a second manager to return all objects. I could retrieve an object using the second manager, make it active, save it and it would then be returned by the default manager. It doesn't seem possible to do that now. Please tell me I'm wrong.

[James Bennett]

You can call save(). It's just that if you've replaced the default manager with something which doesn't neecessarily "know" about all your objects, you may have to add some manual validation logic of your own to ensure that things really are valid before you do the save.

"You must exercise care when doing this" is not the same as "you can not do this".

[scott@…]

Do you actually have working code doing this? You seem really sure it's not a problem, but from what I'm seeing, there's a lot more to it than "be careful".

The save_base() method checks if the model instance has a primary key (it does), checks if that primary key exists within the default manager (it does not), and then tries to insert it. Insert fails because it has already been inserted at some point in the past.

Other than overriding and reimplementing base_save() or doing something funky like swapping in a different default manager during save, how do you suggest it is possible to get an update instead of an insert?

[James Bennett]

When you override get_query_set(), which you're allowed to do because Django assumes you understand your application's needs better than it does, you're taking away some of Django's ability to automatically handle things for you. This means you may need to add in your own manually-written code to replace functionality Django would have provided. The extent of the code you'll need to write will vary from situation to situation, but it all boils down to, again, you needing to be careful in what you write and how. That's not a bug or a problem, it's something you should simply expect based on what your code is doing.