Merge lp:~wgrant/launchpad/webhook-deletion into lp:launchpad

[William Grant]

Allow Webhooks to be deleted through the API, and prune WebhookJobs older than a month.

Allow Webhooks to be deleted through the API, and prune WebhookJobs older than a month.

[Colin Watson (cjwatson)]

review: Approve

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2015-07-21 10:51:36 +0000
+++ database/schema/security.cfg	2015-07-22 07:10:43 +0000
@@ -2301,6 +2301,7 @@
 public.teamparticipation                = SELECT, DELETE
 public.translationmessage               = SELECT, DELETE
 public.translationtemplateitem          = SELECT, DELETE
+public.webhookjob                       = SELECT, DELETE
 type=user
 
 [garbo_daily]

=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py	2015-07-15 06:25:36 +0000
+++ lib/lp/code/model/gitrepository.py	2015-07-22 07:10:43 +0000
@@ -146,6 +146,7 @@
     get_property_cache,
     )
 from lp.services.webapp.authorization import available_with_permission
+from lp.services.webhooks.interfaces import IWebhookSource
 from lp.services.webhooks.model import WebhookTargetMixin
 
 
@@ -1075,6 +1076,7 @@
         self._deleteRepositoryAccessGrants()
         self._deleteRepositorySubscriptions()
         self._deleteJobs()
+        getUtility(IWebhookSource).delete(self.webhooks)
 
         # Now destroy the repository.
         repository_name = self.unique_name

=== modified file 'lib/lp/code/model/tests/test_gitrepository.py'
--- lib/lp/code/model/tests/test_gitrepository.py	2015-07-10 22:24:49 +0000
+++ lib/lp/code/model/tests/test_gitrepository.py	2015-07-22 07:10:43 +0000
@@ -16,6 +16,7 @@
 from lazr.lifecycle.snapshot import Snapshot
 import pytz
 from sqlobject import SQLObjectNotFound
+from storm.exceptions import LostObjectError
 from storm.store import Store
 from testtools.matchers import (
     EndsWith,
@@ -321,7 +322,7 @@
 class TestGitRepositoryDeletion(TestCaseWithFactory):
     """Test the different cases that make a repository deletable or not."""
 
-    layer = LaunchpadZopelessLayer
+    layer = LaunchpadFunctionalLayer
 
     def setUp(self):
         super(TestGitRepositoryDeletion, self).setUp()
@@ -414,10 +415,9 @@
 
     def test_related_GitJobs_deleted(self):
         # A repository with an associated job will delete those jobs.
-        repository = self.factory.makeGitRepository()
-        GitAPI(None, None).notify(repository.getInternalPath())
-        store = Store.of(repository)
-        repository.destroySelf()
+        GitAPI(None, None).notify(self.repository.getInternalPath())
+        store = Store.of(self.repository)
+        self.repository.destroySelf()
         # Need to commit the transaction to fire off the constraint checks.
         transaction.commit()
         jobs = store.find(GitJob, GitJob.job_type == GitJobType.REF_SCAN)
@@ -426,10 +426,9 @@
     def test_creates_job_to_reclaim_space(self):
         # When a repository is deleted from the database, a job is created
         # to remove the repository from disk as well.
-        repository = self.factory.makeGitRepository()
-        repository_path = repository.getInternalPath()
-        store = Store.of(repository)
-        repository.destroySelf()
+        repository_path = self.repository.getInternalPath()
+        store = Store.of(self.repository)
+        self.repository.destroySelf()
         jobs = store.find(
             GitJob,
             GitJob.job_type == GitJobType.RECLAIM_REPOSITORY_SPACE)
@@ -468,6 +467,13 @@
         )
         self.repository.destroySelf(break_references=True)
 
+    def test_related_webhooks_deleted(self):
+        webhook = self.factory.makeWebhook(target=self.repository)
+        webhook.ping()
+        self.repository.destroySelf()
+        transaction.commit()
+        self.assertRaises(LostObjectError, getattr, webhook, 'target')
+
 
 class TestGitRepositoryDeletionConsequences(TestCaseWithFactory):
     """Test determination and application of repository deletion

=== modified file 'lib/lp/scripts/garbo.py'
--- lib/lp/scripts/garbo.py	2015-07-21 08:42:30 +0000
+++ lib/lp/scripts/garbo.py	2015-07-22 07:10:43 +0000
@@ -119,6 +119,8 @@
     )
 from lp.services.session.model import SessionData
 from lp.services.verification.model.logintoken import LoginToken
+from lp.services.webhooks.interfaces import IWebhookJobSource
+from lp.services.webhooks.model import WebhookJob
 from lp.soyuz.model.archive import Archive
 from lp.soyuz.model.livefsbuild import LiveFSFile
 from lp.soyuz.model.publishing import SourcePackagePublishingHistory
@@ -1112,6 +1114,28 @@
         """
 
 
+class WebhookJobPruner(TunableLoop):
+    """Prune `WebhookJobs` that finished more than a month ago."""
+
+    maximum_chunk_size = 5000
+
+    @property
+    def old_jobs(self):
+        return IMasterStore(WebhookJob).using(WebhookJob, Job).find(
+            (WebhookJob.job_id,),
+            Job.id == WebhookJob.job_id,
+            Job.date_finished < SQL(
+                "CURRENT_TIMESTAMP AT TIME ZONE 'UTC' - '30 days'::interval"))
+
+    def __call__(self, chunksize):
+        getUtility(IWebhookJobSource).deleteByIDs(
+            list(self.old_jobs[:int(chunksize)].values(WebhookJob.job_id)))
+        transaction.commit()
+
+    def isDone(self):
+        return self.old_jobs.is_empty()
+
+
 class BugHeatUpdater(TunableLoop):
     """A `TunableLoop` for bug heat calculations."""
 
@@ -1663,12 +1687,12 @@
     """
     script_name = 'garbo-frequently'
     tunable_loops = [
+        AntiqueSessionPruner,
         BugSummaryJournalRollup,
+        OpenIDConsumerAssociationPruner,
         OpenIDConsumerNoncePruner,
-        OpenIDConsumerAssociationPruner,
-        AntiqueSessionPruner,
+        PopulateLatestPersonSourcePackageReleaseCache,
         VoucherRedeemer,
-        PopulateLatestPersonSourcePackageReleaseCache,
         ]
     experimental_tunable_loops = []
 
@@ -1685,11 +1709,11 @@
     """
     script_name = 'garbo-hourly'
     tunable_loops = [
+        BugHeatUpdater,
+        BugWatchScheduler,
+        DuplicateSessionPruner,
         RevisionCachePruner,
-        BugWatchScheduler,
         UnusedSessionPruner,
-        DuplicateSessionPruner,
-        BugHeatUpdater,
         ]
     experimental_tunable_loops = []
 
@@ -1714,6 +1738,7 @@
         BugWatchActivityPruner,
         CodeImportEventPruner,
         CodeImportResultPruner,
+        DiffPruner,
         GitJobPruner,
         HWSubmissionEmailLinker,
         LiveFSFilePruner,
@@ -1721,17 +1746,17 @@
         ObsoleteBugAttachmentPruner,
         OldTimeLimitedTokenDeleter,
         PersonSettingsENFPopulator,
+        POTranslationPruner,
+        PreviewDiffPruner,
+        ProductVCSPopulator,
         RevisionAuthorEmailLinker,
         ScrubPOFileTranslator,
         SuggestiveTemplatesCacheUpdater,
         TeamMembershipPruner,
-        POTranslationPruner,
         UnlinkedAccountPruner,
         UnusedAccessPolicyPruner,
         UnusedPOTMsgSetPruner,
-        ProductVCSPopulator,
-        PreviewDiffPruner,
-        DiffPruner,
+        WebhookJobPruner,
         ]
     experimental_tunable_loops = [
         PersonPruner,

=== modified file 'lib/lp/scripts/tests/test_garbo.py'
--- lib/lp/scripts/tests/test_garbo.py	2015-07-21 08:42:30 +0000
+++ lib/lp/scripts/tests/test_garbo.py	2015-07-22 07:10:43 +0000
@@ -16,7 +16,10 @@
 import time
 
 from pytz import UTC
-from storm.exceptions import NoneError
+from storm.exceptions import (
+    LostObjectError,
+    NoneError,
+    )
 from storm.expr import (
     In,
     Like,
@@ -979,6 +982,22 @@
         switch_dbuser('testadmin')
         self.assertEqual(1, store.find(GitJob).count())
 
+    def test_WebhookJobPruner(self):
+        # Garbo should remove jobs completed over 30 days ago.
+        switch_dbuser('testadmin')
+
+        webhook = self.factory.makeWebhook()
+        job1 = webhook.ping()
+        removeSecurityProxy(job1).job.date_finished = THIRTY_DAYS_AGO
+        job2 = webhook.ping()
+        removeSecurityProxy(job2).job.date_finished = SEVEN_DAYS_AGO
+
+        self.runDaily()
+
+        switch_dbuser('testadmin')
+        self.assertEqual(webhook, job2.webhook)
+        self.assertRaises(LostObjectError, getattr, job1, 'webhook')
+
     def test_ObsoleteBugAttachmentPruner(self):
         # Bug attachments without a LibraryFileContent record are removed.
 

=== modified file 'lib/lp/services/webhooks/configure.zcml'
--- lib/lp/services/webhooks/configure.zcml	2015-07-13 09:13:57 +0000
+++ lib/lp/services/webhooks/configure.zcml	2015-07-22 07:10:43 +0000
@@ -23,6 +23,11 @@
     </securedutility>
 
     <securedutility
+        component="lp.services.webhooks.model.WebhookJob"
+        provides="lp.services.webhooks.interfaces.IWebhookJobSource">
+        <allow interface="lp.services.webhooks.interfaces.IWebhookJobSource"/>
+    </securedutility>
+    <securedutility
         component="lp.services.webhooks.model.WebhookDeliveryJob"
         provides="lp.services.webhooks.interfaces.IWebhookDeliveryJobSource">
         <allow interface="lp.services.webhooks.interfaces.IWebhookDeliveryJobSource"/>

=== modified file 'lib/lp/services/webhooks/interfaces.py'
--- lib/lp/services/webhooks/interfaces.py	2015-07-07 07:37:54 +0000
+++ lib/lp/services/webhooks/interfaces.py	2015-07-22 07:10:43 +0000
@@ -11,6 +11,7 @@
     'IWebhookDeliveryJob',
     'IWebhookDeliveryJobSource',
     'IWebhookJob',
+    'IWebhookJobSource',
     'IWebhookSource',
     'IWebhookTarget',
     'WebhookFeatureDisabled',
@@ -23,6 +24,7 @@
     call_with,
     error_status,
     export_as_webservice_entry,
+    export_destructor_operation,
     export_factory_operation,
     exported,
     operation_for_version,
@@ -112,6 +114,11 @@
     def ping():
         """Send a test event."""
 
+    @export_destructor_operation()
+    @operation_for_version('devel')
+    def destroySelf():
+        """Delete this webhook."""
+
 
 class IWebhookSource(Interface):
 
@@ -159,6 +166,15 @@
     json_data = Attribute(_("A dict of data about the job."))
 
 
+class IWebhookJobSource(IJobSource):
+
+    def deleteByIDs(webhookjob_ids):
+        """Delete `IWebhookJob`s by their primary key (`Job.id`)."""
+
+    def deleteByWebhooks(webhooks):
+        """Delete all `IWebhookJob`s for the given `IWebhook`."""
+
+
 class IWebhookDeliveryJob(IRunnableJob):
     """A Job that delivers an event to a webhook consumer."""
 

=== modified file 'lib/lp/services/webhooks/model.py'
--- lib/lp/services/webhooks/model.py	2015-07-13 09:13:57 +0000
+++ lib/lp/services/webhooks/model.py	2015-07-22 07:10:43 +0000
@@ -58,6 +58,7 @@
     IWebhookDeliveryJob,
     IWebhookDeliveryJobSource,
     IWebhookJob,
+    IWebhookJobSource,
     IWebhookSource,
     WebhookFeatureDisabled,
     )
@@ -122,6 +123,9 @@
     def ping(self):
         return WebhookDeliveryJob.create(self, {'ping': True})
 
+    def destroySelf(self):
+        getUtility(IWebhookSource).delete([self])
+
     @property
     def event_types(self):
         return (self.json_data or {}).get('event_types', [])
@@ -157,6 +161,8 @@
         return hook
 
     def delete(self, hooks):
+        hooks = list(hooks)
+        getUtility(IWebhookJobSource).deleteByWebhooks(hooks)
         IStore(Webhook).find(
             Webhook, Webhook.id.is_in(set(hook.id for hook in hooks))).remove()
 
@@ -200,6 +206,7 @@
         """)
 
 
+@provider(IWebhookJobSource)
 @implementer(IWebhookJob)
 class WebhookJob(StormBase):
     """See `IWebhookJob`."""
@@ -236,6 +243,24 @@
     def makeDerived(self):
         return WebhookJobDerived.makeSubclass(self)
 
+    @staticmethod
+    def deleteByIDs(webhookjob_ids):
+        """See `IWebhookJobSource`."""
+        # Assumes that Webhook's PK is its FK to Job.id.
+        webookjob_ids = list(webhookjob_ids)
+        IStore(WebhookJob).find(
+            WebhookJob, WebhookJob.job_id.is_in(webhookjob_ids)).remove()
+        IStore(Job).find(Job, Job.id.is_in(webhookjob_ids)).remove()
+
+    @classmethod
+    def deleteByWebhooks(cls, webhooks):
+        """See `IWebhookJobSource`."""
+        result = IStore(WebhookJob).find(
+            WebhookJob,
+            WebhookJob.webhook_id.is_in(hook.id for hook in webhooks))
+        job_ids = list(result.values(WebhookJob.job_id))
+        cls.deleteByIDs(job_ids)
+
 
 @delegate_to(IWebhookJob)
 class WebhookJobDerived(BaseRunnableJob):

=== modified file 'lib/lp/services/webhooks/tests/test_webhook.py'
--- lib/lp/services/webhooks/tests/test_webhook.py	2015-07-12 23:51:16 +0000
+++ lib/lp/services/webhooks/tests/test_webhook.py	2015-07-22 07:10:43 +0000
@@ -3,25 +3,32 @@
 
 from lazr.lifecycle.event import ObjectModifiedEvent
 from storm.store import Store
-from testtools.matchers import GreaterThan
+from testtools.matchers import (
+    Equals,
+    GreaterThan,
+    )
 import transaction
 from zope.component import getUtility
 from zope.event import notify
 from zope.security.checker import getChecker
 
+from lp.services.database.interfaces import IStore
 from lp.services.webapp.authorization import check_permission
 from lp.services.webhooks.interfaces import (
     IWebhook,
     IWebhookSource,
     )
+from lp.services.webhooks.model import WebhookJob
 from lp.testing import (
     admin_logged_in,
     anonymous_logged_in,
     login_person,
     person_logged_in,
+    StormStatementRecorder,
     TestCaseWithFactory,
     )
 from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.matchers import HasQueryCount
 
 
 class TestWebhook(TestCaseWithFactory):
@@ -67,8 +74,9 @@
         expected_get_permissions = {
             'launchpad.View': set((
                 'active', 'date_created', 'date_last_modified', 'deliveries',
-                'delivery_url', 'event_types', 'getDelivery', 'id', 'ping',
-                'registrant', 'registrant_id', 'secret', 'target')),
+                'delivery_url', 'destroySelf', 'event_types', 'getDelivery',
+                'id', 'ping', 'registrant', 'registrant_id', 'secret',
+                'target')),
             }
         webhook = self.factory.makeWebhook()
         checker = getChecker(webhook)
@@ -141,15 +149,25 @@
     def test_delete(self):
         target = self.factory.makeGitRepository()
         login_person(target.owner)
-        hooks = [
-            self.factory.makeWebhook(target, u'http://path/to/%d' % i)
-            for i in range(3)]
+        hooks = []
+        for i in range(3):
+            hook = self.factory.makeWebhook(target, u'http://path/to/%d' % i)
+            hook.ping()
+            hooks.append(hook)
+        self.assertEqual(3, IStore(WebhookJob).find(WebhookJob).count())
         self.assertContentEqual(
             [u'http://path/to/0', u'http://path/to/1', u'http://path/to/2'],
             [hook.delivery_url for hook in
              getUtility(IWebhookSource).findByTarget(target)])
-        getUtility(IWebhookSource).delete(hooks[:2])
+
+        transaction.commit()
+        with StormStatementRecorder() as recorder:
+            getUtility(IWebhookSource).delete(hooks[:2])
+        self.assertThat(recorder, HasQueryCount(Equals(4)))
+
         self.assertContentEqual(
             [u'http://path/to/2'],
             [hook.delivery_url for hook in
              getUtility(IWebhookSource).findByTarget(target)])
+        self.assertEqual(1, IStore(WebhookJob).find(WebhookJob).count())
+        self.assertEqual(1, hooks[2].deliveries.count())

=== modified file 'lib/lp/services/webhooks/tests/test_webhookjob.py'
--- lib/lp/services/webhooks/tests/test_webhookjob.py	2015-07-13 11:20:08 +0000
+++ lib/lp/services/webhooks/tests/test_webhookjob.py	2015-07-22 07:10:43 +0000
@@ -10,6 +10,7 @@
     urlmatch,
     )
 import requests
+from storm.store import Store
 from testtools import TestCase
 from testtools.matchers import (
     Contains,
@@ -22,6 +23,7 @@
     Not,
     )
 import transaction
+from zope.component import getUtility
 
 from lp.services.features.testing import FeatureFixture
 from lp.services.job.interfaces.job import JobStatus
@@ -33,6 +35,7 @@
     IWebhookClient,
     IWebhookDeliveryJob,
     IWebhookJob,
+    IWebhookJobSource,
     )
 from lp.services.webhooks.model import (
     WebhookDeliveryJob,
@@ -40,7 +43,10 @@
     WebhookJobDerived,
     WebhookJobType,
     )
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+    login_person,
+    TestCaseWithFactory,
+    )
 from lp.testing.dbuser import dbuser
 from lp.testing.fixture import (
     CaptureOops,
@@ -65,6 +71,41 @@
             WebhookJob(hook, WebhookJobType.DELIVERY, {}), IWebhookJob)
 
 
+class TestWebhookJobSource(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_deleteByIDs(self):
+        target = self.factory.makeGitRepository()
+        login_person(target.owner)
+        hook = self.factory.makeWebhook(target=target)
+        job1 = hook.ping()
+        job2 = hook.ping()
+        job3 = hook.ping()
+        self.assertContentEqual([job3, job2, job1], hook.deliveries)
+        getUtility(IWebhookJobSource).deleteByIDs([job1.job_id, job3.job_id])
+        self.assertContentEqual([job2], hook.deliveries)
+
+    def test_deleteByWebhooks(self):
+        target = self.factory.makeGitRepository()
+        login_person(target.owner)
+        hook1 = self.factory.makeWebhook(target=target)
+        job1 = hook1.ping()
+        job2 = hook1.ping()
+        hook2 = self.factory.makeWebhook(target=target)
+        job3 = hook2.ping()
+        hook3 = self.factory.makeWebhook(target=target)
+        job4 = hook3.ping()
+        store = Store.of(hook1)
+        self.assertEqual(4, store.find(WebhookJob).count())
+        self.assertContentEqual([job2, job1], hook1.deliveries)
+        self.assertContentEqual([job3], hook2.deliveries)
+        self.assertContentEqual([job4], hook3.deliveries)
+        getUtility(IWebhookJobSource).deleteByWebhooks([hook1, hook2])
+        self.assertEqual(1, store.find(WebhookJob).count())
+        self.assertContentEqual([job4], hook3.deliveries)
+
+
 class TestWebhookJobDerived(TestCaseWithFactory):
     """Tests for `WebhookJobDerived`."""
 

=== modified file 'lib/lp/services/webhooks/tests/test_webservice.py'
--- lib/lp/services/webhooks/tests/test_webservice.py	2015-07-07 07:37:54 +0000
+++ lib/lp/services/webhooks/tests/test_webservice.py	2015-07-22 07:10:43 +0000
@@ -126,6 +126,16 @@
             get_deliveries, create_delivery, 2)
         self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
 
+    def test_delete(self):
+        with person_logged_in(self.owner):
+            self.webhook.ping()
+        delete_response = self.webservice.delete(
+            self.webhook_url, api_version='devel')
+        self.assertEqual(200, delete_response.status)
+        get_response = self.webservice.get(
+            self.webhook_url, api_version='devel')
+        self.assertEqual(404, get_response.status)
+
 
 class TestWebhookDelivery(TestCaseWithFactory):
     layer = DatabaseFunctionalLayer