launchpad-reviewers team mailing list archive

[Данило Шеган <danilo@xxxxxxxxxxxxx>]

Данило Шеган has proposed merging lp:~danilo/launchpad/devel-bug-720826-delete-race into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #720826 Add subscription description header for bug notifications
  https://bugs.launchpad.net/bugs/720826

For more details, see:
https://code.launchpad.net/~danilo/launchpad/devel-bug-720826-delete-race/+merge/51890

= Improvements to BugSubscriptionFilter.delete() =

BugSubscriptionFilter.delete() is not complete yet: it's supposed to revert
all the fields to their default values.

There is also a potential for a race-condition which we guard against by
locking all the rows that must not change when we do the check and delete
using SELECT FOR UPDATE.  I have not provided a test-case because that's
likely to be pretty complex.

If we wanted to guard on the database level, we could add a trigger
that stops removal using something like

  https://pastebin.canonical.com/44142/

However, Stuart believes that's too complex for very little benefit.
I agree that what we want to guard against is malicious users, and not
developers who can shoot themselves in the foot (because we've got so
many ways to do it) if they don't use the recommended API for deletion.
So, the trigger is left to the side for now.

== Pre-implementation notes ==

Discussed this with Stuart.  He recommends against the trigger and just guarding the Python code (though, he's partial to that as well). To quote his example:

"If you have a form """( ) bugsubscription a (x) bugsubscription b""", it is reasonable to assume someone clicks 'delete', thinks 'oh shit', changes the check box and clicks submit again. So catching this in Python is reasonable."

== Tests ==

bin/test -cvvt BugSubscriptionFilter.test_delete -t BugSubscriptionFilter.test_has_other_filters

== Demo and Q/A ==

Go to https://bugs.qastaging.launchpad.net/people/+me/+structural-subscriptions and try removing the final filter with some settings on the last filter.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/model/bugsubscriptionfilter.py
  lib/lp/bugs/model/tests/test_bugsubscriptionfilter.py
-- 
https://code.launchpad.net/~danilo/launchpad/devel-bug-720826-delete-race/+merge/51890
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/devel-bug-720826-delete-race into lp:launchpad.

=== modified file 'lib/lp/bugs/model/bugsubscriptionfilter.py'
--- lib/lp/bugs/model/bugsubscriptionfilter.py	2011-03-01 16:47:47 +0000
+++ lib/lp/bugs/model/bugsubscriptionfilter.py	2011-03-02 12:31:48 +0000
@@ -12,12 +12,14 @@
     Bool,
     Int,
     Reference,
+    SQL,
     Store,
     Unicode,
     )
 from zope.interface import implements
 
 from canonical.database.enumcol import DBEnum
+from canonical.database.sqlbase import sqlvalues
 from canonical.launchpad import searchbuilder
 from canonical.launchpad.interfaces.lpstorm import IStore
 from lp.bugs.enum import BugNotificationLevel
@@ -193,6 +195,12 @@
     def _has_other_filters(self):
         """Are there other filters for parent `StructuralSubscription`?"""
         store = Store.of(self)
+        # Avoid race conditions by locking all the rows
+        # that we do our check over.
+        store.execute(SQL(
+            """SELECT * FROM BugSubscriptionFilter
+                 WHERE structuralsubscription=%s
+                 FOR UPDATE""" % sqlvalues(self.structural_subscription_id)))
         return bool(store.find(
             BugSubscriptionFilter,
             (BugSubscriptionFilter.structural_subscription ==
@@ -202,9 +210,14 @@
     def delete(self):
         """See `IBugSubscriptionFilter`."""
         self.importances = self.statuses = self.tags = ()
-        # Revert bug notification level to the default.
-        self.bug_notification_level = (
-            IBugSubscriptionFilter.getDescriptionFor(
-                'bug_notification_level').default)
+        # Revert all attributes to their default values from the inteface.
+        for attribute in ['bug_notification_level', 'find_all_tags',
+                          'include_any_tags', 'exclude_any_tags']:
+            setattr(
+                self, attribute,
+                IBugSubscriptionFilter.getDescriptionFor(
+                    attribute).default)
+        self.description = None
+
         if self._has_other_filters():
             Store.of(self).remove(self)

=== modified file 'lib/lp/bugs/model/tests/test_bugsubscriptionfilter.py'
--- lib/lp/bugs/model/tests/test_bugsubscriptionfilter.py	2011-03-01 15:51:44 +0000
+++ lib/lp/bugs/model/tests/test_bugsubscriptionfilter.py	2011-03-02 12:31:48 +0000
@@ -114,23 +114,36 @@
         # Delete.
         bug_subscription_filter.delete()
         IStore(bug_subscription_filter).flush()
+        # It doesn't exist in the database anymore.
         self.assertIs(None, Store.of(bug_subscription_filter))
 
     def test_delete_final(self):
         # Final remaining `BugSubscriptionFilter` can't be deleted.
-        # Only the linked data is removed.
+        # Only the linked data is removed and/or unset.
         bug_subscription_filter = self.subscription.bug_filters.one()
         bug_subscription_filter.bug_notification_level = (
             BugNotificationLevel.LIFECYCLE)
+        bug_subscription_filter.find_all_tags = True
+        bug_subscription_filter.exclude_any_tags = True
+        bug_subscription_filter.include_any_tags = True
+        bug_subscription_filter.description = u"Description"
         bug_subscription_filter.importances = [BugTaskImportance.LOW]
         bug_subscription_filter.statuses = [BugTaskStatus.NEW]
         bug_subscription_filter.tags = [u"foo"]
         IStore(bug_subscription_filter).flush()
         self.assertIsNot(None, Store.of(bug_subscription_filter))
+
         # Delete.
         bug_subscription_filter.delete()
         IStore(bug_subscription_filter).flush()
+
+        # It is not deleted from the database.
         self.assertIsNot(None, Store.of(bug_subscription_filter))
+        # But all the data is set back to defaults.
+        self.assertIs(None, bug_subscription_filter.description)
+        self.assertFalse(bug_subscription_filter.find_all_tags)
+        self.assertFalse(bug_subscription_filter.include_any_tags)
+        self.assertFalse(bug_subscription_filter.exclude_any_tags)
         self.assertEquals(BugNotificationLevel.COMMENTS,
                           bug_subscription_filter.bug_notification_level)
         self.assertContentEqual([], bug_subscription_filter.statuses)