launchpad-reviewers team mailing list archive

[Celso Providelo <celso.providelo@xxxxxxxxxxxxx>]

Celso Providelo has proposed merging lp:~cprov/launchpad/bug-1295318 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1295318 in Launchpad itself: "getPublishedSources launchpadlib query times out"
  https://bugs.launchpad.net/launchpad/+bug/1295318

For more details, see:
https://code.launchpad.net/~cprov/launchpad/bug-1295318/+merge/216731

Create a wrapper for IArchive.getPublishedSources() API calls. It is used for pre-caching related objects and simplifying security checks, resulting in less queries issued per call.
-- 
https://code.launchpad.net/~cprov/launchpad/bug-1295318/+merge/216731
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cprov/launchpad/bug-1295318 into lp:launchpad.

=== modified file 'lib/lp/_schema_circular_imports.py'
--- lib/lp/_schema_circular_imports.py	2014-03-11 10:29:43 +0000
+++ lib/lp/_schema_circular_imports.py	2014-04-22 14:30:58 +0000
@@ -438,13 +438,13 @@
     IArchive, 'getArchiveDependency', 'dependency', IArchive)
 patch_entry_return_type(IArchive, 'getArchiveDependency', IArchiveDependency)
 patch_plain_parameter_type(
-    IArchive, 'getPublishedSources', 'distroseries', IDistroSeries)
+    IArchive, '_getPublishedSources', 'distroseries', IDistroSeries)
 patch_collection_return_type(
-    IArchive, 'getPublishedSources', ISourcePackagePublishingHistory)
-patch_choice_parameter_type(
-    IArchive, 'getPublishedSources', 'status', PackagePublishingStatus)
-patch_choice_parameter_type(
-    IArchive, 'getPublishedSources', 'pocket', PackagePublishingPocket)
+    IArchive, '_getPublishedSources', ISourcePackagePublishingHistory)
+patch_choice_parameter_type(
+    IArchive, '_getPublishedSources', 'status', PackagePublishingStatus)
+patch_choice_parameter_type(
+    IArchive, '_getPublishedSources', 'pocket', PackagePublishingPocket)
 patch_plain_parameter_type(
     IArchive, 'getAllPublishedBinaries', 'distroarchseries',
     IDistroArchSeries)
@@ -761,7 +761,7 @@
     "getBuildSummariesForSourceIds", "getComponentsForQueueAdmin",
     "getPackagesetsForSource", "getPackagesetsForSourceUploader",
     "getPackagesetsForUploader", "getPermissionsForPerson",
-    "getPublishedSources", "getQueueAdminsForComponent",
+    "_getPublishedSources", "getQueueAdminsForComponent",
     "getUploadersForComponent", "getUploadersForPackage",
     "getUploadersForPackageset", "isSourceUploadAllowed",
     "newComponentUploader", "newPackageUploader", "newPackagesetUploader",

=== modified file 'lib/lp/soyuz/interfaces/archive.py'
--- lib/lp/soyuz/interfaces/archive.py	2014-03-18 23:16:22 +0000
+++ lib/lp/soyuz/interfaces/archive.py	2014-04-22 14:30:58 +0000
@@ -405,6 +405,7 @@
         :return: A IArchiveAuthToken, or None if the user has none.
         """
 
+    @export_operation_as('getPublishedSources')
     @rename_parameters_as(name="source_name", distroseries="distro_series")
     @operation_parameters(
         name=TextLine(title=_("Source package name"), required=False),
@@ -439,9 +440,18 @@
         )
     # Really returns ISourcePackagePublishingHistory, see below for
     # patch to avoid circular import.
-    @call_with(eager_load=True)
     @operation_returns_collection_of(Interface)
     @export_read_operation()
+    def _getPublishedSources(name=None, version=None, status=None,
+                             distroseries=None, pocket=None,
+                             exact_match=False, created_since_date=None,
+                             component_name=None):
+        """Wrapper for getPublishedSources.
+
+        Prefills `SourcePackageRelease.published_archives` with the
+        context `Archive` so the security checks do not hit the DB.
+        """
+
     def getPublishedSources(name=None, version=None, status=None,
                             distroseries=None, pocket=None,
                             exact_match=False, created_since_date=None,

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2013-11-25 07:36:12 +0000
+++ lib/lp/soyuz/model/archive.py	2014-04-22 14:30:58 +0000
@@ -490,6 +490,59 @@
             return getUtility(IBuildFarmJobSet).getBuildsForArchive(
                 self, status=build_state)
 
+    def _getPublishedSources(self, name=None, version=None, status=None,
+                             distroseries=None, pocket=None,
+                             exact_match=False, created_since_date=None,
+                             component_name=None):
+        """See `IArchive`."""
+        # 'eager_load' and 'include_removed' arguments are always True
+        # for API calls.
+        published_sources = self.getPublishedSources(
+            name, version, status, distroseries, pocket, exact_match,
+            created_since_date, True, component_name, True)
+
+        def cache_related(rows):
+            # Cache extra related-objects immediately used by API calls.
+            from lp.registry.model.sourcepackagename import SourcePackageName
+            from lp.soyuz.model.queue import (
+                PackageUpload,
+                PackageUploadSource,
+                )
+            store = Store.of(self)
+            # Pre-cache related `PackageUpload`s, needed for security checks.
+            pu_ids = set(map(attrgetter('packageuploadID'), rows))
+            list(store.find(PackageUpload, PackageUpload.id.is_in(pu_ids)))
+            # Pre-cache related `SourcePackageName`s, needed for the published
+            # record title (materialized in the API).
+            spn_ids = set(map(
+                attrgetter('sourcepackagerelease.sourcepackagenameID'), rows))
+            list(store.find(SourcePackageName,
+                            SourcePackageName.id.is_in(spn_ids)))
+            # Load and organise related `PackageUploadSources` for filling
+            # `PackageUpload.sources` cachedproperty.
+            pu_sources = store.find(
+                PackageUploadSource,
+                PackageUploadSource.packageuploadID.is_in(pu_ids))
+            pu_sources_dict = dict(zip(pu_ids, pu_sources))
+
+            # Fill publishing records.
+            for pub_source in rows:
+                # Bypass PackageUpload security-check query by caching
+                # `SourcePackageRelease.published_archives` results.
+                # All published sources are visible to the user, since they
+                # are published in the context archive. No need to check
+                # additional archives the source is published.
+                spr = pub_source.sourcepackagerelease
+                get_property_cache(spr).published_archives = [self]
+                # Fill `PackageUpload.sources`.
+                if pub_source.packageupload is not None:
+                    upload = pub_source.packageupload
+                    get_property_cache(upload).sources = [
+                        pu_sources_dict.get(upload.id)]
+
+        return DecoratedResultSet(published_sources,
+                                  pre_iter_hook=cache_related)
+
     def getPublishedSources(self, name=None, version=None, status=None,
                             distroseries=None, pocket=None,
                             exact_match=False, created_since_date=None,

=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py	2013-11-21 03:37:58 +0000
+++ lib/lp/soyuz/tests/test_archive.py	2014-04-22 14:30:58 +0000
@@ -13,6 +13,7 @@
 from pytz import UTC
 from testtools.matchers import (
     DocTestMatches,
+    LessThan,
     MatchesRegex,
     MatchesStructure,
     )
@@ -41,6 +42,7 @@
 from lp.services.database.sqlbase import sqlvalues
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.propertycache import clear_property_cache
+from lp.services.webapp.interfaces import OAuthPermission
 from lp.services.worlddata.interfaces.country import ICountrySet
 from lp.soyuz.adapters.archivedependencies import (
     get_sources_list_for_building,
@@ -102,6 +104,9 @@
     LaunchpadFunctionalLayer,
     LaunchpadZopelessLayer,
     )
+from lp.testing.matchers import HasQueryCount
+from lp.testing.pages import webservice_for_person
+from lp.testing._webservice import QueryCollector
 
 
 class TestGetPublicationsInArchive(TestCaseWithFactory):
@@ -2126,6 +2131,58 @@
         self.assertEqual('universe', filtered.component.name)
 
 
+class GetPublishedSourcesWebServiceTests(TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super(GetPublishedSourcesWebServiceTests, self).setUp()
+
+    def create_testing_ppa(self):
+        """Creates and populates a PPA for API performance tests.
+
+        Creates a public PPA and populates it with 5 distinct source
+        publications with corresponding `PackageUpload` records.
+        """
+        ppa = self.factory.makeArchive(
+            name='ppa', purpose=ArchivePurpose.PPA)
+        distroseries = self.factory.makeDistroSeries()
+        # XXX cprov 2014-04-22: currently the target archive owner cannot
+        # 'addSource' to a `PackageUpload` ('launchpad.Edit'). It seems
+        # too restrive to me.
+        from zope.security.proxy import removeSecurityProxy
+        with person_logged_in(ppa.owner):
+            for i in range(5):
+                upload = self.factory.makePackageUpload(
+                    distroseries=distroseries, archive=ppa)
+                pub = self.factory.makeSourcePackagePublishingHistory(
+                    archive=ppa, distroseries=distroseries,
+                    creator=ppa.owner, spr_creator=ppa.owner,
+                    maintainer=ppa.owner, packageupload=upload)
+                naked_upload = removeSecurityProxy(upload)
+                naked_upload.addSource(pub.sourcepackagerelease)
+        return ppa
+
+    def test_query_count(self):
+        # IArchive.getPublishedSources() webservice is exposed
+        # via a wrapper to improving performance (by reducing the
+        # number of queries issued)
+        ppa = self.create_testing_ppa()
+        ppa_url = '/~{}/+archive/ppa'.format(ppa.owner.name)
+        webservice = webservice_for_person(
+            ppa.owner, permission=OAuthPermission.READ_PRIVATE)
+
+        collector = QueryCollector()
+        collector.register()
+        self.addCleanup(collector.unregister)
+
+        response = webservice.named_get(ppa_url, 'getPublishedSources')
+
+        self.assertEqual(200, response.status)
+        self.assertEqual(5, response.jsonBody()['total_size'])
+        self.assertThat(collector, HasQueryCount(LessThan(28)))
+
+
 class TestCopyPackage(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2014-02-24 07:19:52 +0000
+++ lib/lp/testing/factory.py	2014-04-22 14:30:58 +0000
@@ -3625,6 +3625,7 @@
                                            scheduleddeletiondate=None,
                                            ancestor=None,
                                            creator=None,
+                                           packageupload=None,
                                            spr_creator=None,
                                            **kwargs):
         """Make a `SourcePackagePublishingHistory`.
@@ -3683,7 +3684,8 @@
             spph = getUtility(IPublishingSet).newSourcePublication(
                 archive, sourcepackagerelease, distroseries,
                 sourcepackagerelease.component, sourcepackagerelease.section,
-                pocket, ancestor=ancestor, creator=creator)
+                pocket, ancestor=ancestor, creator=creator,
+                packageupload=packageupload)
 
         naked_spph = removeSecurityProxy(spph)
         naked_spph.status = status