launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/queue-api-fix-urls into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1006173 in Launchpad itself: "Queue tool requires direct DB access"
  https://bugs.launchpad.net/launchpad/+bug/1006173

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/queue-api-fix-urls/+merge/113776

== 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.
-- 
https://code.launchpad.net/~cjwatson/launchpad/queue-api-fix-urls/+merge/113776
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/queue-api-fix-urls into lp:launchpad.

=== 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-06 17:10:27 +0000
@@ -259,8 +259,7 @@
         """See `IPackageUpload`."""
         if self.contains_source:
             return [
-                ProxiedLibraryFileAlias(
-                    file.libraryfile, self.archive).http_url
+                file.libraryfile.getURL()
                 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]
@@ -313,10 +312,7 @@
 
     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`."""

=== 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-06 17:10:27 +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."""
 
@@ -1031,10 +1045,12 @@
         self.assertNotEqual(0, len(ws_source_file_urls))
         with person_logged_in(person):
             source_file_urls = [
-                ProxiedLibraryFileAlias(
-                    file.libraryfile, upload.archive).http_url
+                file.libraryfile.getURL()
                 for file in upload.sourcepackagerelease.files]
         self.assertContentEqual(source_file_urls, ws_source_file_urls)
+        for ws_source_file_url in ws_source_file_urls:
+            urlopen(ws_source_file_url).close()
+        urlopen(ws_upload.changes_file_url).close()
 
     def test_overrideSource_limited_component_permissions(self):
         # Overriding between two components requires queue admin of both.
@@ -1113,12 +1129,26 @@
         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]
+            email = str(person.preferredemail.email)
         self.assertContentEqual(binary_file_urls, ws_binary_file_urls)
 
+        # 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
+        browser.addHeader("Authorization", "Basic %s:test" % email)
+        for ws_binary_file_url in ws_binary_file_urls:
+            redirection = self.assertRaises(
+                HTTPError, browser.open, ws_binary_file_url)
+            self.assertEqual(303, redirection.code)
+            urlopen(redirection.hdrs["Location"]).close()
+        urlopen(ws_upload.changes_file_url).close()
+
     def test_overrideBinaries_limited_component_permissions(self):
         # Overriding between two components requires queue admin of both.
         person = self.makeQueueAdmin([self.universe])
@@ -1257,10 +1287,11 @@
         self.assertNotEqual(0, len(ws_custom_file_urls))
         with person_logged_in(person):
             custom_file_urls = [
-                ProxiedLibraryFileAlias(
-                    file.libraryfilealias, upload.archive).http_url
-                for file in upload.customfiles]
+                file.libraryfilealias.getURL() for file in upload.customfiles]
         self.assertContentEqual(custom_file_urls, ws_custom_file_urls)
+        for ws_custom_file_url in ws_custom_file_urls:
+            urlopen(ws_custom_file_url).close()
+        urlopen(ws_upload.changes_file_url).close()
 
     def test_binary_and_custom_info(self):
         # API clients can inspect properties of uploads containing both