launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/fix-check-copy-permissions into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #795005 in Launchpad itself: "Asynchronous PPA copying with default series OOPSes"
  https://bugs.launchpad.net/launchpad/+bug/795005

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/fix-check-copy-permissions/+merge/112832

== Summary ==

William Grant pointed out that my change in https://code.launchpad.net/~cjwatson/launchpad/copy-packages-with-default-series/+merge/112557 has incorrect semantics, because it checks the component of the source rather than of the target in the "series is None" (i.e. copy to same series as source) case.

== Proposed fix ==

Check the target component instead.  This requires using archive.getPublishedSources; with some care, we can consolidate duplicate code again.

== LOC Rationale ==

+32.  Same rationale as https://code.launchpad.net/~cjwatson/launchpad/copy-packages-with-default-series/+merge/112557 - removing delayed copies, which this is part of the road to, is worth at least 1100 lines.

== Tests ==

bin/test -vvct test_archive.TestSyncSource

== Demo and Q/A ==

As https://code.launchpad.net/~cjwatson/launchpad/copy-packages-with-default-series/+merge/112557, but copy into a distribution archive with various permissions.  PPA publications are always in main, so the ability to copy a new version of a package in universe with only universe component upload permissions is a good indicator that this is working.

== Lint ==

''Paste output from `make lint` here - if there's lint related to your code, fix it so that you don't have to paste it here!''
-- 
https://code.launchpad.net/~cjwatson/launchpad/fix-check-copy-permissions/+merge/112832
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/fix-check-copy-permissions into lp:launchpad.

=== modified file 'lib/lp/soyuz/scripts/packagecopier.py'
--- lib/lp/soyuz/scripts/packagecopier.py	2012-06-29 01:03:10 +0000
+++ lib/lp/soyuz/scripts/packagecopier.py	2012-06-29 18:46:21 +0000
@@ -16,6 +16,7 @@
     'update_files_privacy',
     ]
 
+from itertools import repeat
 from operator import attrgetter
 import os
 import tempfile
@@ -231,34 +232,32 @@
     # 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.
+    sourcepackagenames = [
+        source.sourcepackagerelease.sourcepackagename for source in sources]
     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)
+        series_iter = map(attrgetter("distroseries"), sources)
     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)
+        series_iter = repeat(series)
+    for spn, dest_series in set(zip(sourcepackagenames, series_iter)):
+        ancestries = archive.getPublishedSources(
+            name=spn.name, exact_match=True, status=active_publishing_status,
+            distroseries=dest_series, pocket=pocket)
+        try:
+            destination_component = ancestries[0].component
+        except IndexError:
+            destination_component = None
+
+        # 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, dest_series, spn, destination_component, pocket,
+            strict_component=strict_component)
+
+        if reason is not None:
+            raise CannotCopy(reason)
 
 
 class CopyChecker:

=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py	2012-06-28 12:21:01 +0000
+++ lib/lp/soyuz/tests/test_archive.py	2012-06-29 18:46:21 +0000
@@ -2548,6 +2548,39 @@
             [source.distroseries for source in sources],
             [copy_job.target_distroseries for copy_job in copy_jobs])
 
+    def test_copyPackages_with_default_distroseries_and_override(self):
+        # If to_series is None, copyPackages checks permissions based on the
+        # component in the target archive, not the component in the source
+        # archive.
+        (source, source_archive, source_name, target_archive, to_pocket,
+         to_series, version) = self._setup_copy_data(
+            target_purpose=ArchivePurpose.PRIMARY)
+        sources = [source]
+        uploader = self.factory.makePerson()
+        main = self.factory.makeComponent(name="main")
+        universe = self.factory.makeComponent(name="universe")
+        ComponentSelection(distroseries=to_series, component=main)
+        ComponentSelection(distroseries=to_series, component=universe)
+        with person_logged_in(target_archive.owner):
+            target_archive.newComponentUploader(uploader, universe)
+        self.factory.makeSourcePackagePublishingHistory(
+            distroseries=source.distroseries, archive=target_archive,
+            pocket=to_pocket, status=PackagePublishingStatus.PUBLISHED,
+            sourcepackagename=source_name, version="%s~1" % version,
+            component=universe)
+        names = [source.sourcepackagerelease.sourcepackagename.name
+                 for source in sources]
+
+        with person_logged_in(uploader):
+            target_archive.copyPackages(
+                names, source_archive, to_pocket.name, include_binaries=False,
+                person=uploader)
+
+        # There should be a copy job with the correct target series.
+        job_source = getUtility(IPlainPackageCopyJobSource)
+        copy_job = job_source.getActiveJobs(target_archive).one()
+        self.assertEqual(source.distroseries, copy_job.target_distroseries)
+
 
 class TestgetAllPublishedBinaries(TestCaseWithFactory):