Merge lp:~stevenk/launchpad/bulk-eid-to-obj into lp:launchpad

[Steve Kowalik]

Destroy enterpriseid_to_object for being terrible, its replacement is enterpriseids_to_objects.

Destroy enterpriseid_to_object for being terrible and issuing one query per eid passed to it. Its replacement, enterpriseids_to_objects will make one query per type of object.

[William Grant (wgrant)]

20 + eids = []
21 + for entry in logs['log-entries']:
22 + eids.extend([entry['actor'], entry['object']])

This should probably be a set; there will likely be many dupes.

76 +def enterpriseids_to_objects(eids):
77 + """Given a list of SOA Enterprise IDs, return a dict that maps the ID to
78 + its concrete object."""

That's a prolix and non-compliant docstring. "Dereference multiple SOA Enterprise IDs.", maybe?

79 + map_id_to_obj = {}
80 + obj_id_to_eid = defaultdict(dict)

Aren't those "eid_to_obj" and "dbid_to_eid"? map_id_to_obj also isn't needed until right at the end; I'd define it there for readability.

89 + type_ids[scheme[1]].append(int(scheme[2]))
90 + obj_id_to_eid[scheme[1]][int(scheme[2])] = eid

These are redundant. type_ids[foo] is just obj_id_to_eid[foo].keys(). Dropping type_ids also has the side-effect of eliminating dupes.

92 + for kind in types:

This could just iterate over obj_id_to_eid instead, to get just the types relevant to this query.

137 + def test_enterpriseids_to_objects(self):

This probably wants to also test for multiple classes.

review: Needs Fixing (code)

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/lp/services/auditor/client.py'
--- lib/lp/services/auditor/client.py	2013-06-18 03:57:26 +0000
+++ lib/lp/services/auditor/client.py	2013-06-27 04:36:27 +0000
@@ -12,7 +12,7 @@
 
 from lp.services.config import config
 from lp.services.enterpriseid import (
-    enterpriseid_to_object,
+    enterpriseids_to_objects,
     object_to_enterpriseid,
     )
 
@@ -42,7 +42,11 @@
         logs = super(AuditorClient, self).receive(
             obj, operation, actorobj, limit)
         # Process the actors and objects back from enterprise ids.
-        for entry in logs['log-entries']:
-            entry['actor'] = enterpriseid_to_object(entry['actor'])
-            entry['object'] = enterpriseid_to_object(entry['object'])
+        eids = set()
+        for entry in logs['log-entries']:
+            eids |= set([entry['actor'], entry['object']])
+        map_eids_to_obj = enterpriseids_to_objects(eids)
+        for entry in logs['log-entries']:
+            entry['actor'] = map_eids_to_obj.get(entry['actor'], None)
+            entry['object'] = map_eids_to_obj.get(entry['object'], None)
         return logs['log-entries']
 
=== modified file 'lib/lp/services/enterpriseid.py'
--- lib/lp/services/enterpriseid.py	2012-07-26 22:12:53 +0000
+++ lib/lp/services/enterpriseid.py	2013-06-27 04:36:27 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Enterprise ID utilities."""
@@ -6,12 +6,14 @@
 __metaclass__ = type
 __all__ = [
     'object_to_enterpriseid',
-    'enterpriseid_to_object',
+    'enterpriseids_to_objects',
     ]
 
+from collections import defaultdict
 import os
 
 from lp.services.config import config
+from lp.services.database.bulk import load
 
 
 def object_to_enterpriseid(obj):
@@ -25,17 +27,28 @@
     return '%s:%s:%d' % (instance, otype, obj.id)
 
 
-def enterpriseid_to_object(eid):
-    """Given an SOA Enterprise ID, return the object that it references."""
+def _known_types():
     # Circular imports.
     from lp.registry.model.person import Person
     from lp.soyuz.model.queue import PackageUpload
-    scheme = eid.split(':')
-    if not scheme[0].startswith('lp'):
-        raise TypeError
-    known_types = {
+    return {
         'PackageUpload': PackageUpload,
         'Person': Person,
     }
-    klass = known_types[scheme[1]]
-    return klass.get(scheme[2])
+
+
+def enterpriseids_to_objects(eids):
+    """Dereference multiple SOA Enterprise IDs."""
+    dbid_to_eid = defaultdict(dict)
+    for eid in eids:
+        if not eid.startswith('lp'):
+            raise TypeError
+        instance, cls, id = eid.split(':')
+        dbid_to_eid[cls][int(id)] = eid
+    types = _known_types()
+    eid_to_obj = {}
+    for kind in dbid_to_eid:
+        objs = load(types[kind], dbid_to_eid[kind].keys())
+        for obj in objs:
+            eid_to_obj[dbid_to_eid[kind][obj.id]] = obj
+    return eid_to_obj
 
=== modified file 'lib/lp/services/tests/test_enterpriseid.py'
--- lib/lp/services/tests/test_enterpriseid.py	2012-03-05 02:24:17 +0000
+++ lib/lp/services/tests/test_enterpriseid.py	2013-06-27 04:36:27 +0000
@@ -1,20 +1,27 @@
-# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test Enterprise ID utilities."""
 
 __metaclass__ = type
 
+from testtools.matchers import Equals
+
+from lp.services.database.interfaces import IStore
 from lp.services.enterpriseid import (
-    enterpriseid_to_object,
+    enterpriseids_to_objects,
     object_to_enterpriseid,
     )
-from lp.testing import TestCaseWithFactory
-from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing import (
+    StormStatementRecorder,
+    TestCaseWithFactory,
+    )
+from lp.testing.layers import LaunchpadFunctionalLayer
+from lp.testing.matchers import HasQueryCount
 
 
 class TestEnterpriseId(TestCaseWithFactory):
-    layer = DatabaseFunctionalLayer
+    layer = LaunchpadFunctionalLayer
 
     def test_object_to_enterpriseid(self):
         person = self.factory.makePerson()
@@ -22,8 +29,21 @@
         expected = 'lp-development:Person:%d' % person.id
         self.assertEqual(expected, eid)
 
-    def test_enterpriseid_to_object(self):
-        person = self.factory.makePerson()
-        eid = 'lp-development:Person:%d' % person.id
-        obj = enterpriseid_to_object(eid)
-        self.assertEqual(person, obj)
+    def test_enterpriseids_to_objects(self):
+        expected = {}
+        for x in range(5):
+            person = self.factory.makePerson()
+            upload = self.factory.makePackageUpload()
+            expected['lp-development:Person:%d' % person.id] = person
+            expected['lp-development:PackageUpload:%d' % upload.id] = upload
+        IStore(expected.values()[0].__class__).invalidate()
+        with StormStatementRecorder() as recorder:
+            objects = enterpriseids_to_objects(expected.keys())
+        self.assertThat(recorder, HasQueryCount(Equals(2)))
+        self.assertContentEqual(expected.keys(), objects.keys())
+        self.assertContentEqual(expected.values(), objects.values())
+
+    def test_enterpriseids_to_objects_non_existent_id(self):
+        objects = enterpriseids_to_objects(
+            ['lp-development:PackageUpload:10000'])
+        self.assertEqual({}, objects)