launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #28004
Re: [Merge] ~lgp171188/launchpad:enforce-stricter-permission-check-when-a-bug-is-locked-down into launchpad:master
Review: Needs Fixing
Thanks for working on this!
Having read this, I think I'd in fact prefer you to revert the sampledata changes from this MP. We need to make sure that we run tests on a database that has some rows with NULL lock_status, in order to avoid transitional mistakes. Sampledata can be updated once garbo has run.
Could you add screenshots of what (a) the ordinary bug log (i.e. what you see if you just load the bug's default web page) and the +activity page for the bug look like for a bug that has been locked? There are some fiddly interactions here, and I'd like to make sure that both of them come out looking reasonable.
Diff comments:
> diff --git a/lib/lp/bugs/adapters/bugchange.py b/lib/lp/bugs/adapters/bugchange.py
> index ba0e99a..6b15f36 100644
> --- a/lib/lp/bugs/adapters/bugchange.py
> +++ b/lib/lp/bugs/adapters/bugchange.py
> @@ -27,6 +27,8 @@ __all__ = [
> 'BugTaskStatusChange',
> 'BugTaskTargetChange',
> 'BugTitleChange',
> + 'BugLocked',
Could you keep this list sorted? 'BugTitleChange' > 'BugLocked'.
(I tried https://pypi.org/project/sort-all/, but it crashed on me. Or maybe we should switch to something like https://pypi.org/project/atpublic/ - the latest version needs a more current version of Python, but 1.0 would work.)
> + 'BugUnlocked',
> 'BugWatchAdded',
> 'BugWatchRemoved',
> 'CHANGED_DUPLICATE_MARKER',
> @@ -735,6 +740,58 @@ class CveUnlinkedFromBug(BugChangeBase):
> return {'text': "** CVE removed: %s" % self.cve.url}
>
>
> +class BugLocked(BugChangeBase):
> + """Used to represent the locking of a bug."""
> +
> + def __init__(self, when, person, new_status=BugLockStatus.COMMENT_ONLY,
> + lock_reason=None, **kwargs):
> + super().__init__(when, person)
> + self.new_status = new_status
> + self.lock_reason = lock_reason
> +
> + def getBugActivity(self):
> + """See `IBugChange'."""
> + activity = dict(
> + whatchanged='bug lock status',
Just 'lock status' - `BugActivity.whatchanged` doesn't normally have a 'bug ' prefix.
> + oldvalue=str(BugLockStatus.UNLOCKED),
Is this necessarily accurate? If we add another lock status later, then a bug might reasonably change from comments-only to completely locked. I'd prefer that we pass the old status into the constructor of this class so that we can use it here.
> + newvalue=str(self.new_status),
> + )
> + if self.lock_reason:
> + activity['message'] = self.lock_reason
> +
> + return activity
> +
> + def getBugNotification(self):
> + """See `IBugChange`."""
> + return {
> + 'text': (
> + "** Bug locked\n"
> + " Reason: {}"
Maybe just "** Bug locked ({})" rather than two lines.
Is it worth being more explicit about (a) the actual lock status (which might be something other than comment-only in future) and (b) roughly who can still edit bug metadata? Perhaps something like "Bug metadata locked ({}) and limited to project staff" if the new status is `COMMENT_ONLY`, and if we add a more stringent lock status later then we can adjust the text for that.
How will this look if somebody changes the lock reason but not the lock status? Probably OK, but check.
> + ).format(self.lock_reason)
> + }
> +
> +class BugUnlocked(BugChangeBase):
> + """Used to represent the unlocking of a bug."""
> +
> + def __init__(self, when, person, old_status, **kwargs):
> + super().__init__(when, person)
> + self.old_status = old_status
> +
> + def getBugActivity(self):
> + """See `IBugChange'."""
> + return dict(
> + whatchanged='bug lock status',
Just 'lock status' - `BugActivity.whatchanged` doesn't normally have a 'bug ' prefix.
> + newvalue=str(BugLockStatus.UNLOCKED),
> + oldvalue=str(self.old_status),
> + )
> +
> + def getBugNotification(self):
> + """See `IBugChange`."""
> + return {
> + 'text': "** Bug unlocked"
> + }
My first instinct would have been to include these two in the `get_bug_changes` system instead, so you'd end up with separate activity rows for `lock_status` and `lock_reason`, partly because using `BugActivity.message` for the lock reason is slightly surprising. However, on reflection, the slightly higher-level approach you've taken here seems OK - just make sure it looks right on the Bug:+activity web page.
> +
> +
> class BugTaskAttributeChange(AttributeChange):
> """Used to represent a change in a BugTask's attributes.
>
> diff --git a/lib/lp/bugs/interfaces/bug.py b/lib/lp/bugs/interfaces/bug.py
> index db94b63..5250211 100644
> --- a/lib/lp/bugs/interfaces/bug.py
> +++ b/lib/lp/bugs/interfaces/bug.py
> @@ -978,9 +996,42 @@ class IBugAppend(Interface):
> Return None if no bugtask was edited.
> """
>
> +class IBugModerate(Interface):
> + """IBug attributes that require the launchpad.Moderate permission."""
> +
> + @operation_parameters(
> + status=copy_field(
> + IBugView['lock_status'],
I think you ought to arrange for the vocabulary for this option to filter out `UNLOCKED` from `BugLockStatus`, otherwise it'll show up in the API documentation with 'Unlocked' as an option and that seems odd. There are a few possible approaches to that kind of filtering - something like `values=[item for item in BugLockStatus.items.items if item != BugLockStatus.UNLOCKED], vocabulary=None` here might work, or you could subclass `BugLockStatus` to exclude `UNLOCKED`.
> + default=BugLockStatus.COMMENT_ONLY
I wonder if we'll feel that this default is weird later if and when we add another stricter lock status. Would it perhaps make sense to have this parameter be required, with no default, even if it'd always currently be 'Comment-only'?
> + ),
> + reason=copy_field(IBugView['lock_reason'])
> + )
> + @export_write_operation()
> + @call_with(who=REQUEST_USER)
> + @operation_for_version("devel")
> + def lock(who, status=BugLockStatus.COMMENT_ONLY, reason=None):
> + """
> + Lock the bug metadata edits to the relevant roles.
> +
> + :param status: The lock status of the bug - one of the values
> + in the BugLockStatus enum.
> + :param reason: The reason for locking this bug.
> + :param who: The IPerson who is making the change.
> + """
> +
> + @export_write_operation()
> + @call_with(who=REQUEST_USER)
> + @operation_for_version("devel")
> + def unlock(who):
> + """
> + Unlock the bug metadata edits to the default roles.
> +
> + :param who: The IPerson who is making the change.
> + """
> +
>
> @exported_as_webservice_entry()
> -class IBug(IBugPublic, IBugView, IBugAppend, IHasLinkedBranches):
> +class IBug(IBugPublic, IBugView, IBugAppend, IBugModerate, IHasLinkedBranches):
> """The core bug entry."""
>
> linked_bugbranches = exported(
> diff --git a/lib/lp/bugs/model/bug.py b/lib/lp/bugs/model/bug.py
> index f7c588c..04197bf 100644
> --- a/lib/lp/bugs/model/bug.py
> +++ b/lib/lp/bugs/model/bug.py
> @@ -2198,6 +2206,38 @@ class Bug(SQLBase, InformationTypeMixin):
> BugActivity.datechanged <= end_date)
> return activity_in_range
>
> + def lock(self, who, status=BugLockStatus.COMMENT_ONLY, reason=None):
> + """See `IBug`."""
> + if status != BugLockStatus.UNLOCKED and status != self.lock_status:
IMO trying to pass `status=BugLockStatus.UNLOCKED` should raise some kind of exception with HTTP status 400, since it doesn't make sense here.
Also, perhaps it should be possible to change the lock reason without changing the lock status?
> + self.lock_status = status
> + if reason:
> + self.lock_reason = reason
> +
> + Store.of(self).flush()
This isn't a problem, but I wonder why it's necessary, and perhaps it could use a comment. Something to do with event handlers that have implicit flushes disabled?
> + self.addChange(
> + BugLocked(
> + when=UTC_NOW,
> + person=who,
> + status=status,
> + lock_reason=reason
> + )
> + )
> +
> + def unlock(self, who):
> + """See `IBug`."""
> + if self.lock_status != BugLockStatus.UNLOCKED:
> + old_lock_status = self.lock_status
> + self.lock_status = BugLockStatus.UNLOCKED
> + self.lock_reason = None
> + Store.of(self).flush()
> + self.addChange(
> + BugUnlocked(
> + when=UTC_NOW,
> + person=who,
> + old_status=old_lock_status
> + )
> + )
> +
>
> @ProxyFactory
> def get_also_notified_subscribers(
> diff --git a/lib/lp/bugs/security.py b/lib/lp/bugs/security.py
> index b60b426..bdef636 100644
> --- a/lib/lp/bugs/security.py
> +++ b/lib/lp/bugs/security.py
> @@ -166,6 +167,21 @@ class EditBug(AuthorizationBase):
> user, permission='launchpad.Append'):
> # The user cannot even see the bug.
> return False
> +
> + def in_allowed_roles():
> + return (
> + # The bug reporter can always edit their own bug.
> + user.inTeam(self.obj.owner) or
This is perhaps up for debate, but I think that the bug-reporter-can-always-edit case should only apply to unlocked bugs. Think ahead to when we allow locking bugs so that even comments aren't allowed: somebody might well want to do that because the reporter is being obnoxious. A similar thing might apply to a bug where the reporter keeps vexatiously reopening it.
> + # Users with relevant roles can edit the bug.
> + user.in_admin or
> + user.in_commercial_admin or
> + user.in_registry_experts or
> + _has_any_bug_role(user, self.obj.bugtasks)
> + )
> +
> + if self.obj.lock_status == BugLockStatus.COMMENT_ONLY:
> + return in_allowed_roles()
> +
> return (
> # If the bug is private, then we don't need more elaborate
> # checks as they must have been explicitly subscribed.
> @@ -173,18 +189,40 @@ class EditBug(AuthorizationBase):
> # If the user seems generally legitimate, let them through.
> self.forwardCheckAuthenticated(
> user, permission='launchpad.AnyLegitimatePerson') or
> - # The bug reporter can always edit their own bug.
> - user.inTeam(self.obj.owner) or
> - # Users with relevant roles can edit the bug.
> - user.in_admin or user.in_commercial_admin or
> - user.in_registry_experts or
> - _has_any_bug_role(user, self.obj.bugtasks))
> + in_allowed_roles())
>
> def checkUnauthenticated(self):
> """Never allow unauthenticated users to edit a bug."""
> return False
>
>
> +class ModerateBug(AuthorizationBase):
> + """Security adapter for moderating bugs.
> +
> + This is used for operations like locking and unlocking a bug to the
> + relevant roles.
> + """
> + permission = 'launchpad.Moderate'
> + usedfor = IBug
> +
> + def checkAuthenticated(self, user):
> + if not self.forwardCheckAuthenticated(
> + user, permission='launchpad.Append'):
> + # The user cannot even see the bug.
> + return False
> +
> + return (
> + user.in_admin or
> + user.in_commercial_admin or
> + user.in_registry_experts or
> + _has_any_bug_role(user, self.obj.bugtasks)
> + )
> +
> + def checkUnauthenticated(self):
> + """Never allow unauthenticated users to moderate a bug."""
> + return False
This is the default in `AuthorizationBase.checkUnauthenticated` and isn't going to change there; I don't think we really need to repeat it.
> +
> +
> class PublicToAllOrPrivateToExplicitSubscribersForBug(AuthorizationBase):
> permission = 'launchpad.View'
> usedfor = IBug
> diff --git a/lib/lp/bugs/tests/test_bug.py b/lib/lp/bugs/tests/test_bug.py
> index 0c1e24b..bc28835 100644
> --- a/lib/lp/bugs/tests/test_bug.py
> +++ b/lib/lp/bugs/tests/test_bug.py
> @@ -410,3 +419,305 @@ class TestBugPermissions(TestCaseWithFactory, KarmaTestMixin):
> person)
> with person_logged_in(person):
> self.assertTrue(checkPermission('launchpad.Edit', self.bug))
> +
> +
> +class TestBugLocking(TestCaseWithFactory):
> + """
> + Tests for the bug locking functionality.
> + """
> +
> + layer = DatabaseFunctionalLayer
> +
> + def setUp(self):
> + super().setUp()
> + self.person = self.factory.makePerson()
> + self.target = self.factory.makeProduct()
> +
> + def test_bug_lock_status_lock_reason_default_values(self):
> + bug = self.factory.makeBug(owner=self.person, target=self.target)
> + self.assertEqual(bug.lock_status, BugLockStatus.UNLOCKED)
Could you flip the order of this and the other equality assertions here so that they follow the `self.assertEqual(expected, observed)` pattern?
> + self.assertIsNone(bug.lock_reason)
> +
> + def test_bug_lock_status_invalid_values(self):
> + bug = self.factory.makeBug(owner=self.person, target=self.target)
> + invalid_values_list = ["test", 1.23, 123, self.person]
> + with person_logged_in(self.target.owner):
> + for invalid_value in invalid_values_list:
> + # Direct assignment
> + with ExpectedException(TypeError):
> + removeSecurityProxy(bug).lock_status = invalid_value
> +
> + # Via the lock() method
> + with ExpectedException(TypeError):
> + bug.lock(
> + who=removeSecurityProxy(
> + bug.default_bugtask.target
> + ).owner,
> + status=invalid_value)
> +
> + def test_bug_locking_when_bug_already_locked(self):
> + bug = self.factory.makeBug(owner=self.person, target=self.target)
> + with person_logged_in(self.target.owner):
> + bug.lock(who=self.target.owner)
> + self.assertEqual(bug.lock_status, BugLockStatus.COMMENT_ONLY)
> + bug.lock(who=self.target.owner)
> + self.assertEqual(bug.lock_status, BugLockStatus.COMMENT_ONLY)
> +
> + def test_bug_locking_with_status_set_to_unlocked_does_nothing(self):
> + bug = self.factory.makeBug(owner=self.person, target=self.target)
> + with person_logged_in(self.target.owner):
> + bug.lock(who=self.target.owner, status=BugLockStatus.UNLOCKED)
> + self.assertEqual(bug.lock_status, BugLockStatus.UNLOCKED)
> + bug.lock(who=self.target.owner)
> + self.assertEqual(bug.lock_status, BugLockStatus.COMMENT_ONLY)
> + bug.lock(who=self.target.owner, status=BugLockStatus.UNLOCKED)
> + self.assertEqual(bug.lock_status, BugLockStatus.COMMENT_ONLY)
> +
> + def test_bug_locking_with_a_reason_works(self):
> + bug = self.factory.makeBug(owner=self.person, target=self.target)
> + with person_logged_in(self.target.owner):
> + bug.lock(who=self.person, reason='too hot')
> + self.assertEqual(bug.lock_reason, 'too hot')
> +
> + def test_bug_unlocking_clears_the_reason(self):
> + bug = self.factory.makeBug(owner=self.person, target=self.target)
> + with person_logged_in(self.target.owner):
> + bug.lock(who=self.person, reason='too hot')
> + bug.unlock(who=self.person)
> + self.assertIsNone(bug.lock_reason)
> +
> + def test_bug_locking_unlocking_adds_bug_activity_entries(self):
> + bug = self.factory.makeBug(owner=self.person, target=self.target)
> + self.assertEqual(bug.activity.count(), 1)
> + with person_logged_in(self.target.owner):
> + bug.lock(who=self.target.owner, reason='too hot')
> + self.assertEqual(bug.activity.count(), 2)
> + self.assertThat(
> + bug.activity[1],
> + MatchesStructure.byEquality(
> + person=self.target.owner,
> + whatchanged='bug lock status',
> + oldvalue=str(BugLockStatus.UNLOCKED),
> + newvalue=str(BugLockStatus.COMMENT_ONLY),
> + )
> + )
> + bug.unlock(who=self.target.owner)
> + self.assertEqual(bug.activity.count(), 3)
> + self.assertThat(
> + bug.activity[2],
> + MatchesStructure.byEquality(
> + person=self.target.owner,
> + whatchanged='bug lock status',
> + oldvalue=str(BugLockStatus.COMMENT_ONLY),
> + newvalue=str(BugLockStatus.UNLOCKED),
> + )
> + )
> +
> + def test_edit_permission_restrictions_when_a_bug_is_locked(self):
> + bug = self.factory.makeBug(owner=self.person, target=self.target)
> + another_person = self.factory.makePerson()
> +
> + with person_logged_in(self.target.owner):
> + bug.lock(who=self.target.owner)
> +
> + self.assertEqual(bug.lock_status, BugLockStatus.COMMENT_ONLY)
> +
> + # A user without the relevant role cannot edit a locked bug.
> + with person_logged_in(another_person):
> + self.assertFalse(checkPermission('launchpad.Edit', bug))
> +
> + # The bug reporter can edit a locked bug.
> + with person_logged_in(self.person):
> + self.assertTrue(checkPermission('launchpad.Edit', bug))
> +
> + # Target driver can can edit a locked bug.
> + new_person = self.factory.makePerson()
> + removeSecurityProxy(bug.default_bugtask.target).driver = new_person
> + with person_logged_in(new_person):
> + self.assertTrue(checkPermission('launchpad.Edit', bug))
> +
> + # Admins can edit a locked bug.
> + with admin_logged_in():
> + self.assertTrue(checkPermission('launchpad.Edit', bug))
> +
> + # Commercial admins can edit a locked bug.
> + with celebrity_logged_in('commercial_admin'):
> + self.assertTrue(checkPermission('launchpad.Edit', bug))
> +
> + # Registry experts can edit a locked bug.
> + with celebrity_logged_in('registry_experts'):
> + self.assertTrue(checkPermission('launchpad.Edit', bug))
> +
> +
> + def test_only_those_with_moderate_permission_can_lock_unlock_a_bug(self):
> + bug = self.factory.makeBug(owner=self.person, target=self.target)
> + another_person = self.factory.makePerson()
> +
> + # Unauthenticated person cannot moderate a bug.
> + self.assertFalse(checkPermission('launchpad.Moderate', bug))
> +
> + # A user without the relevant role cannot moderate a bug.
> + with person_logged_in(another_person):
> + self.assertFalse(checkPermission('launchpad.Moderate', bug))
> +
> + # The bug reporter cannot moderate a bug.
> + with person_logged_in(self.person):
> + self.assertFalse(checkPermission('launchpad.Moderate', bug))
> +
> + # Admins can moderate a bug.
> + with admin_logged_in():
> + self.assertTrue(checkPermission('launchpad.Moderate', bug))
> +
> + # Commercial admins can moderate a bug.
> + with celebrity_logged_in('commercial_admin'):
> + self.assertTrue(checkPermission('launchpad.Moderate', bug))
> +
> + # Registry experts can moderate a bug.
> + with celebrity_logged_in('registry_experts'):
> + self.assertTrue(checkPermission('launchpad.Moderate', bug))
> +
> + # Target owner can moderate a bug.
> + with person_logged_in(
> + removeSecurityProxy(bug.default_bugtask.target).owner
> + ):
> + self.assertTrue(checkPermission('launchpad.Moderate', bug))
> +
> + # Target driver can moderate a bug.
> + new_person = self.factory.makePerson()
> + removeSecurityProxy(bug.default_bugtask.target).driver = new_person
> + with person_logged_in(new_person):
> + self.assertTrue(checkPermission('launchpad.Moderate', bug))
> +
> + yet_another_person = self.factory.makePerson()
> + removeSecurityProxy(
> + bug.default_bugtask.target
> + ).bug_supervisor = yet_another_person
> + with person_logged_in(yet_another_person):
> + self.assertTrue(checkPermission('launchpad.Moderate', bug))
> +
> +
> +class TestBugLockingWebService(TestCaseWithFactory):
> + """Tests for the bug locking and unlocking web service methods."""
> +
> + layer = DatabaseFunctionalLayer
> +
> + def setUp(self):
> + super().setUp()
> + self.person = self.factory.makePerson()
> + self.target = self.factory.makeProduct()
> +
> + def test_who_value_for_lock_is_correctly_set(self):
> + bug = self.factory.makeBug(owner=self.person, target=self.target)
> + self.assertEqual(bug.activity.count(), 1)
> + bug_url = api_url(bug)
> + webservice = webservice_for_person(
> + self.target.owner,
> + permission=OAuthPermission.WRITE_PRIVATE
> + )
> + webservice.default_api_version = "devel"
> +
> + response = webservice.named_post(
> + bug_url, 'lock'
> + )
> + with person_logged_in(ANONYMOUS):
> + self.assertEqual(bug.activity.count(), 2)
> + self.assertThat(
> + bug.activity[1],
> + MatchesStructure.byEquality(
> + person=self.target.owner,
> + oldvalue=str(BugLockStatus.UNLOCKED),
> + newvalue=str(BugLockStatus.COMMENT_ONLY)
> + )
> + )
> +
> + response = webservice.named_post(
> + bug_url, 'unlock'
> + )
> + self.assertEqual(200, response.status)
> + with person_logged_in(ANONYMOUS):
> + self.assertEqual(bug.activity.count(), 3)
> + self.assertThat(
> + bug.activity[2],
> + MatchesStructure.byEquality(
> + person=self.target.owner,
> + whatchanged='bug lock status',
> + oldvalue=str(BugLockStatus.COMMENT_ONLY),
> + newvalue=str(BugLockStatus.UNLOCKED),
> + )
> + )
> +
> + def test_who_value_for_unlock_is_correctly_set(self):
> + bug = self.factory.makeBug(owner=self.person, target=self.target)
> + self.assertEqual(bug.activity.count(), 1)
> + with person_logged_in(self.target.owner):
> + bug.lock(who=self.target.owner)
> + self.assertEqual(bug.activity.count(), 2)
> + bug_url = api_url(bug)
> + webservice = webservice_for_person(
> + self.target.owner,
> + permission=OAuthPermission.WRITE_PRIVATE
> + )
> + webservice.default_api_version = "devel"
> +
> + response = webservice.named_post(
> + bug_url, 'unlock'
> + )
> + self.assertEqual(200, response.status)
> + with person_logged_in(ANONYMOUS):
> + self.assertEqual(bug.activity.count(), 3)
> + self.assertThat(
> + bug.activity[2],
> + MatchesStructure.byEquality(
> + person=self.target.owner,
> + oldvalue=str(BugLockStatus.COMMENT_ONLY),
> + newvalue=str(BugLockStatus.UNLOCKED),
> + )
> + )
> +
> + def test_lock_status_lock_reason_values_unlocked_bug(self):
> + bug = self.factory.makeBug(owner=self.person, target=self.target)
> + bug_url = api_url(bug)
> +
> + webservice = webservice_for_person(
> + self.target.owner,
> + permission=OAuthPermission.WRITE_PRIVATE
> + )
> + webservice.default_api_version = 'devel'
> +
> + response = webservice.get(bug_url)
> + self.assertEqual(200, response.status)
> + response_json = response.jsonBody()
> + self.assertEqual(
> + response_json['lock_status'],
> + str(BugLockStatus.UNLOCKED)
> + )
> + self.assertIsNone(response_json['lock_reason'])
> +
> + def test_lock_status_lock_reason_values_after_locking(self):
> + bug = self.factory.makeBug(owner=self.person, target=self.target)
> + bug_url = api_url(bug)
> +
> + with person_logged_in(self.target.owner):
> + bug.lock(
> + who=self.target.owner,
> + status=BugLockStatus.COMMENT_ONLY,
> + reason='too hot'
> + )
> +
> + webservice = webservice_for_person(
> + self.target.owner,
> + permission=OAuthPermission.WRITE_PRIVATE
> + )
> + webservice.default_api_version = 'devel'
> +
> + response = webservice.get(bug_url)
> + self.assertEqual(200, response.status)
> + response_json = response.jsonBody()
> + self.assertEqual(
> + response_json['lock_status'],
> + str(BugLockStatus.COMMENT_ONLY)
> + )
> + self.assertEqual(
> + response_json['lock_reason'],
> + 'too hot'
> + )
> diff --git a/lib/lp/scripts/garbo.py b/lib/lp/scripts/garbo.py
> index 5eabeb6..b91acda 100644
> --- a/lib/lp/scripts/garbo.py
> +++ b/lib/lp/scripts/garbo.py
> @@ -1762,6 +1763,33 @@ class PopulateSnapBuildStoreRevision(TunableLoop):
> transaction.commit()
>
>
> +class PopulateBugLockStatusDefaultUnlocked(TunableLoop):
> + """
> + Populates Bug.lock_status to BugLockStatus.UNLOCKED if not set
> + """
> +
> + maximum_chunk_size = 5000
> +
> + def __init__(self, log, abort_time=None):
> + super().__init__(log, abort_time)
> + self.store = IMasterStore(Bug)
> +
> + def findBugsLockStatusNone(self):
> + return self.store.find(
> + Bug,
> + Bug.lock_status == None
> + )
> +
> + def isDone(self):
> + return self.findBugsLockStatusNone().is_empty()
> +
> + def __call__(self, chunk_size):
> + self.findBugsLockStatusNone()[:chunk_size].set(
> + lock_status=BugLockStatus.UNLOCKED
> + )
> + transaction.commit()
`Bug.lock_status` is unindexed, so querying for the rows where this is unset is kind of a slow operation since it has to do a full table scan. That was a reasonable decision in the DB patch since I don't think we'll need to query on `Bug.lock_status` other than in this temporary job, but we should probably make some slight adjustments so that this job's queries are a bit less expensive.
I suggest: (1) `self.start_at = 1` in the constructor; (2) add `Bug.id >= self.start_at` to the conditions in `findBugsLockStatusNone`, and also add `.order_by(Bug.id)` there so that results come back in order; (3) change `__call__` to something like:
bugs = list(self.findBugsLockStatusNone()[:chunk_size])
bugs.set(lock_status=BugLockStatus.UNLOCKED)
if bugs:
self.start_at = bugs[-1].id + 1
transaction.commit()
> +
> +
> class BaseDatabaseGarbageCollector(LaunchpadCronScript):
> """Abstract base class to run a collection of TunableLoops."""
> script_name = None # Script name for locking and database user. Override.
> @@ -2022,6 +2050,7 @@ class HourlyDatabaseGarbageCollector(BaseDatabaseGarbageCollector):
> GitRepositoryPruner,
> RevisionCachePruner,
> UnusedSessionPruner,
> + PopulateBugLockStatusDefaultUnlocked,
Please keep these lists of tunable loops sorted by class name.
I'd also suggest moving this new loop to `DailyDatabaseGarbageCollector` for lower overhead.
> ]
> experimental_tunable_loops = []
>
--
https://code.launchpad.net/~lgp171188/launchpad/+git/launchpad/+merge/414097
Your team Launchpad code reviewers is requested to review the proposed merge of ~lgp171188/launchpad:enforce-stricter-permission-check-when-a-bug-is-locked-down into launchpad:master.
References
