Merge lp:~cjwatson/launchpad/queue-api-overrides into lp:launchpad

[Colin Watson]

Export PackageUpload.overrideSource and PackageUpload.overrideBinaries.

== Summary ==

Replace the queue tool in Launchpad with an API client (part five).

== Proposed fix ==

This branch exports the override methods on PackageUpload. With this in addition to what's already landed, the client should be feature-complete, and the remaining work should consist of cleanups.

== Pre-implementation notes ==

I've gone round a few times with various people, particularly William Grant, on the exact way to export all of this stuff, because I gather that we want to avoid exposing the current data model in order that it can be rearranged in the future. This has led to the following design choices:

 * Everything is on devel. The only clients for this should be tools such as those in lp:ubuntu-archive-tools, which can be kept up to date if there's a need to change these interfaces.
 * There are source packages with lots of binaries that sometimes need to be overridden individually (e.g. linux) and API requests aren't especially fast. In a previous branch (currently QAed and awaiting deployment) I amended Archive.overrideBinaries to take a similar list of dicts as a "changes" parameter, allowing many override changes to be made in a single request; this exports that and improves test coverage.

I extracted this branch from https://code.launchpad.net/~cjwatson/launchpad/queue-api/+merge/108967 at Benji's request.

== Implementation details ==

The only thing I think is notable here is that I copied the new-component-requires-new-archive check from lib/lp/soyuz/scripts/queue.py into PackageUpload.overrideBinaries. This should be acceptable temporary duplication since: (a) it really belongs in the model anyway; (b) lp.soyuz.scripts.queue calls BPR.override directly rather than going through PackageUpload.overrideBinaries, so the check doesn't happen twice at run-time; (c) I will be removing lp.soyuz.scripts.queue soon so there's no point bothering to refactor it.

== LOC Rationale ==

+218. As with https://code.launchpad.net/~cjwatson/launchpad/queue-api/+merge/108967, this arc will be LoC-negative.

== Tests ==

bin/test -vvct test_packageupload

== Demo and Q/A ==

http://paste.ubuntu.com/1072964/ is the current version of my client. With this branch, it should be possible to override sources and (individual) binaries in queue items.

[j.c.sackett (jcsackett)]

This looks good, thanks for breaking it out into smaller branches.

review: Approve

=== modified file 'lib/lp/soyuz/browser/queue.py'
--- lib/lp/soyuz/browser/queue.py	2012-07-05 09:49:55 +0000
+++ lib/lp/soyuz/browser/queue.py	2012-07-06 16:20:24 +0000
@@ -45,6 +45,7 @@
 from lp.soyuz.interfaces.queue import (
     IPackageUpload,
     IPackageUploadSet,
+    QueueAdminUnauthorizedError,
     QueueInconsistentStateError,
     )
 from lp.soyuz.interfaces.section import ISectionSet
@@ -388,7 +389,8 @@
                     }]
                 binary_overridden = queue_item.overrideBinaries(
                     binary_changes, allowed_components)
-            except QueueInconsistentStateError as info:
+            except (QueueAdminUnauthorizedError,
+                    QueueInconsistentStateError) as info:
                 failure.append("FAILED: %s (%s)" %
                                (queue_item.displayname, info))
                 continue
@@ -409,7 +411,8 @@
 
             try:
                 getattr(self, 'queue_action_' + action)(queue_item)
-            except QueueInconsistentStateError as info:
+            except (QueueAdminUnauthorizedError,
+                    QueueInconsistentStateError) as info:
                 failure.append('FAILED: %s (%s)' %
                                (queue_item.displayname, info))
             else:

=== modified file 'lib/lp/soyuz/doc/distroseriesqueue.txt'
--- lib/lp/soyuz/doc/distroseriesqueue.txt	2012-07-03 09:29:35 +0000
+++ lib/lp/soyuz/doc/distroseriesqueue.txt	2012-07-06 16:20:24 +0000
@@ -648,7 +648,7 @@
 In addition to these parameters, you must also supply
 "allowed_components", which is a sequence of IComponent.  Any overrides
 must have the existing and new component in this sequence otherwise
-QueueInconsistentStateError is raised.
+QueueAdminUnauthorizedError is raised.
 
 The alsa-utils source is already in the queue with component "main"
 and section "base".
@@ -673,7 +673,7 @@
     ...     allowed_components=(universe,))
     Traceback (most recent call last):
     ...
-    QueueInconsistentStateError: No rights to override to restricted
+    QueueAdminUnauthorizedError: No rights to override to restricted
 
 Allowing "restricted" still won't work because the original component
 is "main":
@@ -683,7 +683,7 @@
     ...     allowed_components=(restricted,))
     Traceback (most recent call last):
     ...
-    QueueInconsistentStateError: No rights to override from main
+    QueueAdminUnauthorizedError: No rights to override from main
 
 Specifying both main and restricted allows the override to restricted/web.
 overrideSource() returns True if it completed the task.
@@ -719,13 +719,13 @@
     ...     binary_changes, allowed_components=(universe,))
     Traceback (most recent call last):
     ...
-    QueueInconsistentStateError: No rights to override to restricted
+    QueueAdminUnauthorizedError: No rights to override to restricted
 
     >>> print item.overrideBinaries(
     ...     binary_changes, allowed_components=(restricted,))
     Traceback (most recent call last):
     ...
-    QueueInconsistentStateError: No rights to override from main
+    QueueAdminUnauthorizedError: No rights to override from main
 
     >>> print item.overrideBinaries(
     ...     binary_changes, allowed_components=(main,restricted))

=== modified file 'lib/lp/soyuz/interfaces/queue.py'
--- lib/lp/soyuz/interfaces/queue.py	2012-07-04 13:02:32 +0000
+++ lib/lp/soyuz/interfaces/queue.py	2012-07-06 16:20:24 +0000
@@ -16,6 +16,7 @@
     'IPackageUploadCustom',
     'IPackageUploadSet',
     'NonBuildableSourceUploadError',
+    'QueueAdminUnauthorizedError',
     'QueueBuildAcceptError',
     'QueueInconsistentStateError',
     'QueueSourceAcceptError',
@@ -26,12 +27,15 @@
 
 from lazr.enum import DBEnumeratedType
 from lazr.restful.declarations import (
+    call_with,
     error_status,
     export_as_webservice_entry,
     export_read_operation,
     export_write_operation,
     exported,
     operation_for_version,
+    operation_parameters,
+    REQUEST_USER,
     )
 from lazr.restful.fields import Reference
 from zope.interface import (
@@ -42,10 +46,12 @@
     Bool,
     Choice,
     Datetime,
+    Dict,
     Int,
     List,
     TextLine,
     )
+from zope.security.interfaces import Unauthorized
 
 from lp import _
 from lp.soyuz.enums import PackageUploadStatus
@@ -69,6 +75,10 @@
     """
 
 
+class QueueAdminUnauthorizedError(Unauthorized):
+    """User not permitted to perform a queue administration operation."""
+
+
 class NonBuildableSourceUploadError(QueueInconsistentStateError):
     """Source upload will not result in any build record.
 
@@ -394,7 +404,14 @@
         :param logger: Specify a logger object if required.  Mainly for tests.
         """
 
-    def overrideSource(new_component, new_section, allowed_components):
+    @operation_parameters(
+        new_component=TextLine(title=u"The new component name."),
+        new_section=TextLine(title=u"The new section name."))
+    @call_with(allowed_components=None, user=REQUEST_USER)
+    @export_write_operation()
+    @operation_for_version('devel')
+    def overrideSource(new_component=None, new_section=None,
+                       allowed_components=None, user=None):
         """Override the source package contained in this queue item.
 
         :param new_component: An IComponent to replace the existing one
@@ -403,6 +420,8 @@
             in the upload's source.
         :param allowed_components: A sequence of components that the
             callsite is allowed to override from and to.
+        :param user: The user requesting the override change, used if
+            allowed_components is None.
 
         :raises QueueInconsistentStateError: if either the existing
             or the new_component are not in the allowed_components
@@ -414,7 +433,21 @@
         :return: True if the source was overridden.
         """
 
-    def overrideBinaries(changes, allowed_components):
+    @operation_parameters(
+        changes=List(
+            title=u"A sequence of changes to apply.",
+            description=(
+                u"Each item may have a 'name' item which specifies the binary "
+                "package name to override; otherwise, the change applies to "
+                "all binaries in the upload. It may also have 'component', "
+                "'section', and 'priority' items which replace the "
+                "corresponding existing one in the upload's overridden "
+                "binaries."),
+            value_type=Dict(key_type=TextLine())))
+    @call_with(allowed_components=None, user=REQUEST_USER)
+    @export_write_operation()
+    @operation_for_version('devel')
+    def overrideBinaries(changes, allowed_components=None, user=None):
         """Override binary packages in a binary queue item.
 
         :param changes: A sequence of mappings of changes to apply. Each
@@ -426,6 +459,8 @@
             left unchanged.
         :param allowed_components: A sequence of components that the
             callsite is allowed to override from and to.
+        :param user: The user requesting the override change, used if
+            allowed_components is None.
 
         :raises QueueInconsistentStateError: if either the existing
             or the new_component are not in the allowed_components

=== modified file 'lib/lp/soyuz/model/queue.py'
--- lib/lp/soyuz/model/queue.py	2012-07-06 13:51:34 +0000
+++ lib/lp/soyuz/model/queue.py	2012-07-06 16:20:24 +0000
@@ -84,6 +84,7 @@
     PriorityNotFound,
     SectionNotFound,
     )
+from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet
 from lp.soyuz.interfaces.component import IComponentSet
 from lp.soyuz.interfaces.packagecopyjob import IPackageCopyJobSource
 from lp.soyuz.interfaces.publishing import (
@@ -99,6 +100,7 @@
     IPackageUploadSet,
     IPackageUploadSource,
     NonBuildableSourceUploadError,
+    QueueAdminUnauthorizedError,
     QueueBuildAcceptError,
     QueueInconsistentStateError,
     QueueSourceAcceptError,
@@ -1038,7 +1040,7 @@
         allowed_component_names = [
             component.name for component in allowed_components]
         if copy_job.component_name not in allowed_component_names:
-            raise QueueInconsistentStateError(
+            raise QueueAdminUnauthorizedError(
                 "No rights to override from %s" % copy_job.component_name)
         copy_job.addSourceOverride(SourceOverride(
             copy_job.package_name, new_component, new_section))
@@ -1055,7 +1057,7 @@
             if old_component not in allowed_components:
                 # The old component is not in the list of allowed components
                 # to override.
-                raise QueueInconsistentStateError(
+                raise QueueAdminUnauthorizedError(
                     "No rights to override from %s" % old_component.name)
             source.sourcepackagerelease.override(
                 component=new_component, section=new_section)
@@ -1068,7 +1070,8 @@
 
         return made_changes
 
-    def overrideSource(self, new_component, new_section, allowed_components):
+    def overrideSource(self, new_component=None, new_section=None,
+                       allowed_components=None, user=None):
         """See `IPackageUpload`."""
         if new_component is None and new_section is None:
             # Nothing needs overriding, bail out.
@@ -1077,8 +1080,19 @@
         new_component = self._nameToComponent(new_component)
         new_section = self._nameToSection(new_section)
 
+        if allowed_components is None and user is not None:
+            # Get a list of components for which the user has rights to
+            # override to or from.
+            permission_set = getUtility(IArchivePermissionSet)
+            permissions = permission_set.componentsForQueueAdmin(
+                self.distroseries.main_archive, user)
+            allowed_components = set(
+                permission.component for permission in permissions)
+        assert allowed_components is not None, (
+            "Must provide allowed_components for non-webservice calls.")
+
         if new_component not in list(allowed_components) + [None]:
-            raise QueueInconsistentStateError(
+            raise QueueAdminUnauthorizedError(
                 "No rights to override to %s" % new_component.name)
 
         return (
@@ -1113,7 +1127,7 @@
 
         return changes_by_name, changes_for_all
 
-    def overrideBinaries(self, changes, allowed_components):
+    def overrideBinaries(self, changes, allowed_components=None, user=None):
         """See `IPackageUpload`."""
         if not self.contains_build:
             return False
@@ -1122,6 +1136,17 @@
             # Nothing needs overriding, bail out.
             return False
 
+        if allowed_components is None and user is not None:
+            # Get a list of components for which the user has rights to
+            # override to or from.
+            permission_set = getUtility(IArchivePermissionSet)
+            permissions = permission_set.componentsForQueueAdmin(
+                self.distroseries.main_archive, user)
+            allowed_components = set(
+                permission.component for permission in permissions)
+        assert allowed_components is not None, (
+            "Must provide allowed_components for non-webservice calls.")
+
         changes_by_name, changes_for_all = self._filterBinaryChanges(changes)
 
         new_components = set()
@@ -1135,12 +1160,23 @@
             component.name
             for component in new_components.difference(allowed_components))
         if disallowed_components:
-            raise QueueInconsistentStateError(
+            raise QueueAdminUnauthorizedError(
                 "No rights to override to %s" %
                 ", ".join(disallowed_components))
 
         made_changes = False
         for build in self.builds:
+            # See if the new component requires a new archive on the build.
+            for component in new_components:
+                distroarchseries = build.build.distro_arch_series
+                distribution = distroarchseries.distroseries.distribution
+                new_archive = distribution.getArchiveByComponent(
+                    component.name)
+                if new_archive != build.build.archive:
+                    raise QueueInconsistentStateError(
+                        "Overriding component to '%s' failed because it "
+                        "would require a new archive." % component.name)
+
             for binarypackage in build.build.binarypackages:
                 change = changes_by_name.get(
                     binarypackage.name, changes_for_all)
@@ -1148,7 +1184,7 @@
                     if binarypackage.component not in allowed_components:
                         # The old component is not in the list of allowed
                         # components to override.
-                        raise QueueInconsistentStateError(
+                        raise QueueAdminUnauthorizedError(
                             "No rights to override from %s" %
                             binarypackage.component.name)
                     binarypackage.override(**change)

=== modified file 'lib/lp/soyuz/tests/test_packageupload.py'
--- lib/lp/soyuz/tests/test_packageupload.py	2012-07-05 13:01:48 +0000
+++ lib/lp/soyuz/tests/test_packageupload.py	2012-07-06 16:20:24 +0000
@@ -42,6 +42,7 @@
 from lp.soyuz.interfaces.queue import (
     IPackageUpload,
     IPackageUploadSet,
+    QueueAdminUnauthorizedError,
     QueueInconsistentStateError,
     )
 from lp.soyuz.interfaces.section import ISectionSet
@@ -444,8 +445,7 @@
         only_allowed_component = self.factory.makeComponent()
         section = self.factory.makeSection()
         self.assertRaises(
-            QueueInconsistentStateError,
-            pu.overrideSource,
+            QueueAdminUnauthorizedError, pu.overrideSource,
             only_allowed_component, section, [only_allowed_component])
 
     def test_overrideSource_checks_permission_for_new_component(self):
@@ -454,8 +454,7 @@
         disallowed_component = self.factory.makeComponent()
         section = self.factory.makeSection()
         self.assertRaises(
-            QueueInconsistentStateError,
-            pu.overrideSource,
+            QueueAdminUnauthorizedError, pu.overrideSource,
             disallowed_component, section, [current_component])
 
     def test_overrideSource_ignores_None_component_change(self):
@@ -971,6 +970,9 @@
     def test_edit_permissions(self):
         self.assertRequiresEdit("acceptFromQueue")
         self.assertRequiresEdit("rejectFromQueue")
+        self.assertRequiresEdit("overrideSource", new_component="main")
+        self.assertRequiresEdit(
+            "overrideBinaries", changes=[{"component": "main"}])
 
     def test_acceptFromQueue_archive_admin(self):
         # acceptFromQueue as an archive admin accepts the upload.
@@ -1034,6 +1036,45 @@
                 for file in upload.sourcepackagerelease.files]
         self.assertContentEqual(source_file_urls, ws_source_file_urls)
 
+    def test_overrideSource_limited_component_permissions(self):
+        # Overriding between two components requires queue admin of both.
+        person = self.makeQueueAdmin([self.universe])
+        upload, ws_upload = self.makeSourcePackageUpload(
+            person, component=self.universe)
+
+        self.assertEqual("New", ws_upload.status)
+        self.assertEqual("universe", ws_upload.component_name)
+        self.assertRaises(Unauthorized, ws_upload.overrideSource,
+                          new_component="main")
+
+        with admin_logged_in():
+            upload.overrideSource(
+                new_component=self.main,
+                allowed_components=[self.main, self.universe])
+        transaction.commit()
+        self.assertEqual("main", upload.component_name)
+        self.assertRaises(Unauthorized, ws_upload.overrideSource,
+                          new_component="universe")
+
+    def test_overrideSource_changes_properties(self):
+        # Running overrideSource changes the corresponding properties.
+        person = self.makeQueueAdmin([self.main, self.universe])
+        upload, ws_upload = self.makeSourcePackageUpload(
+            person, component=self.universe)
+        with person_logged_in(person):
+            new_section = self.factory.makeSection()
+        transaction.commit()
+
+        self.assertEqual("New", ws_upload.status)
+        self.assertEqual("universe", ws_upload.component_name)
+        self.assertNotEqual(new_section.name, ws_upload.section_name)
+        ws_upload.overrideSource(
+            new_component="main", new_section=new_section.name)
+        self.assertEqual("main", ws_upload.component_name)
+        self.assertEqual(new_section.name, ws_upload.section_name)
+        ws_upload.overrideSource(new_component="universe")
+        self.assertEqual("universe", ws_upload.component_name)
+
     def assertBinaryPropertiesMatch(self, arch, bpr, ws_binary):
         expected_binary = {
             "is_new": True,
@@ -1078,6 +1119,112 @@
                 for file in bpr.files]
         self.assertContentEqual(binary_file_urls, ws_binary_file_urls)
 
+    def test_overrideBinaries_limited_component_permissions(self):
+        # Overriding between two components requires queue admin of both.
+        person = self.makeQueueAdmin([self.universe])
+        upload, ws_upload = self.makeBinaryPackageUpload(
+            person, binarypackagename="hello", component=self.universe)
+
+        self.assertEqual("New", ws_upload.status)
+        self.assertEqual(
+            set(["universe"]),
+            set(binary["component"]
+                for binary in ws_upload.getBinaryProperties()))
+        self.assertRaises(
+            Unauthorized, ws_upload.overrideBinaries,
+            changes=[{"component": "main"}])
+
+        with admin_logged_in():
+            upload.overrideBinaries(
+                [{"component": self.main}],
+                allowed_components=[self.main, self.universe])
+        transaction.commit()
+
+        self.assertEqual(
+            set(["main"]),
+            set(binary["component"]
+                for binary in ws_upload.getBinaryProperties()))
+        self.assertRaises(
+            Unauthorized, ws_upload.overrideBinaries,
+            changes=[{"component": "universe"}])
+
+    def test_overrideBinaries_disallows_new_archive(self):
+        # overrideBinaries refuses to override the component to something
+        # that requires a different archive.
+        partner = self.factory.makeComponent("partner")
+        self.factory.makeComponentSelection(
+            distroseries=self.distroseries, component=partner)
+        person = self.makeQueueAdmin([self.universe, partner])
+        upload, ws_upload = self.makeBinaryPackageUpload(
+            person, component=self.universe)
+
+        self.assertEqual(
+            "universe", ws_upload.getBinaryProperties()[0]["component"])
+        self.assertRaises(
+            BadRequest, ws_upload.overrideBinaries,
+            changes=[{"component": "partner"}])
+
+    def test_overrideBinaries_without_name_changes_all_properties(self):
+        # Running overrideBinaries with a change entry containing no "name"
+        # field changes the corresponding properties of all binaries.
+        person = self.makeQueueAdmin([self.main, self.universe])
+        upload, ws_upload = self.makeBinaryPackageUpload(
+            person, component=self.universe)
+        with person_logged_in(person):
+            new_section = self.factory.makeSection()
+        transaction.commit()
+
+        self.assertEqual("New", ws_upload.status)
+        for binary in ws_upload.getBinaryProperties():
+            self.assertEqual("universe", binary["component"])
+            self.assertNotEqual(new_section.name, binary["section"])
+            self.assertEqual("OPTIONAL", binary["priority"])
+        changes = [{
+            "component": "main",
+            "section": new_section.name,
+            "priority": "extra",
+            }]
+        ws_upload.overrideBinaries(changes=changes)
+        for binary in ws_upload.getBinaryProperties():
+            self.assertEqual("main", binary["component"])
+            self.assertEqual(new_section.name, binary["section"])
+            self.assertEqual("EXTRA", binary["priority"])
+
+    def test_overrideBinaries_with_name_changes_selected_properties(self):
+        # Running overrideBinaries with change entries containing "name"
+        # fields changes the corresponding properties of only the selected
+        # binaries.
+        person = self.makeQueueAdmin([self.main, self.universe])
+        upload, ws_upload = self.makeBinaryPackageUpload(
+            person, component=self.universe)
+        with person_logged_in(person):
+            new_section = self.factory.makeSection()
+        transaction.commit()
+
+        self.assertEqual("New", ws_upload.status)
+        ws_binaries = ws_upload.getBinaryProperties()
+        for binary in ws_binaries:
+            self.assertEqual("universe", binary["component"])
+            self.assertNotEqual(new_section.name, binary["section"])
+            self.assertEqual("OPTIONAL", binary["priority"])
+        change_one = {
+            "name": ws_binaries[0]["name"],
+            "component": "main",
+            "priority": "standard",
+            }
+        change_two = {
+            "name": ws_binaries[1]["name"],
+            "section": new_section.name,
+            }
+        ws_upload.overrideBinaries(changes=[change_one, change_two])
+        ws_binaries = ws_upload.getBinaryProperties()
+        self.assertEqual("main", ws_binaries[0]["component"])
+        self.assertNotEqual(new_section.name, ws_binaries[0]["section"])
+        self.assertEqual("STANDARD", ws_binaries[0]["priority"])
+        self.assertEqual("universe", ws_binaries[1]["component"])
+        self.assertEqual(new_section.name, ws_binaries[1]["section"])
+        self.assertEqual("OPTIONAL", ws_binaries[1]["priority"])
+
     def test_custom_info(self):
         # API clients can inspect properties of custom uploads.
         person = self.makeQueueAdmin([self.universe])