Merge lp:~jtv/lazr.batchnavigator/bug-574159 into lp:lazr.batchnavigator

[Jeroen T. Vermeulen]

Ignore negative batch sizes.

= Bug 574159 =

The batch navigator happily accepts negative batch sizes, which as far as I can tell have no meaning and cause oopses in Launchpad.

Here's a fix to make it ignore negative batch sizes. If we ever do decide to assign a useful meaning to negative batch sizes, at least this makes the choice explicit and tests it.

At least, that's the theory. The fact is that I haven't figured out how to run the test yet.

[Jeroen T. Vermeulen (jtv)]

Had to patch up the test a bit, but it's passing now.

[Barry Warsaw (barry)]

> Had to patch up the test a bit, but it's passing now.

Well the branch diff /still/ isn't available on the page, but I managed to
trick Launchpad into giving it to me.

=== modified file 'src/lazr/batchnavigator/README.txt'
--- src/lazr/batchnavigator/README.txt 2009-03-23 16:14:28 +0000
+++ src/lazr/batchnavigator/README.txt 2010-05-04 03:10:46 +0000
> @@ -191,6 +191,20 @@
> >>> navigator.nextBatchURL()
> 'http://www.example.com/foo?start=3&batch=1'
>
> +The batch argument must be positive. Negative numbers or zero are ignored.
> +
> + >>> from cgi import parse_qs
> + >>> BatchNavigator(reindeer, request=build_request([('batch', '0')]))
> + >>> next_batch = reindeer_batch_navigator_post_with_qs.nextBatchURL()
> + >>> print parse_qs(next_batch)['batch']
> + 50
> +
> + >>> BatchNavigator(reindeer, request=build_request([('batch', '-1')]))
> + >>> next_batch = reindeer_batch_navigator_post_with_qs.nextBatchURL()
> + >>> print parse_qs(next_batch)['batch']
> + 50

I'm not sure how this shows that the batch is ignored. Without looking at the
rest of the test, what does '50' mean? Maybe the docs before this explain
that this is the default batch size, but if not, could you provide a little
more context?

> +
> +
> =============
> Empty Batches
> =============

=== modified file 'src/lazr/batchnavigator/_batchnavigator.py'
--- src/lazr/batchnavigator/_batchnavigator.py 2009-03-23 16:14:28 +0000
+++ src/lazr/batchnavigator/_batchnavigator.py 2010-05-04 03:10:46 +0000
> @@ -108,16 +108,19 @@
> self.default_size = size
>
> request_size = batch_params_source.get(self.batch_variable_name, None)
> - if request_size:
> + if request_size is not None:
> try:
> - size = int(request_size)
> + request_size = int(request_size)

I don't love the fact that you're changing the type of the local variable
`request_size` within the conditional. Are you sure request_size isn't used
or referenced outside of this block?

> except (ValueError, TypeError):
> - pass
> - if size > self.max_batch_size:
> + request_size = 0
> +
> + if request_size > self.max_batch_size:
> raise InvalidBatchSizeError(
> 'Maximum for "%s" parameter is %d.' %
> (self.batch_variable_name,
> self.max_batch_size))
> + if request_size > 0:
> + size = request_size

No InvalidBatchSizeError for when the request is < 0? Do you think an error
makes more sense?

>
> if size is None:
> size = self.default_batch_size

[lp:~jtv/lazr.batchnavigator/bug-574159]

30. By Jeroen T. Vermeulen on 2010-05-05

Extracted method.

[Jeroen T. Vermeulen (jtv)]

Hi Barry,

Thanks for looking at it despite the difficulties.

> Well the branch diff /still/ isn't available on the page, but I managed to
> trick Launchpad into giving it to me.

Unfortunately you still got an outdated diff. Meanwhile I've updated it again; it's probably best to do this using bzr.

> === modified file 'src/lazr/batchnavigator/README.txt'
> --- src/lazr/batchnavigator/README.txt 2009-03-23 16:14:28 +0000
> +++ src/lazr/batchnavigator/README.txt 2010-05-04 03:10:46 +0000
> > @@ -191,6 +191,20 @@
> > >>> navigator.nextBatchURL()
> > 'http://www.example.com/foo?start=3&batch=1'
> >
> > +The batch argument must be positive. Negative numbers or zero are ignored.
> > +
> > + >>> from cgi import parse_qs
> > + >>> BatchNavigator(reindeer, request=build_request([('batch', '0')]))
> > + >>> next_batch = reindeer_batch_navigator_post_with_qs.nextBatchURL()
> > + >>> print parse_qs(next_batch)['batch']
> > + 50
> > +
> > + >>> BatchNavigator(reindeer, request=build_request([('batch', '-1')]))
> > + >>> next_batch = reindeer_batch_navigator_post_with_qs.nextBatchURL()
> > + >>> print parse_qs(next_batch)['batch']
> > + 50
>
> I'm not sure how this shows that the batch is ignored. Without looking at the
> rest of the test, what does '50' mean? Maybe the docs before this explain
> that this is the default batch size, but if not, could you provide a little
> more context?

This is where the outdated diff hurts us. I had changed the test to state that the default batch size was used instead, and show the default batch size (which is 50) before showing that the actual batch sizes for these two batch navigators are also 50.

> === modified file 'src/lazr/batchnavigator/_batchnavigator.py'
> --- src/lazr/batchnavigator/_batchnavigator.py 2009-03-23 16:14:28 +0000
> +++ src/lazr/batchnavigator/_batchnavigator.py 2010-05-04 03:10:46 +0000
> > @@ -108,16 +108,19 @@
> > self.default_size = size
> >
> > request_size = batch_params_source.get(self.batch_variable_name,
> None)
> > - if request_size:
> > + if request_size is not None:
> > try:
> > - size = int(request_size)
> > + request_size = int(request_size)
>
> I don't love the fact that you're changing the type of the local variable
> `request_size` within the conditional. Are you sure request_size isn't used
> or referenced outside of this block?

Quite. I've now done the right thing though: extracted the method and used separate names for the variables. It also makes the logic a bit easier to follow, because the new method can just "return None" whenever things don't work out.

> > except (ValueError, TypeError):
> > - pass
> > - if size > self.max_batch_size:
> > + request_size = 0
> > +
> > + if request_size > self.max_batch_size:
> > raise InvalidBatchSizeError(
> > 'Maximum for "%s" parameter is %d.' %
> > (self.batch_variable_name,
> > self.max_batch_size))
> > + if request_size > 0:
> > + siz...

[Jeroen T. Vermeulen (jtv)]

The diff updated promptly this time.

[Abel Deuring (adeuring)]

review: Approve (code)

=== modified file 'src/lazr/batchnavigator/README.txt'
--- src/lazr/batchnavigator/README.txt	2009-03-23 16:14:28 +0000
+++ src/lazr/batchnavigator/README.txt	2010-05-05 03:40:45 +0000
@@ -191,6 +191,27 @@
     >>> navigator.nextBatchURL()
     'http://www.example.com/foo?start=3&batch=1'
 
+The batch argument must be positive.  Other numbers are ignored, and the
+default batch size is used instead.
+
+    >>> from cgi import parse_qs
+    >>> request = build_request({'batch': '0'})
+    >>> navigator = BatchNavigator(range(99), request=request)
+    >>> print navigator.default_batch_size
+    50
+    >>> for value in parse_qs(navigator.nextBatchURL())['batch']:
+    ...     print value
+    50
+
+    >>> request = build_request({'batch': '-1'})
+    >>> navigator = BatchNavigator(range(99), request=request)
+    >>> print navigator.default_batch_size
+    50
+    >>> for value in parse_qs(navigator.nextBatchURL())['batch']:
+    ...     print value
+    50
+
+
 =============
 Empty Batches
 =============

=== modified file 'src/lazr/batchnavigator/_batchnavigator.py'
--- src/lazr/batchnavigator/_batchnavigator.py	2009-03-23 16:14:28 +0000
+++ src/lazr/batchnavigator/_batchnavigator.py	2010-05-05 03:40:45 +0000
@@ -1,4 +1,4 @@
-# Copyright 2004-2009 Canonical Ltd.  All rights reserved.
+# Copyright 2004-2010 Canonical Ltd.  All rights reserved.
 #
 # This file is part of lazr.batchnavigator
 #
@@ -106,19 +106,9 @@
                 self.start = start
 
         self.default_size = size
-
-        request_size = batch_params_source.get(self.batch_variable_name, None)
-        if request_size:
-            try:
-                size = int(request_size)
-            except (ValueError, TypeError):
-                pass
-            if size > self.max_batch_size:
-                raise InvalidBatchSizeError(
-                    'Maximum for "%s" parameter is %d.' %
-                    (self.batch_variable_name,
-                     self.max_batch_size))
-
+        request_size = self._getRequestedSize(batch_params_source)
+        if request_size is not None:
+            size = request_size
         if size is None:
             size = self.default_batch_size
 
@@ -128,6 +118,36 @@
         self.setHeadings(
             self.default_singular_heading, self.default_plural_heading)
 
+    def _getRequestedSize(self, batch_params_source):
+        """Figure out what batch size the user requested, if any.
+
+        Sizes that are not positive numbers are ignored.
+
+        :return: An acceptable batch size requested by the user, or
+            None.
+        :raise: `InvalidBatchSizeError` if the requested size exceeds
+            `max_batch_size`.
+        """
+        size_string = batch_params_source.get(self.batch_variable_name, None)
+        if size_string is None:
+            return None
+
+        try:
+            request_size = int(size_string)
+        except (ValueError, TypeError):
+            return None
+
+        if request_size <= 0:
+            return None
+
+        if request_size > self.max_batch_size:
+            raise InvalidBatchSizeError(
+                'Maximum for "%s" parameter is %d.' %
+                (self.batch_variable_name,
+                 self.max_batch_size))
+
+        return request_size
+
     @property
     def heading(self):
         """See `IBatchNavigator`"""