launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/get_bug_tags-with-added-sanity into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/get_bug_tags-with-added-sanity/+merge/117578

get_bug_tags() is horrible. It uses a global string that contains an SQL fragment, which then has bits replaced and executed. Interfaces that implement IHasBug then call into get_bug_tags passing in strings which get added onto said SQL fragment. This is beyond horrible. I have replaced it with shiny Storm pieces.
-- 
https://code.launchpad.net/~stevenk/launchpad/get_bug_tags-with-added-sanity/+merge/117578
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/get_bug_tags-with-added-sanity into lp:launchpad.

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2012-07-31 22:13:45 +0000
+++ lib/lp/bugs/model/bug.py	2012-08-01 08:10:26 +0000
@@ -197,7 +197,6 @@
 from lp.services.database.enumcol import EnumCol
 from lp.services.database.lpstorm import IStore
 from lp.services.database.sqlbase import (
-    cursor,
     SQLBase,
     sqlvalues,
     )
@@ -228,11 +227,6 @@
 from lp.services.webapp.interfaces import ILaunchBag
 
 
-_bug_tag_query_template = """
-        SELECT %(columns)s FROM %(tables)s WHERE
-            %(condition)s GROUP BY BugTag.tag ORDER BY BugTag.tag"""
-
-
 def snapshot_bug_params(bug_params):
     """Return a snapshot of a `CreateBugParams` object."""
     return Snapshot(
@@ -254,20 +248,16 @@
 def get_bug_tags(context_clause):
     """Return all the bug tags as a list of strings.
 
-    context_clause is a SQL condition clause, limiting the tags to a
-    specific context. The SQL clause can only use the BugTask table to
-    choose the context.
+    context_clause is a Storm clause, limiting the tags to a specific
+    context, which can only use the BugTask table to choose the context.
     """
-    from_tables = ['BugTag', 'BugTask']
-    select_columns = ['BugTag.tag']
-    conditions = ['BugTag.bug = BugTask.bug', '(%s)' % context_clause]
+    # Circular imports.
+    from lp.bugs.model.bugtask import BugTask
 
-    cur = cursor()
-    cur.execute(_bug_tag_query_template % dict(
-            columns=', '.join(select_columns),
-            tables=', '.join(from_tables),
-            condition=' AND '.join(conditions)))
-    return shortlist([row[0] for row in cur.fetchall()])
+    return list(IStore(BugTag).find(
+        BugTag.tag,
+        BugTag.bugID == BugTask.bugID, context_clause).group_by(
+            BugTag.tag).order_by(BugTag.tag))
 
 
 def get_bug_tags_open_count(context_condition, user, tag_limit=0,

=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py	2012-07-27 04:35:50 +0000
+++ lib/lp/registry/model/distribution.py	2012-08-01 08:10:26 +0000
@@ -89,6 +89,7 @@
     BugTargetBase,
     OfficialBugTagTargetMixin,
     )
+from lp.bugs.model.bugtask import BugTask
 from lp.bugs.model.structuralsubscription import (
     StructuralSubscriptionTargetMixin,
     )
@@ -628,7 +629,7 @@
 
     def getUsedBugTags(self):
         """See `IBugTarget`."""
-        return get_bug_tags("BugTask.distribution = %s" % sqlvalues(self))
+        return get_bug_tags(BugTask.distributionID == self.id)
 
     def getBranchTips(self, user=None, since=None):
         """See `IDistribution`."""

=== modified file 'lib/lp/registry/model/distroseries.py'
--- lib/lp/registry/model/distroseries.py	2012-07-05 09:43:58 +0000
+++ lib/lp/registry/model/distroseries.py	2012-08-01 08:10:26 +0000
@@ -57,6 +57,7 @@
 from lp.bugs.interfaces.bugtaskfilter import OrderedBugTask
 from lp.bugs.model.bug import get_bug_tags
 from lp.bugs.model.bugtarget import BugTargetBase
+from lp.bugs.model.bugtask import BugTask
 from lp.bugs.model.structuralsubscription import (
     StructuralSubscriptionTargetMixin,
     )
@@ -782,7 +783,7 @@
 
     def getUsedBugTags(self):
         """See `IHasBugs`."""
-        return get_bug_tags("BugTask.distroseries = %s" % sqlvalues(self))
+        return get_bug_tags(BugTask.distroseriesID == self.id)
 
     @property
     def has_any_specifications(self):

=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2012-07-26 04:36:03 +0000
+++ lib/lp/registry/model/product.py	2012-08-01 08:10:26 +0000
@@ -884,7 +884,7 @@
 
     def getUsedBugTags(self):
         """See `IBugTarget`."""
-        return get_bug_tags("BugTask.product = %s" % sqlvalues(self))
+        return get_bug_tags(BugTask.productID == self.id)
 
     series = SQLMultipleJoin('ProductSeries', joinColumn='product',
         orderBy='name')

=== modified file 'lib/lp/registry/model/productseries.py'
--- lib/lp/registry/model/productseries.py	2012-02-08 06:00:46 +0000
+++ lib/lp/registry/model/productseries.py	2012-08-01 08:10:26 +0000
@@ -54,6 +54,7 @@
 from lp.bugs.interfaces.bugtaskfilter import OrderedBugTask
 from lp.bugs.model.bug import get_bug_tags
 from lp.bugs.model.bugtarget import BugTargetBase
+from lp.bugs.model.bugtask import BugTask
 from lp.bugs.model.structuralsubscription import (
     StructuralSubscriptionTargetMixin,
     )
@@ -443,7 +444,7 @@
 
     def getUsedBugTags(self):
         """See IBugTarget."""
-        return get_bug_tags("BugTask.productseries = %s" % sqlvalues(self))
+        return get_bug_tags(BugTask.productseriesID == self.id)
 
     def createBug(self, bug_params):
         """See IBugTarget."""

=== modified file 'lib/lp/registry/model/projectgroup.py'
--- lib/lp/registry/model/projectgroup.py	2012-04-16 23:02:44 +0000
+++ lib/lp/registry/model/projectgroup.py	2012-08-01 08:10:26 +0000
@@ -61,6 +61,7 @@
     BugTargetBase,
     OfficialBugTag,
     )
+from lp.bugs.model.bugtask import BugTask
 from lp.bugs.model.structuralsubscription import (
     StructuralSubscriptionTargetMixin,
     )
@@ -337,9 +338,8 @@
         """See `IHasBugs`."""
         if not self.products:
             return []
-        product_ids = sqlvalues(*self.products)
         return get_bug_tags(
-            "BugTask.product IN (%s)" % ",".join(product_ids))
+            BugTask.productID.is_in([p.id for p in self.products]))
 
     def getBugSummaryContextWhereClause(self):
         """See BugTargetBase."""