Merge lp:~flo-fuchs/mailman/db_collection_limit_prototype into lp:mailman

[Florian Fuchs]

Hi Barry,

like discussed on IRC here's a prototype branch to add limit and offset to db results (for pagination etc.).

Initial problem: Currently, if a collection is accessed trough a manager (ListMananager, ...) the manager will turn the storm ResultSet into a list before passing it on. This causes storm to query and return *all* records in the database, even if only a subset is needed.

Solution 1: Instead of turning the ResultSet into a List, the manager could just return the ResultSet itself.
But then the abstraction is lost, the code retrieving the data has to be aware that this is a DB result, use .count instead of len etc.

Solution 2: The ResultSet is turned into a "DBResultList" which is just a list-like proxy around ResultSet. Any code that is using that object can use it like a native list, but access to list items via an index or a start:stop range will cause the SQL query to use a proper limit and offset.

Limitations: At the moment, __setitem__ and __delitem__ are not implemented. The test for the DBResultList is rather isolated and should probably be integrated better with the existing tests. I put the DBResultList class into database/base.py for now.

Cheers
Florian

Unmerged revisions

7213. By Florian Fuchs on 2013-04-18

added missing test file...

7212. By Florian Fuchs on 2013-04-18

* added DBResultList class to hide db implementation and provide a list-like object
* ListManager uses DBResultList

7211. By Florian Fuchs on 2013-04-04

db collections

=== modified file 'src/mailman/database/base.py'
--- src/mailman/database/base.py	2013-01-01 14:05:42 +0000
+++ src/mailman/database/base.py	2013-04-18 13:37:38 +0000
@@ -20,6 +20,7 @@
 __metaclass__ = type
 __all__ = [
     'StormBaseDatabase',
+    'DBResultList',
     ]
 
 
@@ -234,3 +235,31 @@
     @staticmethod
     def _make_temporary():
         raise NotImplementedError
+
+
+class DBResultList:
+    """Turn a storm.store.ResultSet into a list-like object.
+
+    Use this to hide the db implementation.
+    """
+    
+    def __init__(self, response_object):
+        self._response_object = response_object
+
+    def __repr__(self):
+        return '{0}'.format(list(self._response_object))
+   
+    def __len__(self):
+        """map ``len`` to ``.count``."""
+        return self._response_object.count()
+
+    def __reversed__(self):
+        return list(reversed(list(self._response_object)))
+  
+    def __getitem__(self, key):
+        # Storm will return another ResultSet if given a key range.
+        # Let's turn that into a list.
+        if type(key) == slice:
+            return list(self._response_object[key])
+        # Otherwise return a single model instance.
+        return self._response_object[key]

=== added file 'src/mailman/database/tests/test_db_result_list.py'
--- src/mailman/database/tests/test_db_result_list.py	1970-01-01 00:00:00 +0000
+++ src/mailman/database/tests/test_db_result_list.py	2013-04-18 13:37:38 +0000
@@ -0,0 +1,125 @@
+# Copyright (C) 2012-2013 by the Free Software Foundation, Inc.
+#
+# This file is part of GNU Mailman.
+#
+# GNU Mailman is free software: you can redistribute it and/or modify it under
+# the terms of the GNU General Public License as published by the Free
+# Software Foundation, either version 3 of the License, or (at your option)
+# any later version.
+#
+# GNU Mailman is distributed in the hope that it will be useful, but WITHOUT
+# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+# FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+# more details.
+#
+# You should have received a copy of the GNU General Public License along with
+# GNU Mailman.  If not, see <http://www.gnu.org/licenses/>.
+
+"""Test schema migrations."""
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+__metaclass__ = type
+__all__ = [
+    'TestDBResultList',
+    ]
+
+
+import unittest
+import cStringIO
+
+from storm.locals import create_database, Int, Store, Unicode
+from storm.tracer import debug
+
+from mailman.database.base import DBResultList
+from mailman.testing.layers import ConfigLayer
+
+
+class Item:
+    __storm_table__ = 'items'
+    id = Int(primary=True)
+    name = Unicode()
+
+    def __init__(self, name):
+        self.name = name
+
+    def __repr__(self):
+        return '<Item name="{0}">'.format(self.name)
+
+
+class TestDBResultList(unittest.TestCase):
+    """Test the DBResultList class."""
+
+    layer = ConfigLayer
+
+    def setUp(self):
+        # Create a test db and add test data.
+        database = create_database('sqlite:')
+        self.store = Store(database)
+        self.store.execute('CREATE TABLE items (id INTEGER PRIMARY KEY, '
+                           'name VARCHAR)')
+        self.store.add(Item('Shoe')) 
+        self.store.add(Item('Sock')) 
+        self.store.add(Item('Shirt')) 
+        self.store.commit()
+        # Log db queries during tests.
+        self.query_log = cStringIO.StringIO()
+        debug(True, stream=self.query_log)
+        # Create a DBResultList instance.
+        self.result_list = DBResultList(self.store.find(Item))
+
+    def tearDown(self):
+        # Stop db query debugging.
+        debug(False)
+
+    def test_len(self):
+        # The DBResultList instance should be len-able.
+        self.assertEqual(len(self.result_list), 3)
+        # Check the first log entry, only the part after the timestamp
+        log_line = self.query_log.getvalue().split('\n')[0]
+        self.assertEqual(log_line.split('] ')[1],
+                         "EXECUTE: u'SELECT COUNT(*) FROM items', ()")
+
+    def test_iteration(self):
+        for index, value in enumerate(self.result_list):
+            self.assertEqual(self.result_list[index].id,
+                             value.id)
+
+    def test_repr(self):
+        # It should also have a list-like string representation.
+        self.assertEqual(str(self.result_list), u'[<Item name="Shoe">, '
+                         '<Item name="Sock">, <Item name="Shirt">]')
+
+    def test_reversed(self):
+        # It also should support `reversed`.
+        self.assertEqual(reversed(self.result_list)[0].name, 'Shirt')
+
+    def test_query_on_list_index(self):
+        # Accessing a list index should return the object, not a list.
+        self.assertEqual(self.result_list[1].name, 'Sock')
+        # LIMIT and OFFSET must be set correctly.
+        log_line = self.query_log.getvalue().split('\n')[0]
+        self.assertEqual(log_line.split('] ')[1],
+                         "EXECUTE: u'SELECT items.id, items.name FROM items "
+                         "LIMIT 1 OFFSET 1', ()")
+
+    def test_query_on_range(self):
+        # Accessing a list range should return a new list.
+        self.assertEqual(str(self.result_list[1:3]),
+                         '[<Item name="Sock">, <Item name="Shirt">]')
+        self.assertEqual(type(self.result_list[1:3]), list)
+        # LIMIT and OFFSET must be set correctly.
+        log_line = self.query_log.getvalue().split('\n')[0]
+        self.assertEqual(log_line.split('] ')[1],
+                         "EXECUTE: u'SELECT items.id, items.name FROM items "
+                         "LIMIT 2 OFFSET 1', ()")
+
+    def test_query_on_range_size_one(self):
+        # Accessing a list range with a size of 1 should return a list.
+        self.assertEqual(str(self.result_list[1:2]),
+                         '[<Item name="Sock">]')
+        # LIMIT and OFFSET must be set correctly.
+        log_line = self.query_log.getvalue().split('\n')[0]
+        self.assertEqual(log_line.split('] ')[1],
+                         "EXECUTE: u'SELECT items.id, items.name FROM items "
+                         "LIMIT 1 OFFSET 1', ()")

=== modified file 'src/mailman/model/listmanager.py'
--- src/mailman/model/listmanager.py	2013-03-06 21:59:15 +0000
+++ src/mailman/model/listmanager.py	2013-04-18 13:37:38 +0000
@@ -28,6 +28,7 @@
 from zope.event import notify
 from zope.interface import implementer
 
+from mailman.database.base import DBResultList
 from mailman.database.transaction import dbconnection
 from mailman.interfaces.address import InvalidEmailAddressError
 from mailman.interfaces.listmanager import (
@@ -36,7 +37,6 @@
 from mailman.model.mailinglist import MailingList
 from mailman.utilities.datetime import now
 
-
 

 @implementer(IListManager)
 class ListManager:
@@ -86,8 +86,7 @@
     @dbconnection
     def mailing_lists(self, store):
         """See `IListManager`."""
-        for mlist in store.find(MailingList):
-            yield mlist
+        return DBResultList(store.find(MailingList))
 
     @dbconnection
     def __iter__(self, store):

=== modified file 'src/mailman/model/tests/test_listmanager.py'
--- src/mailman/model/tests/test_listmanager.py	2013-03-06 21:59:15 +0000
+++ src/mailman/model/tests/test_listmanager.py	2013-04-18 13:37:38 +0000
@@ -52,6 +52,28 @@
     def _record_event(self, event):
         self._events.append(event)
 
+    def test_get_one_list_from_collection(self):
+        self._ant = create_list('[e-mail]')
+        self._bee = create_list('[e-mail]')
+        self._bee = create_list('[e-mail]')
+        mlists = getUtility(IListManager).mailing_lists
+        self.assertEqual(mlists[1].fqdn_listname, '[e-mail]')
+
+    def test_get_multiple_lists_from_collection(self):
+        self._ant = create_list('[e-mail]')
+        self._bee = create_list('[e-mail]')
+        self._bee = create_list('[e-mail]')
+        self._bee = create_list('[e-mail]')
+        mlists = getUtility(IListManager).mailing_lists
+        self.assertEqual(len(mlists), 4)
+        self.assertEqual(len(mlists[2:4]), 2)
+        self.assertEqual(mlists[2:4][0].fqdn_listname, '[e-mail]')
+        self.assertEqual(mlists[2:4][1].fqdn_listname, '[e-mail]')
+        self.assertEqual(len(mlists[:3]), 3)
+        self.assertEqual(mlists[:3][0].fqdn_listname, '[e-mail]')
+        self.assertEqual(mlists[:3][1].fqdn_listname, '[e-mail]')
+        self.assertEqual(mlists[:3][2].fqdn_listname, '[e-mail]')
+
     def test_create_list_event(self):
         # Test that creating a list in the list manager propagates the
         # expected events.

=== modified file 'src/mailman/rest/lists.py'
--- src/mailman/rest/lists.py	2013-03-21 18:35:58 +0000
+++ src/mailman/rest/lists.py	2013-04-18 13:37:38 +0000
@@ -118,7 +118,8 @@
     @paginate
     def _get_collection(self, request):
         """See `CollectionMixin`."""
-        return list(getUtility(IListManager))
+        return getUtility(IListManager).mailing_lists
+        # return list(getUtility(IListManager))
 
 
 class AList(_ListBase):