launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-702260-fix-countExpiredLFAs into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #702260 PackageDiff._countExpiredLFAs needs to also count LFAs with content IS NULL and expires IS NULL
  https://bugs.launchpad.net/bugs/702260

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-702260-fix-countExpiredLFAs/+merge/46113

Before attempting to grab the files necessary for generating a package diff, PackageDiff._countExpiredLFAs checks whether they are all still available from the librarian. However, it checks for expires IS NOT NULL and content IS NULL, missing files that we have explicitly deleted by just setting content=NULL. Requiring expires IS NOT NULL is overly restrictive.

This branch removes the expires IS NOT NULL check.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-702260-fix-countExpiredLFAs/+merge/46113
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-702260-fix-countExpiredLFAs into lp:launchpad.

=== modified file 'lib/lp/soyuz/model/packagediff.py'
--- lib/lp/soyuz/model/packagediff.py	2010-11-05 09:16:14 +0000
+++ lib/lp/soyuz/model/packagediff.py	2011-01-13 10:17:08 +0000
@@ -153,7 +153,6 @@
                 spr.id IN %s
                 AND sprf.SourcePackageRelease = spr.id
                 AND sprf.libraryfile = lfa.id
-                AND lfa.expires IS NOT NULL
                 AND lfa.content IS NULL
             """ % sqlvalues((self.from_source.id, self.to_source.id))
         result = store.execute(query).get_one()

=== modified file 'lib/lp/soyuz/tests/test_packagediff.py'
--- lib/lp/soyuz/tests/test_packagediff.py	2010-10-04 19:50:45 +0000
+++ lib/lp/soyuz/tests/test_packagediff.py	2011-01-13 10:17:08 +0000
@@ -35,20 +35,22 @@
         diff.performDiff()
         self.assertEqual(PackageDiffStatus.COMPLETED, diff.status)
 
-    def expireLFAsForSource(self, source, delete_as_well=True):
+    def expireLFAsForSource(self, source, expire=True, delete=True):
         """Expire the files associated with the given source package in the
         librarian."""
+        assert expire or delete
         store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
         query = """
             UPDATE LibraryFileAlias lfa
             SET
-                expires = %s
-            """ % sqlvalues(datetime.utcnow())
-        if delete_as_well:
-            # Expire *and* delete files from librarian.
-            query += """
-                , content = NULL
-                """
+            """
+
+        if expire:
+            query += "expires = %s" % sqlvalues(datetime.utcnow())
+        if expire and delete:
+            query += ", "
+        if delete:
+            query += "content = NULL"
         query += """
             FROM
                 SourcePackageRelease spr, SourcePackageReleaseFile sprf
@@ -82,9 +84,22 @@
         [diff] = self.getPendingDiffs()
         # Expire but don't delete the files associated with the 'from_source'
         # package.
-        self.expireLFAsForSource(diff.from_source, delete_as_well=False)
+        self.expireLFAsForSource(diff.from_source, expire=True, delete=False)
         # The helper method now finds no expired files.
         self.assertEqual(0, removeSecurityProxy(diff)._countExpiredLFAs())
         diff.performDiff()
         # The diff succeeds as expected.
         self.assertEqual(PackageDiffStatus.COMPLETED, diff.status)
+
+    def test_packagediff_with_deleted_but_not_expired_lfas(self):
+        # Test the case where files required for the diff have been
+        # deleted explicitly, not through expiry.
+        [diff] = self.getPendingDiffs()
+        # Expire but don't delete the files associated with the 'from_source'
+        # package.
+        self.expireLFAsForSource(diff.from_source, expire=False, delete=True)
+        # The helper method now finds 3 expired files.
+        self.assertEqual(3, removeSecurityProxy(diff)._countExpiredLFAs())
+        diff.performDiff()
+        # The diff fails due to the presence of expired files.
+        self.assertEqual(PackageDiffStatus.FAILED, diff.status)