Merge lp:~cjwatson/launchpad/sourcefileurls-include-meta into lp:launchpad

[Colin Watson]

Add include_meta option to SPPH.sourceFileUrls, paralleling BPPH.binaryFileUrls.

Add include_meta option to SPPH.sourceFileUrls, paralleling BPPH.binaryFileUrls.

This is mainly to support dgit, which needs to be able to fetch .dsc files from Launchpad and verify their contents without having to fetch a full Sources file (even if one were available for the .dsc in question) and its trust chain.

The include_meta option is rather weird, in that it causes the method in question to return a completely different type, and had I been doing this from scratch I would probably have added a new getSourceFiles method instead and deprecated sourceFileUrls over time. However, BPPH.binaryFileUrls(include_meta=True) already existed (https://code.launchpad.net/~michael.nelson/launchpad/include-binary-size/+merge/150831), and other things being equal I prefer new interfaces to look like existing ones. I tried to make the implementations a little more in line with each other as well.

While I was here, I also added sha256 to BPPH.binaryFileUrls(include_meta=True), to drag things a little further into the modern world. I didn't see any point in exporting sha1 in the new SPPH.sourceFileUrls(include_meta=True) interface.

[William Grant (wgrant)]

I'd lean toward consistency wrt. SHA-1, but it doesn't really matter.

review: Approve (code)

=== modified file 'lib/lp/soyuz/browser/tests/test_publishing_webservice.py'
--- lib/lp/soyuz/browser/tests/test_publishing_webservice.py	2015-11-26 13:31:45 +0000
+++ lib/lp/soyuz/browser/tests/test_publishing_webservice.py	2015-12-02 13:21:01 +0000
@@ -3,20 +3,84 @@
 
 """Test webservice methods related to the publisher."""
 
-from testtools.matchers import IsInstance
+from functools import partial
 
-from lp.services.database.sqlbase import flush_database_caches
+from lp.services.librarian.browser import ProxiedLibraryFileAlias
 from lp.services.webapp.interfaces import OAuthPermission
 from lp.testing import (
     api_url,
+    login_person,
     person_logged_in,
-    RequestTimelineCollector,
+    record_two_runs,
     TestCaseWithFactory,
     )
 from lp.testing.layers import LaunchpadFunctionalLayer
+from lp.testing.matchers import HasQueryCount
 from lp.testing.pages import webservice_for_person
 
 
+class SourcePackagePublishingHistoryWebserviceTests(TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+
+    def make_spph_for(self, person):
+        with person_logged_in(person):
+            spr = self.factory.makeSourcePackageRelease()
+            self.factory.makeSourcePackageReleaseFile(sourcepackagerelease=spr)
+            spph = self.factory.makeSourcePackagePublishingHistory(
+                sourcepackagerelease=spr)
+            return spph, api_url(spph)
+
+    def test_sourceFileUrls(self):
+        person = self.factory.makePerson()
+        webservice = webservice_for_person(
+            person, permission=OAuthPermission.READ_PUBLIC)
+        spph, url = self.make_spph_for(person)
+
+        response = webservice.named_get(
+            url, 'sourceFileUrls', api_version='devel')
+
+        self.assertEqual(200, response.status)
+        urls = response.jsonBody()
+        with person_logged_in(person):
+            sprf = spph.sourcepackagerelease.files[0]
+            expected_urls = [
+                ProxiedLibraryFileAlias(
+                    sprf.libraryfile, spph.archive).http_url]
+        self.assertEqual(expected_urls, urls)
+
+    def test_sourceFileUrls_include_meta(self):
+        person = self.factory.makePerson()
+        webservice = webservice_for_person(
+            person, permission=OAuthPermission.READ_PUBLIC)
+        spph, url = self.make_spph_for(person)
+
+        def create_file():
+            self.factory.makeSourcePackageReleaseFile(
+                sourcepackagerelease=spph.sourcepackagerelease)
+
+        def get_urls():
+            return webservice.named_get(
+                url, 'sourceFileUrls', include_meta=True, api_version='devel')
+
+        recorder1, recorder2 = record_two_runs(
+            get_urls, create_file, 2,
+            login_method=partial(login_person, person), record_request=True)
+        self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
+
+        response = get_urls()
+        self.assertEqual(200, response.status)
+        info = response.jsonBody()
+        with person_logged_in(person):
+            expected_info = [{
+                "url": ProxiedLibraryFileAlias(
+                    sprf.libraryfile, spph.archive).http_url,
+                "size": sprf.libraryfile.content.filesize,
+                "sha256": sprf.libraryfile.content.sha256,
+                } for sprf in spph.sourcepackagerelease.files]
+        self.assertContentEqual(expected_info, info)
+
+
 class BinaryPackagePublishingHistoryWebserviceTests(TestCaseWithFactory):
 
     layer = LaunchpadFunctionalLayer
@@ -33,36 +97,48 @@
         person = self.factory.makePerson()
         webservice = webservice_for_person(
             person, permission=OAuthPermission.READ_PUBLIC)
+        bpph, url = self.make_bpph_for(person)
 
         response = webservice.named_get(
-            self.make_bpph_for(person)[1], 'binaryFileUrls',
-            api_version='devel')
+            url, 'binaryFileUrls', api_version='devel')
 
         self.assertEqual(200, response.status)
         urls = response.jsonBody()
-        self.assertEqual(1, len(urls))
-        self.assertTrue(urls[0], IsInstance(unicode))
+        with person_logged_in(person):
+            bpf = bpph.binarypackagerelease.files[0]
+            expected_urls = [
+                ProxiedLibraryFileAlias(
+                    bpf.libraryfile, bpph.archive).http_url]
+        self.assertEqual(expected_urls, urls)
 
     def test_binaryFileUrls_include_meta(self):
         person = self.factory.makePerson()
         webservice = webservice_for_person(
             person, permission=OAuthPermission.READ_PUBLIC)
-
         bpph, url = self.make_bpph_for(person)
-        query_counts = []
-        for i in range(3):
-            flush_database_caches()
-            with RequestTimelineCollector() as collector:
-                response = webservice.named_get(
-                    url, 'binaryFileUrls', include_meta=True,
-                    api_version='devel')
-            query_counts.append(collector.count)
-            with person_logged_in(person):
-                self.factory.makeBinaryPackageFile(
-                    binarypackagerelease=bpph.binarypackagerelease)
-        self.assertEqual(query_counts[0], query_counts[-1])
-
+
+        def create_file():
+            self.factory.makeBinaryPackageFile(
+                binarypackagerelease=bpph.binarypackagerelease)
+
+        def get_urls():
+            return webservice.named_get(
+                url, 'binaryFileUrls', include_meta=True, api_version='devel')
+
+        recorder1, recorder2 = record_two_runs(
+            get_urls, create_file, 2,
+            login_method=partial(login_person, person), record_request=True)
+        self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
+
+        response = get_urls()
         self.assertEqual(200, response.status)
-        urls = response.jsonBody()
-        self.assertEqual(3, len(urls))
-        self.assertThat(urls[0], IsInstance(dict))
+        info = response.jsonBody()
+        with person_logged_in(person):
+            expected_info = [{
+                "url": ProxiedLibraryFileAlias(
+                    bpf.libraryfile, bpph.archive).http_url,
+                "size": bpf.libraryfile.content.filesize,
+                "sha1": bpf.libraryfile.content.sha1,
+                "sha256": bpf.libraryfile.content.sha256,
+                } for bpf in bpph.binarypackagerelease.files]
+        self.assertContentEqual(expected_info, info)
 
=== modified file 'lib/lp/soyuz/interfaces/publishing.py'
--- lib/lp/soyuz/interfaces/publishing.py	2015-04-08 10:35:22 +0000
+++ lib/lp/soyuz/interfaces/publishing.py	2015-12-02 13:21:01 +0000
@@ -578,9 +578,13 @@
         """
 
     @export_read_operation()
-    def sourceFileUrls():
+    @operation_parameters(
+        include_meta=Bool(title=_("Include Metadata"), required=False))
+    def sourceFileUrls(include_meta=False):
         """URLs for this source publication's uploaded source files.
 
+        :param include_meta: Return a list of dicts with keys url, size, and
+            sha256 for each URL instead of a simple list.
         :return: A collection of URLs for this source.
         """
 
@@ -869,8 +873,8 @@
     def binaryFileUrls(include_meta=False):
         """URLs for this binary publication's binary files.
 
-        :param include_meta: Return a list of dicts with keys url, size
-            and sha1 for each url instead of a simple list.
+        :param include_meta: Return a list of dicts with keys url, size,
+            sha1, and sha256 for each URL instead of a simple list.
         :return: A collection of URLs for this binary.
         """
 
 
=== modified file 'lib/lp/soyuz/model/publishing.py'
--- lib/lp/soyuz/model/publishing.py	2015-07-08 16:05:11 +0000
+++ lib/lp/soyuz/model/publishing.py	2015-12-02 13:21:01 +0000
@@ -643,11 +643,23 @@
         return getUtility(
             IPublishingSet).getBuildStatusSummaryForSourcePublication(self)
 
-    def sourceFileUrls(self):
+    def sourceFileUrls(self, include_meta=False):
         """See `ISourcePackagePublishingHistory`."""
+        sources = Store.of(self).find(
+            (LibraryFileAlias, LibraryFileContent),
+            LibraryFileContent.id == LibraryFileAlias.contentID,
+            LibraryFileAlias.id == SourcePackageReleaseFile.libraryfileID,
+            SourcePackageReleaseFile.sourcepackagerelease ==
+                SourcePackageRelease.id,
+            SourcePackageRelease.id == self.sourcepackagereleaseID)
         source_urls = proxied_urls(
-            [file.libraryfile for file in self.sourcepackagerelease.files],
-             self.archive)
+            [source for source, _ in sources], self.archive)
+        if include_meta:
+            meta = [
+                (content.filesize, content.sha256) for _, content in sources]
+            return [
+                dict(url=url, size=size, sha256=sha256)
+                for url, (size, sha256) in zip(source_urls, meta)]
         return source_urls
 
     def binaryFileUrls(self):
@@ -1064,9 +1076,10 @@
             [binary for binary, _ in binaries], self.archive)
         if include_meta:
             meta = [
-                (content.filesize, content.sha1) for _, content in binaries]
-            return [dict(url=url, size=size, sha1=sha1)
-                for url, (size, sha1) in zip(binary_urls, meta)]
+                (content.filesize, content.sha1, content.sha256)
+                for _, content in binaries]
+            return [dict(url=url, size=size, sha1=sha1, sha256=sha256)
+                for url, (size, sha1, sha256) in zip(binary_urls, meta)]
         return binary_urls
 
 
 
=== modified file 'lib/lp/soyuz/tests/test_publishing_models.py'
--- lib/lp/soyuz/tests/test_publishing_models.py	2015-05-14 13:23:47 +0000
+++ lib/lp/soyuz/tests/test_publishing_models.py	2015-12-02 13:21:01 +0000
@@ -8,6 +8,7 @@
 
 from lp.app.errors import NotFoundError
 from lp.buildmaster.enums import BuildStatus
+from lp.registry.interfaces.sourcepackage import SourcePackageFileType
 from lp.services.database.constants import UTC_NOW
 from lp.services.librarian.browser import ProxiedLibraryFileAlias
 from lp.services.webapp.publisher import canonical_url
@@ -154,12 +155,68 @@
         spph = self.factory.makeSourcePackagePublishingHistory()
         self.assertRaises(NotFoundError, spph.getFileByName, 'not-changelog')
 
+    def getURLsForSPPH(self, spph, include_meta=False):
+        spr = spph.sourcepackagerelease
+        archive = spph.archive
+        urls = [ProxiedLibraryFileAlias(f.libraryfile, archive).http_url
+            for f in spr.files]
+
+        if include_meta:
+            meta = [(
+                f.libraryfile.content.filesize,
+                f.libraryfile.content.sha256,
+            ) for f in spr.files]
+
+            return [dict(url=url, size=size, sha256=sha256)
+                for url, (size, sha256) in zip(urls, meta)]
+        return urls
+
+    def makeSPPH(self, num_files=1):
+        archive = self.factory.makeArchive(private=False)
+        spr = self.factory.makeSourcePackageRelease(archive=archive)
+        filetypes = [
+            SourcePackageFileType.DSC, SourcePackageFileType.ORIG_TARBALL]
+        for count in range(num_files):
+            self.factory.makeSourcePackageReleaseFile(
+                sourcepackagerelease=spr, filetype=filetypes[count % 2])
+        return self.factory.makeSourcePackagePublishingHistory(
+            sourcepackagerelease=spr, archive=archive)
+
+    def test_sourceFileUrls_no_files(self):
+        spph = self.makeSPPH(num_files=0)
+
+        urls = spph.sourceFileUrls()
+
+        self.assertContentEqual([], urls)
+
+    def test_sourceFileUrls_one_file(self):
+        spph = self.makeSPPH(num_files=1)
+
+        urls = spph.sourceFileUrls()
+
+        self.assertContentEqual(self.getURLsForSPPH(spph), urls)
+
+    def test_sourceFileUrls_two_files(self):
+        spph = self.makeSPPH(num_files=2)
+
+        urls = spph.sourceFileUrls()
+
+        self.assertContentEqual(self.getURLsForSPPH(spph), urls)
+
+    def test_sourceFileUrls_include_meta(self):
+        spph = self.makeSPPH(num_files=2)
+
+        urls = spph.sourceFileUrls(include_meta=True)
+
+        self.assertContentEqual(
+            self.getURLsForSPPH(spph, include_meta=True), urls)
+
 
 class TestBinaryPackagePublishingHistory(TestCaseWithFactory):
 
     layer = LaunchpadFunctionalLayer
 
-    def get_urls_for_bpph(self, bpph, include_meta=False):
+    def getURLsForBPPH(self, bpph, include_meta=False):
         bpr = bpph.binarypackagerelease
         archive = bpph.archive
         urls = [ProxiedLibraryFileAlias(f.libraryfile, archive).http_url
@@ -169,13 +226,14 @@
             meta = [(
                 f.libraryfile.content.filesize,
                 f.libraryfile.content.sha1,
+                f.libraryfile.content.sha256,
             ) for f in bpr.files]
 
-            return [dict(url=url, size=size, sha1=sha1)
-                for url, (size, sha1) in zip(urls, meta)]
+            return [dict(url=url, size=size, sha1=sha1, sha256=sha256)
+                for url, (size, sha1, sha256) in zip(urls, meta)]
         return urls
 
-    def make_bpph(self, num_binaries=1):
+    def makeBPPH(self, num_binaries=1):
         archive = self.factory.makeArchive(private=False)
         bpr = self.factory.makeBinaryPackageRelease()
         filetypes = [BinaryPackageFileType.DEB, BinaryPackageFileType.DDEB]
@@ -186,40 +244,40 @@
             binarypackagerelease=bpr, archive=archive)
 
     def test_binaryFileUrls_no_binaries(self):
-        bpph = self.make_bpph(num_binaries=0)
+        bpph = self.makeBPPH(num_binaries=0)
 
         urls = bpph.binaryFileUrls()
 
         self.assertContentEqual([], urls)
 
     def test_binaryFileUrls_one_binary(self):
-        bpph = self.make_bpph(num_binaries=1)
+        bpph = self.makeBPPH(num_binaries=1)
 
         urls = bpph.binaryFileUrls()
 
-        self.assertContentEqual(self.get_urls_for_bpph(bpph), urls)
+        self.assertContentEqual(self.getURLsForBPPH(bpph), urls)
 
     def test_binaryFileUrls_two_binaries(self):
-        bpph = self.make_bpph(num_binaries=2)
+        bpph = self.makeBPPH(num_binaries=2)
 
         urls = bpph.binaryFileUrls()
 
-        self.assertContentEqual(self.get_urls_for_bpph(bpph), urls)
+        self.assertContentEqual(self.getURLsForBPPH(bpph), urls)
 
     def test_binaryFileUrls_include_meta(self):
-        bpph = self.make_bpph(num_binaries=2)
+        bpph = self.makeBPPH(num_binaries=2)
 
         urls = bpph.binaryFileUrls(include_meta=True)
 
         self.assertContentEqual(
-            self.get_urls_for_bpph(bpph, include_meta=True), urls)
+            self.getURLsForBPPH(bpph, include_meta=True), urls)
 
     def test_binaryFileUrls_removed(self):
         # binaryFileUrls returns URLs even if the files have been removed
         # from the published archive.
-        bpph = self.make_bpph(num_binaries=2)
-        expected_urls = self.get_urls_for_bpph(bpph)
-        expected_urls_meta = self.get_urls_for_bpph(bpph, include_meta=True)
+        bpph = self.makeBPPH(num_binaries=2)
+        expected_urls = self.getURLsForBPPH(bpph)
+        expected_urls_meta = self.getURLsForBPPH(bpph, include_meta=True)
         self.assertContentEqual(expected_urls, bpph.binaryFileUrls())
         self.assertContentEqual(
             expected_urls_meta, bpph.binaryFileUrls(include_meta=True))
 
=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py	2015-10-04 01:28:19 +0000
+++ lib/lp/testing/__init__.py	2015-12-02 13:21:01 +0000
@@ -425,7 +425,8 @@
 
 
 def record_two_runs(tested_method, item_creator, first_round_number,
-                    second_round_number=None, login_method=None):
+                    second_round_number=None, login_method=None,
+                    record_request=False):
     """A helper that returns the two storm statement recorders
     obtained when running tested_method after having run the
     method {item_creator} {first_round_number} times and then
@@ -435,9 +436,17 @@
     If {login_method} is not None, it is called before each batch of
     {item_creator} calls.
 
+    If {record_request} is True, `RequestTimelineCollector` is used to get
+    the query counts, so {tested_method} should make a web request.
+    Otherwise, `StormStatementRecorder` is used to get the query count.
+
     :return: a tuple containing the two recorders obtained by the successive
         runs.
     """
+    if record_request:
+        recorder_factory = RequestTimelineCollector
+    else:
+        recorder_factory = StormStatementRecorder
     if login_method is not None:
         login_method()
     for i in range(first_round_number):
@@ -449,7 +458,7 @@
     if queryInteraction() is not None:
         clear_permission_cache()
     getUtility(ILaunchpadCelebrities).clearCache()
-    with StormStatementRecorder() as recorder1:
+    with recorder_factory() as recorder1:
         tested_method()
     # Run {item_creator} {second_round_number} more times.
     if second_round_number is None:
@@ -463,7 +472,7 @@
     if queryInteraction() is not None:
         clear_permission_cache()
     getUtility(ILaunchpadCelebrities).clearCache()
-    with StormStatementRecorder() as recorder2:
+    with recorder_factory() as recorder2:
         tested_method()
     return recorder1, recorder2