launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #12497
[Merge] lp:~wgrant/launchpad/like-totally into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/like-totally into lp:launchpad.
Commit message:
Replace most quote_like callsites with native Storm code.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/like-totally/+merge/126418
This branch removes most uses of quote_like, replacing them with built-in Storm methods like contains_string and endswith, and getting rid of some pretty hideous double- and triple-escaping. A couple of methods were Stormified in the process, and a few quote_like callsites remain because their Stormification is a bit more complicated.
I also obliterated three basically unused findByName methods, and cleaned up DistroSeriesSet a bit.
--
https://code.launchpad.net/~wgrant/launchpad/like-totally/+merge/126418
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/like-totally into lp:launchpad.
=== modified file 'lib/lp/registry/doc/distribution.txt'
--- lib/lp/registry/doc/distribution.txt 2012-06-13 07:35:50 +0000
+++ lib/lp/registry/doc/distribution.txt 2012-09-26 09:54:25 +0000
@@ -261,20 +261,20 @@
expected results:
>>> results = ubuntu.searchBinaryPackages(
- ... "mozilla-firefox", exact_match=True)
+ ... u"mozilla-firefox", exact_match=True)
>>> print [result.name for result in results]
[u'mozilla-firefox']
An exact match search with no matches on any package name returns
an empty result set:
- >>> results = ubuntu.searchBinaryPackages("mozilla", exact_match=True)
+ >>> results = ubuntu.searchBinaryPackages(u"mozilla", exact_match=True)
>>> results.count()
0
Loosening to substring matches gives another result:
- >>> results = ubuntu.searchBinaryPackages("mozilla", exact_match=False)
+ >>> results = ubuntu.searchBinaryPackages(u"mozilla", exact_match=False)
>>> print results[0]
<...DistributionSourcePackageCache instance ...
@@ -286,7 +286,7 @@
The results of searchBinaryPackages() are simply ordered alphabetically
for the moment until we have a better FTI rank to order with.
- >>> results = ubuntu.searchBinaryPackages("m")
+ >>> results = ubuntu.searchBinaryPackages(u"m")
>>> for result in results:
... print result.name
mozilla-firefox
=== modified file 'lib/lp/registry/doc/distroseries.txt'
--- lib/lp/registry/doc/distroseries.txt 2012-09-07 05:13:57 +0000
+++ lib/lp/registry/doc/distroseries.txt 2012-09-26 09:54:25 +0000
@@ -39,12 +39,6 @@
>>> print warty.fullseriesname
Ubuntu Warty
-Or IDistroSeriesSet.findByName:
-
- >>> for distroseries in distroseriesset.findByName("warty"):
- ... print distroseries.name
- warty
-
To get one specific release by name, use queryByName:
>>> ubuntu = getUtility(IDistributionSet).getByName("ubuntu")
@@ -52,18 +46,15 @@
>>> warty = distroseriesset.queryByName(ubuntu, "warty")
>>> warty.name
u'warty'
-
-If the release by that name doesn't exist, None will be returned:
-
- >>> foobar = distroseriesset.queryByName(ubuntu, "foobar")
- >>> print foobar
+ >>> print distroseriesset.queryByName(ubuntu, "foobar")
None
-Or IDistroSeriesSet.findByVersion:
+Or IDistroSeriesSet.queryByVersion:
- >>> for distroseries in distroseriesset.findByVersion("5.04"):
- ... print distroseries.name
+ >>> print distroseriesset.queryByVersion(ubuntu, "5.04").name
hoary
+ >>> print distroseriesset.queryByVersion(ubuntu, "5.05")
+ None
We verify that a distroseries does in fact fully provide IDistroSeries:
=== modified file 'lib/lp/registry/doc/person.txt'
--- lib/lp/registry/doc/person.txt 2012-09-07 17:50:32 +0000
+++ lib/lp/registry/doc/person.txt 2012-09-26 09:54:25 +0000
@@ -1073,13 +1073,13 @@
The results returned can be filtered by providing a token to refine the
search.
- >>> for project in mark.getOwnedProjects(match_name='java'):
+ >>> for project in mark.getOwnedProjects(match_name=u'java'):
... print project.displayname
Tomcat
Searching for a non-existent project returns no matches.
- >>> list(mark.getOwnedProjects(match_name='nosuchthing'))
+ >>> list(mark.getOwnedProjects(match_name=u'nosuchthing'))
[]
=== modified file 'lib/lp/registry/interfaces/distroseries.py'
--- lib/lp/registry/interfaces/distroseries.py 2012-09-26 04:14:01 +0000
+++ lib/lp/registry/interfaces/distroseries.py 2012-09-26 09:54:25 +0000
@@ -142,15 +142,10 @@
The distribution is the context's distribution (which may
the context itself); A version is unique to a distribution.
"""
- found_series = None
- for series in getUtility(IDistroSeriesSet).findByVersion(version):
- if (series.distribution == self._distribution
- and series != self.context):
- # A version is unique to a distribution, but a distroseries
- # may edit itself.
- found_series = series
- break
- return found_series
+ existing = getUtility(IDistroSeriesSet).queryByVersion(
+ self._distribution, version)
+ if existing != self.context:
+ return existing
def _getByAttribute(self, version):
"""Return the content object with the given attribute."""
@@ -1014,12 +1009,6 @@
"""Return a set of distroseriess that can be translated in
rosetta."""
- def findByName(name):
- """Find a DistroSeries by name.
-
- Returns a list of matching distributions, which may be empty.
- """
-
def queryByName(distribution, name):
"""Query a DistroSeries by name.
@@ -1029,10 +1018,13 @@
Returns the matching DistroSeries, or None if not found.
"""
- def findByVersion(version):
- """Find a DistroSeries by version.
-
- Returns a list of matching distributions, which may be empty.
+ def queryByVersion(distribution, version):
+ """Query a DistroSeries by version.
+
+ :distribution: An IDistribution.
+ :name: A string.
+
+ Returns the matching DistroSeries, or None if not found.
"""
def fromSuite(distribution, suite):
=== modified file 'lib/lp/registry/interfaces/sourcepackagename.py'
--- lib/lp/registry/interfaces/sourcepackagename.py 2011-12-24 16:54:44 +0000
+++ lib/lp/registry/interfaces/sourcepackagename.py 2012-09-26 09:54:25 +0000
@@ -58,9 +58,6 @@
def getAll():
"""return an iselectresults representing all package names"""
- def findByName(name):
- """Find sourcepackagenames by its name or part of it"""
-
def queryByName(name):
"""Get a sourcepackagename by its name atttribute.
=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py 2012-09-17 15:19:10 +0000
+++ lib/lp/registry/model/distribution.py 2012-09-26 09:54:25 +0000
@@ -154,7 +154,6 @@
from lp.services.database.sqlbase import (
cursor,
quote,
- quote_like,
SQLBase,
sqlvalues,
)
@@ -1120,12 +1119,6 @@
SourcePackageName.id),
]
- # quote_like SQL-escapes the string in addition to LIKE-escaping
- # it, so we can't use params=. So we need to double-escape the %
- # on either side of the string: once to survive the formatting
- # here, and once to survive Storm's formatting during
- # compilation. Storm should really %-escape literal SQL strings,
- # but it doesn't.
conditions = [
DistributionSourcePackageCache.distribution == self,
DistributionSourcePackageCache.archiveID.is_in(
@@ -1133,8 +1126,8 @@
Or(
SQL("DistributionSourcePackageCache.fti @@ ftq(?)",
params=(text,)),
- SQL("DistributionSourcePackageCache.name "
- "LIKE '%%%%' || %s || '%%%%'" % quote_like(text.lower())),
+ DistributionSourcePackageCache.name.contains_string(
+ text.lower()),
),
]
=== modified file 'lib/lp/registry/model/distroseries.py'
--- lib/lp/registry/model/distroseries.py 2012-09-26 04:14:01 +0000
+++ lib/lp/registry/model/distroseries.py 2012-09-26 09:54:25 +0000
@@ -25,10 +25,11 @@
SQLRelatedJoin,
StringCol,
)
-from storm.locals import (
+from storm.expr import (
And,
Desc,
Join,
+ Or,
SQL,
)
from storm.store import (
@@ -105,7 +106,6 @@
flush_database_caches,
flush_database_updates,
quote,
- quote_like,
SQLBase,
sqlvalues,
)
@@ -1239,13 +1239,12 @@
# raised.
package_caches = store.using(*origin).find(
find_spec,
- """DistroSeriesPackageCache.distroseries = %s AND
- DistroSeriesPackageCache.archive IN %s AND
- (fti @@ ftq(%s) OR
- DistroSeriesPackageCache.name ILIKE '%%' || %s || '%%')
- """ % (quote(self),
- quote(self.distribution.all_distro_archive_ids),
- quote(text), quote_like(text)),
+ DistroSeriesPackageCache.distroseries == self,
+ DistroSeriesPackageCache.archiveID.is_in(
+ self.distribution.all_distro_archive_ids),
+ Or(
+ SQL("DistroSeriesPackageCache.fti @@ ftq(?)", params=(text,)),
+ DistroSeriesPackageCache.name.contains_string(text.lower())),
).config(distinct=True)
# Create a function that will decorate the results, converting
@@ -1646,17 +1645,14 @@
result_set = result_set.config(distinct=True)
return result_set
- def findByName(self, name):
- """See `IDistroSeriesSet`."""
- return DistroSeries.selectBy(name=name)
-
def queryByName(self, distribution, name):
"""See `IDistroSeriesSet`."""
return DistroSeries.selectOneBy(distribution=distribution, name=name)
- def findByVersion(self, version):
+ def queryByVersion(self, distribution, version):
"""See `IDistroSeriesSet`."""
- return DistroSeries.selectBy(version=version)
+ return DistroSeries.selectOneBy(
+ distribution=distribution, version=version)
def _parseSuite(self, suite):
"""Parse 'suite' into a series name and a pocket."""
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2012-09-19 01:19:35 +0000
+++ lib/lp/registry/model/person.py 2012-09-26 09:54:25 +0000
@@ -257,7 +257,6 @@
from lp.services.database.sqlbase import (
cursor,
quote,
- quote_like,
SQLBase,
sqlvalues,
)
@@ -1161,29 +1160,23 @@
# Import here to work around a circular import problem.
from lp.registry.model.product import Product
- clauses = ["""
- SELECT DISTINCT Product.id
- FROM Product, TeamParticipation
- WHERE TeamParticipation.person = %(person)s
- AND owner = TeamParticipation.team
- AND Product.active IS TRUE
- """ % sqlvalues(person=self)]
+ clauses = [
+ Product.active == True,
+ Product._ownerID == TeamParticipation.teamID,
+ TeamParticipation.person == self,
+ ]
# We only want to use the extra query if match_name is not None and it
# is not the empty string ('' or u'').
if match_name:
- like_query = "'%%' || %s || '%%'" % quote_like(match_name)
- quoted_query = quote(match_name)
clauses.append(
- """(Product.name LIKE %s OR
- Product.displayname LIKE %s OR
- fti @@ ftq(%s))""" % (like_query,
- like_query,
- quoted_query))
- query = " AND ".join(clauses)
- results = Product.select("""id IN (%s)""" % query,
- orderBy=['displayname'])
- return results
+ Or(
+ Product.name.contains_string(match_name),
+ Product.displayname.contains_string(match_name),
+ SQL("Product.fti @@ ftq(?)", params=(match_name))))
+ return IStore(Product).find(
+ Product, *clauses
+ ).config(distinct=True).order_by(Product.displayname)
def isAnyPillarOwner(self):
"""See IPerson."""
=== modified file 'lib/lp/registry/model/sourcepackagename.py'
--- lib/lp/registry/model/sourcepackagename.py 2011-12-30 06:14:56 +0000
+++ lib/lp/registry/model/sourcepackagename.py 2012-09-26 09:54:25 +0000
@@ -29,7 +29,6 @@
)
from lp.services.database.sqlbase import (
cursor,
- quote_like,
SQLBase,
sqlvalues,
)
@@ -84,11 +83,6 @@
"""See `ISourcePackageNameSet`."""
return SourcePackageName.select()
- def findByName(self, name):
- """Find sourcepackagenames by its name or part of it."""
- query = "name ILIKE '%%' || %s || '%%'" % quote_like(name)
- return SourcePackageName.select(query)
-
def queryByName(self, name):
"""See `ISourcePackageNameSet`."""
return SourcePackageName.selectOneBy(name=name)
=== modified file 'lib/lp/soyuz/doc/distroarchseries.txt'
--- lib/lp/soyuz/doc/distroarchseries.txt 2012-06-19 22:40:41 +0000
+++ lib/lp/soyuz/doc/distroarchseries.txt 2012-09-26 09:54:25 +0000
@@ -67,7 +67,7 @@
Check the behavior of the provided search method, which returns a
list of IDARBPR instances containing the matching packages.
- >>> results = hoary_i386.searchBinaryPackages(text='pmount')
+ >>> results = hoary_i386.searchBinaryPackages(text=u'pmount')
>>> results.count()
1
>>> pmount = results[0]
@@ -77,10 +77,10 @@
>>> warty = ubuntu.getSeries('warty')
>>> warty_i386 = warty['i386']
- >>> results = warty_i386.searchBinaryPackages(text='linux-2.6.12')
+ >>> results = warty_i386.searchBinaryPackages(text=u'linux-2.6.12')
>>> results.count()
1
- >>> results = warty_i386.searchBinaryPackages(text='a')
+ >>> results = warty_i386.searchBinaryPackages(text=u'a')
>>> for dasbp in results:
... print "%s: %s" % (dasbp.__class__.__name__, dasbp.name)
DistroArchSeriesBinaryPackageRelease: at
=== modified file 'lib/lp/soyuz/doc/package-cache.txt'
--- lib/lp/soyuz/doc/package-cache.txt 2012-06-13 07:35:50 +0000
+++ lib/lp/soyuz/doc/package-cache.txt 2012-09-26 09:54:25 +0000
@@ -198,7 +198,7 @@
>>> foobar_bin_pub.status.name
'DELETED'
- >>> warty.searchPackages('foobar').count()
+ >>> warty.searchPackages(u'foobar').count()
1
>>> foobar_bin_cache = DistroSeriesPackageCache.selectOneBy(
@@ -216,7 +216,7 @@
>>> transaction.commit()
- >>> warty.searchPackages('foobar').count()
+ >>> warty.searchPackages(u'foobar').count()
0
>>> foobar_bin_cache = DistroSeriesPackageCache.selectOneBy(
@@ -233,7 +233,7 @@
>>> cdrkit_bin_pub.status.name
'PUBLISHED'
- >>> warty.searchPackages('cdrkit').count()
+ >>> warty.searchPackages(u'cdrkit').count()
0
>>> cdrkit_bin_cache = DistroSeriesPackageCache.selectOneBy(
@@ -266,7 +266,7 @@
Now we see that the 'cdrkit' binary is part of the caches and can be
reached via searches:
- >>> warty.searchPackages('cdrkit').count()
+ >>> warty.searchPackages(u'cdrkit').count()
1
>>> cdrkit_bin_cache = DistroSeriesPackageCache.selectOneBy(
=== modified file 'lib/lp/soyuz/interfaces/binarypackagename.py'
--- lib/lp/soyuz/interfaces/binarypackagename.py 2011-12-24 16:54:44 +0000
+++ lib/lp/soyuz/interfaces/binarypackagename.py 2012-09-26 09:54:25 +0000
@@ -41,9 +41,6 @@
def getAll():
"""return an iselectresults representing all package names"""
- def findByName(name):
- """Find binarypackagenames by its name or part of it"""
-
def queryByName(name):
"""Return a binary package name.
=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py 2012-09-25 07:34:46 +0000
+++ lib/lp/soyuz/model/archive.py 2012-09-26 09:54:25 +0000
@@ -517,8 +517,7 @@
storm_clauses.append(SourcePackageName.name == name)
else:
clauses.append(
- "SourcePackageName.name LIKE '%%%%' || %s || '%%%%'"
- % quote_like(name))
+ SourcePackageName.name.contains_string(name))
elif len(name) != 0:
clauses.append(
"SourcePackageName.name IN %s"
=== modified file 'lib/lp/soyuz/model/binarypackagebuild.py'
--- lib/lp/soyuz/model/binarypackagebuild.py 2012-09-25 23:54:26 +0000
+++ lib/lp/soyuz/model/binarypackagebuild.py 2012-09-26 09:54:25 +0000
@@ -56,7 +56,6 @@
IStore,
)
from lp.services.database.sqlbase import (
- quote_like,
SQLBase,
sqlvalues,
)
@@ -979,8 +978,7 @@
origin.extend([SourcePackageRelease, SourcePackageName])
if not isinstance(name, (list, tuple)):
clauses.append(
- "SourcepackageName.name LIKE '%%%%' || %s || '%%%%'" % (
- quote_like(name)))
+ SourcePackageName.name.contains_string(name))
else:
clauses.append(SourcePackageName.name.is_in(name))
=== modified file 'lib/lp/soyuz/model/binarypackagename.py'
--- lib/lp/soyuz/model/binarypackagename.py 2012-02-28 04:24:19 +0000
+++ lib/lp/soyuz/model/binarypackagename.py 2012-09-26 09:54:25 +0000
@@ -67,12 +67,6 @@
"""See `IBinaryPackageNameSet`."""
return BinaryPackageName.select()
- def findByName(self, name):
- """Find binarypackagenames by its name or part of it."""
- return IStore(BinaryPackageName).find(
- BinaryPackageName,
- BinaryPackageName.name.contains_string(ensure_unicode(name)))
-
def queryByName(self, name):
return IStore(BinaryPackageName).find(
BinaryPackageName, name=ensure_unicode(name)).one()
=== modified file 'lib/lp/soyuz/model/distroarchseries.py'
--- lib/lp/soyuz/model/distroarchseries.py 2012-07-05 09:27:23 +0000
+++ lib/lp/soyuz/model/distroarchseries.py 2012-09-26 09:54:25 +0000
@@ -19,6 +19,7 @@
)
from storm.locals import (
Join,
+ Or,
SQL,
)
from storm.store import EmptyResultSet
@@ -31,8 +32,6 @@
from lp.services.database.decoratedresultset import DecoratedResultSet
from lp.services.database.enumcol import EnumCol
from lp.services.database.sqlbase import (
- quote,
- quote_like,
SQLBase,
sqlvalues,
)
@@ -203,21 +202,18 @@
)
archives = self.distroseries.distribution.getArchiveIDList()
- # Note: When attempting to convert the query below into straight
- # Storm expressions, a 'tuple index out-of-range' error was always
- # raised.
- query = """
- BinaryPackagePublishingHistory.distroarchseries = %s AND
- BinaryPackagePublishingHistory.archive IN %s AND
- BinaryPackagePublishingHistory.dateremoved is NULL
- """ % (quote(self), quote(archives))
+ clauses = [
+ BinaryPackagePublishingHistory.distroarchseries == self,
+ BinaryPackagePublishingHistory.archiveID.is_in(archives),
+ BinaryPackagePublishingHistory.dateremoved == None,
+ ]
if text:
- query += """
- AND (BinaryPackageRelease.fti @@ ftq(%s) OR
- BinaryPackageName.name ILIKE '%%' || %s || '%%')
- """ % (quote(text), quote_like(text))
+ clauses.append(
+ Or(
+ SQL("BinaryPackageRelease.fti @@ ftq(?)", params=(text,)),
+ BinaryPackageName.name.contains_string(text.lower())))
result = store.using(*origin).find(
- find_spec, query).config(distinct=True)
+ find_spec, *clauses).config(distinct=True)
if text:
result = result.order_by("rank DESC, BinaryPackageName.name")
=== modified file 'lib/lp/soyuz/tests/test_binarypackagename.py'
--- lib/lp/soyuz/tests/test_binarypackagename.py 2012-01-01 02:58:52 +0000
+++ lib/lp/soyuz/tests/test_binarypackagename.py 2012-09-26 09:54:25 +0000
@@ -38,17 +38,6 @@
name = self.factory.makeBinaryPackageName()
self.assertIn(name, self.name_set.getAll())
- def test_findByName_not_found(self):
- self.assertEqual([], list(self.name_set.findByName("notfound")))
-
- def test_findByName_found(self):
- name1 = self.factory.makeBinaryPackageName("prefixname")
- name2 = self.factory.makeBinaryPackageName("name")
- name3 = self.factory.makeBinaryPackageName("namesuffix")
- name4 = self.factory.makeBinaryPackageName("other")
- self.assertEqual(
- [name1, name2, name3], list(self.name_set.findByName("name")))
-
def test_queryByName_not_found(self):
self.assertEqual(None, self.name_set.queryByName("notfound"))
=== modified file 'lib/lp/translations/model/potemplate.py'
--- lib/lp/translations/model/potemplate.py 2012-06-29 08:40:05 +0000
+++ lib/lp/translations/model/potemplate.py 2012-09-26 09:54:25 +0000
@@ -69,7 +69,6 @@
from lp.services.database.sqlbase import (
flush_database_updates,
quote,
- quote_like,
SQLBase,
sqlvalues,
)
@@ -1252,9 +1251,10 @@
def findUniquePathlessMatch(self, filename):
"""See `IPOTemplateSubset`."""
result = self._build_query(
- ("(POTemplate.path = %s OR POTemplate.path LIKE '%%%%/' || %s)"
- % (quote(filename), quote_like(filename))),
- ordered=False)
+ Or(
+ POTemplate.path == filename,
+ POTemplate.path.endswith(u'/%s' % filename)),
+ ordered=False)
candidates = list(result.config(limit=2))
if len(candidates) == 1:
=== modified file 'lib/lp/translations/scripts/tests/test_copy_distroseries_translations.py'
--- lib/lp/translations/scripts/tests/test_copy_distroseries_translations.py 2012-01-01 02:58:52 +0000
+++ lib/lp/translations/scripts/tests/test_copy_distroseries_translations.py 2012-09-26 09:54:25 +0000
@@ -11,7 +11,7 @@
from zope.component import getUtility
-from lp.registry.interfaces.distroseries import IDistroSeriesSet
+from lp.registry.interfaces.distribution import IDistributionSet
from lp.testing.faketransaction import FakeTransaction
from lp.testing.layers import LaunchpadZopelessLayer
from lp.translations.scripts.copy_distroseries_translations import (
@@ -25,33 +25,28 @@
def test_flagsHandling(self):
"""Flags are correctly restored, no matter what their values."""
- series_set = getUtility(IDistroSeriesSet)
- sid = series_set.findByName('sid')[0]
+ sid = getUtility(IDistributionSet)['debian']['sid']
sid.hide_all_translations = True
sid.defer_translation_imports = True
copy_distroseries_translations(sid, self.txn, logging)
- sid = series_set.findByName('sid')[0]
self.assertTrue(sid.hide_all_translations)
self.assertTrue(sid.defer_translation_imports)
sid.hide_all_translations = True
sid.defer_translation_imports = False
copy_distroseries_translations(sid, self.txn, logging)
- sid = series_set.findByName('sid')[0]
self.assertTrue(sid.hide_all_translations)
self.assertFalse(sid.defer_translation_imports)
sid.hide_all_translations = False
sid.defer_translation_imports = True
copy_distroseries_translations(sid, self.txn, logging)
- sid = series_set.findByName('sid')[0]
self.assertFalse(sid.hide_all_translations)
self.assertTrue(sid.defer_translation_imports)
sid.hide_all_translations = False
sid.defer_translation_imports = False
copy_distroseries_translations(sid, self.txn, logging)
- sid = series_set.findByName('sid')[0]
self.assertFalse(sid.hide_all_translations)
self.assertFalse(sid.defer_translation_imports)
Follow ups
