launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:abolish-quote-like into launchpad:master.

Commit message:
Abolish quote_like

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/400197

The `startswith`/`endswith`/`contains_string` helper methods on `storm.expr.Comparable` are much easier to use in most cases, and avoid some escaping hazards when substituting the output of `quote_like` into SQL statements.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:abolish-quote-like into launchpad:master.

diff --git a/lib/lp/registry/vocabularies.py b/lib/lp/registry/vocabularies.py
index 952899d..a38082b 100644
--- a/lib/lp/registry/vocabularies.py
+++ b/lib/lp/registry/vocabularies.py
@@ -176,13 +176,12 @@ from lp.services.database import bulk
 from lp.services.database.decoratedresultset import DecoratedResultSet
 from lp.services.database.interfaces import IStore
 from lp.services.database.sqlbase import (
-    quote,
-    quote_like,
     SQLBase,
     sqlvalues,
     )
 from lp.services.database.stormexpr import (
     fti_search,
+    rank_by_fti,
     RegexpMatch,
     )
 from lp.services.helpers import shortlist
@@ -310,23 +309,25 @@ class ProductVocabulary(SQLObjectVocabularyBase):
         if query is None or an empty string.
         """
         if query:
-            like_query = query.lower()
-            like_query = "'%%%%' || %s || '%%%%'" % quote_like(like_query)
-            fti_query = quote(query)
+            query = six.ensure_text(query)
             if vocab_filter is None:
                 vocab_filter = []
             where_clause = And(
-                SQL(
-                    "active = 't' AND (name LIKE %s OR fti @@ ftq(%s))" % (
-                        like_query, fti_query)),
+                self._table.active,
+                Or(
+                    self._table.name.contains_string(query.lower()),
+                    fti_search(self._table, query)),
                 ProductSet.getProductPrivacyFilter(
                     getUtility(ILaunchBag).user), *vocab_filter)
-            order_by = SQL(
-                '(CASE name WHEN %s THEN 1 '
-                ' ELSE ts_rank(fti, ftq(%s)) END) DESC, displayname, name'
-                % (fti_query, fti_query))
+            order_by = (
+                Case(
+                    cases=((query, -1),),
+                    expression=self._table.name,
+                    default=rank_by_fti(self._table, query)),
+                self._table.display_name,
+                self._table.name)
             return IStore(Product).find(self._table, where_clause).order_by(
-                order_by).config(limit=100)
+                *order_by).config(limit=100)
 
         return self.emptySelectResults()
 
@@ -368,12 +369,13 @@ class ProjectGroupVocabulary(SQLObjectVocabularyBase):
         if query is None or an empty string.
         """
         if query:
-            like_query = query.lower()
-            like_query = "'%%' || %s || '%%'" % quote_like(like_query)
-            fti_query = quote(query)
-            sql = "active = 't' AND (name LIKE %s OR fti @@ ftq(%s))" % (
-                    like_query, fti_query)
-            return self._table.select(sql)
+            query = six.ensure_text(query)
+            return IStore(self._table).find(
+                self._table,
+                self._table.active,
+                Or(
+                    self._table.name.contains_string(query.lower()),
+                    fti_search(self._table, query)))
         return self.emptySelectResults()
 
 
@@ -1526,12 +1528,12 @@ class DistributionVocabulary(NamedSQLObjectVocabulary):
         if not query:
             return self.emptySelectResults()
 
-        query = query.lower()
-        like_query = "'%%' || %s || '%%'" % quote_like(query)
-        kw = {}
+        rows = IStore(self._table).find(
+            self._table,
+            self._table.name.contains_string(six.ensure_text(query).lower()))
         if self._orderBy:
-            kw['orderBy'] = self._orderBy
-        return self._table.select("name LIKE %s" % like_query, **kw)
+            rows = rows.order_by(self._orderBy)
+        return rows
 
 
 class DistroSeriesVocabulary(NamedSQLObjectVocabulary):
diff --git a/lib/lp/services/database/sqlbase.py b/lib/lp/services/database/sqlbase.py
index 65881f7..5c71a1c 100644
--- a/lib/lp/services/database/sqlbase.py
+++ b/lib/lp/services/database/sqlbase.py
@@ -20,7 +20,6 @@ __all__ = [
     'ISOLATION_LEVEL_REPEATABLE_READ',
     'ISOLATION_LEVEL_SERIALIZABLE',
     'quote',
-    'quote_like',
     'quoteIdentifier',
     'quote_identifier',
     'reset_store',
@@ -41,7 +40,6 @@ from psycopg2.extensions import (
     ISOLATION_LEVEL_SERIALIZABLE,
     )
 import pytz
-import six
 from sqlobject.sqlbuilder import sqlrepr
 import storm
 from storm.databases.postgres import compile as postgres_compile
@@ -71,7 +69,6 @@ from lp.services.database.interfaces import (
     IStoreSelector,
     MAIN_STORE,
     )
-from lp.services.helpers import backslashreplace
 from lp.services.propertycache import clear_property_cache
 
 # Default we want for scripts, and the PostgreSQL default. Note psycopg1 will
@@ -302,7 +299,6 @@ def get_transaction_timestamp(store):
 
 def quote(x):
     r"""Quote a variable ready for inclusion into an SQL statement.
-    Note that you should use quote_like to create a LIKE comparison.
 
     Basic SQL quoting works
 
@@ -366,45 +362,6 @@ def quote(x):
     return sqlrepr(x, 'postgres')
 
 
-def quote_like(x):
-    r"""Quote a variable ready for inclusion in a SQL statement's LIKE clause
-
-    XXX: StuartBishop 2004-11-24:
-    Including the single quotes was a stupid decision.
-
-    To correctly generate a SELECT using a LIKE comparision, we need
-    to make use of the SQL string concatination operator '||' and the
-    quote_like method to ensure that any characters with special meaning
-    to the LIKE operator are correctly escaped.
-
-    >>> "SELECT * FROM mytable WHERE mycol LIKE '%%' || %s || '%%'" \
-    ...     % quote_like('%')
-    "SELECT * FROM mytable WHERE mycol LIKE '%' || E'\\\\%' || '%'"
-
-    Note that we need 2 backslashes to quote, as per the docs on
-    the LIKE operator. This is because, unless overridden, the LIKE
-    operator uses the same escape character as the SQL parser.
-
-    >>> quote_like('100%')
-    "E'100\\\\%'"
-    >>> quote_like('foobar_alpha1')
-    "E'foobar\\\\_alpha1'"
-    >>> quote_like('hello')
-    "E'hello'"
-
-    Only strings are supported by this method.
-
-    >>> quote_like(1)
-    Traceback (most recent call last):
-        [...]
-    TypeError: Not a string (<... 'int'>)
-
-    """
-    if not isinstance(x, six.string_types):
-        raise TypeError('Not a string (%s)' % type(x))
-    return quote(x.replace('%', r'\%').replace('_', r'\_'))
-
-
 def sqlvalues(*values, **kwvalues):
     """Return a tuple of converted sql values for each value in some_tuple.
 
diff --git a/lib/lp/translations/model/translationimportqueue.py b/lib/lp/translations/model/translationimportqueue.py
index b8830ff..8ecd0a1 100644
--- a/lib/lp/translations/model/translationimportqueue.py
+++ b/lib/lp/translations/model/translationimportqueue.py
@@ -28,9 +28,12 @@ import posixpath
 import pytz
 import six
 from storm.expr import (
+    Alias,
     And,
+    Func,
     Or,
     Select,
+    SQL,
     )
 from storm.locals import (
     Bool,
@@ -69,10 +72,7 @@ from lp.services.database.interfaces import (
     ISlaveStore,
     IStore,
     )
-from lp.services.database.sqlbase import (
-    quote,
-    quote_like,
-    )
+from lp.services.database.sqlbase import quote
 from lp.services.database.stormbase import StormBase
 from lp.services.database.stormexpr import IsFalse
 from lp.services.librarian.interfaces.client import ILibrarianClient
@@ -1420,29 +1420,29 @@ class TranslationImportQueue:
         returned by this method.
         """
         importer = getUtility(ITranslationImporter)
-        template_patterns = "(%s)" % ' OR '.join([
-            "path LIKE ('%%' || %s)" % quote_like(suffix)
-            for suffix in importer.template_suffixes])
 
         store = self._getSlaveStore()
-        result = store.execute("""
-            SELECT
-                distroseries,
-                sourcepackagename,
-                productseries,
-                regexp_replace(
-                    regexp_replace(path, '^[^/]*$', ''),
-                    '/[^/]*$',
-                    '') AS directory
-            FROM TranslationImportQueueEntry
-            WHERE %(is_template)s
-            GROUP BY distroseries, sourcepackagename, productseries, directory
-            HAVING bool_and(status = %(blocked)s)
-            ORDER BY distroseries, sourcepackagename, productseries, directory
-            """ % {
-                'blocked': quote(RosettaImportStatus.BLOCKED),
-                'is_template': template_patterns,
-            })
+        TIQE = TranslationImportQueueEntry
+        result = store.find(
+            (TIQE.distroseries_id, TIQE.sourcepackagename_id,
+             TIQE.productseries_id,
+             Alias(
+                 Func("regexp_replace",
+                      Func("regexp_replace", TIQE.path, r"^[^/]*$", ""),
+                      r"/[^/]*$",
+                      ""),
+                 "directory")),
+            Or(*(
+                TIQE.path.endswith(suffix)
+                for suffix in importer.template_suffixes)))
+        result = result.group_by(
+            TIQE.distroseries_id, TIQE.sourcepackagename_id,
+            TIQE.productseries_id, SQL("directory"))
+        result = result.having(
+            Func("bool_and", TIQE.status == RosettaImportStatus.BLOCKED))
+        result = result.order_by(
+            TIQE.distroseries_id, TIQE.sourcepackagename_id,
+            TIQE.productseries_id, SQL("directory"))
 
         return set(result)