launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/launchpad/bulk-load into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~allenap/launchpad/bulk-load/+merge/52474

This adds a simple function, lp.services.database.bulk.load(), that
loads a homogeneous pile of objects by their primary key.

In practice this doesn't save a lot of typing, but it has the
advantage that it is tested already, and more importantly it is
greppable.

It is my expectation that it be imported and used like so:

  from lp.services.database import bulk
  bulk.load(...)

The import line itself is greppable, so the form it's used as doesn't
matter too much. The point is that it can clearly mark where we are
bulk loading from the database for performance. Also, it helps to
reduce the for-performance code noise in the calling code a little
bit.

-- 
https://code.launchpad.net/~allenap/launchpad/bulk-load/+merge/52474
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/bulk-load into lp:launchpad.

=== modified file 'lib/lp/services/database/bulk.py'
--- lib/lp/services/database/bulk.py	2010-11-05 09:16:14 +0000
+++ lib/lp/services/database/bulk.py	2011-03-07 21:23:05 +0000
@@ -11,11 +11,12 @@
 
 from collections import defaultdict
 
-from storm.base import Storm
 from storm.info import get_cls_info
 from storm.store import Store
 from zope.security.proxy import removeSecurityProxy
 
+from canonical.launchpad.interfaces.lpstorm import IStore
+
 
 def collate(things, key):
     """Collate the given objects according to a key function.
@@ -41,10 +42,6 @@
 def gen_reload_queries(objects):
     """Prepare queries to reload the given objects."""
     for object_type, objects in collate(objects, get_type):
-        if not issubclass(object_type, Storm):
-            raise AssertionError(
-                "Cannot load objects of type %s: %r" % (
-                    object_type.__name__, objects))
         primary_key = get_cls_info(object_type).primary_key
         if len(primary_key) != 1:
             raise AssertionError(
@@ -62,3 +59,18 @@
     """Reload a large number of objects efficiently."""
     for query in gen_reload_queries(objects):
         list(query)
+
+
+def load(object_type, primary_keys, store=None):
+    """Load a large number of objects efficiently."""
+    primary_key = get_cls_info(object_type).primary_key
+    if len(primary_key) != 1:
+        raise AssertionError(
+            "Compound primary keys are not supported: %s." %
+            object_type.__name__)
+    primary_key_column = primary_key[0]
+    condition = primary_key_column.is_in(primary_keys)
+    if store is None:
+        store = IStore(object_type)
+    query = store.find(object_type, condition)
+    return list(query)

=== modified file 'lib/lp/services/database/tests/test_bulk.py'
--- lib/lp/services/database/tests/test_bulk.py	2010-10-04 19:50:45 +0000
+++ lib/lp/services/database/tests/test_bulk.py	2011-03-07 21:23:05 +0000
@@ -5,21 +5,24 @@
 
 __metaclass__ = type
 
-import unittest
-
+from storm.exceptions import ClassInfoError
 from storm.info import get_obj_info
+from storm.store import Store
 import transaction
-import zope.security.checker
-import zope.security.proxy
+from zope.security import (
+    checker,
+    proxy,
+    )
 
 from canonical.launchpad.interfaces.lpstorm import (
     IMasterStore,
     ISlaveStore,
     IStore,
     )
-from canonical.testing.layers import LaunchpadZopelessLayer
+from canonical.testing.layers import DatabaseFunctionalLayer
 from lp.bugs.model.bug import BugAffectsPerson
 from lp.services.database import bulk
+from lp.soyuz.model.component import Component
 from lp.testing import (
     TestCase,
     TestCaseWithFactory,
@@ -51,14 +54,13 @@
         self.failUnlessEqual(object, bulk.get_type(object()))
 
     def test_get_type_with_proxied_object(self):
-        proxied_object = zope.security.proxy.Proxy(
-            'fred', zope.security.checker.Checker({}))
+        proxied_object = proxy.Proxy('fred', checker.Checker({}))
         self.failUnlessEqual(str, bulk.get_type(proxied_object))
 
 
 class TestLoaders(TestCaseWithFactory):
 
-    layer = LaunchpadZopelessLayer
+    layer = DatabaseFunctionalLayer
 
     def test_gen_reload_queries_with_empty_list(self):
         self.failUnlessEqual([], list(bulk.gen_reload_queries([])))
@@ -124,10 +126,8 @@
 
     def test_gen_reload_queries_with_non_Storm_objects(self):
         # gen_reload_queries() does not like non-Storm objects.
-        self.assertRaisesWithContent(
-            AssertionError,
-            "Cannot load objects of type str: ['fred']",
-            list, bulk.gen_reload_queries(['fred']))
+        self.assertRaises(
+            ClassInfoError, list, bulk.gen_reload_queries(['bogus']))
 
     def test_gen_reload_queries_with_compound_primary_keys(self):
         # gen_reload_queries() does not like compound primary keys.
@@ -137,11 +137,11 @@
             'Compound primary keys are not supported: BugAffectsPerson.',
             list, db_queries)
 
-    def test_load(self):
-        # load() loads the given objects using queries generated by
+    def test_reload(self):
+        # reload() loads the given objects using queries generated by
         # gen_reload_queries().
         db_object = self.factory.makeComponent()
-        db_object_naked = zope.security.proxy.removeSecurityProxy(db_object)
+        db_object_naked = proxy.removeSecurityProxy(db_object)
         db_object_info = get_obj_info(db_object_naked)
         IStore(db_object).flush()
         self.failUnlessEqual(None, db_object_info.get('invalidated'))
@@ -150,6 +150,44 @@
         bulk.reload([db_object])
         self.failUnlessEqual(None, db_object_info.get('invalidated'))
 
-
-def test_suite():
-    return unittest.TestLoader().loadTestsFromName(__name__)
+    def test_load(self):
+        # load() loads objects of the given type by their primary keys.
+        db_objects = [
+            self.factory.makeComponent(),
+            self.factory.makeComponent(),
+            ]
+        db_object_ids = [db_object.id for db_object in db_objects]
+        self.assertEqual(
+            bulk.load(Component, db_object_ids),
+            db_objects)
+
+    def test_load_with_non_Storm_objects(self):
+        # load() does not like non-Storm objects.
+        self.assertRaises(
+            ClassInfoError, bulk.load, str, [])
+
+    def test_load_with_compound_primary_keys(self):
+        # load() does not like compound primary keys.
+        self.assertRaisesWithContent(
+            AssertionError,
+            'Compound primary keys are not supported: BugAffectsPerson.',
+            bulk.load, BugAffectsPerson, [])
+
+    def test_load_with_store(self):
+        # load() can use an alternative store.
+        db_object = self.factory.makeComponent()
+        # Commit so the database object is available in both master
+        # and slave stores.
+        transaction.commit()
+        # Master store.
+        master_store = IMasterStore(db_object)
+        [db_object_from_master] = bulk.load(
+            Component, [db_object.id], store=master_store)
+        self.assertEqual(
+            Store.of(db_object_from_master), master_store)
+        # Slave store.
+        slave_store = ISlaveStore(db_object)
+        [db_object_from_slave] = bulk.load(
+            Component, [db_object.id], store=slave_store)
+        self.assertEqual(
+            Store.of(db_object_from_slave), slave_store)