Merge lp:~stevenk/launchpad/performant-getSourcesForDeletion into lp:launchpad

[Steve Kowalik]

Rewrite IArchive.getSourcesForDeletion() to be performant, and Storm-ify it.

Rewrite IArchive.getSourcesForDeletion() to be performant, and Storm-ify it. It now makes use of SPPH.scheduleddeletiondate, which required some test fiddling.

Perform a little bit of clean-up, like destroying the use of SQLConstant, but this branch was already net-negative just due to switching to a simpler query. Drop a unneeded database perm that was missed by a testfix of mine.

[William Grant (wgrant)]

I'd like to see a comment describing why we use scheduleddeletiondate, otherwise fine.

review: Approve (code)

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2013-03-12 05:51:28 +0000
+++ database/schema/security.cfg	2013-03-18 04:11:22 +0000
@@ -2197,7 +2197,6 @@
 public.translationrelicensingagreement  = SELECT, UPDATE
 public.translator                       = SELECT, UPDATE
 public.usertouseremail                  = SELECT, UPDATE
-public.validpersoncache                 = SELECT
 public.vote                             = SELECT, UPDATE
 public.votecast                         = SELECT, UPDATE
 public.wikiname                         = SELECT, UPDATE

=== modified file 'lib/lp/soyuz/doc/archive.txt'
--- lib/lp/soyuz/doc/archive.txt	2013-02-20 05:43:59 +0000
+++ lib/lp/soyuz/doc/archive.txt	2013-03-18 04:11:22 +0000
@@ -573,8 +573,10 @@
 
     >>> cprov_archive.number_of_sources
     3
-    >>> cprov_archive.getPublishedSources(
-    ...     name=u'cdrkit').first().supersede()
+    >>> cdrkit = cprov_archive.getPublishedSources(name=u'cdrkit').first()
+    >>> cdrkit.supersede()
+    >>> from lp.services.database.constants import UTC_NOW
+    >>> cdrkit.scheduleddeletiondate = UTC_NOW
 
     >>> cprov_archive.number_of_sources
     2
@@ -608,7 +610,7 @@
 This method can optionally receive a source package name filter (SQL
 LIKE) to restrict its result.
 
-    >>> cprov_archive.getSourcesForDeletion(name='ice').count()
+    >>> cprov_archive.getSourcesForDeletion(name=u'ice').count()
     1
 
 If only the source publication is DELETED, leaving its binary behind,
@@ -622,29 +624,28 @@
     >>> login("[e-mail]")
     >>> removal_candidate.requestDeletion(cprov, 'go away !')
 
-    >>> cprov_archive.getSourcesForDeletion(name='ice').count()
+    >>> cprov_archive.getSourcesForDeletion(name=u'ice').count()
     1
 
 The status filter can be used to only return sources that can be
 deleted matching a given status.
 
     >>> cprov_archive.getSourcesForDeletion(
-    ...      name='ice', status=PackagePublishingStatus.DELETED).count()
+    ...      name=u'ice', status=PackagePublishingStatus.DELETED).count()
     1
 
     >>> cprov_archive.getSourcesForDeletion(
-    ...      name='ice', status=PackagePublishingStatus.PUBLISHED).count()
+    ...      name=u'ice', status=PackagePublishingStatus.PUBLISHED).count()
     0
 
 The status filter can also be a sequence of status.
 
     >>> irrelevant_status = (
     ...     PackagePublishingStatus.SUPERSEDED,
-    ...     PackagePublishingStatus.DELETED,
-    ...     )
+    ...     PackagePublishingStatus.DELETED)
 
     >>> cprov_archive.getSourcesForDeletion(
-    ...      name='ice', status=irrelevant_status).count()
+    ...      name=u'ice', status=irrelevant_status).count()
     1
 
 The series filter can be used to return only sources from a certain
@@ -655,20 +656,18 @@
     >>> cprov_archive.getSourcesForDeletion(distroseries=hoary).count()
     0
 
-The source publication is only excluded from 'deletion list' when its
-binary is also DELETED.
-
-    >>> for bin in removal_candidate.getPublishedBinaries():
-    ...     bin.requestDeletion(cprov, 'go away !')
-
-    >>> cprov_archive.getSourcesForDeletion(name='ice').count()
+The source publication is only excluded from 'deletion list' when it's
+scheduled deletion date is set.
+
+    >>> from zope.security.proxy import removeSecurityProxy
+    >>> removeSecurityProxy(removal_candidate).scheduleddeletiondate = UTC_NOW
+    >>> cprov_archive.getSourcesForDeletion(name=u'ice').count()
     0
 
 Flush the database caches to invalidate old caches from the
 corresponding publishing Postgres views.
 
-    >>> from lp.services.database.sqlbase import flush_database_caches
-    >>> flush_database_caches()
+    >>> transaction.commit()
 
 
 Build Lookup

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2013-03-07 01:09:59 +0000
+++ lib/lp/soyuz/model/archive.py	2013-03-18 04:11:22 +0000
@@ -23,7 +23,6 @@
     IntCol,
     StringCol,
     )
-from sqlobject.sqlbuilder import SQLConstant
 from storm.expr import (
     And,
     Desc,
@@ -75,6 +74,7 @@
 from lp.registry.model.sourcepackagename import SourcePackageName
 from lp.registry.model.teammembership import TeamParticipation
 from lp.services.config import config
+from lp.services.database.bulk import load_related
 from lp.services.database.constants import UTC_NOW
 from lp.services.database.datetimecol import UtcDateTimeCol
 from lp.services.database.decoratedresultset import DecoratedResultSet
@@ -87,7 +87,6 @@
 from lp.services.database.lpstorm import ISlaveStore
 from lp.services.database.sqlbase import (
     cursor,
-    quote,
     quote_like,
     SQLBase,
     sqlvalues,
@@ -608,70 +607,40 @@
                 list(store.find(GPGKey, GPGKey.id.is_in(ids)))
         return DecoratedResultSet(resultset, pre_iter_hook=eager_load)
 
-    def getSourcesForDeletion(self, name=None, status=None,
-            distroseries=None):
+    def getSourcesForDeletion(self, name=None, status=None, distroseries=None):
         """See `IArchive`."""
-        clauses = ["""
-            SourcePackagePublishingHistory.archive = %s AND
-            SourcePackagePublishingHistory.sourcepackagerelease =
-                SourcePackageRelease.id AND
-            SourcePackagePublishingHistory.sourcepackagename =
-                SourcePackageName.id
-        """ % sqlvalues(self)]
-
-        has_published_binaries_clause = """
-            EXISTS (SELECT TRUE FROM
-                BinaryPackagePublishingHistory bpph,
-                BinaryPackageRelease bpr, BinaryPackageBuild
-            WHERE
-                bpph.archive = %s AND
-                bpph.status = %s AND
-                bpph.binarypackagerelease = bpr.id AND
-                bpr.build = BinaryPackageBuild.id AND
-                BinaryPackageBuild.source_package_release =
-                    SourcePackageRelease.id)
-        """ % sqlvalues(self, PackagePublishingStatus.PUBLISHED)
-
-        source_deletable_states = (
-            PackagePublishingStatus.PENDING,
-            PackagePublishingStatus.PUBLISHED,
-            )
-        clauses.append("""
-           (%s OR SourcePackagePublishingHistory.status IN %s)
-        """ % (has_published_binaries_clause,
-               quote(source_deletable_states)))
-
-        if status is not None:
+        # We will return sources that can be deleted, or deleted sources that
+        # still have published binaries. We can use scheduleddeletiondate
+        # rather than linking through BPB, BPR and BPPH since we don't condemn
+        # sources until their binaries are all gone due to GPL compliance.
+        clauses = [
+            SourcePackagePublishingHistory.archiveID == self.id,
+            SourcePackagePublishingHistory.sourcepackagereleaseID ==
+                SourcePackageRelease.id,
+            SourcePackagePublishingHistory.sourcepackagenameID ==
+                SourcePackageName.id,
+            SourcePackagePublishingHistory.scheduleddeletiondate == None]
+
+        if status:
             try:
                 status = tuple(status)
             except TypeError:
                 status = (status, )
-            clauses.append("""
-                SourcePackagePublishingHistory.status IN %s
-            """ % sqlvalues(status))
-
-        if distroseries is not None:
-            clauses.append("""
-                SourcePackagePublishingHistory.distroseries = %s
-            """ % sqlvalues(distroseries))
-
-        clauseTables = ['SourcePackageRelease', 'SourcePackageName']
-
-        order_const = "SourcePackageRelease.version"
-        desc_version_order = SQLConstant(order_const + " DESC")
-        orderBy = ['SourcePackageName.name', desc_version_order,
-                   '-SourcePackagePublishingHistory.id']
-
-        if name is not None:
-            clauses.append("""
-                    SourcePackageName.name LIKE '%%' || %s || '%%'
-                """ % quote_like(name))
-
-        preJoins = ['sourcepackagerelease']
-        sources = SourcePackagePublishingHistory.select(
-            ' AND '.join(clauses), clauseTables=clauseTables, orderBy=orderBy,
-            prejoins=preJoins)
-
+            clauses.append(SourcePackagePublishingHistory.status.is_in(status))
+
+        if distroseries:
+            clauses.append(
+                SourcePackagePublishingHistory.distroseriesID == 
+                    distroseries.id)
+
+        if name:
+            clauses.append(SourcePackageName.name.contains_string(name))
+
+        sources = Store.of(self).find(
+            SourcePackagePublishingHistory, *clauses).order_by(
+                SourcePackageName.name, Desc(SourcePackageRelease.version),
+                Desc(SourcePackagePublishingHistory.id))
+        load_related(SourcePackageRelease, sources, ['sourcepackagereleaseID'])
         return sources
 
     @property
@@ -758,9 +727,7 @@
                 BinaryPackageRelease.version = %s
             """ % sqlvalues(version))
         elif ordered:
-            order_const = "BinaryPackageRelease.version"
-            desc_version_order = SQLConstant(order_const + " DESC")
-            orderBy.insert(1, desc_version_order)
+            orderBy.insert(1, Desc(BinaryPackageRelease.version))
 
         if status is not None:
             try:
@@ -992,8 +959,7 @@
             Component.name.is_in(components))
             for (archive, not_used, pocket, components) in deps])
 
-        store = ISlaveStore(BinaryPackagePublishingHistory)
-        return store.find(
+        return ISlaveStore(BinaryPackagePublishingHistory).find(
             BinaryPackagePublishingHistory,
             BinaryPackageName.name == dep_name,
             BinaryPackagePublishingHistory.binarypackagename ==