launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #08461
[Merge] lp:~wgrant/launchpad/bug-apa-in-app into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/bug-apa-in-app into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-apa-in-app/+merge/108344
This branch implements maintenance of the AccessArtifact and AccessPolicyArtifact rows corresponding to Bugs and BugTasks in the application. They're currently maintained by the mirror_legacy_access database triggers.
The core method (Bug.updateAccessPolicArtifacts) is fairly self-explanatory, and the hook points are simple and few: on information type change, on task creation, on task deletion, and on task target change. The tests are far longer. I added lp.testing.dbtriggers with helpers to disable the triggers to confirm the application code works. Quite some simplification can occur once the triggers are gone.
Both the application and the database functions cope with changes made by each other, so they can coexist without trouble.
--
https://code.launchpad.net/~wgrant/launchpad/bug-apa-in-app/+merge/108344
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-apa-in-app into lp:launchpad.
=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py 2012-05-25 19:35:47 +0000
+++ lib/lp/bugs/model/bug.py 2012-06-01 13:53:25 +0000
@@ -159,9 +159,15 @@
from lp.hardwaredb.interfaces.hwdb import IHWSubmissionBugSet
from lp.registry.enums import (
InformationType,
+ PUBLIC_INFORMATION_TYPES,
PRIVATE_INFORMATION_TYPES,
SECURITY_INFORMATION_TYPES,
)
+from lp.registry.interfaces.accesspolicy import (
+ IAccessArtifactSource,
+ IAccessPolicySource,
+ IAccessPolicyArtifactSource,
+ )
from lp.registry.interfaces.distribution import IDistribution
from lp.registry.interfaces.distroseries import IDistroSeries
from lp.registry.interfaces.person import (
@@ -1780,6 +1786,7 @@
self.subscribe(s, who)
self.information_type = information_type
+ self.updateAccessPolicyArtifacts()
self.updateHeat()
return True
@@ -2159,6 +2166,28 @@
self.heat_last_updated = UTC_NOW
store.flush()
+ def updateAccessPolicyArtifacts(self):
+ if self.information_type in PUBLIC_INFORMATION_TYPES:
+ # If it's public we can delete all the access information.
+ # IAccessArtifactSource handles the cascade.
+ getUtility(IAccessArtifactSource).delete([self])
+ return
+ [artifact] = getUtility(IAccessArtifactSource).ensure([self])
+
+ # Now determine the existing and desired links, and make them
+ # match.
+ apasource = getUtility(IAccessPolicyArtifactSource)
+ wanted_links = set(
+ (artifact, policy) for policy in
+ getUtility(IAccessPolicySource).find(
+ (pillar, self.information_type)
+ for pillar in self.affected_pillars))
+ existing_links = set([
+ (apa.abstract_artifact, apa.policy)
+ for apa in apasource.findByArtifact([artifact])])
+ apasource.create(wanted_links - existing_links)
+ apasource.delete(existing_links - wanted_links)
+
def _attachments_query(self):
"""Helper for the attachments* properties."""
# bug attachments with no LibraryFileContent have been deleted - the
@@ -2729,6 +2758,8 @@
if params.milestone:
bug_task.transitionToMilestone(params.milestone, params.owner)
+ bug.updateAccessPolicyArtifacts()
+
# Tell everyone.
if notify_event:
notify(event)
=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py 2012-05-15 05:20:34 +0000
+++ lib/lp/bugs/model/bugtask.py 2012-06-01 13:53:25 +0000
@@ -639,6 +639,7 @@
notify(ObjectDeletedEvent(self, who))
self.destroySelf()
del get_property_cache(bug).bugtasks
+ self.bug.updateAccessPolicyArtifacts()
# When a task is deleted, we also delete it's BugNomination entry
# if there is one. Sadly, getNominationFor() can return None or
@@ -1174,6 +1175,7 @@
for name, value in new_key.iteritems():
setattr(self, name, value)
self.updateTargetNameCache()
+ self.bug.updateAccessPolicyArtifacts()
# START TEMPORARY BIT FOR BUGTASK AUTOCONFIRM FEATURE FLAG.
# We also should see if we ought to auto-transition to the
@@ -1646,6 +1648,7 @@
bugtask.updateTargetNameCache()
if bugtask.conjoined_slave:
bugtask._syncFromConjoinedSlave()
+ removeSecurityProxy(bug).updateAccessPolicyArtifacts()
return tasks
def createTask(self, bug, owner, target, status=None, importance=None,
=== modified file 'lib/lp/bugs/model/tests/test_bug.py'
--- lib/lp/bugs/model/tests/test_bug.py 2012-05-31 03:54:13 +0000
+++ lib/lp/bugs/model/tests/test_bug.py 2012-06-01 13:53:25 +0000
@@ -23,16 +23,25 @@
)
from lp.bugs.errors import BugCannotBePrivate
from lp.bugs.interfaces.bugnotification import IBugNotificationSet
-from lp.bugs.interfaces.bugtask import BugTaskStatus
+from lp.bugs.interfaces.bugtask import (
+ BugTaskStatus,
+ IBugTaskSet,
+ )
from lp.bugs.mail.bugnotificationrecipients import BugNotificationRecipients
from lp.bugs.model.bug import (
BugNotification,
BugSubscriptionInfo,
)
from lp.bugs.model.bugnotification import BugNotificationRecipient
+from lp.registry.interfaces.accesspolicy import (
+ IAccessArtifactSource,
+ IAccessPolicySource,
+ IAccessPolicyArtifactSource,
+ )
from lp.registry.enums import InformationType
from lp.registry.interfaces.person import PersonVisibility
from lp.testing import (
+ admin_logged_in,
feature_flags,
login_person,
person_logged_in,
@@ -41,6 +50,11 @@
StormStatementRecorder,
TestCaseWithFactory,
)
+from lp.testing.dbtriggers import (
+ disable_trigger,
+ triggers_disabled,
+ )
+from lp.testing.dbuser import dbuser
from lp.testing.layers import DatabaseFunctionalLayer
from lp.testing.matchers import (
Equals,
@@ -49,6 +63,20 @@
)
+def get_policies_for_bug(bug):
+ [artifact] = getUtility(IAccessArtifactSource).find([bug])
+ return [
+ apa.policy for apa in
+ getUtility(IAccessPolicyArtifactSource).findByArtifact([artifact])]
+
+
+LEGACY_ACCESS_TRIGGERS = [
+ ('bug', 'bug_mirror_legacy_access_t'),
+ ('bugtask', 'bugtask_mirror_legacy_access_t'),
+ ('bugsubscription', 'bugsubscription_mirror_legacy_access_t'),
+ ]
+
+
class TestBug(TestCaseWithFactory):
layer = DatabaseFunctionalLayer
@@ -812,6 +840,24 @@
)
[self.assertEqual(m[1], m[0].information_type) for m in mapping]
+ def test_accesspolicyartifacts_updated(self):
+ # transitionToTarget updates the AccessPolicyArtifacts related
+ # to the bug.
+ bug = self.factory.makeBug(
+ information_type=InformationType.EMBARGOEDSECURITY)
+
+ # There are also transitional triggers that do this. Disable
+ # them temporarily so we can be sure the application side works.
+ with triggers_disabled(LEGACY_ACCESS_TRIGGERS):
+ with admin_logged_in():
+ product = bug.default_bugtask.product
+ bug.transitionToInformationType(
+ InformationType.USERDATA, bug.owner)
+
+ [expected_policy] = getUtility(IAccessPolicySource).find(
+ [(product, InformationType.USERDATA)])
+ self.assertContentEqual([expected_policy], get_policies_for_bug(bug))
+
def test_private_to_public_information_type(self):
# A private bug transitioning to public has the correct information
# type.
@@ -1057,3 +1103,105 @@
duplicate_bug = self.factory.makeBug(owner=bug.owner)
duplicate_bug.markAsDuplicate(bug)
self.assertEqual(BugTaskStatus.NEW, bug.bugtasks[0].status)
+
+
+class TestBugUpdateAccessPolicyArtifacts(TestCaseWithFactory):
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestBugUpdateAccessPolicyArtifacts, self).setUp()
+
+ # Disable the transitional triggers that the app code is to
+ # replace.
+ with dbuser('postgres'):
+ for table, trigger in LEGACY_ACCESS_TRIGGERS:
+ disable_trigger(table, trigger)
+
+ def assertPoliciesForBug(self, policy_tuples, bug):
+ self.assertContentEqual(
+ getUtility(IAccessPolicySource).find(policy_tuples),
+ get_policies_for_bug(bug))
+
+ def test_creates_missing_accessartifact(self):
+ # updateAccessPolicyArtifacts creates an AccessArtifact for a
+ # private bug if there isn't one already.
+ bug = self.factory.makeBug(information_type=InformationType.USERDATA)
+ getUtility(IAccessArtifactSource).delete([bug])
+
+ self.assertTrue(
+ getUtility(IAccessArtifactSource).find([bug]).is_empty())
+ removeSecurityProxy(bug).updateAccessPolicyArtifacts()
+ self.assertFalse(
+ getUtility(IAccessArtifactSource).find([bug]).is_empty())
+
+ def test_removes_extra_accessartifact(self):
+ # updateAccessPolicyArtifacts creates an AccessArtifact for a
+ # private bug if there isn't one already.
+ bug = self.factory.makeBug(information_type=InformationType.PUBLIC)
+ getUtility(IAccessArtifactSource).ensure([bug])
+
+ self.assertFalse(
+ getUtility(IAccessArtifactSource).find([bug]).is_empty())
+ removeSecurityProxy(bug).updateAccessPolicyArtifacts()
+ self.assertTrue(
+ getUtility(IAccessArtifactSource).find([bug]).is_empty())
+
+ def test_adds_missing_accesspolicyartifacts(self):
+ # updateAccessPolicyArtifacts adds missing links.
+ product = self.factory.makeProduct()
+ bug = self.factory.makeBug(
+ product=product, information_type=InformationType.USERDATA)
+ [artifact] = getUtility(IAccessArtifactSource).find([bug])
+ getUtility(IAccessPolicyArtifactSource).deleteByArtifact([artifact])
+
+ self.assertPoliciesForBug([], bug)
+ removeSecurityProxy(bug).updateAccessPolicyArtifacts()
+ self.assertPoliciesForBug([(product, InformationType.USERDATA)], bug)
+
+ def test_removes_extra_accesspolicyartifacts(self):
+ # updateAccessPolicyArtifacts removes excess links.
+ product = self.factory.makeProduct()
+ bug = self.factory.makeBug(
+ product=product, information_type=InformationType.USERDATA)
+
+ other_product = self.factory.makeProduct()
+ [other_policy] = getUtility(IAccessPolicySource).find(
+ [(other_product, InformationType.USERDATA)])
+ [artifact] = getUtility(IAccessArtifactSource).find([bug])
+ getUtility(IAccessPolicyArtifactSource).create(
+ [(artifact, other_policy)])
+
+ self.assertPoliciesForBug(
+ [(product, InformationType.USERDATA),
+ (other_product, InformationType.USERDATA)],
+ bug)
+ removeSecurityProxy(bug).updateAccessPolicyArtifacts()
+ self.assertPoliciesForBug([(product, InformationType.USERDATA)], bug)
+
+ def test_all_target_types_work(self):
+ # updateAccessPolicyArtifacts gets the pillar from any task
+ # type.
+ product = self.factory.makeProduct()
+ productseries = self.factory.makeProductSeries()
+ distro = self.factory.makeDistribution()
+ distroseries = self.factory.makeDistroSeries()
+ dsp = self.factory.makeDistributionSourcePackage()
+ sp = self.factory.makeSourcePackage()
+
+ targets = [product, productseries, distro, distroseries, dsp, sp]
+ pillars = [
+ product, productseries.product, distro, distroseries.distribution,
+ dsp.distribution, sp.distribution]
+
+ bug = self.factory.makeBug(
+ product=product, information_type=InformationType.USERDATA)
+ for target in targets[1:]:
+ self.factory.makeBugTask(bug, target=target)
+ [artifact] = getUtility(IAccessArtifactSource).find([bug])
+ getUtility(IAccessPolicyArtifactSource).deleteByArtifact([artifact])
+
+ self.assertPoliciesForBug([], bug)
+ removeSecurityProxy(bug).updateAccessPolicyArtifacts()
+ self.assertPoliciesForBug(
+ [(pillar, InformationType.USERDATA) for pillar in pillars], bug)
=== modified file 'lib/lp/bugs/model/tests/test_bugtask.py'
--- lib/lp/bugs/model/tests/test_bugtask.py 2012-05-24 10:19:59 +0000
+++ lib/lp/bugs/model/tests/test_bugtask.py 2012-06-01 13:53:25 +0000
@@ -59,6 +59,10 @@
_build_status_clause,
_build_tag_search_clause,
)
+from lp.bugs.model.tests.test_bug import (
+ get_policies_for_bug,
+ LEGACY_ACCESS_TRIGGERS,
+ )
from lp.bugs.scripts.bugtasktargetnamecaches import (
BugTaskTargetNameCacheUpdater)
from lp.bugs.tests.bug import create_old_bug
@@ -101,6 +105,7 @@
from lp.services.webapp.interfaces import ILaunchBag
from lp.soyuz.interfaces.archive import ArchivePurpose
from lp.testing import (
+ admin_logged_in,
ANONYMOUS,
EventRecorder,
feature_flags,
@@ -116,6 +121,7 @@
TestCaseWithFactory,
ws_object,
)
+from lp.testing.dbtriggers import triggers_disabled
from lp.testing.dbuser import (
dbuser,
switch_dbuser,
@@ -223,6 +229,27 @@
self.assertEqual(tasks[1][1], a_distro)
self.assertEqual(tasks[2][0], evolution)
+ def test_accesspolicyartifacts_updated(self):
+ # createManyTasks updates the AccessPolicyArtifacts related
+ # to the bug.
+ new_product = self.factory.makeProduct()
+ bug = self.factory.makeBug(
+ information_type=InformationType.USERDATA)
+
+ # There are also transitional triggers that do this. Disable
+ # them temporarily so we can be sure the application side works.
+ with triggers_disabled(LEGACY_ACCESS_TRIGGERS):
+ with admin_logged_in():
+ old_product = bug.default_bugtask.product
+ getUtility(IBugTaskSet).createManyTasks(
+ bug, bug.owner, [new_product])
+
+ expected_policies = getUtility(IAccessPolicySource).find([
+ (new_product, InformationType.USERDATA),
+ (old_product, InformationType.USERDATA),
+ ])
+ self.assertContentEqual(expected_policies, get_policies_for_bug(bug))
+
class TestBugTaskCreationPackageComponent(TestCaseWithFactory):
"""IBugTask contains a convenience method to look up archive component
@@ -2157,6 +2184,34 @@
self.assertEqual([bugtask], bug.bugtasks)
self.assertEqual(bugtask, bug.default_bugtask)
+ def test_accesspolicyartifacts_updated(self):
+ # delete() updates the AccessPolicyArtifacts related
+ # to the bug.
+ new_product = self.factory.makeProduct()
+ bug = self.factory.makeBug(
+ information_type=InformationType.USERDATA)
+ with admin_logged_in():
+ old_product = bug.default_bugtask.product
+ task = getUtility(IBugTaskSet).createTask(
+ bug, bug.owner, new_product)
+
+ expected_policies = getUtility(IAccessPolicySource).find([
+ (old_product, InformationType.USERDATA),
+ (new_product, InformationType.USERDATA),
+ ])
+ self.assertContentEqual(expected_policies, get_policies_for_bug(bug))
+
+ # There are also transitional triggers that do this. Disable
+ # them temporarily so we can be sure the application side works.
+ with triggers_disabled(LEGACY_ACCESS_TRIGGERS):
+ with admin_logged_in():
+ task.delete()
+
+ expected_policies = getUtility(IAccessPolicySource).find([
+ (old_product, InformationType.USERDATA),
+ ])
+ self.assertContentEqual(expected_policies, get_policies_for_bug(bug))
+
class TestStatusCountsForProductSeries(TestCaseWithFactory):
"""Test BugTaskSet.getStatusCountsForProductSeries()."""
@@ -3218,6 +3273,22 @@
new_product.bugtargetdisplayname,
removeSecurityProxy(task).targetnamecache)
+ def test_accesspolicyartifacts_updated(self):
+ # transitionToTarget updates the AccessPolicyArtifacts related
+ # to the bug.
+ new_product = self.factory.makeProduct()
+ bug = self.factory.makeBug(information_type=InformationType.USERDATA)
+
+ # There are also transitional triggers that do this. Disable
+ # them temporarily so we can be sure the application side works.
+ with triggers_disabled(LEGACY_ACCESS_TRIGGERS):
+ with admin_logged_in():
+ bug.default_bugtask.transitionToTarget(new_product)
+
+ [expected_policy] = getUtility(IAccessPolicySource).find(
+ [(new_product, InformationType.USERDATA)])
+ self.assertContentEqual([expected_policy], get_policies_for_bug(bug))
+
def test_matching_sourcepackage_tasks_updated_when_name_changed(self):
# If the sourcepackagename is changed, it's changed on all tasks
# with the same distribution and sourcepackagename.
=== modified file 'lib/lp/bugs/tests/test_bug_mirror_access_triggers.py'
--- lib/lp/bugs/tests/test_bug_mirror_access_triggers.py 2012-05-03 00:08:11 +0000
+++ lib/lp/bugs/tests/test_bug_mirror_access_triggers.py 2012-06-01 13:53:25 +0000
@@ -128,7 +128,7 @@
bug = self.makeBugAndPolicies(private=True)
task = bug.addTask(bug.owner, product)
Store.of(bug).flush()
- removeSecurityProxy(task).destroySelf()
+ removeSecurityProxy(task).delete(who=bug.owner)
self.assertEqual((1, 1), self.assertMirrored(bug))
def test_make_public(self):
=== modified file 'lib/lp/registry/interfaces/accesspolicy.py'
--- lib/lp/registry/interfaces/accesspolicy.py 2012-05-15 08:16:09 +0000
+++ lib/lp/registry/interfaces/accesspolicy.py 2012-06-01 13:53:25 +0000
@@ -159,6 +159,13 @@
pairs.
"""
+ def delete(links):
+ """Delete the specified `IAccessPolicyArtifacts`s.
+
+ :param links: a collection of (`IAccessArtifact`, `IAccessPolicy`)
+ pairs.
+ """
+
def findByArtifact(artifacts):
"""Return all `IAccessPolicyArtifact` objects for the artifacts."""
=== modified file 'lib/lp/registry/model/accesspolicy.py'
--- lib/lp/registry/model/accesspolicy.py 2012-04-26 08:12:23 +0000
+++ lib/lp/registry/model/accesspolicy.py 2012-06-01 13:53:25 +0000
@@ -26,6 +26,7 @@
Int,
)
from storm.references import Reference
+from storm.store import EmptyResultSet
from zope.component import getUtility
from zope.interface import implements
@@ -174,6 +175,9 @@
@classmethod
def find(cls, pillars_and_types):
"""See `IAccessPolicySource`."""
+ pillars_and_types = list(pillars_and_types)
+ if len(pillars_and_types) == 0:
+ return EmptyResultSet()
return IStore(cls).find(
cls,
Or(*(
@@ -222,6 +226,9 @@
@classmethod
def find(cls, links):
"""See `IAccessArtifactGrantSource`."""
+ links = list(links)
+ if len(links) == 0:
+ return EmptyResultSet()
return IStore(cls).find(
cls,
Or(*(
@@ -229,6 +236,10 @@
for (artifact, policy) in links)))
@classmethod
+ def delete(cls, links):
+ cls.find(links).remove()
+
+ @classmethod
def findByArtifact(cls, artifacts):
"""See `IAccessPolicyArtifactSource`."""
ids = [artifact.id for artifact in artifacts]
=== modified file 'lib/lp/registry/tests/test_accesspolicy.py'
--- lib/lp/registry/tests/test_accesspolicy.py 2012-04-26 07:54:34 +0000
+++ lib/lp/registry/tests/test_accesspolicy.py 2012-06-01 13:53:25 +0000
@@ -366,6 +366,15 @@
getUtility(IAccessPolicyArtifactSource).find(
[(link.abstract_artifact, link.policy) for link in links]))
+ def test_delete(self):
+ links = [self.factory.makeAccessPolicyArtifact() for i in range(3)]
+ getUtility(IAccessPolicyArtifactSource).delete([
+ (links[0].abstract_artifact, links[0].policy)])
+ self.assertContentEqual(
+ links[1:],
+ getUtility(IAccessPolicyArtifactSource).find([
+ (link.abstract_artifact, link.policy) for link in links]))
+
def test_findByArtifact(self):
# findByArtifact() finds only the relevant links.
artifact = self.factory.makeAccessArtifact()
=== added file 'lib/lp/testing/dbtriggers.py'
--- lib/lp/testing/dbtriggers.py 1970-01-01 00:00:00 +0000
+++ lib/lp/testing/dbtriggers.py 2012-06-01 13:53:25 +0000
@@ -0,0 +1,55 @@
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Provides helpers for managing database triggers in tests."""
+
+__metaclass__ = type
+
+__all__ = [
+ 'disable_trigger',
+ 'enable_trigger',
+ 'triggers_disabled',
+ ]
+
+from contextlib import contextmanager
+
+from zope.component import getUtility
+
+from lp.services.database.sqlbase import quote_identifier
+from lp.services.webapp.interfaces import (
+ IStoreSelector,
+ MAIN_STORE,
+ MASTER_FLAVOR,
+ )
+from lp.testing.dbuser import dbuser
+
+
+def disable_trigger(table, trigger):
+ store = getUtility(IStoreSelector).get(MAIN_STORE, MASTER_FLAVOR)
+ store.execute(
+ 'ALTER TABLE %s DISABLE TRIGGER %s'
+ % (quote_identifier(table), quote_identifier(trigger)))
+
+
+def enable_trigger(table, trigger):
+ store = getUtility(IStoreSelector).get(MAIN_STORE, MASTER_FLAVOR)
+ store.execute(
+ 'ALTER TABLE %s ENABLE TRIGGER %s'
+ % (quote_identifier(table), quote_identifier(trigger)))
+
+
+@contextmanager
+def triggers_disabled(triggers):
+ """A context manager that temporarily disables selected triggers.
+
+ This commits and starts a new transaction.
+
+ :param triggers: sequence of (table, trigger) tuples to disable.
+ """
+ with dbuser('postgres'):
+ for trigger in triggers:
+ disable_trigger(*trigger)
+ yield
+ with dbuser('postgres'):
+ for trigger in triggers:
+ enable_trigger(*trigger)
Follow ups
