GNU Mailman

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

  1. db_collection_limit_prototype
  2. Merge into 3.0
Proposed by Florian Fuchs
Status: Work in progress
Proposed branch: lp:~flo-fuchs/mailman/db_collection_limit_prototype
Merge into: lp:mailman
Diff against target: 250 lines (+180/-4)
5 files modified
src/mailman/database/base.py (+29/-0)
src/mailman/database/tests/test_db_result_list.py (+125/-0)
src/mailman/model/listmanager.py (+2/-3)
src/mailman/model/tests/test_listmanager.py (+22/-0)
src/mailman/rest/lists.py (+2/-1)
To merge this branch: bzr merge lp:~flo-fuchs/mailman/db_collection_limit_prototype
Reviewer Review Type Date Requested Status
Barry Warsaw Pending
Review via email: mp+159630@code.launchpad.net

Description of the change

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

To post a comment you must log in.

Unmerged revisions

7213. By Florian Fuchs

added missing test file...

7212. By Florian Fuchs

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

7211. By Florian Fuchs

db collections

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/mailman/database/base.py'
2--- src/mailman/database/base.py 2013-01-01 14:05:42 +0000
3+++ src/mailman/database/base.py 2013-04-18 13:37:38 +0000
4@@ -20,6 +20,7 @@
5 __metaclass__ = type
6 __all__ = [
7 'StormBaseDatabase',
8+ 'DBResultList',
9 ]
10
11
12@@ -234,3 +235,31 @@
13 @staticmethod
14 def _make_temporary():
15 raise NotImplementedError
16+
17+
18+class DBResultList:
19+ """Turn a storm.store.ResultSet into a list-like object.
20+
21+ Use this to hide the db implementation.
22+ """
23+
24+ def __init__(self, response_object):
25+ self._response_object = response_object
26+
27+ def __repr__(self):
28+ return '{0}'.format(list(self._response_object))
29+
30+ def __len__(self):
31+ """map ``len`` to ``.count``."""
32+ return self._response_object.count()
33+
34+ def __reversed__(self):
35+ return list(reversed(list(self._response_object)))
36+
37+ def __getitem__(self, key):
38+ # Storm will return another ResultSet if given a key range.
39+ # Let's turn that into a list.
40+ if type(key) == slice:
41+ return list(self._response_object[key])
42+ # Otherwise return a single model instance.
43+ return self._response_object[key]
44
45=== added file 'src/mailman/database/tests/test_db_result_list.py'
46--- src/mailman/database/tests/test_db_result_list.py 1970-01-01 00:00:00 +0000
47+++ src/mailman/database/tests/test_db_result_list.py 2013-04-18 13:37:38 +0000
48@@ -0,0 +1,125 @@
49+# Copyright (C) 2012-2013 by the Free Software Foundation, Inc.
50+#
51+# This file is part of GNU Mailman.
52+#
53+# GNU Mailman is free software: you can redistribute it and/or modify it under
54+# the terms of the GNU General Public License as published by the Free
55+# Software Foundation, either version 3 of the License, or (at your option)
56+# any later version.
57+#
58+# GNU Mailman is distributed in the hope that it will be useful, but WITHOUT
59+# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
60+# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
61+# more details.
62+#
63+# You should have received a copy of the GNU General Public License along with
64+# GNU Mailman. If not, see <http://www.gnu.org/licenses/>.
65+
66+"""Test schema migrations."""
67+
68+from __future__ import absolute_import, print_function, unicode_literals
69+
70+__metaclass__ = type
71+__all__ = [
72+ 'TestDBResultList',
73+ ]
74+
75+
76+import unittest
77+import cStringIO
78+
79+from storm.locals import create_database, Int, Store, Unicode
80+from storm.tracer import debug
81+
82+from mailman.database.base import DBResultList
83+from mailman.testing.layers import ConfigLayer
84+
85+
86+class Item:
87+ __storm_table__ = 'items'
88+ id = Int(primary=True)
89+ name = Unicode()
90+
91+ def __init__(self, name):
92+ self.name = name
93+
94+ def __repr__(self):
95+ return '<Item name="{0}">'.format(self.name)
96+
97+
98+class TestDBResultList(unittest.TestCase):
99+ """Test the DBResultList class."""
100+
101+ layer = ConfigLayer
102+
103+ def setUp(self):
104+ # Create a test db and add test data.
105+ database = create_database('sqlite:')
106+ self.store = Store(database)
107+ self.store.execute('CREATE TABLE items (id INTEGER PRIMARY KEY, '
108+ 'name VARCHAR)')
109+ self.store.add(Item('Shoe'))
110+ self.store.add(Item('Sock'))
111+ self.store.add(Item('Shirt'))
112+ self.store.commit()
113+ # Log db queries during tests.
114+ self.query_log = cStringIO.StringIO()
115+ debug(True, stream=self.query_log)
116+ # Create a DBResultList instance.
117+ self.result_list = DBResultList(self.store.find(Item))
118+
119+ def tearDown(self):
120+ # Stop db query debugging.
121+ debug(False)
122+
123+ def test_len(self):
124+ # The DBResultList instance should be len-able.
125+ self.assertEqual(len(self.result_list), 3)
126+ # Check the first log entry, only the part after the timestamp
127+ log_line = self.query_log.getvalue().split('\n')[0]
128+ self.assertEqual(log_line.split('] ')[1],
129+ "EXECUTE: u'SELECT COUNT(*) FROM items', ()")
130+
131+ def test_iteration(self):
132+ for index, value in enumerate(self.result_list):
133+ self.assertEqual(self.result_list[index].id,
134+ value.id)
135+
136+ def test_repr(self):
137+ # It should also have a list-like string representation.
138+ self.assertEqual(str(self.result_list), u'[<Item name="Shoe">, '
139+ '<Item name="Sock">, <Item name="Shirt">]')
140+
141+ def test_reversed(self):
142+ # It also should support `reversed`.
143+ self.assertEqual(reversed(self.result_list)[0].name, 'Shirt')
144+
145+ def test_query_on_list_index(self):
146+ # Accessing a list index should return the object, not a list.
147+ self.assertEqual(self.result_list[1].name, 'Sock')
148+ # LIMIT and OFFSET must be set correctly.
149+ log_line = self.query_log.getvalue().split('\n')[0]
150+ self.assertEqual(log_line.split('] ')[1],
151+ "EXECUTE: u'SELECT items.id, items.name FROM items "
152+ "LIMIT 1 OFFSET 1', ()")
153+
154+ def test_query_on_range(self):
155+ # Accessing a list range should return a new list.
156+ self.assertEqual(str(self.result_list[1:3]),
157+ '[<Item name="Sock">, <Item name="Shirt">]')
158+ self.assertEqual(type(self.result_list[1:3]), list)
159+ # LIMIT and OFFSET must be set correctly.
160+ log_line = self.query_log.getvalue().split('\n')[0]
161+ self.assertEqual(log_line.split('] ')[1],
162+ "EXECUTE: u'SELECT items.id, items.name FROM items "
163+ "LIMIT 2 OFFSET 1', ()")
164+
165+ def test_query_on_range_size_one(self):
166+ # Accessing a list range with a size of 1 should return a list.
167+ self.assertEqual(str(self.result_list[1:2]),
168+ '[<Item name="Sock">]')
169+ # LIMIT and OFFSET must be set correctly.
170+ log_line = self.query_log.getvalue().split('\n')[0]
171+ self.assertEqual(log_line.split('] ')[1],
172+ "EXECUTE: u'SELECT items.id, items.name FROM items "
173+ "LIMIT 1 OFFSET 1', ()")
174
175=== modified file 'src/mailman/model/listmanager.py'
176--- src/mailman/model/listmanager.py 2013-03-06 21:59:15 +0000
177+++ src/mailman/model/listmanager.py 2013-04-18 13:37:38 +0000
178@@ -28,6 +28,7 @@
179 from zope.event import notify
180 from zope.interface import implementer
181
182+from mailman.database.base import DBResultList
183 from mailman.database.transaction import dbconnection
184 from mailman.interfaces.address import InvalidEmailAddressError
185 from mailman.interfaces.listmanager import (
186@@ -36,7 +37,6 @@
187 from mailman.model.mailinglist import MailingList
188 from mailman.utilities.datetime import now
189
190-
191
192
193 @implementer(IListManager)
194 class ListManager:
195@@ -86,8 +86,7 @@
196 @dbconnection
197 def mailing_lists(self, store):
198 """See `IListManager`."""
199- for mlist in store.find(MailingList):
200- yield mlist
201+ return DBResultList(store.find(MailingList))
202
203 @dbconnection
204 def __iter__(self, store):
205
206=== modified file 'src/mailman/model/tests/test_listmanager.py'
207--- src/mailman/model/tests/test_listmanager.py 2013-03-06 21:59:15 +0000
208+++ src/mailman/model/tests/test_listmanager.py 2013-04-18 13:37:38 +0000
209@@ -52,6 +52,28 @@
210 def _record_event(self, event):
211 self._events.append(event)
212
213+ def test_get_one_list_from_collection(self):
214+ self._ant = create_list('ant@example.com')
215+ self._bee = create_list('bee@example.com')
216+ self._bee = create_list('bird@example.com')
217+ mlists = getUtility(IListManager).mailing_lists
218+ self.assertEqual(mlists[1].fqdn_listname, 'bee@example.com')
219+
220+ def test_get_multiple_lists_from_collection(self):
221+ self._ant = create_list('ant@example.com')
222+ self._bee = create_list('bee@example.com')
223+ self._bee = create_list('bird@example.com')
224+ self._bee = create_list('bear@example.com')
225+ mlists = getUtility(IListManager).mailing_lists
226+ self.assertEqual(len(mlists), 4)
227+ self.assertEqual(len(mlists[2:4]), 2)
228+ self.assertEqual(mlists[2:4][0].fqdn_listname, 'bird@example.com')
229+ self.assertEqual(mlists[2:4][1].fqdn_listname, 'bear@example.com')
230+ self.assertEqual(len(mlists[:3]), 3)
231+ self.assertEqual(mlists[:3][0].fqdn_listname, 'ant@example.com')
232+ self.assertEqual(mlists[:3][1].fqdn_listname, 'bee@example.com')
233+ self.assertEqual(mlists[:3][2].fqdn_listname, 'bird@example.com')
234+
235 def test_create_list_event(self):
236 # Test that creating a list in the list manager propagates the
237 # expected events.
238
239=== modified file 'src/mailman/rest/lists.py'
240--- src/mailman/rest/lists.py 2013-03-21 18:35:58 +0000
241+++ src/mailman/rest/lists.py 2013-04-18 13:37:38 +0000
242@@ -118,7 +118,8 @@
243 @paginate
244 def _get_collection(self, request):
245 """See `CollectionMixin`."""
246- return list(getUtility(IListManager))
247+ return getUtility(IListManager).mailing_lists
248+ # return list(getUtility(IListManager))
249
250
251 class AList(_ListBase):