Code

Opened 10 months ago

Closed 9 months ago

Last modified 3 months ago

#21174 closed Bug (fixed)

Transaction handling broken in related manager modification methods

Reported by: akaariai Owned by: nobody
Component: Database layer (models, ORM) Version: 1.6-beta-1
Severity: Normal Keywords:
Cc: loic@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by akaariai)

Some of the related manager methods do multiple data-modifying operations but do not use any transaction management at all. This is clearly broken. This isn't a regression, so isn't a release blocker for that reason. However, this is a potential data corruption issue, so might be nice to fix for 1.6, too.

I am not sure how to test this. Maybe post_delete/save signals could be used to force exceptions and thus rollback.

EDIT: Forgot to add that a patch for some problematic methods available in https://github.com/django/django/pull/1684.

Attachments (0)

Change History (8)

comment:1 Changed 10 months ago by akaariai

  • Description modified (diff)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 10 months ago by loic84

  • Cc loic@… added

This also has the benefit of fixing #21169 for GFK RelatedManager.remove().

The same applies to FK RelatedManager.remove() which also suffers from this issue. I would also implement it self.using(db).filter(pk__in=[o.pk for o in objs]).delete(), solving both transaction handling and default filtering at once.

I've got the test cases for these stashed in a branch for #21169.

One thing to note though is the mild backward incompatibility: signals won't be fired anymore.

comment:3 Changed 10 months ago by akaariai

The .delete() code takes care of sending signals. Unfortunately it will also reload the instances back from DB to do so. It might be possible to use the lower level deletion APIs to send the already existing objects to be deleted in one go - no reload that way.

comment:4 Changed 10 months ago by loic84

It just came back to mind that RelatedManager.remove() for FK doesn't delete(), it only SET_NULL, there's a bit of an incompatibility between FK et GFK here...

So yeah for FK the signal incompatibility would be changing the fly of save() by an update().

comment:5 Changed 9 months ago by Anssi Kääriäinen <akaariai@…>

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

In 1df3c49a1a1c11198d181ddd0ce31bbb42e631d8:

Fixed #21174 -- transaction control in related manager methods

comment:6 Changed 8 months ago by Anssi Kääriäinen <akaariai@…>

In 17c3997f6828e88e4646071a8187c1318b65597d:

Fixed #21169 -- Reworked RelatedManager methods use default filtering

The remove() and clear() methods of the related managers created by
ForeignKey, GenericForeignKey, and ManyToManyField suffered from a
number of issues. Some operations ran multiple data modifying queries without
wrapping them in a transaction, and some operations didn't respect default
filtering when it was present (i.e. when the default manager on the related
model implemented a custom get_queryset()).

Fixing the issues introduced some backward incompatible changes:

  • The implementation of remove() for ForeignKey related managers changed from a series of Model.save() calls to a single QuerySet.update() call. The change means that pre_save and post_save signals aren't called anymore.
  • The remove() and clear() methods for GenericForeignKey related managers now perform bulk delete so Model.delete() isn't called anymore.
  • The remove() and clear() methods for ManyToManyField related managers perform nested queries when filtering is involved, which may or may not be an issue depending on the database and the data itself.

Refs. #3871, #21174.

Thanks Anssi Kääriäinen and Tim Graham for the reviews.

comment:7 Changed 4 months ago by loic84

Fixed all the missing transactions I could find in related.py: https://github.com/django/django/pull/2489.

comment:8 Changed 3 months ago by Loic Bistuer <loic.bistuer@…>

In bc9be72bdc9bb4dfc7f967ac3856115f0a6166b8:

Fixed transaction handling for a number of operations on related objects.

Thanks Anssi and Aymeric for the reviews. Refs #21174.

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.