Merge lp:~cjwatson/launchpad/queue-api-fix-urls into lp:launchpad

[Colin Watson]

Fix source, binary, and custom URLs returned by PackageUpload methods.

== Summary ==

Following the deployment of https://code.launchpad.net/~cjwatson/launchpad/queue-api-readonly/+merge/113202, I noticed that "queue fetch" did not work properly: changes files could be retrieved, but everything else resulted in error pages.

== Proposed fix ==

Source and custom files should be served directly from the librarian rather than being proxied through an Archive context. Archive.getFileByName cannot serve source files that do not have an associated SourcePackagePublishingHistory (true for most interesting files in upload queues; they only get an SPPH once they're accepted) and cannot serve custom files at all.

Binary files should be served from a BinaryPackageBuild context, not Archive. This matches how they're served from +build pages (e.g. https://launchpad.net/ubuntu/+source/libmath-tamuanova-perl/1.0.2-1ubuntu1/+build/3630878).

This time, rather than just having the tests check the URLs which obviously wasn't good enough, I arranged for them to actually try to open the files.

== LOC Rationale ==

+27. So far, including this branch, this arc of work has come in at +805. The final branch of the arc will remove the queue tool, all its works, and all its empty promises, currently amounting to -1955. This is therefore very comfortably ahead, even if I find more things I need to fix after this.

== Tests ==

bin/test -vvct TestPackageUploadWebservice

== Demo and Q/A ==

The queue API client is now in lp:ubuntu-archive-tools: use it to fetch source, binary, and custom files, and make sure they come back as real files and not error pages.

[Steve Kowalik (stevenk)]

review: Approve (code)

=== modified file 'lib/lp/soyuz/browser/configure.zcml'
--- lib/lp/soyuz/browser/configure.zcml	2011-12-24 17:49:30 +0000
+++ lib/lp/soyuz/browser/configure.zcml	2012-07-09 13:05:29 +0000
@@ -802,6 +802,10 @@
         path_expression="string:+upload/${id}"
         attribute_to_parent="distroseries"
         />
+    <browser:navigation
+        module="lp.soyuz.browser.queue"
+        classes="PackageUploadNavigation"
+        />
     <browser:url
         for="lp.soyuz.interfaces.archivedependency.IArchiveDependency"
         path_expression="string:+dependency/${dependency/id}"

=== modified file 'lib/lp/soyuz/browser/queue.py'
--- lib/lp/soyuz/browser/queue.py	2012-07-06 16:17:57 +0000
+++ lib/lp/soyuz/browser/queue.py	2012-07-09 13:05:29 +0000
@@ -6,6 +6,7 @@
 __metaclass__ = type
 
 __all__ = [
+    'PackageUploadNavigation',
     'QueueItemsView',
     ]
 
@@ -26,7 +27,11 @@
     load_related,
     )
 from lp.services.job.model.job import Job
-from lp.services.webapp import LaunchpadView
+from lp.services.librarian.browser import FileNavigationMixin
+from lp.services.webapp import (
+    GetitemNavigation,
+    LaunchpadView,
+    )
 from lp.services.webapp.authorization import check_permission
 from lp.services.webapp.batching import BatchNavigator
 from lp.soyuz.enums import (
@@ -458,6 +463,10 @@
         return (priority for priority in PackagePublishingPriority)
 
 
+class PackageUploadNavigation(GetitemNavigation, FileNavigationMixin):
+    usedfor = IPackageUpload
+
+
 class CompletePackageUpload:
     """A decorated `PackageUpload` including sources, builds and packages.
 

=== modified file 'lib/lp/soyuz/configure.zcml'
--- lib/lp/soyuz/configure.zcml	2012-07-04 13:02:32 +0000
+++ lib/lp/soyuz/configure.zcml	2012-07-09 13:05:29 +0000
@@ -168,6 +168,7 @@
                 custom_file_urls
                 customFileUrls
                 getBinaryProperties
+                getFileByName
                 date_created
                 sourcepackagerelease
                 component_name

=== modified file 'lib/lp/soyuz/interfaces/queue.py'
--- lib/lp/soyuz/interfaces/queue.py	2012-07-04 17:08:56 +0000
+++ lib/lp/soyuz/interfaces/queue.py	2012-07-09 13:05:29 +0000
@@ -301,6 +301,23 @@
             single binary.
         """
 
+    def getFileByName(filename):
+        """Return the corresponding `ILibraryFileAlias` in this context.
+
+        The following file types (and extension) can be looked up in the
+        PackageUpload context:
+
+         * Changes files: '.changes';
+         * Source files: '.orig.tar.gz', 'tar.gz', '.diff.gz' and '.dsc'.
+         * Custom files: '.tar.gz'.
+
+        :param filename: the exact filename to be looked up.
+
+        :raises NotFoundError if no file could be found.
+
+        :return the corresponding `ILibraryFileAlias` if the file was found.
+        """
+
     def setNew():
         """Set queue state to NEW."""
 

=== modified file 'lib/lp/soyuz/interfaces/sourcepackagerelease.py'
--- lib/lp/soyuz/interfaces/sourcepackagerelease.py	2012-07-05 09:04:09 +0000
+++ lib/lp/soyuz/interfaces/sourcepackagerelease.py	2012-07-09 13:05:29 +0000
@@ -170,6 +170,21 @@
         in this package.
         """
 
+    def getFileByName(filename):
+        """Return the corresponding `ILibraryFileAlias` in this context.
+
+        The following file types (and extension) can be looked up in the
+        SourcePackageRelease context:
+
+         * Source files: '.orig.tar.gz', 'tar.gz', '.diff.gz' and '.dsc'.
+
+        :param filename: the exact filename to be looked up.
+
+        :raises NotFoundError if no file could be found.
+
+        :return the corresponding `ILibraryFileAlias` if the file was found.
+        """
+
     def createBuild(distroarchseries, pocket, archive, processor=None,
                     status=None):
         """Create a build for a given distroarchseries/pocket/archive

=== modified file 'lib/lp/soyuz/model/queue.py'
--- lib/lp/soyuz/model/queue.py	2012-07-06 16:17:57 +0000
+++ lib/lp/soyuz/model/queue.py	2012-07-09 13:05:29 +0000
@@ -259,8 +259,7 @@
         """See `IPackageUpload`."""
         if self.contains_source:
             return [
-                ProxiedLibraryFileAlias(
-                    file.libraryfile, self.archive).http_url
+                ProxiedLibraryFileAlias(file.libraryfile, self).http_url
                 for file in self.sourcepackagerelease.files]
         else:
             return []
@@ -272,7 +271,7 @@
     def binaryFileUrls(self):
         """See `IPackageUpload`."""
         return [
-            ProxiedLibraryFileAlias(file.libraryfile, self.archive).http_url
+            ProxiedLibraryFileAlias(file.libraryfile, build.build).http_url
             for build in self.builds
             for bpr in build.build.binarypackages
             for file in bpr.files]
@@ -280,7 +279,7 @@
     @property
     def changes_file_url(self):
         if self.changesfile is not None:
-            return self.changesfile.getURL()
+            return ProxiedLibraryFileAlias(self.changesfile, self).http_url
         else:
             return None
 
@@ -309,14 +308,12 @@
     def custom_file_urls(self):
         """See `IPackageUpload`."""
         return tuple(
-            file.libraryfilealias.getURL() for file in self.customfiles)
+            ProxiedLibraryFileAlias(file.libraryfilealias, self).http_url
+            for file in self.customfiles)
 
     def customFileUrls(self):
         """See `IPackageUpload`."""
-        return [
-            ProxiedLibraryFileAlias(
-                file.libraryfilealias, self.archive).http_url
-            for file in self.customfiles]
+        return self.custom_file_urls
 
     def getBinaryProperties(self):
         """See `IPackageUpload`."""
@@ -329,6 +326,29 @@
                 })
         return properties
 
+    def getFileByName(self, filename):
+        """See `IPackageUpload`."""
+        if (self.changesfile is not None and
+            self.changesfile.filename == filename):
+            return self.changesfile
+
+        if self.sourcepackagerelease is not None:
+            try:
+                return self.sourcepackagerelease.getFileByName(filename)
+            except NotFoundError:
+                pass
+
+        custom = Store.of(self).find(
+            PackageUploadCustom,
+            PackageUploadCustom.packageupload == self.id,
+            LibraryFileAlias.id ==
+                PackageUploadCustom.libraryfilealiasID,
+            LibraryFileAlias.filename == filename).one()
+        if custom is not None:
+            return custom.libraryfilealias
+
+        raise NotFoundError(filename)
+
     def setNew(self):
         """See `IPackageUpload`."""
         if self.status == PackageUploadStatus.NEW:

=== modified file 'lib/lp/soyuz/model/sourcepackagerelease.py'
--- lib/lp/soyuz/model/sourcepackagerelease.py	2012-07-05 09:06:48 +0000
+++ lib/lp/soyuz/model/sourcepackagerelease.py	2012-07-09 13:05:29 +0000
@@ -38,6 +38,7 @@
 from zope.component import getUtility
 from zope.interface import implements
 
+from lp.app.errors import NotFoundError
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.archiveuploader.utils import determine_source_file_type
 from lp.buildmaster.enums import BuildStatus
@@ -309,6 +310,18 @@
             filetype=determine_source_file_type(file.filename),
             libraryfile=file)
 
+    def getFileByName(self, filename):
+        """See `ISourcePackageRelease`."""
+        sprf = Store.of(self).find(
+            SourcePackageReleaseFile,
+            SourcePackageReleaseFile.sourcepackagerelease == self.id,
+            LibraryFileAlias.id == SourcePackageReleaseFile.libraryfileID,
+            LibraryFileAlias.filename == filename).one()
+        if sprf:
+            return sprf.libraryfile
+        else:
+            raise NotFoundError(filename)
+
     def getPackageSize(self):
         """See ISourcePackageRelease."""
         size_query = """

=== modified file 'lib/lp/soyuz/tests/test_packageupload.py'
--- lib/lp/soyuz/tests/test_packageupload.py	2012-07-06 16:17:57 +0000
+++ lib/lp/soyuz/tests/test_packageupload.py	2012-07-09 13:05:29 +0000
@@ -7,6 +7,10 @@
 from email import message_from_string
 import os
 import shutil
+from urllib2 import (
+    HTTPError,
+    urlopen,
+    )
 
 from lazr.restfulclient.errors import (
     BadRequest,
@@ -17,6 +21,8 @@
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 from zope.schema import getFields
+from zope.testbrowser.browser import Browser
+from zope.testbrowser.testing import PublisherMechanizeBrowser
 
 from lp.archivepublisher.interfaces.publisherconfig import IPublisherConfigSet
 from lp.archiveuploader.tests import datadir
@@ -889,6 +895,14 @@
         self.assertEqual(PackageUploadStatus.REJECTED, pu.status)
 
 
+class NonRedirectingMechanizeBrowser(PublisherMechanizeBrowser):
+    """A `mechanize.Browser` that does not handle redirects."""
+
+    default_features = [
+        feature for feature in PublisherMechanizeBrowser.default_features
+        if feature != "_redirect"]
+
+
 class TestPackageUploadWebservice(TestCaseWithFactory):
     """Test the exposure of queue methods to the web service."""
 
@@ -958,6 +972,22 @@
         transaction.commit()
         return upload, self.load(upload, person)
 
+    def makeNonRedirectingBrowser(self, person):
+        # The test browser can only work with the appserver, not the
+        # librarian, so follow one layer of redirection through the
+        # appserver and then ask the librarian for the real file.
+        browser = Browser(mech_browser=NonRedirectingMechanizeBrowser())
+        browser.handleErrors = False
+        with person_logged_in(person):
+            browser.addHeader(
+                "Authorization", "Basic %s:test" % person.preferredemail.email)
+        return browser
+
+    def assertCanOpenRedirectedUrl(self, browser, url):
+        redirection = self.assertRaises(HTTPError, browser.open, url)
+        self.assertEqual(303, redirection.code)
+        urlopen(redirection.hdrs["Location"]).close()
+
     def assertRequiresEdit(self, method_name, **kwargs):
         """Test that a web service queue method requires launchpad.Edit."""
         with admin_logged_in():
@@ -1027,15 +1057,20 @@
         person = self.makeQueueAdmin([self.universe])
         upload, ws_upload = self.makeSourcePackageUpload(
             person, component=self.universe)
+
         ws_source_file_urls = ws_upload.sourceFileUrls()
         self.assertNotEqual(0, len(ws_source_file_urls))
         with person_logged_in(person):
             source_file_urls = [
-                ProxiedLibraryFileAlias(
-                    file.libraryfile, upload.archive).http_url
+                ProxiedLibraryFileAlias(file.libraryfile, upload).http_url
                 for file in upload.sourcepackagerelease.files]
         self.assertContentEqual(source_file_urls, ws_source_file_urls)
 
+        browser = self.makeNonRedirectingBrowser(person)
+        for ws_source_file_url in ws_source_file_urls:
+            self.assertCanOpenRedirectedUrl(browser, ws_source_file_url)
+        self.assertCanOpenRedirectedUrl(browser, ws_upload.changes_file_url)
+
     def test_overrideSource_limited_component_permissions(self):
         # Overriding between two components requires queue admin of both.
         person = self.makeQueueAdmin([self.universe])
@@ -1113,12 +1148,17 @@
         self.assertNotEqual(0, len(ws_binary_file_urls))
         with person_logged_in(person):
             binary_file_urls = [
-                ProxiedLibraryFileAlias(
-                    file.libraryfile, upload.archive).http_url
-                for bpr in upload.builds[0].build.binarypackages
+                ProxiedLibraryFileAlias(file.libraryfile, build.build).http_url
+                for build in upload.builds
+                for bpr in build.build.binarypackages
                 for file in bpr.files]
         self.assertContentEqual(binary_file_urls, ws_binary_file_urls)
 
+        browser = self.makeNonRedirectingBrowser(person)
+        for ws_binary_file_url in ws_binary_file_urls:
+            self.assertCanOpenRedirectedUrl(browser, ws_binary_file_url)
+        self.assertCanOpenRedirectedUrl(browser, ws_upload.changes_file_url)
+
     def test_overrideBinaries_limited_component_permissions(self):
         # Overriding between two components requires queue admin of both.
         person = self.makeQueueAdmin([self.universe])
@@ -1253,15 +1293,20 @@
         upload, ws_upload = self.makeCustomPackageUpload(
             person, custom_type=PackageUploadCustomFormat.DEBIAN_INSTALLER,
             filename="debian-installer-images_1.tar.gz")
+
         ws_custom_file_urls = ws_upload.customFileUrls()
         self.assertNotEqual(0, len(ws_custom_file_urls))
         with person_logged_in(person):
             custom_file_urls = [
-                ProxiedLibraryFileAlias(
-                    file.libraryfilealias, upload.archive).http_url
+                ProxiedLibraryFileAlias(file.libraryfilealias, upload).http_url
                 for file in upload.customfiles]
         self.assertContentEqual(custom_file_urls, ws_custom_file_urls)
 
+        browser = self.makeNonRedirectingBrowser(person)
+        for ws_custom_file_url in ws_custom_file_urls:
+            self.assertCanOpenRedirectedUrl(browser, ws_custom_file_url)
+        self.assertCanOpenRedirectedUrl(browser, ws_upload.changes_file_url)
+
     def test_binary_and_custom_info(self):
         # API clients can inspect properties of uploads containing both
         # ordinary binaries and custom upload files.