Opened 16 years ago

Closed 16 years ago

Last modified 16 years ago

#8990 closed (fixed)

Cannot save objects when using extra() in default manager

Reported by: miracle2k Owned by: Malcolm Tredinnick
Component: Database layer (models, ORM) Version: dev
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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?

Attachments (1)

reset-extra.diff (7.0 KB ) - added by miracle2k 16 years ago.

Download all attachments as: .zip

Change History (7)

by miracle2k, 16 years ago

Attachment: reset-extra.diff added

comment:1 by miracle2k, 16 years ago

Has patch: set

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.

comment:2 by Malcolm Tredinnick, 16 years ago

Triage Stage: UnreviewedAccepted

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.

comment:3 by (none), 16 years ago

milestone: post-1.0

Milestone post-1.0 deleted

comment:4 by Malcolm Tredinnick, 16 years ago

Owner: changed from nobody to Malcolm Tredinnick
Status: newassigned

comment:5 by Malcolm Tredinnick, 16 years ago

Resolution: fixed
Status: assignedclosed

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

comment:6 by Malcolm Tredinnick, 16 years ago

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

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