Merge lp:~cprov/launchpad/bug-1295318 into lp:launchpad

[Celso Providelo]

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.

[William Grant (wgrant)]

52 + def _getPublishedSources(name=None, version=None, status=None,
53 + distroseries=None, pocket=None,
54 + exact_match=False, created_since_date=None,
55 + component_name=None):
56 + """Wrapper for getPublishedSources.

This should be something like api_getPublishedSources to indicate its purpose.

92 + # Pre-cache related `PackageUpload`s, needed for security checks.

The PackageUploads are needed for the SPPH.packageupload field. We pre-populate the sources attribute of each PackageUpload to avoid the security check.

93 + pu_ids = set(map(attrgetter('packageuploadID'), rows))
94 + list(store.find(PackageUpload, PackageUpload.id.is_in(pu_ids)))
95 + # Pre-cache related `SourcePackageName`s, needed for the published
96 + # record title (materialized in the API).
97 + spn_ids = set(map(
98 + attrgetter('sourcepackagerelease.sourcepackagenameID'), rows))
99 + list(store.find(SourcePackageName,
100 + SourcePackageName.id.is_in(spn_ids)))

These should be able to be simplified using lp.services.database.bulk.load_related.

116 + get_property_cache(spr).published_archives = [self]
117 + # Fill `PackageUpload.sources`.
118 + if pub_source.packageupload is not None:
119 + upload = pub_source.packageupload
120 + get_property_cache(upload).sources = [
121 + pu_sources_dict.get(upload.id)]

Setting published_archives and sources to just the elements that we know are sufficient here is fine for security purposes. But the attributes aren't limited to security checks, so this may introduce confusing correctness bugs -- they'll magically be erroneously non-exhaustive when they happen to have been touched by this API method.

I'd consider renaming the attributes to make it clear that they're only good for security checks. This might mean creating a new cachedproperty around PU.sources, as that attribute is used for more than just security.

170 + def create_testing_ppa(self):

This isn't a legal method name.

review: Needs Fixing (code)

[Celso Providelo (cprov)]

Thanks for the comments, William.

Comments inline ...

On Wed, Apr 23, 2014 at 8:00 AM, William Grant <email address hidden> wrote:
>
> Review: Needs Fixing code
>
> 52 + def _getPublishedSources(name=None, version=None, status=None,
> 53 + distroseries=None, pocket=None,
> 54 + exact_match=False, created_since_date=None,
> 55 + component_name=None):
> 56 + """Wrapper for getPublishedSources.
>
> This should be something like api_getPublishedSources to indicate its purpose

Good point.

> 92 + # Pre-cache related `PackageUpload`s, needed for security checks.
>
> The PackageUploads are needed for the SPPH.packageupload field. We pre-populate the sources attribute of each PackageUpload to avoid the security check

Comment adjusted.

>
> 93 + pu_ids = set(map(attrgetter('packageuploadID'), rows))
> 94 + list(store.find(PackageUpload, PackageUpload.id.is_in(pu_ids)))
> 95 + # Pre-cache related `SourcePackageName`s, needed for the published
> 96 + # record title (materialized in the API).
> 97 + spn_ids = set(map(
> 98 + attrgetter('sourcepackagerelease.sourcepackagenameID'), rows))
> 99 + list(store.find(SourcePackageName,
> 100 + SourcePackageName.id.is_in(spn_ids)))
>
> These should be able to be simplified using lp.services.database.bulk.load_related.
>

Right, load_related() and load_referencing() helped with PU and PUS.
Unfortunately, they do not support nested attributes and could not be
used for SPN.

https://bugs.launchpad.net/launchpad/+bug/1311690

> 116 + get_property_cache(spr).published_archives = [self]
> 117 + # Fill `PackageUpload.sources`.
> 118 + if pub_source.packageupload is not None:
> 119 + upload = pub_source.packageupload
> 120 + get_property_cache(upload).sources = [
> 121 + pu_sources_dict.get(upload.id)]
>
> Setting published_archives and sources to just the elements that we know are sufficient here is fine for security purposes. But the attributes aren't limited to security checks, so this may introduce confusing correctness bugs -- they'll magically be erroneously non-exhaustive when they happen to have been touched by this API method.
>
> I'd consider renaming the attributes to make it clear that they're only good for security checks. This might mean creating a new cachedproperty around PU.sources, as that attribute is used for more than just security.
>

We discussed this further on IRC and caching PU.sources is fine and
correct because of coincidence of constraints, we only accept
single-source uploads and once created PU contents never change. So,
in this scenario, caching PU.sources outside the upload-processing
period will be always "correct".

That said, there are plans to redesign the package upload workflow
(and queue) entirely and this particular aspect might change and those
callsites will have to be revisited.

>
> 170 + def create_testing_ppa(self):
>
> This isn't a legal method name.

Right, fixed.

[]

--
Celso Providelo
<email address hidden>

[William Grant (wgrant)]

56 + """API wrapper for getPublishedSources.
57 +
58 + It loads additional related objects only needed in the API call
59 + context (i.e. security checks and entries marshalling).
60 + """

This will appear as the API documentation for getPublishedSources. You probably want to mention the APIness in a comment, not the docstring.

review: Approve (code)

=== 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-24 02:14:17 +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, 'api_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, 'api_getPublishedSources', ISourcePackagePublishingHistory)
+patch_choice_parameter_type(
+    IArchive, 'api_getPublishedSources', 'status', PackagePublishingStatus)
+patch_choice_parameter_type(
+    IArchive, 'api_getPublishedSources', 'pocket', PackagePublishingPocket)
 patch_plain_parameter_type(
     IArchive, 'getAllPublishedBinaries', 'distroarchseries',
     IDistroArchSeries)
@@ -761,7 +761,7 @@
     "getBuildSummariesForSourceIds", "getComponentsForQueueAdmin",
     "getPackagesetsForSource", "getPackagesetsForSourceUploader",
     "getPackagesetsForUploader", "getPermissionsForPerson",
-    "getPublishedSources", "getQueueAdminsForComponent",
+    "api_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-24 02:14:17 +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,16 @@
         )
     # 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 api_getPublishedSources(name=None, version=None, status=None,
+                                distroseries=None, pocket=None,
+                                exact_match=False, created_since_date=None,
+                                component_name=None):
+        """All `ISourcePackagePublishingHistory` target to this archive."""
+        # It loads additional related objects only needed in the API call
+        # context (i.e. security checks and entries marshalling).
+
     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-24 02:14:17 +0000
@@ -77,7 +77,10 @@
 from lp.registry.model.sourcepackagename import SourcePackageName
 from lp.registry.model.teammembership import TeamParticipation
 from lp.services.config import config
-from lp.services.database.bulk import load_related
+from lp.services.database.bulk import (
+    load_referencing,
+    load_related,
+    )
 from lp.services.database.constants import UTC_NOW
 from lp.services.database.datetimecol import UtcDateTimeCol
 from lp.services.database.decoratedresultset import DecoratedResultSet
@@ -490,6 +493,49 @@
             return getUtility(IBuildFarmJobSet).getBuildsForArchive(
                 self, status=build_state)
 
+    def api_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 load_api_extra_objects(rows):
+            """Load extra related-objects needed by API calls."""
+            # Pre-cache related `PackageUpload`s and `PackageUploadSource`s
+            # which are immediatelly used in the API context for checking
+            # permissions on the returned entries.
+            uploads = load_related(PackageUpload, rows, ['packageuploadID'])
+            pu_sources = load_referencing(
+                PackageUploadSource, uploads, ['packageuploadID'])
+            for pu_source in pu_sources:
+                upload = pu_source.packageupload
+                get_property_cache(upload).sources = [pu_source]
+            # Pre-cache `SourcePackageName`s which are immediatelly used
+            # in the API context for materializing the returned entries.
+            # XXX cprov 2014-04-23: load_related() does not support
+            # nested attributes as foreign keys (uses getattr instead of
+            # attrgetter).
+            spn_ids = set(map(
+                attrgetter('sourcepackagerelease.sourcepackagenameID'), rows))
+            list(Store.of(self).find(
+                SourcePackageName, SourcePackageName.id.is_in(spn_ids)))
+            # 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.
+            for pub_source in rows:
+                spr = pub_source.sourcepackagerelease
+                get_property_cache(spr).published_archives = [self]
+
+        return DecoratedResultSet(published_sources,
+                                  pre_iter_hook=load_api_extra_objects)
+
     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-24 02:14:17 +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 createTestingPPA(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.createTestingPPA()
+        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-04-23 10:50:06 +0000
+++ lib/lp/testing/factory.py	2014-04-24 02:14:17 +0000
@@ -3626,6 +3626,7 @@
                                            scheduleddeletiondate=None,
                                            ancestor=None,
                                            creator=None,
+                                           packageupload=None,
                                            spr_creator=None,
                                            **kwargs):
         """Make a `SourcePackagePublishingHistory`.
@@ -3684,7 +3685,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