launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1023372 in Launchpad itself: "Direct-copying an already-published package OOPSes"
  https://bugs.launchpad.net/launchpad/+bug/1023372
  Bug #1031089 in Launchpad itself: "PPA async copy attempting to "undelete" a package OOPSes"
  https://bugs.launchpad.net/launchpad/+bug/1031089

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/repeated-copy-oops/+merge/120248

== Summary ==

Bug 1023372 and bug 1031089 report a couple of OOPSes that can occur while trying to create PackageDiffs at the end of processing PCJs.

== Proposed fix ==

These are technically distinct problems, but the fixes were right next to each other, and both amount to being more careful about creating PackageDiffs, so I just dealt with both at once.  In the double-copy case, we just need to guard the whole thing with non-empty copied_sources; in the undeletion case, we need to try more than just the first entry in the ancestry until we find one that's distinct from the one being copied.  I wrote separate tests for each.

== LOC Rationale ==

+35.  I have 4179 lines of credit and this is one of the last remaining things blocking the removal of the old synchronous +copy-packages path and delayed copies.

== Tests ==

bin/test -vvc lp.soyuz.tests.test_packagecopyjob

== Demo and Q/A ==

Try copy/wait-for-processing/re-copy, and copy/wait-for-processing/delete/re-copy.  Both sequences should succeed and produce sensible diffs.
-- 
https://code.launchpad.net/~cjwatson/launchpad/repeated-copy-oops/+merge/120248
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/repeated-copy-oops into lp:launchpad.

=== modified file 'lib/lp/soyuz/model/packagecopyjob.py'
--- lib/lp/soyuz/model/packagecopyjob.py	2012-08-10 23:18:33 +0000
+++ lib/lp/soyuz/model/packagecopyjob.py	2012-08-18 02:04:21 +0000
@@ -596,7 +596,7 @@
         # The package is free to go right in, so just copy it now.
         ancestry = self.target_archive.getPublishedSources(
             name=name, distroseries=self.target_distroseries,
-            pocket=self.target_pocket, exact_match=True).first()
+            pocket=self.target_pocket, exact_match=True)
         override = self.getSourceOverride()
         copy_policy = self.getPolicyImplementation()
         send_email = copy_policy.send_email(self.target_archive)
@@ -610,14 +610,16 @@
             unembargo=self.unembargo)
 
         # Add a PackageDiff for this new upload if it has ancestry.
-        if ancestry is not None:
-            to_sourcepackagerelease = ancestry.sourcepackagerelease
-            copied_source = copied_sources[0]
-            try:
-                to_sourcepackagerelease.requestDiffTo(
-                    self.requester, copied_source.sourcepackagerelease)
-            except PackageDiffAlreadyRequested:
-                pass
+        if copied_sources and not ancestry.is_empty():
+            from_spr = copied_sources[0].sourcepackagerelease
+            for ancestor in ancestry:
+                to_spr = ancestor.sourcepackagerelease
+                if from_spr != to_spr:
+                    try:
+                        to_spr.requestDiffTo(self.requester, from_spr)
+                    except PackageDiffAlreadyRequested:
+                        pass
+                    break
 
         if pu is not None:
             # A PackageUpload will only exist if the copy job had to be

=== modified file 'lib/lp/soyuz/tests/test_packagecopyjob.py'
--- lib/lp/soyuz/tests/test_packagecopyjob.py	2012-08-10 23:18:33 +0000
+++ lib/lp/soyuz/tests/test_packagecopyjob.py	2012-08-18 02:04:21 +0000
@@ -915,6 +915,39 @@
 
         self.assertEqual('multiverse', pcj.metadata['component_override'])
 
+    def test_double_copy(self):
+        # Copying a package already in the target successfully does nothing.
+        job = create_proper_job(self.factory)
+        self.runJob(job)
+        self.assertEqual(JobStatus.COMPLETED, job.status)
+        published_sources = job.target_archive.getPublishedSources(
+            name=job.package_name)
+        self.assertEqual(2, len(list(published_sources)))
+        switch_dbuser("launchpad_main")
+        second_job = getUtility(IPlainPackageCopyJobSource).create(
+            job.package_name, job.source_archive, job.target_archive,
+            job.target_distroseries, job.target_pocket,
+            include_binaries=job.include_binaries,
+            package_version=job.package_version, requester=job.requester)
+        self.runJob(second_job)
+        self.assertEqual(JobStatus.COMPLETED, second_job.status)
+        published_sources = job.target_archive.getPublishedSources(
+            name=job.package_name)
+        self.assertEqual(2, len(list(published_sources)))
+
+    def test_copying_resurrects_deleted_package(self):
+        # A copy job can be used to resurrect previously-deleted packages.
+        archive = self.factory.makeArchive(self.distroseries.distribution)
+        spph = self.publisher.getPubSource(
+            distroseries=self.distroseries, sourcename="copyme",
+            status=PackagePublishingStatus.DELETED, archive=archive)
+        job = self.createCopyJobForSPPH(
+            spph, archive, archive, requester=archive.owner)
+        self.runJob(job)
+        self.assertEqual(JobStatus.COMPLETED, job.status)
+        published_sources = archive.getPublishedSources(name="copyme")
+        self.assertIsNotNone(published_sources.any())
+
     def test_copying_to_main_archive_unapproved(self):
         # Uploading to a series that is in a state that precludes auto
         # approval will cause the job to suspend and a packageupload