launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-834388 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #834388 in Launchpad itself: "PublishingSet.requestDeletion bypasses ORM"
  https://bugs.launchpad.net/launchpad/+bug/834388

For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-834388/+merge/73483

= Summary =

IPublishingSet.setMultipleDeleted updates the database through raw SQL.  This necessitates commits in tests, a separate ORM-based implementation in [SB]PPH for individual records, and so on.


== Proposed fix ==

Use storm.ResultSet.set() instead.  Implement [SB]PPH.setDeleted in terms of the updated method.  Remove the now unnecessary commits from tests.


== Pre-implementation notes ==

Suggested by wgrant; I wasn't aware of storm.ResultSet.set.


== Implementation details ==

A lot of the diff is in ftpmaster.py.  It goes through some contortions to delete [SB]PPHs one by one, when the normal case is actually ideally suited for mass deletions.  I changed that, but in a roundabout way: I now need to filter the BPPHs out of the list of removables after first gathering them for no better reason than to maintain logging output.

Still, I hope it's a first step towards moving even more of this into IPublishingSet.requestDeletion in the future.  We may decide that we don't need to log binaries that are deleted as a consequence of source deletion, or we may add a source-only option to requestDeletion, or we may decide that we don't need source-only and/or binary-only deletions at all.  Adding an informational return value to requestDeletion may obviate some of the contortions.  I wish I could go into it and do a better job right now, but we're on a feature death march.  The only reason I made this change, really, is that I just added DSDJ creation to requestDeletion, and thus slowed this inefficient code down even further.


== Tests ==

{{{
./bin/test lp.soyuz.tests.test_publishing
./bin/test lp.soyuz -t ftpmaster
}}}


== Demo and Q/A ==

Archive deletion must still work, and mark publication records as deleted.


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/soyuz/tests/test_publishing.py
  lib/lp/soyuz/model/publishing.py
  lib/lp/soyuz/scripts/ftpmaster.py
-- 
https://code.launchpad.net/~jtv/launchpad/bug-834388/+merge/73483
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-834388 into lp:launchpad.

=== modified file 'lib/lp/soyuz/model/publishing.py'
--- lib/lp/soyuz/model/publishing.py	2011-08-30 16:48:26 +0000
+++ lib/lp/soyuz/model/publishing.py	2011-08-31 04:54:24 +0000
@@ -334,10 +334,8 @@
 
     def setDeleted(self, removed_by, removal_comment=None):
         """Set to DELETED status."""
-        self.status = PackagePublishingStatus.DELETED
-        self.datesuperseded = UTC_NOW
-        self.removed_by = removed_by
-        self.removal_comment = removal_comment
+        getUtility(IPublishingSet).setMultipleDeleted(
+            self.__class__, [self.id], removed_by, removal_comment)
 
     def requestObsolescence(self):
         """See `IArchivePublisher`."""
@@ -1966,28 +1964,19 @@
         if len(ids) == 0:
             return
 
-        table = publication_class.__name__
-        permitted_tables = [
-            'BinaryPackagePublishingHistory',
-            'SourcePackagePublishingHistory',
+        permitted_classes = [
+            BinaryPackagePublishingHistory,
+            SourcePackagePublishingHistory,
             ]
-        assert table in permitted_tables, "Deleting wrong type."
-
-        params = sqlvalues(
-            deleted=PackagePublishingStatus.DELETED, now=UTC_NOW,
-            removal_comment=removal_comment, removed_by=removed_by)
-
-        IMasterStore(publication_class).execute("\n".join([
-            "UPDATE %s" % table,
-            """
-            SET
-                status = %(deleted)s,
-                datesuperseded = %(now)s,
-                removed_by = %(removed_by)s,
-                removal_comment = %(removal_comment)s
-            """ % params,
-            "WHERE id IN %s" % sqlvalues(ids),
-            ]))
+        assert publication_class in permitted_classes, "Deleting wrong type."
+
+        affected_pubs = IMasterStore(publication_class).find(
+            publication_class, publication_class.id.is_in(ids))
+        affected_pubs.set(
+            status=PackagePublishingStatus.DELETED,
+            datesuperseded=UTC_NOW,
+            removed_byID=removed_by.id,
+            removal_comment=removal_comment)
 
     def requestDeletion(self, sources, removed_by, removal_comment=None):
         """See `IPublishingSet`."""

=== modified file 'lib/lp/soyuz/scripts/ftpmaster.py'
--- lib/lp/soyuz/scripts/ftpmaster.py	2011-08-25 08:29:37 +0000
+++ lib/lp/soyuz/scripts/ftpmaster.py	2011-08-31 04:54:24 +0000
@@ -52,6 +52,7 @@
     )
 from lp.registry.interfaces.series import SeriesStatus
 from lp.registry.interfaces.sourcepackage import SourcePackageFileType
+from lp.services.browser_helpers import get_plural_text
 from lp.services.scripts.base import (
     LaunchpadScript,
     LaunchpadScriptFailure,
@@ -62,6 +63,10 @@
     )
 from lp.soyuz.enums import PackagePublishingStatus
 from lp.soyuz.interfaces.binarypackagename import IBinaryPackageNameSet
+from lp.soyuz.interfaces.publishing import (
+    IPublishingSet,
+    ISourcePackagePublishingHistory,
+    )
 from lp.soyuz.scripts.ftpmasterbase import (
     SoyuzScript,
     SoyuzScriptError,
@@ -1256,26 +1261,38 @@
 
         self.logger.info("Removing candidates:")
         for removable in removables:
-            self.logger.info('\t%s' % removable.displayname)
+            self.logger.info('\t%s', removable.displayname)
 
-        self.logger.info("Removed-by: %s" % removed_by.displayname)
-        self.logger.info("Comment: %s" % self.options.removal_comment)
+        self.logger.info("Removed-by: %s", removed_by.displayname)
+        self.logger.info("Comment: %s", self.options.removal_comment)
 
         removals = []
-        for removable in removables:
-            removable.requestDeletion(
-                removed_by=removed_by,
+        if self.options.binaryonly or self.options.sourceonly:
+            # Tedious.  Delete each BPPH/SPPH individually.
+            for removable in removables:
+                removable.requestDeletion(
+                    removed_by=removed_by,
+                    removal_comment=self.options.removal_comment)
+                removals.append(removable)
+        else:
+            # Mass-delete the sources, and the binaries will follow
+            # automatically.  The binaries are only interesting for
+            # logging.
+            sources = [
+                removable
+                for removable in removables
+                    if ISourcePackagePublishingHistory.implementedBy(
+                            removable)]
+            getUtility(IPublishingSet).requestDeletion(
+                sources, removed_by,
                 removal_comment=self.options.removal_comment)
-            removals.append(removable)
 
-        if len(removals) == 1:
-            self.logger.info(
-                "%s package successfully removed." % len(removals))
-        elif len(removals) > 1:
-            self.logger.info(
-                "%s packages successfully removed." % len(removals))
-        else:
+        if len(removals) == 0:
             self.logger.info("No package removed (bug ?!?).")
+        else:
+            self.logger.info(
+                "%d %s successfully removed.", len(removals),
+                get_plural_text(len(removals), "package", "packages"))
 
         # Information returned mainly for the benefit of the test harness.
         return removals

=== modified file 'lib/lp/soyuz/tests/test_publishing.py'
--- lib/lp/soyuz/tests/test_publishing.py	2011-08-26 09:17:08 +0000
+++ lib/lp/soyuz/tests/test_publishing.py	2011-08-31 04:54:24 +0000
@@ -1179,8 +1179,6 @@
         spph = self.factory.makeSourcePackagePublishingHistory()
         getUtility(IPublishingSet).requestDeletion(
             [spph], self.factory.makePerson())
-        # XXX JeroenVermeulen 2011-08-25, bug=834388: obviate commit.
-        transaction.commit()
         self.assertEqual(PackagePublishingStatus.DELETED, spph.status)
 
     def test_requestDeletion_leaves_other_SPPHs_alone(self):
@@ -1188,8 +1186,6 @@
         other_spph = self.factory.makeSourcePackagePublishingHistory()
         getUtility(IPublishingSet).requestDeletion(
             [other_spph], self.factory.makePerson())
-        # XXX JeroenVermeulen 2011-08-25, bug=834388: obviate commit.
-        transaction.commit()
         self.assertEqual(PackagePublishingStatus.PENDING, spph.status)
 
     def test_requestDeletion_marks_attached_BPPHs_deleted(self):
@@ -1197,8 +1193,6 @@
         spph = self.factory.makeSPPHForBPPH(bpph)
         getUtility(IPublishingSet).requestDeletion(
             [spph], self.factory.makePerson())
-        # XXX JeroenVermeulen 2011-08-25, bug=834388: obviate commit.
-        transaction.commit()
         self.assertEqual(PackagePublishingStatus.DELETED, spph.status)
 
     def test_requestDeletion_leaves_other_BPPHs_alone(self):
@@ -1206,8 +1200,6 @@
         unrelated_spph = self.factory.makeSourcePackagePublishingHistory()
         getUtility(IPublishingSet).requestDeletion(
             [unrelated_spph], self.factory.makePerson())
-        # XXX JeroenVermeulen 2011-08-25, bug=834388: obviate commit.
-        transaction.commit()
         self.assertEqual(PackagePublishingStatus.PENDING, bpph.status)
 
     def test_requestDeletion_accepts_empty_sources_list(self):