Opened 5 years ago

Closed 5 years ago

#16818 closed Bug (fixed)

Regression introduced by r16739 -- `ManyRelatedManager.add()` doesn't commit to database

Reported by: Daniel Swarbrick Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Release blocker Keywords:
Cc: real.human@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Models are as follows (simplified):

class OrgUnit(models.Model):
    name = models.CharField(max_length=64, unique=True)
    description = models.CharField(_('description'), max_length=128)

class UserProfile(models.Model):
    user = models.ForeignKey(User, unique=True)
    favourites = models.ManyToManyField(OrgUnit, blank=True)

Following code does not work as it used to (in a view):

fav_list = request.user.get_profile().favourites
orgunit = OrgUnit.objects.get(pk=13)
fav_list.add(orgunit)

Immediately after the fav_list.add(), fav_list appears to contain the orgunit (cached), but it is not committed to the database (Postgres).

Reverting to r16738 restores the correct behaviour.

Attachments (2)

16818_test.diff (1.6 KB) - added by Preston Holmes 5 years ago.
regression test
16818.diff (4.0 KB) - added by Karen Tracey 5 years ago.
Tentative fix

Download all attachments as: .zip

Change History (22)

comment:1 Changed 5 years ago by Ramiro Morales

What trunk SVN revision are you using? Make you sure it's a very recent one because there were followup commits related to the feature added in such commit.

comment:2 Changed 5 years ago by Daniel Swarbrick

Using latest trunk (r16819), current as of a moment ago.

Interestingly, fav_list.remove(orgunit) will work. It just seems buggy when trying to add objects.

comment:3 Changed 5 years ago by Alex Gaynor

Triage Stage: UnreviewedAccepted

Marking as accepted because this sounds very plausible, however can you please try to come up with a unittest for Django's tests on this, I'm not quite bright enough to extract one from the description. Thanks for taking the time to test against trunk!

comment:4 Changed 5 years ago by Daniel Swarbrick

I don't have time to write a unit test, but I can shed a little more light on the problem. After turning on Postgres query logging, it appears that bulk_create() does the INSERT, but never commits it. For example:

2011-09-18 20:22:29 CEST LOG:  statement: BEGIN
2011-09-18 20:22:29 CEST LOG:  statement: SELECT "provisioning_device_sip"."sip_id" FROM "provisioning_device_sip" WHERE ("provisioning_device_sip"."device_id" = 131  AND "provisioning_device_sip"."sip_id" IN (160, 214, 159))
2011-09-18 20:22:29 CEST LOG:  statement: INSERT INTO "provisioning_device_sip" ("device_id", "sip_id") VALUES (131, 160), (131, 214), (131, 159)
2011-09-18 20:22:29 CEST LOG:  statement: SHOW default_transaction_isolation
2011-09-18 20:22:29 CEST LOG:  statement: SET default_transaction_isolation TO 'read uncommitted'
2011-09-18 20:22:29 CEST LOG:  statement: BEGIN
2011-09-18 20:22:29 CEST LOG:  statement: SELECT "auth_user"."id", "auth_user"."username", "auth_user"."first_name", "auth_user"."last_name", "auth_user"."email", "auth_user"."password", "auth_user"."is_staff", "auth_user"."is_active", "auth_user"."is_superuser", "auth_user"."last_login", "auth_user"."date_joined" FROM "auth_user" WHERE "auth_user"."id" = 1 

The second SELECT is after a form.save() and HTTP redirect. Note that there is never an explicit commit of the bulk insert.

On a whim, I monkey-patched a "transaction.commit(using=self.db)" just before "return objs" in bulk_create() in query.py. Whilst possibly not optimal, the explicit commit fixed the problem.

comment:5 Changed 5 years ago by Daniel Swarbrick

Make that a "transaction.commit_unless_managed(using=self.db)" :)

comment:6 Changed 5 years ago by Daniel Swarbrick

<bump>

Am I seriously the only one seeing this so far?

comment:7 Changed 5 years ago by Julien Phalip

Severity: NormalRelease blocker

Marking as blocker to be sure this potential regression doesn't fall through the cracks. Note that providing a failing testcase will greatly increase the speed of this getting fixed (that is, if a bug is actually evidenced).

Last edited 5 years ago by Julien Phalip (previous) (diff)

comment:8 Changed 5 years ago by Daniel Swarbrick

Can do, although I'm amazed nobody else has spotted this so far. Any m2m ModelForm will exhibit the bug - although only the very observant will notice something amiss, as it fails silently.

Will try to add a test case this afternoon.

EDIT: Correction - m2m ModelForms don't exhibit the bug, as I suspect they are internally calling the save() method on the parent object. However, manipulating a list of m2m children manually will exhibit the bug.

Last edited 5 years ago by Daniel Swarbrick (previous) (diff)

comment:9 Changed 5 years ago by Daniel Swarbrick

I think I may have found the cause of this bug. When using a vanilla django checkout, without my explicit transaction commit mentioned above, the following will not work:

parent_obj.m2m_children.add(new_child_obj)

Inspecting parent_obj.m2m_children.all() shows the new_child_obj (cached), but if the cache is invalidate (eg. start new django shell, or new request) the new_child_obj is absent from parent_obj.m2m_children.all().

However, if the parent_obj is explicitly saved after adding the new_child_obj, eg.

parent_obj.m2m_children.add(new_child_obj)
parent_obj.save()

then the new_child_obj is committed to the DB, and present in subsequent requests.

This is new behaviour - the explicit parent_obj.save() was not required r16739. I'm not sure if this still qualifies as a bug, or whether it is in fact a fix for a long-standing, incorrect behaviour.

Version 0, edited 5 years ago by Daniel Swarbrick (next)

comment:10 Changed 5 years ago by Daniel Swarbrick

Furthermore, an explicit parent_obj.save() is not required when doing

parent_obj.m2m_children.remove(child_obj)

The child_obj is removed from the m2m_children table immediately.

This smells to me kinda like inconsistent behaviour, that should either be remedied, or very clearly documented.

comment:11 Changed 5 years ago by Ramiro Morales

#17112 was a duplicate and has some additional notes. Among them this one:

I'm not sure how to create an automated test for this, because the problem only exhibits when reading from the database from a different process or transaction.

comment:12 Changed 5 years ago by Ramiro Morales

Summary: Regression introduced by r16739Regression introduced by r16739 -- `ManyRelatedManager.add()` doesn't commit to database

Changed 5 years ago by Preston Holmes

Attachment: 16818_test.diff added

regression test

comment:13 Changed 5 years ago by Tai Lee

Cc: real.human@… added

comment:14 Changed 5 years ago by Karen Tracey

Owner: changed from nobody to Karen Tracey

comment:15 Changed 5 years ago by Karen Tracey

I am confused by this test case:

class TestManyToManyAddTransaction(TransactionTestCase):
    def test_manyrelated_add_commit(self):
        """test for 16818"""
        a = M2mA.objects.create()
        b = M2mB.objects.create(fld=10)
        a.others.add(b)
        transaction.rollback()
        self.assertQuerysetEqual(a.others.all(),['<M2mB: M2mB object>'])

The test explicitly rolls back what it's done and then tries to check that what it did wasn't rolled back?

comment:16 Changed 5 years ago by anonymous

The .add() issues a BEGIN but no COMMIT. The ROLLBACK will undo the uncommitted bulk insert, confirming the bug (missing COMMIT). When fixed, the bulk insert will already be committed and the ROLLBACK won't undo it. That's my understanding, anyway :) This type of test was suggested to ptone by Alex Gaynor on IRC.

comment:17 Changed 5 years ago by Tai Lee

The comment above was from me. Forgot to log in.

comment:18 Changed 5 years ago by Karen Tracey

Oh I see, it's assuming autocommit behavior should be in effect, which I guess it should since it's a TransactionTestCase and it's done nothing to change from the default autocommit behavior.

Changed 5 years ago by Karen Tracey

Attachment: 16818.diff added

Tentative fix

comment:19 Changed 5 years ago by Karen Tracey

Has patch: set
Owner: changed from Karen Tracey to nobody

Attached patch makes the test run successfully on the 3 backends I have available to test where it was failing before (sqlite3, MySQL/InnoDB, PostgreSQL). Possibly it would be a good idea to factor out that force-managed stuff into a reusable bit that could be shared by different operations that require it...in a brief scan I see it in the insert (where I stole it from) and now bulk_insert and django/db/models/deletion.py has something very similar. But first I'd like some confirmation from someone with more ORM knowledge than me that that's the right thing to do to correct the error here.

comment:20 Changed 5 years ago by Adrian Holovaty

Resolution: fixed
Status: newclosed

In [17189]:

Fixed #16818 -- Fixed ORM bug with many-to-many add() method where it wasn't committing the change. Thanks, pressureman and kmtracey

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