Django

Code

Ticket #9527 (closed: fixed)

Opened 1 year ago

Last modified 1 year ago

Model.save_base() depends on Model._default_manager

Reported by: Daniel Pope <dan@mauveinternet.co.uk> Assigned to: mtredinnick
Milestone: Component: Database layer (models, ORM)
Version: 1.0 Keywords:
Cc: Triage Stage: Accepted
Has patch: 0 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 0

Description

In qs-rf, Model.save_base was refactored to check whether to perform an SQL UPDATE versus INSERT using the Model's default manager rather than raw SQL. This means that the ORM now breaks if the default manager does filtering. Any model instance that is not returned by default cannot be successfully updated. The ORM does not find the copy already saved, and tries to insert instead, causing an IntegrityError.

My manager looks like this:

class PropertyManager(models.Manager):
        def get_query_set(self):
                """By default, exclude properties not even committed"""
                return super(PropertyManager, self).get_query_set().exclude(status='--')

        def uncommitted(self):
                """Retrieve those properties that are not queried by default"""
                return super(PropertyManager, self).get_query_set().filter(status='--')

The intention was for the default manager to only return committed properties. Nowhere on the site should uncommitted properties appear: these uncommitted properties exist only while being constructed using a wizard (but need to be in the database, not just the session, so models.ImageField works).

According to the documentation for default managers (http://docs.djangoproject.com/en/dev/topics/db/managers/), you must "be careful in your choice of default manager", but this does not state that a default manager must return all rows or risk breakage.

The expectation is that saving should work regardless of the default manager.

Attachments

Change History

11/05/08 07:43:08 changed by Daniel Pope <dan@mauveinternet.co.uk>

  • needs_better_patch changed.
  • needs_tests changed.
  • needs_docs changed.

A workaround is to monkey-patch _default_manager for the duration of the .save() call

        objects = PropertyManager()
        raw_objects = models.Manager()
    
        ...

        def save(self, force_insert=False, force_update=False):
                cls = self.__class__
                default = cls._default_manager
                cls._default_manager = cls.raw_objects
                try:
                        super(Property, self).save(force_insert=force_insert, force_update=force_update)
                finally:
                        cls._default_manager = default

02/26/09 15:07:01 changed by jacob

  • stage changed from Unreviewed to Accepted.

03/13/09 01:58:11 changed by mtredinnick

  • owner changed from nobody to mtredinnick.
  • status changed from new to assigned.

03/14/09 22:41:33 changed by mtredinnick

  • status changed from assigned to closed.
  • resolution set to fixed.

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

03/14/09 22:45:32 changed 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/Change #9527 (Model.save_base() depends on Model._default_manager)




Change Properties
Action