Merge lp:~adeuring/lazr.batchnavigator/lazr.batchnavigator-urlencode-batch-params into lp:lazr.batchnavigator

[Abel Deuring]

The class lazr.batchnavigator.BatchNavigator delegates the creation of the batch parameters to an IRangeFactory.

Batch URLs are generated by BatchNavigator.generateBatchURL(), which builds the URL from batch.range_memo, among other parameters. These parameters were not URL-escaped. This is OK, if the parameters themselves are URL safe, like string representations of integers.

But the main idea of IRangeFactory is that we can use other factories than the only currently existing implementation, ListRangeFactory, which integers for the memo value. Specifically, I am working on a class StormRangeFactory, which will create memo parameters from SQL table columns used to sort the query result. (This allows us to replace the OFFSET clause with something like WHERE Table.index_col > last_batch_row.index_col, as suggested by Jeroen some time ago.)

This means that we will have soon more or less arbitrary values in the memo field, and we could end up with a memo value like 'foo & bar'. In other words: URL query parameter 'meo' must be properly URL-escaped.

The implementation and the test are straightforward, I think.

Ah, and I removed trailing whitespaces o a few lines.

[Benji York (benji)]

This looks good. Thanks.

review: Approve (code)

=== modified file 'src/lazr/batchnavigator/_batchnavigator.py'
--- src/lazr/batchnavigator/_batchnavigator.py	2011-07-13 05:49:45 +0000
+++ src/lazr/batchnavigator/_batchnavigator.py	2011-07-25 15:56:22 +0000
@@ -32,7 +32,7 @@
     )
 
 __all__ = [
-    'BatchNavigator', 
+    'BatchNavigator',
     'ListRangeFactory',
     ]
 
@@ -159,7 +159,7 @@
         self.default_size. The base class implementation uses the size passed
         to the constructor, but other implementations may choose to clamp it or
         force a particular default size.
-        
+
         :param size: Size passed to the constructor.
         :param batch_params_source: User parameters dict.
         :return: The size to be used for this batch.
@@ -214,9 +214,9 @@
         self._singular_heading = singular
         self._plural_heading = plural
 
-    def getCleanQueryString(self, params=None):
-        """Removes batch nav params if present and returns a query
-        string.
+    def getCleanQueryParams(self, params=None):
+        """Removes batch nav params if present and returns a sequence
+        of key-values pairs.
 
         If ``params`` is None, uses the current query_string_params.
         """
@@ -232,40 +232,45 @@
 
         # We need the doseq=True because some url params are for multi-value
         # fields.
-        return urllib.urlencode(
-            [(key, value) for (key, value) in sorted(params)
-             if key not in self.transient_parameters],
-             doseq=True)
+        return [
+            (key, value) for (key, value) in sorted(params)
+            if key not in self.transient_parameters]
+
+    def getCleanQueryString(self, params=None):
+        """Removes batch nav params if present and returns a query
+        string.
+
+        If ``params`` is None, uses the current query_string_params.
+        """
+        # We need the doseq=True because some url params are for multi-value
+        # fields.
+        return urllib.urlencode(self.getCleanQueryParams(params), doseq=True)
 
     def generateBatchURL(self, batch, backwards=False):
         url = ""
         if batch is None:
             return url
 
-        qs = self.getCleanQueryString()
-        if qs:
-            elements = [qs]
-        else:
-            elements = []
+        params = self.getCleanQueryParams()
 
         size = batch.size
         if size != self.default_size:
             # The current batch size should only be part of the URL if it's
             # different from the default batch size.
-            elements.append("%s=%s" % (self.batch_variable_name, size))
+            params.append((self.batch_variable_name, size))
 
         if backwards:
-            elements.append("%s=backwards" % (self.direction_variable_name,))
+            params.append((self.direction_variable_name, "backwards"))
 
         if batch.range_memo:
-            elements.append("%s=%s" % (self.memo_variable_name, batch.range_memo))
+            params.append((self.memo_variable_name, batch.range_memo))
 
         start = batch.startNumber() - 1
         if start:
-            elements.append("%s=%s" % (self.start_variable_name, start))
+            params.append((self.start_variable_name, start))
 
         base_url = str(self.request.URL)
-        return "%s?%s" % (base_url, '&'.join(elements))
+        return "%s?%s" % (base_url, urllib.urlencode(params))
 
     def firstBatchURL(self):
         batch = self.batch.firstBatch()
@@ -292,7 +297,7 @@
 
     def _update_variable_names(self):
         """Update self.x_variable_name with self.variable_name_prefix.
-        
+
         This gives the concrete instance the same prefix for all variables.
         """
         prefix = self.variable_name_prefix or ''
@@ -317,7 +322,7 @@
 
     def getEndpointMemos(self, batch):
         """See `IRangeFactory`
-        
+
         Most implementations will want to use batch.sliced_list to retrieve
         database keys.
         """

=== modified file 'src/lazr/batchnavigator/tests/test_batchnavigator.py'
--- src/lazr/batchnavigator/tests/test_batchnavigator.py	2011-07-13 05:49:45 +0000
+++ src/lazr/batchnavigator/tests/test_batchnavigator.py	2011-07-25 15:56:22 +0000
@@ -149,6 +149,24 @@
             Equals(SERVER_URL + '?prefix_batch=3&prefix_direction=backwards'
                 '&prefix_memo=6&prefix_start=3'))
 
+    def test_range_factory_producing_url_unsafe_memos(self):
+        # Memo values containing characters with special meaning in URLs
+        # are properly escaped by generateBatchURL().
+        class WeirdRangeFactory(ListRangeFactory):
+            def getEndpointMemos(self, batch):
+                return ('start&/1', 'end&/2')
+
+        collection = range(1, 11)
+        request = TestRequest(
+            SERVER_URL=SERVER_URL, method='GET',
+            environ={'QUERY_STRING': "start=6&batch=3&memo=6"})
+        range_factory = WeirdRangeFactory(collection)
+        batchnav = BatchNavigator(
+            collection, request, range_factory=range_factory)
+        self.assertThat(
+            batchnav.nextBatchURL(),
+            Equals(SERVER_URL + '?batch=3&memo=end%26%2F2&start=9'))
+
 
 class TestListRangeFactory(testtools.TestCase):