launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/launchpad/load-related-with-attrgetter into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~allenap/launchpad/load-related-with-attrgetter/+merge/56965

Makes load_related() work with a single foreign key name - i.e. doesn't necessitate it being in a list for simple cases - and allows "compound" foreign key names like "owner.teamownerID".
-- 
https://code.launchpad.net/~allenap/launchpad/load-related-with-attrgetter/+merge/56965
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/load-related-with-attrgetter into lp:launchpad.

=== modified file 'lib/lp/services/database/bulk.py'
--- lib/lp/services/database/bulk.py	2011-04-01 14:46:21 +0000
+++ lib/lp/services/database/bulk.py	2011-04-08 14:52:32 +0000
@@ -12,7 +12,8 @@
 
 
 from collections import defaultdict
-from functools import partial
+from itertools import imap
+from operator import attrgetter
 
 from storm.info import get_cls_info
 from storm.store import Store
@@ -92,9 +93,11 @@
     :param object_type: The object type to load - e.g. Person.
     :param owning_objects: The objects holding the references. E.g. Bug.
     :param foreign_keys: A list of attributes that should be inspected for
-        keys. e.g. ['ownerID']
+        keys. e.g. ['ownerID'], or a single unadorned attribute name.
     """
-    keys = set()
-    for owning_object in owning_objects:
-        keys.update(map(partial(getattr, owning_object), foreign_keys))
+    if isinstance(foreign_keys, (unicode, str)):
+        foreign_keys = (foreign_keys,)
+    keys = imap(attrgetter(*foreign_keys), owning_objects)
+    if len(foreign_keys) > 1:
+        keys = (key for key_tuple in keys for key in key_tuple)
     return load(object_type, keys)

=== modified file 'lib/lp/services/database/tests/test_bulk.py'
--- lib/lp/services/database/tests/test_bulk.py	2011-03-29 00:11:57 +0000
+++ lib/lp/services/database/tests/test_bulk.py	2011-04-08 14:52:32 +0000
@@ -199,7 +199,48 @@
             self.factory.makeBug(),
             ]
         expected = set(bug.owner for bug in owning_objects)
-        self.assertEqual(expected,
-            set(bulk.load_related(Person, owning_objects, ['ownerID'])))
-
-
+        self.assertContentEqual(
+            expected, bulk.load_related(
+                Person, owning_objects, ['ownerID']))
+        # A single foreign key can be passed unadorned.
+        self.assertContentEqual(
+            expected, bulk.load_related(
+                Person, owning_objects, 'ownerID'))
+
+    def test_load_related_with_multiple_keys(self):
+        driver = self.factory.makePerson()
+        registrant = self.factory.makePerson()
+        owning_objects = [
+            self.factory.makeProduct(registrant=registrant),
+            self.factory.makeProduct(registrant=registrant),
+            ]
+        naked_owning_objects = map(
+            proxy.removeSecurityProxy, owning_objects)
+        for naked_owning_object in naked_owning_objects:
+            naked_owning_object.driver = driver
+        self.assertContentEqual(
+            [driver, registrant], bulk.load_related(
+                Person, naked_owning_objects,
+                ['driverID', "registrantID"]))
+
+    def test_load_related_with_compound_key(self):
+        # You can traverse multiple levels of the object tree using
+        # load_related(). However, don't do this unless you've already loaded
+        # the earlier parts of the traversal, or you'll end up smashing the
+        # database in exactly the way you're trying to avoid.
+        driver = self.factory.makeTeam()
+        owning_objects = [
+            self.factory.makeProduct(),
+            self.factory.makeProduct(),
+            ]
+        naked_owning_objects = map(
+            proxy.removeSecurityProxy, owning_objects)
+        for naked_owning_object in naked_owning_objects:
+            naked_owning_object.driver = driver
+        # We'll load the drivers first to avoid database smashing. Not really
+        # needed here because they're already loaded, just an example of good
+        # behaviour.
+        bulk.load_related(Person, naked_owning_objects, "driverID")
+        self.assertContentEqual(
+            [driver.teamowner], bulk.load_related(
+                Person, naked_owning_objects, 'driver.teamownerID'))