Merge lp:~lifeless/launchpad/bug-752153 into lp:launchpad

[Robert Collins]

[r=stevenk][bug=752153] Update lazr.batchnavigator to 1.2.4.

Update our batchnavigator dependency to 1.2.4 which supports database friendly batching (including stable batchs that don't skip/repeat things when folk remove or add items).

There is one caveat which is that the ListRangeFactory will, for 'last' links, do one more count(*) than we do at the moment; I think that this will break things that are already basically broken, or be harmless, and so am going to leave it in place for now. The custom IRangeFactories we will write for complex collections will be able to avoid this - and its needed because of bug 239767.

QA for this should involve checking some api collections as well as e.g. search result pagination. Only the OOPS report will weigh in on the last link change impact.

[Steve Kowalik (stevenk)]

review: Approve (code)

=== modified file 'lib/canonical/launchpad/doc/batch_navigation.txt'
--- lib/canonical/launchpad/doc/batch_navigation.txt	2011-04-12 11:36:11 +0000
+++ lib/canonical/launchpad/doc/batch_navigation.txt	2011-04-12 20:07:39 +0000
@@ -47,88 +47,18 @@
   ...     'Cupid', 'Donner', 'Blitzen', 'Rudolph']
 
 
-Performance with SQLObject
-==========================
-
-This section demonstrates that batching generates sensible SQL queries when
-used with SQLObject, i.e. that it puts appropriate LIMIT clauses on queries.
-
-Imports and initialization:
-
-  >>> from lp.testing.pgsql import CursorWrapper
-  >>> from canonical.launchpad.database.emailaddress import EmailAddress
-  >>> from canonical.launchpad.interfaces.lpstorm import IStore
-  >>> ignore = IStore(EmailAddress) # Prime the database connection.
-  ... # (Priming is important if this test is run in isolation).
-  >>> CursorWrapper.record_sql = True
-
-Prepare a query, and create a batch of the results:
-
-  >>> select_results = EmailAddress.select(orderBy='id')
-  >>> batch_nav = BatchNavigator(select_results, build_request(), size=10)
-  >>> email_batch = batch_nav.currentBatch()
-  >>> batch_items = list(email_batch)
-
-Because we're only looking at the first batch, the database is only
-asked for the first 11 rows. (lazr.batchnavigator asks for 11 instead
-of 10 so that it can reliably detect the end of the dataset).
-
-  >>> len(CursorWrapper.last_executed_sql)
-  1
-  >>> print CursorWrapper.last_executed_sql[0]
-  SELECT ... FROM EmailAddress ... LIMIT 11...
-
-Get the next 10.  The database is only asked for the next 11 rows:
-
-  >>> CursorWrapper.last_executed_sql = []
-  >>> email_batch2 = email_batch.nextBatch()
-  >>> batch_items = list(email_batch2)
-  >>> len(CursorWrapper.last_executed_sql)
-  1
-  >>> CursorWrapper.last_executed_sql[0].endswith('LIMIT 11 OFFSET 10')
-  True
-
-As seen above, simply accessing the batch doesn't trigger a SQL query
-asking for the length of the entire resultset. But explicitly asking
-for the length will trigger a SQL query in most circumstances.
-
-  >>> CursorWrapper.last_executed_sql = []
-  >>> ignored = email_batch.total()
-  >>> print CursorWrapper.last_executed_sql[0]
-  SELECT COUNT(*) FROM EmailAddress
-
-There are exceptions. When the current batch is the last one in the
-list, it's possible to get the length of the entire resultset without
-triggering a COUNT query.
-
-  >>> CursorWrapper.last_executed_sql = []
-  >>> batch_nav = BatchNavigator(select_results, build_request(), size=50)
-  >>> final_batch = batch_nav.currentBatch().nextBatch()
-  >>> batch_items = list(final_batch)
-  >>> ignored = final_batch.total()
-  >>> print "\n".join(CursorWrapper.last_executed_sql)
-  SELECT ... FROM EmailAddress ... OFFSET 0
-  SELECT ... FROM EmailAddress ... OFFSET 50
-
-When the current batch contains the entire resultset, it's possible to
-get the length of the resultset without triggering a COUNT query.
-
-  >>> CursorWrapper.last_executed_sql = []
-  >>> one_page_nav = BatchNavigator(select_results, build_request(), size=200)
-  >>> only_batch = one_page_nav.currentBatch()
-  >>> batch_items = list(only_batch)
-  >>> ignored = only_batch.total()
-  >>> print "\n".join(CursorWrapper.last_executed_sql)
-  SELECT ... FROM EmailAddress ... OFFSET 0
-
 Multiple pages
 ==============
 
 The batch navigator tells us whether multiple pages will be used.
 
+    >>> from canonical.launchpad.database.emailaddress import EmailAddress
+    >>> select_results = EmailAddress.select(orderBy='id')
+    >>> batch_nav = BatchNavigator(select_results, build_request(), size=50)
     >>> batch_nav.has_multiple_pages
     True
 
+    >>> one_page_nav = BatchNavigator(select_results, build_request(), size=200)
     >>> one_page_nav.has_multiple_pages
     False
 

=== modified file 'lib/lp/app/browser/root.py'
--- lib/lp/app/browser/root.py	2011-04-12 11:36:11 +0000
+++ lib/lp/app/browser/root.py	2011-04-12 20:07:39 +0000
@@ -13,6 +13,7 @@
 import time
 
 import feedparser
+from lazr.batchnavigator import ListRangeFactory
 from lazr.batchnavigator.z3batching import batch
 from zope.component import getUtility
 from zope.schema.interfaces import TooLong
@@ -560,6 +561,7 @@
 class GoogleBatchNavigator(BatchNavigator):
     """A batch navigator with a fixed size of 20 items per batch."""
 
+    _batch_factory = WindowedListBatch
     # Searches generally don't show the 'Last' link when there is a
     # good chance of getting over 100,000 results.
     show_last_link = False
@@ -567,7 +569,9 @@
     singular_heading = 'page'
     plural_heading = 'pages'
 
-    def __init__(self, results, request, start=0, size=20, callback=None):
+    def __init__(self, results, request, start=0, size=20, callback=None,
+                 transient_parameters=None, force_start=False,
+                 range_factory=None):
         """See `BatchNavigator`.
 
         :param results: A `PageMatches` object that contains the matching
@@ -578,25 +582,13 @@
         :param size: The batch size is fixed to 20, The param is not used.
         :param callback: Not used.
         """
-        # We do not want to call super() because it will use the batch
-        # size from the URL.
-        # pylint: disable-msg=W0231
         results = WindowedList(results, start, results.total)
-        self.request = request
-        request_start = request.get(self.start_variable_name, None)
-        if request_start is None:
-            self.start = start
-        else:
-            try:
-                self.start = int(request_start)
-            except (ValueError, TypeError):
-                self.start = start
+        super(GoogleBatchNavigator, self).__init__(results, request,
+            start=start, size=size, callback=callback,
+            transient_parameters=transient_parameters, force_start=force_start,
+            range_factory=range_factory)
 
+    def determineSize(self, size, batch_params_source):
+        # Force the default and users requested sizes to 20.
         self.default_size = 20
-
-        self.transient_parameters = [self.start_variable_name]
-
-        self.batch = WindowedListBatch(
-            results, start=self.start, size=self.default_size)
-        self.setHeadings(
-            self.default_singular_heading, self.default_plural_heading)
+        return 20

=== modified file 'lib/lp/app/browser/tests/launchpad-search-pages.txt'
--- lib/lp/app/browser/tests/launchpad-search-pages.txt	2011-04-12 11:36:11 +0000
+++ lib/lp/app/browser/tests/launchpad-search-pages.txt	2011-04-12 20:07:39 +0000
@@ -444,8 +444,6 @@
 provides a link to the next batch, which also happens to be the last
 batch.
 
-    >>> len(search_view.pages.getBatches())
-    2
     >>> search_view.pages.nextBatchURL()
     '...start=20'
     >>> search_view.pages.lastBatchURL()

=== modified file 'lib/lp/bugs/stories/webservice/xx-bug-tracker.txt'
--- lib/lp/bugs/stories/webservice/xx-bug-tracker.txt	2011-04-12 11:36:11 +0000
+++ lib/lp/bugs/stories/webservice/xx-bug-tracker.txt	2011-04-12 20:07:39 +0000
@@ -10,7 +10,7 @@
     >>> bug_tracker_collection = anon_webservice.get(
     ...     '/bugs/bugtrackers').jsonBody()
     >>> pprint_collection(bug_tracker_collection)
-    next_collection_link: u'http://.../bugs/bugtrackers?ws.start=5&ws.size=5'
+    next_collection_link: u'http://.../bugs/bugtrackers?ws.size=5&memo=5&ws.start=5'
     resource_type_link: u'http://.../#bug_trackers'
     start: 0
     total_size: 8

=== modified file 'lib/lp/bugs/stories/webservice/xx-bug.txt'
--- lib/lp/bugs/stories/webservice/xx-bug.txt	2011-04-12 11:36:11 +0000
+++ lib/lp/bugs/stories/webservice/xx-bug.txt	2011-04-12 20:07:39 +0000
@@ -1837,7 +1837,7 @@
 
     >>> cves = webservice.get("/bugs/cve").jsonBody()
     >>> pprint_collection(cves)
-    next_collection_link: u'http://.../bugs/cve?ws.start=5&ws.size=5'
+    next_collection_link: u'http://.../bugs/cve?ws.size=5&memo=5&ws.start=5'
     resource_type_link: u'http://.../#cves'
     start: 0
     total_size: 10
@@ -2126,7 +2126,7 @@
   >>> activity = webservice.get(
   ...     bug_one['activity_collection_link']).jsonBody()
   >>> pprint_collection(activity)
-  next_collection_link: u'http://.../bugs/1/activity?ws.start=5&ws.size=5'
+  next_collection_link: u'http://.../bugs/1/activity?ws.size=5&memo=5&ws.start=5'
   resource_type_link: u'http://.../#bug_activity-page-resource'
   start: 0
   total_size: 25

=== modified file 'lib/lp/services/features/browser/tests/test_changelog.py'
--- lib/lp/services/features/browser/tests/test_changelog.py	2011-04-12 11:36:11 +0000
+++ lib/lp/services/features/browser/tests/test_changelog.py	2011-04-12 20:07:39 +0000
@@ -80,7 +80,7 @@
         self.assertEqual('change', batch._singular_heading)
         self.assertEqual('changes', batch._plural_heading)
         self.assertEqual(10, batch.default_size)
-        self.assertEqual(2, len(batch.getBatches()))
+        self.assertEqual(None, batch.currentBatch().nextBatch().nextBatch())
 
     def test_page_batched_changes(self):
         self.makeFeatureFlagChanges()

=== modified file 'lib/lp/translations/browser/tests/pofile-views.txt'
--- lib/lp/translations/browser/tests/pofile-views.txt	2011-04-12 11:36:11 +0000
+++ lib/lp/translations/browser/tests/pofile-views.txt	2011-04-12 20:07:39 +0000
@@ -241,7 +241,7 @@
 Also, we get redirected to the next batch.
 
     >>> pofile_view.request.response.getHeader('Location')
-    'http://127.0.0.1?start=10'
+    'http://127.0.0.1?memo=10&start=10'
 
 The messsage's sequence is the position of that message in latest imported
 template. We are going to test now what happens when we submit a potmsgset

=== modified file 'versions.cfg'
--- versions.cfg	2011-04-12 11:36:11 +0000
+++ versions.cfg	2011-04-12 20:07:39 +0000
@@ -29,7 +29,7 @@
 keyring = 0.5.1
 launchpadlib = 1.9.3
 lazr.authentication = 0.1.1
-lazr.batchnavigator = 1.2.2
+lazr.batchnavigator = 1.2.4
 lazr.config = 1.1.3
 lazr.delegates = 1.2.0
 lazr.enum = 1.1.2