launchpad-reviewers team mailing list archive

[Leonard Richardson <leonard.richardson@xxxxxxxxxxxxx>]

Leonard Richardson has proposed merging lp:~leonardr/launchpad/optimized-length into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)


The version of lazr.batchnavigator currently integrated into Launchpad causes the web service to raise a Storm exception in a few simple cases. I was able to land the bad version because we didn't test this particular code path--it's tested in lazr.restful, but lazr.restful doesn't use Storm.

This branch tests the code path that fails, and integrates a new version of lazr.batchnavigator into Launchpad to make it stop failing.

Some facts about the new version of lazr.batchnavigator:

1. I've optimized it to avoid triggering the code that causes the exception. This is good on its own, because even if it worked, that code would be very inefficient.

2. The new version can occasionally tell you the length of a collection without sending out a COUNT query. I added Launchpad tests for this at the SQL-peeking level. Unfortunately, we can't really take advantage of this feature right now--the way I found out about these failures in the first place was while integrating a version of lazr.restful that took advantage of the feature. If I start taking advantage of this feature, the datamodel.txt tests I've added will start failing again.

-- 
https://code.launchpad.net/~leonardr/launchpad/optimized-length/+merge/33132
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~leonardr/launchpad/optimized-length into lp:launchpad.

=== modified file 'lib/canonical/launchpad/doc/batch_navigation.txt'
--- lib/canonical/launchpad/doc/batch_navigation.txt	2010-08-12 15:03:51 +0000
+++ lib/canonical/launchpad/doc/batch_navigation.txt	2010-08-19 16:54:54 +0000
@@ -89,14 +89,38 @@
   True
 
 As seen above, simply accessing the batch doesn't trigger a SQL query
-asking for the length. But explicitly asking for the length will
-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
 ==============
 
@@ -105,6 +129,8 @@
     >>> batch_nav.has_multiple_pages
     True
 
+    >>> one_page_nav.has_multiple_pages
+    False
 
 Maximum batch size
 ==================

=== added file 'lib/canonical/launchpad/pagetests/webservice/datamodel.txt'
--- lib/canonical/launchpad/pagetests/webservice/datamodel.txt	1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/pagetests/webservice/datamodel.txt	2010-08-19 16:54:54 +0000
@@ -0,0 +1,35 @@
+***************************
+End-to-end data model tests
+***************************
+
+The lazr.restful tests use in-memory Python objects, and Launchpad
+uses Storm backed by a database. This means it's nice to have some
+end-to-end tests of code paths that, on the surface, look like they're
+already tested in lazr.restful.
+
+  >>> def get_collection(version, start=0, size=2):
+  ...     collection = webservice.get(
+  ...         ("/people?ws.op=find&text=s&ws.start=%s&ws.size=%s" %
+  ...          (start, size)),
+  ...         api_version=version)
+  ...     return collection.jsonBody()
+
+Even if an entire collection fits on one page (so that the total size
+of the collection is obvious), 'total_size_link' will be served
+instead of 'total_size'. (It would be nice to fix this.)
+
+  >>> collection = get_collection("devel", size=100)
+  >>> print sorted(collection.keys())
+  [u'entries', u'start', u'total_size_link']
+  >>> print webservice.get(collection['total_size_link']).jsonBody()
+  9
+
+Even if the last page of the collection is fetched (so that the total
+size of the collection is semi-obvious), 'total_size_link' will be
+served instead of 'total_size'. (It would be nice to fix this.)
+
+  >>> collection = get_collection("devel", start=8)
+  >>> print sorted(collection.keys())
+  [u'entries', u'prev_collection_link', u'start', u'total_size_link']
+  >>> print webservice.get(collection['total_size_link']).jsonBody()
+  9

=== modified file 'lib/canonical/launchpad/pagetests/webservice/multiversion.txt'
--- lib/canonical/launchpad/pagetests/webservice/multiversion.txt	2010-08-13 17:22:53 +0000
+++ lib/canonical/launchpad/pagetests/webservice/multiversion.txt	2010-08-19 16:54:54 +0000
@@ -9,25 +9,27 @@
 return collections will return a 'total_size_link' pointing to the
 total size of the collection.
 
-  >>> def get_collection(version):
+  >>> def get_collection(version, start=0, size=2):
   ...     collection = webservice.get(
-  ...         "/people?ws.op=find&text=salgado", api_version=version)
+  ...         ("/people?ws.op=find&text=s&ws.start=%s&ws.size=%s" %
+  ...          (start, size)),
+  ...         api_version=version)
   ...     return collection.jsonBody()
 
   >>> collection = get_collection("devel")
   >>> print sorted(collection.keys())
-  [u'entries', u'start', u'total_size_link']
+  [u'entries', u'next_collection_link', u'start', u'total_size_link']
   >>> print webservice.get(collection['total_size_link']).jsonBody()
-  1
+  9
 
 In previous versions, the same named operations will return a
 'total_size' containing the actual size of the collection.
 
   >>> collection = get_collection("1.0")
   >>> print sorted(collection.keys())
-  [u'entries', u'start', u'total_size']
+  [u'entries', u'next_collection_link', u'start', u'total_size']
   >>> print collection['total_size']
-  1
+  9
 
 Mutator operations
 ==================

=== modified file 'versions.cfg'
--- versions.cfg	2010-08-17 20:20:28 +0000
+++ versions.cfg	2010-08-19 16:54:54 +0000
@@ -26,7 +26,7 @@
 ipython = 0.9.1
 launchpadlib = 1.6.4
 lazr.authentication = 0.1.1
-lazr.batchnavigator = 1.2.1
+lazr.batchnavigator = 1.2.2
 lazr.config = 1.1.3
 lazr.delegates = 1.2.0
 lazr.enum = 1.1.2