Code

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#8990 closed (fixed)

Cannot save objects when using extra() in default manager

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

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 6 years ago.

Download all attachments as: .zip

Change History (7)

Changed 6 years ago by miracle2k

comment:1 Changed 6 years ago by miracle2k

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 Changed 6 years ago by mtredinnick

  • Triage Stage changed from Unreviewed to Accepted

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 Changed 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:4 Changed 5 years ago by mtredinnick

  • Owner changed from nobody to mtredinnick
  • Status changed from new to assigned

comment:5 Changed 5 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from assigned to closed

(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 Changed 5 years ago by mtredinnick

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.