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

[Colin Watson]

Refactor PackageUpload.overrideSource and PackageUpload.overrideBinaries interfaces in preparation for exporting them on the webservice.

== Summary ==

The PackageUpload.overrideBinaries interface will be unacceptably slow when exported on the API.

The PackageUpload.overrideSource and PackageUpload.overrideBinaries interfaces will need to accept strings as well as component/section/priority objects once exported on the API.

== Proposed fix ==

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. I've therefore 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.

== Pre-implementation notes ==

Override design based on conversations with William Grant.

This is split out of a larger branch, https://code.launchpad.net/~cjwatson/launchpad/queue-api/+merge/108967, at Benji's suggestion in an attempt to make it more easily reviewable.

== LOC Rationale ==

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

== Tests ==

bin/test -vvct nascentupload-ddebs.txt -t distroseriesqueue.txt

Test coverage should be sufficient to avoid regressions, but will nevertheless be improved by a follow-up branch to export things on the webservice.

[Benji York (benji)]

This looks good. I like the way the data for the changes is
packaged/unpacked.

The only thing I spotted were some extra parens:

    raise QueueInconsistentStateError(
        "No rights to override from %s" % (
            binarypackage.component.name))

Since the % expression is already in parenthesis, those around
"binarypackage.component.name" are unnecessary.

review: Approve (code)

=== modified file 'lib/lp/archiveuploader/tests/nascentupload-ddebs.txt'
--- lib/lp/archiveuploader/tests/nascentupload-ddebs.txt	2012-01-20 16:11:11 +0000
+++ lib/lp/archiveuploader/tests/nascentupload-ddebs.txt	2012-07-03 09:34:24 +0000
@@ -89,7 +89,8 @@
 
     >>> switch_dbuser('launchpad')
 
-    >>> bin.queue_root.overrideBinaries(main, devel, None, [main, universe])
+    >>> bin.queue_root.overrideBinaries(
+    ...     [{"component": main, "section": devel}], [main, universe])
     True
     >>> bin.queue_root.acceptFromQueue()
 

=== modified file 'lib/lp/soyuz/browser/queue.py'
--- lib/lp/soyuz/browser/queue.py	2012-06-29 08:40:05 +0000
+++ lib/lp/soyuz/browser/queue.py	2012-07-03 09:34:24 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Browser views for package queue."""
@@ -385,9 +385,13 @@
             try:
                 source_overridden = queue_item.overrideSource(
                     new_component, new_section, allowed_components)
+                binary_changes = [{
+                    "component": new_component,
+                    "section": new_section,
+                    "priority": new_priority,
+                    }]
                 binary_overridden = queue_item.overrideBinaries(
-                    new_component, new_section, new_priority,
-                    allowed_components)
+                    binary_changes, allowed_components)
             except QueueInconsistentStateError as info:
                 failure.append("FAILED: %s (%s)" %
                                (queue_item.displayname, info))

=== modified file 'lib/lp/soyuz/doc/distroseriesqueue.txt'
--- lib/lp/soyuz/doc/distroseriesqueue.txt	2012-06-29 08:40:05 +0000
+++ lib/lp/soyuz/doc/distroseriesqueue.txt	2012-07-03 09:34:24 +0000
@@ -710,29 +710,25 @@
     main/base/Important
 
     >>> from lp.soyuz.enums import PackagePublishingPriority
+    >>> binary_changes = [{
+    ...     "component": restricted,
+    ...     "section": web,
+    ...     "priority": PackagePublishingPriority.EXTRA,
+    ...     }]
     >>> print item.overrideBinaries(
-    ...     new_component=restricted,
-    ...     new_section=web,
-    ...     new_priority=PackagePublishingPriority.EXTRA,
-    ...     allowed_components=(universe,))
+    ...     binary_changes, allowed_components=(universe,))
     Traceback (most recent call last):
     ...
     QueueInconsistentStateError: No rights to override to restricted
 
     >>> print item.overrideBinaries(
-    ...     new_component=restricted,
-    ...     new_section=web,
-    ...     new_priority=PackagePublishingPriority.EXTRA,
-    ...     allowed_components=(restricted,))
+    ...     binary_changes, allowed_components=(restricted,))
     Traceback (most recent call last):
     ...
     QueueInconsistentStateError: No rights to override from main
 
     >>> print item.overrideBinaries(
-    ...     new_component=restricted,
-    ...     new_section=web,
-    ...     new_priority=PackagePublishingPriority.EXTRA,
-    ...     allowed_components=(main,restricted))
+    ...     binary_changes, allowed_components=(main,restricted))
     True
     >>> print "%s/%s/%s" % (
     ...     binary_package.component.name,

=== modified file 'lib/lp/soyuz/interfaces/archive.py'
--- lib/lp/soyuz/interfaces/archive.py	2012-06-19 23:49:20 +0000
+++ lib/lp/soyuz/interfaces/archive.py	2012-07-03 09:34:24 +0000
@@ -43,6 +43,8 @@
     'NoSuchPPA',
     'NoTokensForTeams',
     'PocketNotFound',
+    'PriorityNotFound',
+    'SectionNotFound',
     'VersionRequiresName',
     'default_name_by_purpose',
     'validate_external_dependencies',
@@ -156,7 +158,7 @@
 
 
 class ComponentNotFound(NameLookupFailed):
-    """Invalid source name."""
+    """Invalid component name."""
     _message_prefix = 'No such component'
 
 
@@ -165,6 +167,16 @@
     """Invalid component name."""
 
 
+class SectionNotFound(NameLookupFailed):
+    """Invalid section name."""
+    _message_prefix = "No such section"
+
+
+class PriorityNotFound(NameLookupFailed):
+    """Invalid priority name."""
+    _message_prefix = "No such priority"
+
+
 class NoSuchPPA(NameLookupFailed):
     """Raised when we try to look up an PPA that doesn't exist."""
     _message_prefix = "No such ppa"

=== modified file 'lib/lp/soyuz/interfaces/queue.py'
--- lib/lp/soyuz/interfaces/queue.py	2012-06-30 17:41:37 +0000
+++ lib/lp/soyuz/interfaces/queue.py	2012-07-03 09:34:24 +0000
@@ -349,16 +349,16 @@
         :return: True if the source was overridden.
         """
 
-    def overrideBinaries(new_component, new_section, new_priority,
-                         allowed_components):
-        """Override all the binaries in a binary queue item.
+    def overrideBinaries(changes, allowed_components):
+        """Override binary packages in a binary queue item.
 
-        :param new_component: An IComponent to replace the existing one
-            in the upload's source.
-        :param new_section: An ISection to replace the existing one
-            in the upload's source.
-        :param new_priority: A valid PackagePublishingPriority to replace
-            the existing one in the upload's binaries.
+        :param changes: A sequence of mappings of changes to apply. Each
+            change mapping 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. Any missing items are
+            left unchanged.
         :param allowed_components: A sequence of components that the
             callsite is allowed to override from and to.
 
@@ -366,10 +366,7 @@
             or the new_component are not in the allowed_components
             sequence.
 
-        The override values may be None, in which case they are not
-        changed.
-
-        :return: True if the binaries were overridden.
+        :return: True if any binaries were overridden.
         """
 
 

=== modified file 'lib/lp/soyuz/model/queue.py'
--- lib/lp/soyuz/model/queue.py	2012-07-03 08:04:35 +0000
+++ lib/lp/soyuz/model/queue.py	2012-07-03 09:34:24 +0000
@@ -72,11 +72,18 @@
     PackageUploadCustomFormat,
     PackageUploadStatus,
     )
-from lp.soyuz.interfaces.archive import MAIN_ARCHIVE_PURPOSES
+from lp.soyuz.interfaces.archive import (
+    ComponentNotFound,
+    MAIN_ARCHIVE_PURPOSES,
+    PriorityNotFound,
+    SectionNotFound,
+    )
+from lp.soyuz.interfaces.component import IComponentSet
 from lp.soyuz.interfaces.packagecopyjob import IPackageCopyJobSource
 from lp.soyuz.interfaces.publishing import (
     IPublishingSet,
     ISourcePackagePublishingHistory,
+    name_priority_map,
     )
 from lp.soyuz.interfaces.queue import (
     IPackageUpload,
@@ -91,6 +98,7 @@
     QueueSourceAcceptError,
     QueueStateWriteProtectedError,
     )
+from lp.soyuz.interfaces.section import ISectionSet
 from lp.soyuz.model.binarypackagename import BinaryPackageName
 from lp.soyuz.model.binarypackagerelease import BinaryPackageRelease
 from lp.soyuz.pas import BuildDaemonPackagesArchSpecific
@@ -913,6 +921,33 @@
         """See `IPackageUpload`."""
         return getUtility(IPackageCopyJobSource).wrap(self.package_copy_job)
 
+    def _nameToComponent(self, component):
+        """Helper to convert a possible string component to IComponent."""
+        try:
+            if isinstance(component, basestring):
+                component = getUtility(IComponentSet)[component]
+            return component
+        except NotFoundError:
+            raise ComponentNotFound(component)
+
+    def _nameToSection(self, section):
+        """Helper to convert a possible string section to ISection."""
+        try:
+            if isinstance(section, basestring):
+                section = getUtility(ISectionSet)[section]
+            return section
+        except NotFoundError:
+            raise SectionNotFound(section)
+
+    def _nameToPriority(self, priority):
+        """Helper to convert a possible string priority to its enum."""
+        try:
+            if isinstance(priority, basestring):
+                priority = name_priority_map[priority]
+            return priority
+        except KeyError:
+            raise PriorityNotFound(priority)
+
     def _overrideSyncSource(self, new_component, new_section,
                             allowed_components):
         """Override source on the upload's `PackageCopyJob`, if any."""
@@ -959,6 +994,9 @@
             # Nothing needs overriding, bail out.
             return False
 
+        new_component = self._nameToComponent(new_component)
+        new_section = self._nameToSection(new_section)
+
         if new_component not in list(allowed_components) + [None]:
             raise QueueInconsistentStateError(
                 "No rights to override to %s" % new_component.name)
@@ -969,35 +1007,74 @@
             self._overrideNonSyncSource(
                 new_component, new_section, allowed_components))
 
-    def overrideBinaries(self, new_component, new_section, new_priority,
-                         allowed_components):
+    def _filterBinaryChanges(self, changes):
+        """Process a binary changes mapping into a more convenient form."""
+        changes_by_name = {}
+        changes_for_all = None
+
+        for change in changes:
+            filtered_change = {}
+            if change.get("component") is not None:
+                filtered_change["component"] = self._nameToComponent(
+                    change.get("component"))
+            if change.get("section") is not None:
+                filtered_change["section"] = self._nameToSection(
+                    change.get("section"))
+            if change.get("priority") is not None:
+                filtered_change["priority"] = self._nameToPriority(
+                    change.get("priority"))
+
+            if "name" in change:
+                changes_by_name[change["name"]] = filtered_change
+            else:
+                # Changes with no "name" item provide a default for all
+                # binaries.
+                changes_for_all = filtered_change
+
+        return changes_by_name, changes_for_all
+
+    def overrideBinaries(self, changes, allowed_components):
         """See `IPackageUpload`."""
         if not self.contains_build:
             return False
 
-        if (new_component is None and new_section is None and
-            new_priority is None):
+        if not changes:
             # Nothing needs overriding, bail out.
             return False
 
-        if new_component not in allowed_components:
+        changes_by_name, changes_for_all = self._filterBinaryChanges(changes)
+
+        new_components = set()
+        for change in changes_by_name.values():
+            if "component" in change:
+                new_components.add(change["component"])
+        if changes_for_all is not None and "component" in changes_for_all:
+            new_components.add(changes_for_all["component"])
+        new_components.discard(None)
+        disallowed_components = sorted(
+            component.name
+            for component in new_components.difference(allowed_components))
+        if disallowed_components:
             raise QueueInconsistentStateError(
-                "No rights to override to %s" % new_component.name)
+                "No rights to override to %s" %
+                ", ".join(disallowed_components))
 
+        made_changes = False
         for build in self.builds:
             for binarypackage in build.build.binarypackages:
-                if binarypackage.component not in allowed_components:
-                    # The old or the new component is not in the list of
-                    # allowed components to override.
-                    raise QueueInconsistentStateError(
-                        "No rights to override from %s" % (
-                            binarypackage.component.name))
-                binarypackage.override(
-                    component=new_component,
-                    section=new_section,
-                    priority=new_priority)
+                change = changes_by_name.get(
+                    binarypackage.name, changes_for_all)
+                if change:
+                    if binarypackage.component not in allowed_components:
+                        # The old component is not in the list of allowed
+                        # components to override.
+                        raise QueueInconsistentStateError(
+                            "No rights to override from %s" %
+                            binarypackage.component.name)
+                    binarypackage.override(**change)
+                    made_changes = True
 
-        return bool(self.builds)
+        return made_changes
 
 
 class PackageUploadBuild(SQLBase):