Merge lp:~henninge/launchpad/bug-461818 into lp:launchpad

[Henning Eggers]

[Henning Eggers (henninge)]

= Bug 461818 =

The +templates page for distribution series was timing out with lots of storm objects being created by single-row SourcePackageName queries. This happened because the list of source packages for an Ubuntu distribution is quite long, obviously, and displays a SourcePackageName.name per line.

== Proposed solution ==

Prejoin SourcePackageName when querying the POTemplates for the list. That will prevent storm from fetching each SourcePackageName in its own query when it is referenced in the page template. The query happens in POTemplageSubSet.

== Implementation details ==

While the solution is quite straight forward, the implementation became a bit more complex because I decided to convert POTemplageSubSet to use storm and to do some clean-ups.

I made use of the "precache" function for storm which I renamed to "prejoin" as Stuart had suggested. You will find the result of this in the first 4 files of the patch. It also fixes a bug in prejoin that made it fail when "one()" was used on None which happens when the result set is empty. Really, I think storm should be returning an EmptyResultSet, but that is not the subject here.

The next (5th) file is model/potemplate.py which has the meat of this branch. Note that I moved the actual construction of the storm query into its own method so that only the base clauses are collected in the constructor. Note also, how I got rid of the useless sorting by path. The rest of the file translates quite nicely into storm.

One more point to note in this file is getAllOrderByDateLastUpdated which used to reconstruct its own clauses with the difference that it did not take from_sourcepackage into account. I replaced that with the standard query for the SubSet (which does prefer from_sourcepackage). This did not break any tests but I am not all sure about the consequences.

== Test ==

I ran all of lp.translations on it because POTemplateSubSet is so widely used.

bin/test -vvt lp.translations -t prejoin

== Demo/QA ==

There should be no more timeouts on this page:
https://translations.staging.launchpad.net/ubuntu/karmic/+templates

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/registry/model/product.py
  lib/lp/services/database/configure.zcml
  lib/lp/services/database/prejoin.py
  lib/lp/services/database/tests/test_prejoin.py
  lib/lp/translations/model/potemplate.py

== Pylint notices ==

lib/lp/registry/model/product.py
    29: [F0401] Unable to import 'lazr.delegates' (No module named delegates)

lib/lp/services/database/prejoin.py
    11: [F0401] Unable to import 'lazr.delegates' (No module named delegates)

[Jeroen T. Vermeulen (jtv)]

Good branch overall, with some much-needed cleanup. A few changes discussed on IRC:

 * just_for_count is ugly; replace with do_prefetch and use the existing ordering parameter if necessary.

 * A comment needed work.

 * It'd be nice, though not absolutely necessary at this point, to have the SQL conditions rewritten in Storm.

Jeroen

review: Approve (code)

=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2009-10-16 15:00:55 +0000
+++ lib/lp/registry/model/product.py	2009-10-28 16:28:12 +0000
@@ -63,7 +63,7 @@
 from lp.registry.model.productlicense import ProductLicense
 from lp.registry.model.productrelease import ProductRelease
 from lp.registry.model.productseries import ProductSeries
-from lp.services.database.precache import precache
+from lp.services.database.prejoin import prejoin
 from lp.answers.model.question import (
     QuestionTargetSearch, QuestionTargetMixin)
 from lp.blueprints.model.specification import (
@@ -1293,7 +1293,7 @@
 
         # We only want Product - the other tables are just to populate
         # the cache.
-        return precache(results)
+        return prejoin(results)
 
     def featuredTranslatables(self, maximumproducts=8):
         """See `IProductSet`"""

=== modified file 'lib/lp/services/database/configure.zcml'
--- lib/lp/services/database/configure.zcml	2009-07-17 02:25:09 +0000
+++ lib/lp/services/database/configure.zcml	2009-10-28 16:28:12 +0000
@@ -8,7 +8,7 @@
     xmlns:i18n="http://namespaces.zope.org/i18n"
     i18n_domain="launchpad">
 
-    <class class="lp.services.database.precache.PrecacheResultSet">
+    <class class="lp.services.database.prejoin.PrejoinResultSet">
         <allow interface="storm.zope.interfaces.IResultSet" />
         <allow attributes="__getslice__" />
     </class>

=== renamed file 'lib/lp/services/database/precache.py' => 'lib/lp/services/database/prejoin.py'
--- lib/lp/services/database/precache.py	2009-10-05 11:33:04 +0000
+++ lib/lp/services/database/prejoin.py	2009-10-28 16:28:12 +0000
@@ -1,21 +1,17 @@
 # Copyright 2009 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-"""precache for Storm.
-
-XXX stub 2009-06-18 bug=388798: This feature may be misnamed, should be
-moved to a better location, and needs tests.
-"""
+"""Prejoin for Storm."""
 
 __metaclass__ = type
-__all__ = ['precache']
+__all__ = ['prejoin']
 
 from storm.zope.interfaces import IResultSet
 
 from lazr.delegates import delegates
 
-class PrecacheResultSet:
-    """Precache support.
+class PrejoinResultSet:
+    """Prejoin support.
 
     Wrap a ResultSet, trimming unwanted items. The unwanted items
     are still pulled from the database and populate the Storm caches.
@@ -25,7 +21,7 @@
     The preferred solution is support in Storm core, so we can just do
     something like:
 
-    >>> results = store.find(Product).precache(
+    >>> results = store.find(Product).prejoin(
     ...     (Person, EmailAddress),
     ...     Product._ownerID == Person.id,
     ...     EmailAddress.personID == Person.id)
@@ -36,9 +32,11 @@
         self.return_slice = return_slice
 
     def _chain(self, result_set):
-        return PrecacheResultSet(result_set, self.return_slice)
+        return PrejoinResultSet(result_set, self.return_slice)
 
     def _chomp(self, row):
+        if row is None:
+            return None
         elems = row[self.return_slice]
         if len(elems) == 1:
             return elems[0]
@@ -113,4 +111,4 @@
         """See `IResultSet`."""
         raise NotImplementedError("cached")
 
-precache = PrecacheResultSet
+prejoin = PrejoinResultSet

=== renamed file 'lib/lp/services/database/tests/test_precache.py' => 'lib/lp/services/database/tests/test_prejoin.py'
--- lib/lp/services/database/tests/test_precache.py	2009-10-05 11:33:04 +0000
+++ lib/lp/services/database/tests/test_prejoin.py	2009-10-28 16:28:12 +0000
@@ -1,24 +1,22 @@
 # Copyright 2009 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-"""precache module tests."""
+"""prejoin module tests."""
 
 __metaclass__ = type
 __all__ = []
 
 import unittest
 
-from storm.expr import And, Join, LeftJoin
-
 from canonical.launchpad.interfaces import IMasterStore
 from canonical.testing.layers import LaunchpadZopelessLayer
 from lp.registry.model.person import Person
 from lp.registry.model.product import Product
-from lp.services.database.precache import precache, PrecacheResultSet
+from lp.services.database.prejoin import prejoin, PrejoinResultSet
 from lp.testing import TestCase
 
 
-class PrecacheTestCase(TestCase):
+class PrejoinTestCase(TestCase):
     layer = LaunchpadZopelessLayer
 
     def setUp(self):
@@ -27,73 +25,82 @@
         # All products
         self.standard_result = self.store.find(Product).order_by(Product.name)
 
-        # All products, with owner and preferred email address precached.
+        # All products, with owner and preferred email address prejoined.
         # Note that because some Product owners have multiple email
-        # addresses, this query returns more results. precache needs
+        # addresses, this query returns more results. prejoin needs
         # to hide this from callsites.
         self.unwrapped_result = self.store.find(
             (Product, Person),
             Product._ownerID == Person.id).order_by(Product.name)
-        self.precache_result = precache(self.unwrapped_result)
+        self.prejoin_result = prejoin(self.unwrapped_result)
 
-    def verify(self, precached, normal):
-        # Ensure our precached result really is a PrecacheResultSet.
+    def verify(self, prejoined, normal):
+        # Ensure our prejoined result really is a PrejoinResultSet.
         # One of our methods might not be sticky, and we have ended up
         # with a normal Storm ResultSet.
         self.assertTrue(
-            isinstance(precached, PrecacheResultSet),
-            "Expected a PrecacheResultSet, got %s" % repr(precached))
+            isinstance(prejoined, PrejoinResultSet),
+            "Expected a PrejoinResultSet, got %s" % repr(prejoined))
 
         # Confirm the two result sets return identical results.
-        self.assertEqual(list(normal), list(precached))
+        self.assertEqual(list(normal), list(prejoined))
 
     def test_count(self):
         self.assertEqual(
             self.standard_result.count(),
-            self.precache_result.count())
+            self.prejoin_result.count())
 
     def test_copy(self):
-        copy = self.precache_result.copy()
+        copy = self.prejoin_result.copy()
         self.verify(copy, self.standard_result)
 
     def test_config(self):
-        self.precache_result.config(offset=3, limit=2)
+        self.prejoin_result.config(offset=3, limit=2)
         self.standard_result.config(offset=3, limit=2)
-        self.verify(self.precache_result, self.standard_result)
+        self.verify(self.prejoin_result, self.standard_result)
 
     def test_config_returnvalue(self):
-        precache_rv = self.precache_result.config(offset=3, limit=2)
+        prejoin_rv = self.prejoin_result.config(offset=3, limit=2)
         standard_rv = self.standard_result.config(offset=3, limit=2)
-        self.verify(precache_rv, standard_rv)
+        self.verify(prejoin_rv, standard_rv)
 
     def test_order_by(self):
         self.verify(
-            self.precache_result.order_by(Product.id),
+            self.prejoin_result.order_by(Product.id),
             self.standard_result.order_by(Product.id))
 
     def test_slice(self):
         self.verify(
-            self.precache_result[4:6],
+            self.prejoin_result[4:6],
             self.standard_result[4:6])
 
     def test_any(self):
-        self.assertIn(self.precache_result.any(), self.standard_result)
+        self.assertIn(self.prejoin_result.any(), self.standard_result)
 
     def test_first(self):
         self.assertEqual(
-            self.standard_result.first(), self.precache_result.first())
+            self.standard_result.first(), self.prejoin_result.first())
 
     def test_one(self):
         standard_result = self.store.find(Product, Product.name == 'firefox')
-        precache_result = precache(self.store.find(
+        prejoin_result = prejoin(self.store.find(
             (Product, Person),
             Person.id == Product._ownerID,
             Product.name == 'firefox'))
-        self.assertEqual(standard_result.one(), precache_result.one())
+        self.assertEqual(standard_result.one(), prejoin_result.one())
+
+    def test_one_empty(self):
+        # For an empty result (None), one returns None, too.
+        name = "none-existent-name"
+        prejoin_result = prejoin(self.store.find(
+            (Product, Person),
+            Person.id == Product._ownerID,
+            Product.name == name))
+        self.assertIs(None, prejoin_result.one())
 
     def test_cache_populated(self):
         # Load a row.
-        product = self.precache_result.first()
+        product = self.prejoin_result.first()
 
         # Destroy our data, without telling Storm. This way we can
         # tell if we are accessing an object from the cache, or if it

=== modified file 'lib/lp/translations/model/potemplate.py'
--- lib/lp/translations/model/potemplate.py	2009-10-27 10:40:46 +0000
+++ lib/lp/translations/model/potemplate.py	2009-10-28 16:28:12 +0000
@@ -24,7 +24,7 @@
 from sqlobject import (
     BoolCol, ForeignKey, IntCol, SQLMultipleJoin, SQLObjectNotFound,
     StringCol)
-from storm.expr import Alias, And, Join, LeftJoin, Or, SQL
+from storm.expr import Alias, And, Desc, Join, LeftJoin, Or, SQL
 from storm.info import ClassAlias
 from storm.store import Store
 from zope.component import getAdapter, getUtility
@@ -40,8 +40,10 @@
 from canonical.launchpad import helpers
 from canonical.launchpad.interfaces.lpstorm import IStore
 from lp.translations.utilities.rosettastats import RosettaStats
+from lp.services.database.prejoin import prejoin
 from lp.services.worlddata.model.language import Language
 from lp.registry.interfaces.person import validate_public_person
+from lp.registry.model.sourcepackagename import SourcePackageName
 from lp.translations.model.pofile import POFile, DummyPOFile
 from lp.translations.model.pomsgid import POMsgID
 from lp.translations.model.potmsgset import POTMsgSet
@@ -1002,8 +1004,8 @@
         self.distroseries = distroseries
         self.productseries = productseries
         self.iscurrent = iscurrent
-        self.clausetables = []
-        self.orderby = ['id']
+        self.orderby = [POTemplate.id]
+        self.clauses = []
 
         assert productseries is None or distroseries is None, (
             'A product series must not be used with a distro series.')
@@ -1011,46 +1013,63 @@
         assert productseries is not None or distroseries is not None, (
             'Either productseries or distroseries must be not None.')
 
+        # Construct the base clauses.
         if productseries is not None:
-            self.query = ('POTemplate.productseries = %s' %
-                sqlvalues(productseries.id))
-        elif distroseries is not None and from_sourcepackagename is not None:
-            self.query = ('POTemplate.from_sourcepackagename = %s AND'
-                          ' POTemplate.distroseries = %s ' %
-                            sqlvalues(from_sourcepackagename.id,
-                                      distroseries.id))
-            self.sourcepackagename = from_sourcepackagename
-        elif distroseries is not None and sourcepackagename is not None:
-            self.query = ('POTemplate.sourcepackagename = %s AND'
-                          ' POTemplate.distroseries = %s ' %
-                            sqlvalues(sourcepackagename.id, distroseries.id))
+            self.clauses.append(
+                POTemplate.productseriesID == productseries.id)
         else:
-            self.query = (
-                'POTemplate.distroseries = DistroSeries.id AND'
-                ' DistroSeries.id = %s' % sqlvalues(distroseries.id))
-            self.orderby.append('DistroSeries.name')
-            self.clausetables.append('DistroSeries')
-
+            self.clauses.append(
+                POTemplate.distroseriesID == distroseries.id)
+            if from_sourcepackagename is not None:
+                self.clauses.append(
+                    POTemplate.from_sourcepackagenameID ==
+                        from_sourcepackagename.id)
+                self.sourcepackagename = from_sourcepackagename
+            elif sourcepackagename is not None:
+                self.clauses.append(
+                    POTemplate.sourcepackagename == sourcepackagename.id)
+            else:
+                # Select all POTemplates in a Distroseries.
+                pass
         # Add the filter for the iscurrent flag if requested.
         if iscurrent is not None:
-            self.query += " AND POTemplate.iscurrent=%s" % (
-                            sqlvalues(iscurrent))
-
-        # Finally, we sort the query by its path in all cases.
-        self.orderby.append('POTemplate.path')
+            self.clauses.append(
+                POTemplate.iscurrent == iscurrent)
+
+    def _build_query(self, additional_clause=None,
+                     ordered=True, do_prejoin=True):
+        """Construct the storm query."""
+        if additional_clause is None:
+            condition = And(self.clauses)
+        else:
+            condition = And(self.clauses + [additional_clause])
+
+        if self.productseries is not None:
+            store = Store.of(self.productseries)
+            query = store.find(POTemplate, condition)
+        else:
+            store = Store.of(self.distroseries)
+            if do_prejoin:
+                query = prejoin(store.find(
+                    (POTemplate, SourcePackageName),
+                    (POTemplate.sourcepackagenameID ==
+                     SourcePackageName.id), condition))
+            else:
+                query = store.find(POTemplate, condition)
+
+        if ordered:
+            return query.order_by(self.orderby)
+        return query
 
     def __iter__(self):
         """See `IPOTemplateSubset`."""
-        res = POTemplate.select(self.query, clauseTables=self.clausetables,
-                                orderBy=self.orderby)
-
-        for potemplate in res:
+        for potemplate in self._build_query():
             yield potemplate
 
     def __len__(self):
         """See `IPOTemplateSubset`."""
-        res = POTemplate.select(self.query, clauseTables=self.clausetables)
-        return res.count()
+        result = self._build_query(do_prejoin=False, ordered=False)
+        return result.count()
 
     def __getitem__(self, name):
         """See `IPOTemplateSubset`."""
@@ -1109,24 +1128,16 @@
 
     def getPOTemplateByName(self, name):
         """See `IPOTemplateSubset`."""
-        queries = [self.query]
-        clausetables = list(self.clausetables)
-        queries.append('POTemplate.name = %s' % sqlvalues(name))
-
-        return POTemplate.selectOne(' AND '.join(queries),
-            clauseTables=clausetables)
+        result = self._build_query(POTemplate.name == name, ordered=False)
+        return result.one()
 
     def getPOTemplateByTranslationDomain(self, translation_domain):
         """See `IPOTemplateSubset`."""
-        queries = [self.query]
-        clausetables = list(self.clausetables)
-
-        queries.append('POTemplate.translation_domain = %s' % sqlvalues(
-            translation_domain))
+        query_result = self._build_query(
+            POTemplate.translation_domain == translation_domain)
 
         # Fetch up to 2 templates, to check for duplicates.
-        matches = POTemplate.select(
-            ' AND '.join(queries), clauseTables=clausetables, limit=2)
+        matches = query_result.config(limit=2)
 
         result = [match for match in matches]
         if len(result) == 0:
@@ -1144,23 +1155,14 @@
 
     def getPOTemplateByPath(self, path):
         """See `IPOTemplateSubset`."""
-        query = '%s AND POTemplate.path = %s' % (self.query, quote(path))
-
-        return POTemplate.selectOne(query, clauseTables=self.clausetables)
+        result = self._build_query(
+            POTemplate.path == path, ordered=False)
+        return result.one()
 
     def getAllOrderByDateLastUpdated(self):
         """See `IPOTemplateSet`."""
-        query = []
-        if self.productseries is not None:
-            query.append('productseries = %s' % sqlvalues(self.productseries))
-        if self.distroseries is not None:
-            query.append('distroseries = %s' % sqlvalues(self.distroseries))
-        if self.sourcepackagename is not None:
-            query.append('sourcepackagename = %s' % sqlvalues(
-                self.sourcepackagename))
-
-        return POTemplate.select(
-            ' AND '.join(query), orderBy=['-date_last_updated'])
+        result = self._build_query(ordered=False)
+        return result.order_by(Desc(POTemplate.date_last_updated))
 
     def getClosestPOTemplate(self, path):
         """See `IPOTemplateSubset`."""
@@ -1190,10 +1192,11 @@
 
     def findUniquePathlessMatch(self, filename):
         """See `IPOTemplateSubset`."""
-        query = self.query + (
-            " AND (POTemplate.path = %s OR POTemplate.path LIKE '%%/' || %s)"
-                % (quote(filename), quote_like(filename)))
-        candidates = list(POTemplate.select(query, limit=2))
+        result = self._build_query(
+            ("(POTemplate.path = %s OR POTemplate.path LIKE '%%%%/' || %s)"
+                % (quote(filename), quote_like(filename))),
+                ordered=False)
+        candidates = list(result.config(limit=2))
 
         if len(candidates) == 1:
             # Found exactly one match.