launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/copy-unpublished into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #837555 in Launchpad itself: "AttributeError: 'NoneType' object has no attribute 'sourcepackagerelease'  syncing source using api"
  https://bugs.launchpad.net/launchpad/+bug/837555

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/copy-unpublished/+merge/119271

== Summary ==

Bug 837555: the various package-copying methods on Archive OOPS if asked to copy a package that isn't published in the source archive.

== Proposed fix ==

Consistently raise CannotCopy in this case for all four relevant methods.

I took the liberty of arranging for Archive.syncSources to raise an exception rather than silently doing nothing; my reasoning is that the user clearly expected it to do something and thus doing nothing should be regarded as an exceptional condition here.  Besides, it's consistent with Archive.copyPackages, and this allows the error handling to be pushed down into a common method.

== LOC Rationale ==

+50.  I have ~4000 lines of credit and more to come, so I'd like to use a bit of that on reducing the critical queue slightly.

== Tests ==

bin/test -vvct lib/lp/soyuz/doc/archive.txt -t lp.soyuz.tests.test_archive.TestSyncSource

== Demo and Q/A ==

There's a reproduction recipe in the bug description.

== Lint ==

Just a false positive:

./lib/lp/soyuz/model/archive.py
     346: redefinition of function 'private' from line 342
-- 
https://code.launchpad.net/~cjwatson/launchpad/copy-unpublished/+merge/119271
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/copy-unpublished into lp:launchpad.

=== modified file 'lib/lp/soyuz/doc/archive.txt'
--- lib/lp/soyuz/doc/archive.txt	2012-06-07 16:32:03 +0000
+++ lib/lp/soyuz/doc/archive.txt	2012-08-12 14:40:25 +0000
@@ -2092,6 +2092,14 @@
     ...
     NoSuchDistroSeries: No such distribution series: 'badseries'.
 
+If a package exists but not in the source archive, we get an error:
+
+    >>> mark.archive.syncSources(["pack"], mark.archive, "release")
+    Traceback (most recent call last):
+    ...
+    CannotCopy: None of the supplied package names are published in PPA for
+    Mark Shuttleworth.
+
 If a package exists in multiple distroseries, we can use the `from_series`
 parameter to select the distroseries to synchronise from:
 
@@ -2151,6 +2159,13 @@
     >>> print pack.sourcepackagerelease.version
     1.0
 
+If the supplied package exists but not in the source archive, we get an error:
+
+    >>> mark.archive.syncSource("package3", "1.0", mark.archive, "release")
+    Traceback (most recent call last):
+    ...
+    CannotCopy: package3 is not published in PPA for Mark Shuttleworth.
+
 Copy package3 1.0 explicitly:
 
     >>> mark.archive.syncSource("package3", "1.0", cprov.archive,

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2012-08-10 17:08:57 +0000
+++ lib/lp/soyuz/model/archive.py	2012-08-12 14:40:25 +0000
@@ -1653,6 +1653,10 @@
         # Find and validate the source package version required.
         source = from_archive.getPublishedSources(
             name=source_name, version=version, exact_match=True).first()
+        if source is None:
+            raise CannotCopy(
+                "%s is not published in %s." %
+                (source_name, from_archive.displayname))
         return source
 
     def syncSource(self, source_name, version, from_archive, to_pocket,
@@ -1710,9 +1714,6 @@
 
         sources = self._collectLatestPublishedSources(
             from_archive, from_series, source_names)
-        if not sources:
-            raise CannotCopy(
-                "None of the supplied package names are published")
 
         # Now do a mass check of permissions.
         pocket = self._text_to_pocket(to_pocket)
@@ -1745,6 +1746,8 @@
 
         :raises NoSuchSourcePackageName: If any of the source_names do not
             exist.
+        :raises CannotCopy: If none of the source_names are published in
+            from_archive.
         """
         from_series_obj = self._text_to_series(
             from_series, distribution=from_archive.distribution)
@@ -1766,6 +1769,10 @@
             first_source = published_sources.first()
             if first_source is not None:
                 sources.append(first_source)
+        if not sources:
+            raise CannotCopy(
+                "None of the supplied package names are published in %s." %
+                from_archive.displayname)
         return sources
 
     def _text_to_series(self, to_series, distribution=None):

=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py	2012-07-24 19:50:54 +0000
+++ lib/lp/soyuz/tests/test_archive.py	2012-08-12 14:40:25 +0000
@@ -2427,6 +2427,20 @@
         self.assertEqual(1, copy_jobs.count())
         self.assertEqual(source.distroseries, copy_jobs[0].target_distroseries)
 
+    def test_copyPackage_unpublished_source(self):
+        # If the given source name is not published in the source archive,
+        # we get a CannotCopy exception.
+        (source, source_archive, source_name, target_archive, to_pocket,
+         to_series, version) = self._setup_copy_data()
+        with person_logged_in(target_archive.owner):
+            expected_error = (
+                "%s is not published in %s." %
+                (source_name, target_archive.displayname))
+            self.assertRaisesWithContent(
+                CannotCopy, expected_error, target_archive.copyPackage,
+                source_name, version, target_archive, to_pocket.name,
+                target_archive.owner)
+
     def test_copyPackages_with_single_package(self):
         (source, source_archive, source_name, target_archive, to_pocket,
          to_series, version) = self._setup_copy_data()
@@ -2642,6 +2656,20 @@
         copy_job = job_source.getActiveJobs(target_archive).one()
         self.assertEqual(source.distroseries, copy_job.target_distroseries)
 
+    def test_copyPackages_unpublished_source(self):
+        # If none of the given source names are published in the source
+        # archive, we get a CannotCopy exception.
+        (source, source_archive, source_name, target_archive, to_pocket,
+         to_series, version) = self._setup_copy_data()
+        with person_logged_in(target_archive.owner):
+            expected_error = (
+                "None of the supplied package names are published in %s." %
+                target_archive.displayname)
+            self.assertRaisesWithContent(
+                CannotCopy, expected_error, target_archive.copyPackages,
+                [source_name], target_archive, to_pocket.name,
+                target_archive.owner)
+
 
 class TestgetAllPublishedBinaries(TestCaseWithFactory):