Opened 13 years ago

Closed 12 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: dev
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 12 years ago.
regression test
16818.diff (4.0 KB ) - added by Karen Tracey 12 years ago.
Tentative fix

Download all attachments as: .zip

Change History (22)

comment:1 by Ramiro Morales, 13 years ago

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 by Daniel Swarbrick, 13 years ago

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 by Alex Gaynor, 13 years ago

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 by Daniel Swarbrick, 13 years ago

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 by Daniel Swarbrick, 13 years ago

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

comment:6 by Daniel Swarbrick, 12 years ago

<bump>

Am I seriously the only one seeing this so far?

comment:7 by Julien Phalip, 12 years ago

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 12 years ago by Julien Phalip (previous) (diff)

comment:8 by Daniel Swarbrick, 12 years ago

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 12 years ago by Daniel Swarbrick (previous) (diff)

comment:9 by Daniel Swarbrick, 12 years ago

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 invalidated (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 is present in subsequent requests.

This is new behaviour - the explicit parent_obj.save() was not required before 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.

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

comment:10 by Daniel Swarbrick, 12 years ago

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 by Ramiro Morales, 12 years ago

#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 by Ramiro Morales, 12 years ago

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

by Preston Holmes, 12 years ago

Attachment: 16818_test.diff added

regression test

comment:13 by Tai Lee, 12 years ago

Cc: real.human@… added

comment:14 by Karen Tracey, 12 years ago

Owner: changed from nobody to Karen Tracey

comment:15 by Karen Tracey, 12 years ago

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 by anonymous, 12 years ago

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 by Tai Lee, 12 years ago

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

comment:18 by Karen Tracey, 12 years ago

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.

by Karen Tracey, 12 years ago

Attachment: 16818.diff added

Tentative fix

comment:19 by Karen Tracey, 12 years ago

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 by Adrian Holovaty, 12 years ago

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