Merge lp:~cjwatson/launchpad/copy-packages-with-default-series into lp:launchpad

[Colin Watson]

Fix copying packages with the default target series (i.e. the source series).

== Summary ==

In various places related to copying packages, Launchpad documents that passing a target series of None means that the target series will be the same as the source series (but in the target archive, of course). However, this doesn't actually work everywhere and OOPSes (bug 795005).

== Proposed fix ==

The OOPS itself is in check_copy_permissions when series is None. That function's current interface takes a sequence of SourcePackageNames, and with that interface this bug is impossible to fix because we only do one permissions check per SPN even if that corresponds to the same source package in multiple series. However, all the call sites have convenient access to the SourcePackagePublishingHistory, so I changed the interface to take a sequence of SPPHs instead, making this straightforward.

I moved the bulk loading down from Archive.copyPackages to check_copy_permissions (I don't think there was currently any bulk loading happening when copying via the UI, which can't have helped matters), and added a bit more for good measure.

Once this is fixed, it also becomes necessary to ensure that the PCJs have the correct target series in their metadata, rather than leaving it as None and deferring that decision to when the PCJ is run, since otherwise calls such as getPendingJobsForTargetSeries won't work. I therefore went through all sites where PCJs are created and ensured that they handle the target series being None where applicable.

== LOC Rationale ==

+59. This is part of allowing us to remove synchronous copying from the PPA UI, as well as eventually removing delayed copies. Besides, I have 2976 lines of credit.

== Tests ==

bin/test -vvct test_copyPackage -t test_packagecopyjob

I haven't explicitly added browser tests here. This is mainly because I actually ran into this bug while working on another branch to remove synchronous copying from the PPA UI, which caused a failure in lib/lp/soyuz/browser/tests/archive-views.txt attributable to this bug. So, while that isn't tested right now, it soon will be.

== Demo and Q/A ==

Find a PPA with packages in several series, create a new temporary PPA, and use Archive.copyPackages with the to_series argument omitted to copy everything from the first to the second (with include_binaries=True to avoid unnecessary rebuilds). It should work with all the series information copied over.

Get an admin to set the soyuz.derived_series.max_synchronous_syncs feature flag to something small like 5 on qastaging, and try the same thing, only this time using the +copy-packages web UI. You'll need to copy >5 (or whatever you set) packages for this to work.

== Lint ==

Just one false positive:

./lib/lp/soyuz/model/archive.py
     347: redefinition of function 'private' from line 343

[Jelmer Vernooij (jelmer)]

It would be nice to add a test for the case where you're copying two packages with a different distroseries. Other than that, this looks good.

review: Approve (code)

=== modified file 'lib/lp/registry/browser/distroseries.py'
--- lib/lp/registry/browser/distroseries.py	2012-06-27 13:04:42 +0000
+++ lib/lp/registry/browser/distroseries.py	2012-06-29 01:07:36 +0000
@@ -1293,12 +1293,12 @@
                 dsd.parent_source_version,
                 dsd.parent_series.main_archive,
                 target_distroseries.main_archive,
+                target_distroseries,
                 PackagePublishingPocket.RELEASE,
             )
             for dsd in self.getUpgrades()]
         getUtility(IPlainPackageCopyJobSource).createMultiple(
-            target_distroseries, copies, self.user,
-            copy_policy=PackageCopyPolicy.MASS_SYNC)
+            copies, self.user, copy_policy=PackageCopyPolicy.MASS_SYNC)
 
         self.request.response.addInfoNotification(
             (u"Upgrades of {context.displayname} packages have been "

=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py	2012-06-14 03:22:43 +0000
+++ lib/lp/soyuz/browser/archive.py	2012-06-29 01:07:36 +0000
@@ -1311,16 +1311,14 @@
         not permitted.
     """
     if check_permissions:
-        spns = [
-            spph.sourcepackagerelease.sourcepackagename
-            for spph in source_pubs]
         check_copy_permissions(
-            person, dest_archive, dest_series, dest_pocket, spns)
+            person, dest_archive, dest_series, dest_pocket, source_pubs)
 
     job_source = getUtility(IPlainPackageCopyJobSource)
     for spph in source_pubs:
         job_source.create(
-            spph.source_package_name, spph.archive, dest_archive, dest_series,
+            spph.source_package_name, spph.archive, dest_archive,
+            dest_series if dest_series is not None else spph.distroseries,
             dest_pocket, include_binaries=include_binaries,
             package_version=spph.sourcepackagerelease.version,
             copy_policy=PackageCopyPolicy.INSECURE,

=== modified file 'lib/lp/soyuz/browser/tests/test_package_copying_mixin.py'
--- lib/lp/soyuz/browser/tests/test_package_copying_mixin.py	2012-01-01 02:58:52 +0000
+++ lib/lp/soyuz/browser/tests/test_package_copying_mixin.py	2012-06-29 01:07:36 +0000
@@ -251,6 +251,35 @@
         spr = spph.sourcepackagerelease
         self.assertEqual(spr.sourcepackagename.name, job.package_name)
         self.assertEqual(spr.version, job.package_version)
+        self.assertEqual(dest_series, job.target_distroseries)
+
+    def test_copy_asynchronously_handles_no_dest_series(self):
+        # If dest_series is None, copy_asynchronously creates jobs that will
+        # copy each source into the same distroseries in the target archive.
+        distribution = self.makeDistribution()
+        series_one = self.factory.makeDistroSeries(
+            distribution=distribution, registrant=self.person)
+        series_two = self.factory.makeDistroSeries(
+            distribution=distribution, registrant=self.person)
+        spph_one = self.factory.makeSourcePackagePublishingHistory(
+            distroseries=series_one, sourcepackagename="one",
+            maintainer=self.person, creator=self.person)
+        spph_two = self.factory.makeSourcePackagePublishingHistory(
+            distroseries=series_two, sourcepackagename="two",
+            maintainer=self.person, creator=self.person)
+        pocket = self.factory.getAnyPocket()
+        target_archive = self.factory.makeArchive(
+            owner=self.person, distribution=distribution)
+        copy_asynchronously(
+            [spph_one, spph_two], target_archive, None, pocket,
+            include_binaries=False, check_permissions=False,
+            person=self.person)
+        jobs = list(getUtility(IPlainPackageCopyJobSource).getActiveJobs(
+            target_archive))
+        self.assertEqual(2, len(jobs))
+        self.assertContentEqual(
+            [("one", spph_one.distroseries), ("two", spph_two.distroseries)],
+            [(job.package_name, job.target_distroseries) for job in jobs])
 
     def test_do_copy_goes_async_if_canCopySynchronously_says_so(self):
         # The view opts for asynchronous copying if canCopySynchronously

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2012-06-19 23:49:20 +0000
+++ lib/lp/soyuz/model/archive.py	2012-06-29 01:07:36 +0000
@@ -72,7 +72,6 @@
 from lp.registry.model.sourcepackagename import SourcePackageName
 from lp.registry.model.teammembership import TeamParticipation
 from lp.services.config import config
-from lp.services.database.bulk import load_related
 from lp.services.database.constants import UTC_NOW
 from lp.services.database.datetimecol import UtcDateTimeCol
 from lp.services.database.decoratedresultset import DecoratedResultSet
@@ -1641,11 +1640,12 @@
         series = self._text_to_series(to_series)
         # Upload permission checks, this will raise CannotCopy as
         # necessary.
-        sourcepackagename = getUtility(ISourcePackageNameSet)[source_name]
-        check_copy_permissions(
-            person, self, series, pocket, [sourcepackagename])
+        source = self._validateAndFindSource(
+            from_archive, source_name, version)
+        if series is None:
+            series = source.distroseries
+        check_copy_permissions(person, self, series, pocket, [source])
 
-        self._validateAndFindSource(from_archive, source_name, version)
         job_source = getUtility(IPlainPackageCopyJobSource)
         job_source.create(
             package_name=source_name, source_archive=from_archive,
@@ -1667,20 +1667,10 @@
             raise CannotCopy(
                 "None of the supplied package names are published")
 
-        # Bulk-load the sourcepackagereleases so that the list
-        # comprehension doesn't generate additional queries. The
-        # sourcepackagenames themselves will already have been loaded when
-        # generating the list of source publications in "sources".
-        load_related(
-            SourcePackageRelease, sources, ["sourcepackagereleaseID"])
-        sourcepackagenames = [source.sourcepackagerelease.sourcepackagename
-                              for source in sources]
-
         # Now do a mass check of permissions.
         pocket = self._text_to_pocket(to_pocket)
         series = self._text_to_series(to_series)
-        check_copy_permissions(
-            person, self, series, pocket, sourcepackagenames)
+        check_copy_permissions(person, self, series, pocket, sources)
 
         # If we get this far then we can create the PackageCopyJob.
         copy_tasks = []
@@ -1690,14 +1680,14 @@
                 source.sourcepackagerelease.version,
                 from_archive,
                 self,
+                series if series is not None else source.distroseries,
                 PackagePublishingPocket.RELEASE
                 )
             copy_tasks.append(task)
 
         job_source = getUtility(IPlainPackageCopyJobSource)
         job_source.createMultiple(
-            series, copy_tasks, person,
-            copy_policy=PackageCopyPolicy.MASS_SYNC,
+            copy_tasks, person, copy_policy=PackageCopyPolicy.MASS_SYNC,
             include_binaries=include_binaries, sponsored=sponsored,
             unembargo=unembargo)
 

=== modified file 'lib/lp/soyuz/model/packagecopyjob.py'
--- lib/lp/soyuz/model/packagecopyjob.py	2012-06-24 01:56:36 +0000
+++ lib/lp/soyuz/model/packagecopyjob.py	2012-06-29 01:07:36 +0000
@@ -287,9 +287,8 @@
         return derived
 
     @classmethod
-    def _composeJobInsertionTuple(cls, target_distroseries, copy_policy,
-                                  include_binaries, job_id, copy_task,
-                                  sponsored, unembargo):
+    def _composeJobInsertionTuple(cls, copy_policy, include_binaries, job_id,
+                                  copy_task, sponsored, unembargo):
         """Create an SQL fragment for inserting a job into the database.
 
         :return: A string representing an SQL tuple containing initializers
@@ -301,6 +300,7 @@
             package_version,
             source_archive,
             target_archive,
+            target_distroseries,
             target_pocket,
         ) = copy_task
         metadata = cls._makeMetadata(
@@ -313,7 +313,7 @@
         return data
 
     @classmethod
-    def createMultiple(cls, target_distroseries, copy_tasks, requester,
+    def createMultiple(cls, copy_tasks, requester,
                        copy_policy=PackageCopyPolicy.INSECURE,
                        include_binaries=False, sponsored=None,
                        unembargo=False):
@@ -322,8 +322,8 @@
         job_ids = Job.createMultiple(store, len(copy_tasks), requester)
         job_contents = [
             cls._composeJobInsertionTuple(
-                target_distroseries, copy_policy, include_binaries, job_id,
-                task, sponsored, unembargo)
+                copy_policy, include_binaries, job_id, task, sponsored,
+                unembargo)
             for job_id, task in zip(job_ids, copy_tasks)]
         return bulk.create(
                 (PackageCopyJob.job_type, PackageCopyJob.target_distroseries,

=== modified file 'lib/lp/soyuz/scripts/packagecopier.py'
--- lib/lp/soyuz/scripts/packagecopier.py	2012-06-27 17:20:45 +0000
+++ lib/lp/soyuz/scripts/packagecopier.py	2012-06-29 01:07:36 +0000
@@ -16,6 +16,7 @@
     'update_files_privacy',
     ]
 
+from operator import attrgetter
 import os
 import tempfile
 
@@ -26,6 +27,7 @@
 
 from lp.app.errors import NotFoundError
 from lp.buildmaster.enums import BuildStatus
+from lp.services.database.bulk import load_related
 from lp.services.librarian.interfaces import ILibraryFileAliasSet
 from lp.services.librarian.utils import copy_and_close
 from lp.soyuz.adapters.notification import notify
@@ -204,39 +206,59 @@
             return {'status': BuildSetStatus.NEEDSBUILD}
 
 
-def check_copy_permissions(person, archive, series, pocket,
-                           sourcepackagenames):
+def check_copy_permissions(person, archive, series, pocket, sources):
     """Check that `person` has permission to copy a package.
 
     :param person: User attempting the upload.
     :param archive: Destination `Archive`.
     :param series: Destination `DistroSeries`.
     :param pocket: Destination `Pocket`.
-    :param sourcepackagenames: Sequence of `SourcePackageName`s for the
+    :param sources: Sequence of `SourcePackagePublishingHistory`s for the
         packages to be copied.
     :raises CannotCopy: If the copy is not allowed.
     """
+    # Circular import.
+    from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
+
     if person is None:
         raise CannotCopy("Cannot check copy permissions (no requester).")
 
+    if len(sources) > 1:
+        # Bulk-load the data we'll need from each source publication.
+        load_related(SourcePackageRelease, sources, ["sourcepackagereleaseID"])
+
     # If there is a requester, check that he has upload permission into
     # the destination (archive, component, pocket). This check is done
     # here rather than in the security adapter because it requires more
     # info than is available in the security adapter.
-    for spn in set(sourcepackagenames):
-        package = series.getSourcePackage(spn)
-        destination_component = package.latest_published_component
-
-        # If destination_component is not None, make sure the person
-        # has upload permission for this component.  Otherwise, any
-        # upload permission on this archive will do.
-        strict_component = destination_component is not None
-        reason = archive.checkUpload(
-            person, series, spn, destination_component, pocket,
-            strict_component=strict_component)
-
-        if reason is not None:
-            raise CannotCopy(reason)
+    if series is None:
+        # Use each source's series as the destination for that source.
+        for source in sources:
+            reason = archive.checkUpload(
+                person, source.distroseries,
+                source.sourcepackagerelease.sourcepackagename,
+                source.component, pocket, strict_component=True)
+
+            if reason is not None:
+                raise CannotCopy(reason)
+    else:
+        sourcepackagenames = [
+            source.sourcepackagerelease.sourcepackagename
+            for source in sources]
+        for spn in set(sourcepackagenames):
+            package = series.getSourcePackage(spn)
+            destination_component = package.latest_published_component
+
+            # If destination_component is not None, make sure the person
+            # has upload permission for this component.  Otherwise, any
+            # upload permission on this archive will do.
+            strict_component = destination_component is not None
+            reason = archive.checkUpload(
+                person, series, spn, destination_component, pocket,
+                strict_component=strict_component)
+
+            if reason is not None:
+                raise CannotCopy(reason)
 
 
 class CopyChecker:
@@ -463,8 +485,7 @@
         """
         if check_permissions:
             check_copy_permissions(
-                person, self.archive, series, pocket,
-                [source.sourcepackagerelease.sourcepackagename])
+                person, self.archive, series, pocket, [source])
 
         if series not in self.archive.distribution.series:
             raise CannotCopy(

=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py	2012-06-19 23:49:20 +0000
+++ lib/lp/soyuz/tests/test_archive.py	2012-06-29 01:07:36 +0000
@@ -2368,6 +2368,22 @@
         self.assertEqual(target_archive, copy_job.target_archive)
         self.assertTrue(copy_job.unembargo)
 
+    def test_copyPackage_with_default_distroseries(self):
+        # If to_series is None, copyPackage copies into the same series as
+        # the source in the target archive.
+        (source, source_archive, source_name, target_archive, to_pocket,
+         to_series, version) = self._setup_copy_data()
+        with person_logged_in(target_archive.owner):
+            target_archive.copyPackage(
+                source_name, version, source_archive, to_pocket.name,
+                include_binaries=False, person=target_archive.owner)
+
+        # There should be one copy job with the correct target series.
+        job_source = getUtility(IPlainPackageCopyJobSource)
+        copy_jobs = job_source.getActiveJobs(target_archive)
+        self.assertEqual(1, copy_jobs.count())
+        self.assertEqual(source.distroseries, copy_jobs[0].target_distroseries)
+
     def test_copyPackages_with_single_package(self):
         (source, source_archive, source_name, target_archive, to_pocket,
          to_series, version) = self._setup_copy_data()
@@ -2505,6 +2521,33 @@
         self.assertEqual(copy_job, copy_jobs[0])
         self.assertEqual(new_version, copy_jobs[1].package_version)
 
+    def test_copyPackages_with_default_distroseries(self):
+        # If to_series is None, copyPackages copies into the same series as
+        # each source in the target archive.
+        (source, source_archive, source_name, target_archive, to_pocket,
+         to_series, version) = self._setup_copy_data()
+        sources = [source]
+        other_series = self.factory.makeDistroSeries(
+            distribution=target_archive.distribution)
+        sources.append(self.factory.makeSourcePackagePublishingHistory(
+            distroseries=other_series, archive=source_archive,
+            status=PackagePublishingStatus.PUBLISHED))
+        names = [source.sourcepackagerelease.sourcepackagename.name
+                 for source in sources]
+
+        with person_logged_in(target_archive.owner):
+            target_archive.copyPackages(
+                names, source_archive, to_pocket.name, include_binaries=False,
+                person=target_archive.owner)
+
+        # There should be two copy jobs with the correct target series.
+        job_source = getUtility(IPlainPackageCopyJobSource)
+        copy_jobs = job_source.getActiveJobs(target_archive)
+        self.assertEqual(2, copy_jobs.count())
+        self.assertContentEqual(
+            [source.distroseries for source in sources],
+            [copy_job.target_distroseries for copy_job in copy_jobs])
+
 
 class TestgetAllPublishedBinaries(TestCaseWithFactory):
 

=== modified file 'lib/lp/soyuz/tests/test_packagecopyjob.py'
--- lib/lp/soyuz/tests/test_packagecopyjob.py	2012-06-27 00:49:47 +0000
+++ lib/lp/soyuz/tests/test_packagecopyjob.py	2012-06-29 01:07:36 +0000
@@ -243,6 +243,7 @@
                 "1.5mother1",
                 mother.parent_series.main_archive,
                 derived_series.main_archive,
+                derived_series,
                 PackagePublishingPocket.RELEASE,
                 ),
             (
@@ -250,12 +251,11 @@
                 "0.9father1",
                 father.parent_series.main_archive,
                 derived_series.main_archive,
+                derived_series,
                 PackagePublishingPocket.UPDATES,
                 ),
             ]
-        job_ids = list(
-            job_source.createMultiple(mother.derived_series, copy_tasks,
-                                      requester))
+        job_ids = list(job_source.createMultiple(copy_tasks, requester))
         jobs = list(job_source.getActiveJobs(derived_series.main_archive))
         self.assertContentEqual(job_ids, [job.id for job in jobs])
         self.assertEqual(len(copy_tasks), len(set([job.job for job in jobs])))
@@ -269,6 +269,7 @@
                 job.package_version,
                 job.source_archive,
                 job.target_archive,
+                job.target_distroseries,
                 job.target_pocket,
                 )
             for job in jobs]