Merge lp:~michael.nelson/launchpad/include-binary-size into lp:launchpad

[Michael Nelson]

Add optional include_sizes kwarg to BinaryPackagePublishingHistory.binaryFileUrls() and expose on API.

Add an optional include_sizes kwarg option to BPPH.binaryFileUrls(), defaulting to False to preserve current behaviour.

Brief pre-imp with wgrant discussed on bug 1088527.

./bin/test -vvct TestBinaryPackagePublishingHistory -t BinaryPackagePublishingHistoryWebserviceTests

I'll start the full test run on my instance.

[Robert Collins (lifeless)]

18 bpph_url = api_url(bpph)
19 +
20 + return bpph_url

I would just do
18 return api_url(bpph)

and skip the temporary variable and spurious VWS.

Line 27 has more spurious VWS. As does 33. I'll stop listing the spurious VWS here :). Please imagine it throughout the rest of the diff.

36 + self.assertEqual(1, len(urls))
37 + self.assertTrue(type(urls[0]) == unicode)
Would be better as
    self.assertThat(urls, HasLength(1))
    self.assertThat(urls[0], IsInstance(unicode))

ditto at 51

Other than that it looks ok to me.

[Michael Nelson (michael.nelson)]

r16523 adds functionality requested by cjwatson (including the sha1).

08:59 < cjwatson> I know wgrant said that adding hashes would be "probably overcomplicated", but cf. bug 1088527
08:59 < noodles775> Wow - 40mins from 6hrs.
08:59 < noodles775> Looking
08:59 < _mup_> Bug #1088527: Needlessly asks for exact package name, file size, uploaded version <ca-escalated> <Developer registration portal:New for michael.nelson> <Launchpad itself:In Progress by michael.nelson> < https://launchpad.net/bugs/1088527 >
09:00 < cjwatson> Err, sorry
09:00 < cjwatson> cf. bug 1007195
09:01 < cjwatson> is what I meant to say
09:01 < _mup_> Bug #1007195: Librarian-backed HostedFile objects do not expose SHA-1 hash <soyuz-build> <Launchpad itself:Triaged> < https://launchpad.net/bugs/1007195 >
09:01 < cjwatson> (Doesn't specifically have to be the SHA-1 of course; I think that was just what the librarian keyed off at the time or something
09:01 < cjwatson> )
09:02 < cjwatson> Anyway, don't let me get in the way by expanding the scope of your work, it was just in case it was trivial to allow for it
09:03 < noodles775> cjwatson: sure - it should be trivial. So s/include_sizes/include_meta/ and add the the sha1 to the dict?
09:07 < cjwatson> I guess something like that

[Michael Nelson (michael.nelson)]

Robert,

I removed the useless vars, and updated to use the IsInstance matcher, but although I see HasLength on the testtools docs, I don't see it in the LP code or the version currently used by LP:

http://paste.ubuntu.com/5572967/

so I've gone with assertThat(len(urls), Equals(1)) instead.

Thanks!

[William Grant (wgrant)]

I'd like to see a query count test, since you're now accessing more subordinate objects. It happens to be OK now because BPPH.files prejoins LFA/LFC, but a test like http://pastebin.ubuntu.com/5574906/ will ensure that performance doesn't regress.

review: Approve (code)

=== modified file 'lib/lp/soyuz/browser/tests/test_publishing_webservice.py'
--- lib/lp/soyuz/browser/tests/test_publishing_webservice.py	2012-01-01 02:58:52 +0000
+++ lib/lp/soyuz/browser/tests/test_publishing_webservice.py	2013-03-01 08:07:23 +0000
@@ -3,6 +3,7 @@
 
 """Test webservice methods related to the publisher."""
 
+from lp.services.database.sqlbase import flush_database_caches
 from lp.services.webapp.interfaces import OAuthPermission
 from lp.testing import (
     api_url,
@@ -11,24 +12,56 @@
     )
 from lp.testing.layers import LaunchpadFunctionalLayer
 from lp.testing.pages import webservice_for_person
+from lp.testing._webservice import QueryCollector
+from testtools.matchers import IsInstance
 
 
 class BinaryPackagePublishingHistoryWebserviceTests(TestCaseWithFactory):
 
     layer = LaunchpadFunctionalLayer
 
+    def make_bpph_for(self, person):
+        with person_logged_in(person):
+            bpr = self.factory.makeBinaryPackageRelease()
+            self.factory.makeBinaryPackageFile(binarypackagerelease=bpr)
+            bpph = self.factory.makeBinaryPackagePublishingHistory(
+                binarypackagerelease=bpr)
+            return bpph, api_url(bpph)
+
     def test_binaryFileUrls(self):
-        bpr = self.factory.makeBinaryPackageRelease()
-        self.factory.makeBinaryPackageFile(binarypackagerelease=bpr)
-        bpph = self.factory.makeBinaryPackagePublishingHistory(
-            binarypackagerelease=bpr)
         person = self.factory.makePerson()
         webservice = webservice_for_person(
             person, permission=OAuthPermission.READ_PUBLIC)
-        with person_logged_in(person):
-            bpph_url = api_url(bpph)
+
         response = webservice.named_get(
-            bpph_url, 'binaryFileUrls', api_version='devel')
+            self.make_bpph_for(person)[1], 'binaryFileUrls',
+            api_version='devel')
+
         self.assertEqual(200, response.status)
         urls = response.jsonBody()
         self.assertEqual(1, len(urls))
+        self.assertTrue(urls[0], IsInstance(unicode))
+
+    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 QueryCollector() 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] - 1, query_counts[-1])
+
+        self.assertEqual(200, response.status)
+        urls = response.jsonBody()
+        self.assertEqual(3, len(urls))
+        self.assertThat(urls[0], IsInstance(dict))

=== modified file 'lib/lp/soyuz/interfaces/publishing.py'
--- lib/lp/soyuz/interfaces/publishing.py	2013-02-20 12:28:38 +0000
+++ lib/lp/soyuz/interfaces/publishing.py	2013-03-01 08:07:23 +0000
@@ -946,10 +946,14 @@
         """
 
     @export_read_operation()
+    @operation_parameters(
+        include_meta=Bool(title=_("Include Metadata"), required=False))
     @operation_for_version("devel")
-    def binaryFileUrls():
+    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.
         :return: A collection of URLs for this binary.
         """
 

=== modified file 'lib/lp/soyuz/model/publishing.py'
--- lib/lp/soyuz/model/publishing.py	2013-02-20 12:30:47 +0000
+++ lib/lp/soyuz/model/publishing.py	2013-03-01 08:07:23 +0000
@@ -1375,10 +1375,17 @@
         """See `IPublishing`."""
         self.setDeleted(removed_by, removal_comment)
 
-    def binaryFileUrls(self):
+    def binaryFileUrls(self, include_meta=False):
         """See `IBinaryPackagePublishingHistory`."""
         binary_urls = proxied_urls(
             [f.libraryfilealias for f in self.files], self.archive)
+        if include_meta:
+            meta = [(
+                f.libraryfilealias.content.filesize,
+                f.libraryfilealias.content.sha1,
+            ) for f in self.files]
+            return [dict(url=url, size=size, sha1=sha1)
+                for url, (size, sha1) 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	2013-01-24 01:09:04 +0000
+++ lib/lp/soyuz/tests/test_publishing_models.py	2013-03-01 08:07:23 +0000
@@ -4,11 +4,9 @@
 """Test model and set utilities used for publishing."""
 
 from zope.component import getUtility
-from zope.security.proxy import removeSecurityProxy
 
 from lp.app.errors import NotFoundError
 from lp.buildmaster.enums import BuildStatus
-from lp.services.database.constants import UTC_NOW
 from lp.services.librarian.browser import ProxiedLibraryFileAlias
 from lp.services.webapp.publisher import canonical_url
 from lp.soyuz.enums import BinaryPackageFileType
@@ -154,34 +152,57 @@
 
     layer = LaunchpadFunctionalLayer
 
+    def get_urls_for_bpph(self, bpph, include_meta=False):
+        bpr = bpph.binarypackagerelease
+        archive = bpph.archive
+        urls = [ProxiedLibraryFileAlias(f.libraryfile, archive).http_url
+            for f in bpr.files]
+
+        if include_meta:
+            meta = [(
+                f.libraryfile.content.filesize,
+                f.libraryfile.content.sha1,
+            ) for f in bpr.files]
+
+            return [dict(url=url, size=size, sha1=sha1)
+                for url, (size, sha1) in zip(urls, meta)]
+        return urls
+
+    def make_bpph(self, num_binaries=1):
+        archive = self.factory.makeArchive(private=False)
+        bpr = self.factory.makeBinaryPackageRelease()
+        filetypes = [BinaryPackageFileType.DEB, BinaryPackageFileType.DDEB]
+        for count in range(num_binaries):
+            self.factory.makeBinaryPackageFile(binarypackagerelease=bpr,
+                                               filetype=filetypes[count % 2])
+        return self.factory.makeBinaryPackagePublishingHistory(
+            binarypackagerelease=bpr, archive=archive)
+
     def test_binaryFileUrls_no_binaries(self):
-        bpr = self.factory.makeBinaryPackageRelease()
-        bpph = self.factory.makeBinaryPackagePublishingHistory(
-            binarypackagerelease=bpr)
-        expected_urls = []
-        self.assertContentEqual(expected_urls, bpph.binaryFileUrls())
-
-    def get_urls_for_binarypackagerelease(self, bpr, archive):
-        return [ProxiedLibraryFileAlias(f.libraryfile, archive).http_url
-            for f in bpr.files]
+        bpph = self.make_bpph(num_binaries=0)
+
+        urls = bpph.binaryFileUrls()
+
+        self.assertContentEqual([], urls)
 
     def test_binaryFileUrls_one_binary(self):
-        archive = self.factory.makeArchive(private=False)
-        bpr = self.factory.makeBinaryPackageRelease()
-        self.factory.makeBinaryPackageFile(binarypackagerelease=bpr)
-        bpph = self.factory.makeBinaryPackagePublishingHistory(
-            binarypackagerelease=bpr, archive=archive)
-        expected_urls = self.get_urls_for_binarypackagerelease(bpr, archive)
-        self.assertContentEqual(expected_urls, bpph.binaryFileUrls())
+        bpph = self.make_bpph(num_binaries=1)
+
+        urls = bpph.binaryFileUrls()
+
+        self.assertContentEqual(self.get_urls_for_bpph(bpph), urls)
 
     def test_binaryFileUrls_two_binaries(self):
-        archive = self.factory.makeArchive(private=False)
-        bpr = self.factory.makeBinaryPackageRelease()
-        self.factory.makeBinaryPackageFile(
-            binarypackagerelease=bpr, filetype=BinaryPackageFileType.DEB)
-        self.factory.makeBinaryPackageFile(
-            binarypackagerelease=bpr, filetype=BinaryPackageFileType.DDEB)
-        bpph = self.factory.makeBinaryPackagePublishingHistory(
-            binarypackagerelease=bpr, archive=archive)
-        expected_urls = self.get_urls_for_binarypackagerelease(bpr, archive)
-        self.assertContentEqual(expected_urls, bpph.binaryFileUrls())
+        bpph = self.make_bpph(num_binaries=2)
+
+        urls = bpph.binaryFileUrls()
+
+        self.assertContentEqual(self.get_urls_for_bpph(bpph), urls)
+
+    def test_binaryFileUrls_include_meta(self):
+        bpph = self.make_bpph(num_binaries=2)
+
+        urls = bpph.binaryFileUrls(include_meta=True)
+
+        self.assertContentEqual(
+            self.get_urls_for_bpph(bpph, include_meta=True), urls)