launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #19031
[Merge] lp:~wgrant/launchpad/webhook-deletion into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/webhook-deletion into lp:launchpad.
Commit message:
Allow Webhooks to be deleted through the API, and prune WebhookJobs older than a month.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/webhook-deletion/+merge/265360
Allow Webhooks to be deleted through the API, and prune WebhookJobs older than a month.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/webhook-deletion into lp:launchpad.
=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg 2015-07-15 06:20:47 +0000
+++ database/schema/security.cfg 2015-07-21 07:05:40 +0000
@@ -2297,6 +2297,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-21 07:05:40 +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-21 07:05:40 +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-13 00:52:36 +0000
+++ lib/lp/scripts/garbo.py 2015-07-21 07:05:40 +0000
@@ -116,6 +116,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
@@ -1109,6 +1111,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."""
@@ -1689,6 +1713,7 @@
CodeImportEventPruner,
CodeImportResultPruner,
GitJobPruner,
+ WebhookJobPruner,
HWSubmissionEmailLinker,
LiveFSFilePruner,
LoginTokenPruner,
=== modified file 'lib/lp/scripts/tests/test_garbo.py'
--- lib/lp/scripts/tests/test_garbo.py 2015-07-13 00:55:56 +0000
+++ lib/lp/scripts/tests/test_garbo.py 2015-07-21 07:05:40 +0000
@@ -16,6 +16,7 @@
import time
from pytz import UTC
+from storm.exceptions import LostObjectError
from storm.expr import (
In,
Like,
@@ -977,6 +978,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-21 07:05:40 +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-21 07:05:40 +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-21 07:05:40 +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-21 07:05:40 +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-21 07:05:40 +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-21 07:05:40 +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
Follow ups
