launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> === modified file 'lib/lp/app/tests/test_security.py'
> --- lib/lp/app/tests/test_security.py	2015-02-23 03:22:37 +0000
> +++ lib/lp/app/tests/test_security.py	2015-03-05 19:46:21 +0000
> @@ -1,7 +1,9 @@
> -# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  __metaclass__ = type
> +
> +from testtools.matchers import Equals
>  from zope.component import (
>      getSiteManager,
>      getUtility,
> @@ -28,6 +30,7 @@
>  from lp.testing import (
>      admin_logged_in,
>      person_logged_in,
> +    record_two_runs,
>      TestCase,
>      TestCaseWithFactory,
>      )
> @@ -36,6 +39,7 @@
>      DatabaseFunctionalLayer,
>      ZopelessDatabaseLayer,
>      )
> +from lp.testing.matchers import HasQueryCount
>  
>  
>  def registerFakeSecurityAdapter(interface, permission, adapter=None):
> @@ -253,3 +257,30 @@
>          self.assertTeamOwnerCanListPrivateTeamWithTeamStatus(
>              TeamMembershipStatus.EXPIRED)
>  
> +    def test_private_team_query_count(self):
> +        # Testing visibility of a private team involves checking for
> +        # subscriptions to any private PPAs owned by that team.  Make sure
> +        # that this doesn't involve a query for every archive subscription
> +        # the user has.
> +        person = self.factory.makePerson()
> +        team_owner = self.factory.makePerson()
> +        private_team = self.factory.makeTeam(
> +            owner=team_owner, visibility=PersonVisibility.PRIVATE)
> +        checker = PublicOrPrivateTeamsExistence(
> +            removeSecurityProxy(private_team))
> +
> +        def create_subscribed_archive():
> +            with person_logged_in(team_owner):
> +                archive = self.factory.makeArchive(
> +                    owner=private_team, private=True)
> +                archive.newSubscription(person, team_owner)
> +
> +        def check_team_limited_view():
> +            person.clearInTeamCache()
> +            with person_logged_in(person):
> +                self.assertTrue(
> +                    checker.checkAuthenticated(IPersonRoles(person)))
> +
> +        recorder1, recorder2 = record_two_runs(
> +            check_team_limited_view, create_subscribed_archive, 5)
> +        self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
> 
> === modified file 'lib/lp/security.py'
> --- lib/lp/security.py	2015-02-26 17:32:56 +0000
> +++ lib/lp/security.py	2015-03-05 19:46:21 +0000
> @@ -992,8 +992,8 @@
>              and self.obj.visibility == PersonVisibility.PRIVATE):
>              # Grant visibility to people with subscriptions on a private
>              # team's private PPA.
> -            subscriptions = getUtility(
> -                IArchiveSubscriberSet).getBySubscriber(user.person)
> +            subscriptions = getUtility(IArchiveSubscriberSet).getBySubscriber(
> +                user.person, need_archive=True)
>              subscriber_archive_ids = set(
>                  sub.archive.id for sub in subscriptions)
>              team_ppa_ids = set(

You could avoid retrieving all the (potentially largeish) Archive rows by using sub.archiveID instead of sub.archive.id.

> 
> === modified file 'lib/lp/soyuz/interfaces/archivesubscriber.py'
> --- lib/lp/soyuz/interfaces/archivesubscriber.py	2014-03-07 01:35:46 +0000
> +++ lib/lp/soyuz/interfaces/archivesubscriber.py	2015-03-05 19:46:21 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  """ArchiveSubscriber interface."""
> @@ -130,7 +130,8 @@
>  class IArchiveSubscriberSet(Interface):
>      """An interface for the set of all archive subscribers."""
>  
> -    def getBySubscriber(subscriber, archive=None, current_only=True):
> +    def getBySubscriber(subscriber, archive=None, current_only=True,
> +                        need_archive=False):
>          """Return all the subscriptions for a person.
>  
>          :param subscriber: An `IPerson` for whom to return all
> @@ -139,6 +140,7 @@
>              the results to that particular archive.
>          :param current_only: Whether the result should only include current
>              subscriptions (which is the default).
> +        :param need_archive: Whether the archive attribute should be cached.
>          """
>  
>      def getBySubscriberWithActiveToken(subscriber, archive=None):
> 
> === modified file 'lib/lp/soyuz/model/archivesubscriber.py'
> --- lib/lp/soyuz/model/archivesubscriber.py	2014-03-07 00:44:37 +0000
> +++ lib/lp/soyuz/model/archivesubscriber.py	2015-03-05 19:46:21 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  """Database class for table ArchiveSubscriber."""
> @@ -33,6 +33,7 @@
>  from lp.registry.interfaces.person import validate_person
>  from lp.registry.model.person import Person
>  from lp.registry.model.teammembership import TeamParticipation
> +from lp.services.database.bulk import load_related
>  from lp.services.database.constants import UTC_NOW
>  from lp.services.database.decoratedresultset import DecoratedResultSet
>  from lp.services.database.enumcol import DBEnum
> @@ -147,7 +148,7 @@
>      """See `IArchiveSubscriberSet`."""
>  
>      def _getBySubscriber(self, subscriber, archive, current_only,
> -                         with_active_tokens):
> +                         with_active_tokens, need_archive=False):
>          """Return all the subscriptions for a person.
>  
>          :param subscriber: An `IPerson` for whom to return all
> @@ -159,6 +160,7 @@
>          :param with_active_tokens: Indicates whether the tokens for the given
>              subscribers subscriptions should be included in the resultset.
>              By default the tokens are not included in the resultset.
> +        :param need_archive: Whether the archive attribute should be cached.
>          """
>          # Grab the extra Storm expressions, for this query,
>          # depending on the params:
> @@ -185,6 +187,12 @@
>          else:
>              result_row = ArchiveSubscriber
>  
> +        def eager_load(rows):
> +            # Circular import.
> +            from lp.soyuz.model.archive import Archive
> +            if need_archive:
> +                load_related(Archive, rows, ["archive_id"])
> +
>          # Set the main expression to find all the subscriptions for
>          # which the subscriber is a direct subscriber OR is a member
>          # of a subscribed team.
> @@ -193,14 +201,18 @@
>          # showing that each person is a member of the "team" that
>          # consists of themselves.
>          store = Store.of(subscriber)
> -        return store.using(*origin).find(
> +        result_set = store.using(*origin).find(
>              result_row,
>              TeamParticipation.personID == subscriber.id,
>              *extra_exprs).order_by(Desc(ArchiveSubscriber.date_created))
> +        return DecoratedResultSet(result_set, pre_iter_hook=eager_load)
>  
> -    def getBySubscriber(self, subscriber, archive=None, current_only=True):
> +    def getBySubscriber(self, subscriber, archive=None, current_only=True,
> +                        need_archive=False):
>          """See `IArchiveSubscriberSet`."""
> -        return self._getBySubscriber(subscriber, archive, current_only, False)
> +        return self._getBySubscriber(
> +            subscriber, archive, current_only, False,
> +            need_archive=need_archive)
>  
>      def getBySubscriberWithActiveToken(self, subscriber, archive=None):
>          """See `IArchiveSubscriberSet`."""
> 


-- 
https://code.launchpad.net/~cjwatson/launchpad/preload-archive-subscriptions/+merge/252011
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.