Cannot save objects when using extra() in default manager

[miracle2k]

Possibly related to #5768 and #3358 (FIXME: select_related isn't supported in values() comment in source).

The last test should not throw an exception:

from django.db import models

class Shelf(models.Model):
    name = models.CharField(max_length=50)

    def __unicode__(self):
        return u"%s the shelf" % self.name


class BookManager(models.Manager):
    def get_query_set(self):
        return super(BookManager, self).get_query_set().\
            select_related('shelf').\
            extra(select={'shelf_name': 'extra_save_shelf.name'})


class Book(models.Model):
    name = models.CharField(max_length=50)
    shelf = models.ForeignKey(Shelf)

    objects = BookManager()

    def __unicode__(self):
        return u"%s the book" % self.name


__test__ = {'API_TESTS':"""
>>> shelf = Shelf(name="Foo")
>>> shelf.save()
>>> book = Book(name="bar", shelf=shelf)
>>> book.save()              # initial save works fine

>>> Book.objects.get(pk=1)   # querying works as well
<Book: bar the book>

# subsequent saves fail due to missing select_related
>>> book.save()
Traceback (most recent call last):
    ...
OperationalError: no such column: extra_save_shelf.name
"""
}

The problem seems to be that the .extra().values() query that is used to determine whether to do an update or insert doesn't use select_related(), while the manager's extra clause() relies on the tables from select_related existing.

Is there a reason why save's extra() query would need to consider the manager's at all? Wouldn't overwriting existing extra()s be the right thing to do in any case?

[miracle2k]

Added a simple patch that allows to reset a query's extra() data by passing False, and uses this for the values() query. It might be nicer to give extra a "clear" parameter instead (you probably don't want to reset the whole extra rather than specific options anyway most of the time), but I wanted to avoid changing the method signature.

[Malcolm Tredinnick]

This is actually just a symptom of a much larger problem; pretty much all custom managers are inappropriate to use for saving. Ultimately it comes down to using "default manager" for something that isn't really appropriate, but a lot of people are doing that.

It's fairly tricky to fix the root problem properly (since there are some very rare cases like GeoManager that do need to be involved, possibly), but I'm working on it.

[(none)]

Milestone post-1.0 deleted

[Malcolm Tredinnick]

(In [10056]) Use plain model.Manager, or suitable proxy, for model saving.

We can't use the default manager in Model.save_base(), since we need to
retrieve existing objects which might be filtered out by that manager. We now
always use a plain Manager instance at that point (or something that can
replace it, such as a GeoManager), making all existing rows in the
database visible to the saving code.

The logic for detecting a "suitable replacement" plain base is the same as for
related fields: if the use_for_related_fields is set on the manager subclass,
we can use it. The general requirement here is that we want a base class that
returns the appropriate QuerySet subclass, but does not restrict the rows
returned.

Fixed #8990, #9527.

Refs #2698 (which is not fixed by this change, but it's the first part of a
larger change to fix that bug.)

[Malcolm Tredinnick]

(In [10059]) [1.0.X] Use plain model.Manager, or suitable proxy, for model saving.

We can't use the default manager in Model.save_base(), since we need to
retrieve existing objects which might be filtered out by that manager. We now
always use a plain Manager instance at that point (or something that can
replace it, such as a GeoManager), making all existing rows in the
database visible to the saving code.

The logic for detecting a "suitable replacement" plain base is the same as for
related fields: if the use_for_related_fields is set on the manager subclass,
we can use it. The general requirement here is that we want a base class that
returns the appropriate QuerySet subclass, but does not restrict the rows
returned.

Fixed #8990, #9527.

Refs #2698 (which is not fixed by this change, but it's the first part of a
larger change to fix that bug.)

Backport of r10056 from trunk.