Merge lp:~adeuring/launchpad/bug-739052 into lp:launchpad

[Abel Deuring]

[r=lifeless][no-qa] new class StormRangeFactory

This branch adds a class StormRangeFactory, which implements
the IRangeFactory interface. It works for Storm result sets,
and allows to retrieve batches as suggested by Jeroen some
months ago: Instead of an OFFSET clause it uses WHERE
expressions. The WHERE expressions are generated from the
sort expressions of the result set.

Limitations:
  - The class works only for WHERE expressions like Person.id
    or Desc(Person.id). In other words, it assumes that the
    order expression is a Storm PropertyColumn. PropertyColumns
    are Python descriptors, i.e., they provide the methods
    __get__(), __set__(), __delete__(). They allow a usage like

      Person.id.__get__(instance_of_Person)

    where resultset[0] is an instance of class Person.

    Sort expressions like

      resultset.order_by('Person.id')

    or

      resultset.order_by(Person.displayname + Person.name)

    are not supported.

  - The query must return instances of SQLBase-derived classes.
    In other words, a query like

       store.find((Person.id, Person.name))

    is not supported by StormRangeFactory. (A DecoratedResultSet
    should be used in this case.)

The basic concept:

A StormRangeFactory is intended to be used by a
lazr.batchnavigator.BatchNavigator to retrieve a slice of a Storm
result set.

BatchNavigator calls range_factory.getEndpointMemos() to get
parameters which describe how to retrieve the previous or next
batch. These values are use for "memo" parameter in URLs for
the previous/next batch.

StormRangeFactory.getEndpointMemos() returns a JSON representation
of the values of the sort columns of the first and last element
of the current batch.

This JSON representation is used in StormRangeFactory.getSlice()
to generate WHERE expressions.

This branch does a few no-nos:

  - it imports PropertyColumn from storm.properties, but this
    class does not appear in the module's __all__ list.
    This import is not strictly necessary, but it makes the
    detection possible usage problems much easier.
  - it uses storm.store.ResultSet.find(), but this method is not
    declared in IResultSet.

I suspected such problems when I started this branch; lifeless
suggested to start nevertheless -- we could submit patches to
Storm if this work turn out to be useful.

The new method DecoratedResultSet.find() looks a bit convoluted
with its security proxy treatment. This is necessary because
we use this class on both sides of the "security fence":
In model code as well as in browser code.

If a DecoratedResultSet is created in browser code, we need
to remove the security proxy of the plain result set in order
to call its method find(), but the new result set should
again be security proxied.

But if a DecoratedResultSet is created in model code, its
decorator can access "forbidden" attributes, so we cannot
store a proxied Storm ResultSet as self.result_set of the
new DecoratedResultSet generated by find(). An example is
the DecoratedResultSet returned by Bug.attachments: its
decorator (set_indexed_message()) accesses
attachment._messageID.

I "recycled" the file lib/canonical/launchpad/webapp/tests/test_batching.py:
It did not run any doc tests, so I simply used it for the unit
tests.

tests:
./bin/test canonical -vvt canonical.launchpad.webapp.tests.test_batching
./bin/test canonical -vvt lib/canonical/launchpad/components/tests/decoratedresultset.txt

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/canonical/launchpad/components/decoratedresultset.py
  lib/canonical/launchpad/components/tests/decoratedresultset.txt
  lib/canonical/launchpad/webapp/batching.py
  lib/canonical/launchpad/webapp/tests/test_batching.py

./lib/canonical/launchpad/components/tests/decoratedresultset.txt
       1: narrative uses a moin header.
      33: narrative uses a moin header.
      56: narrative uses a moin header.
      74: narrative uses a moin header.
      82: narrative uses a moin header.
      91: narrative uses a moin header.
      99: narrative uses a moin header.
     107: narrative uses a moin header.
     121: narrative uses a moin header.
     132: narrative uses a moin header.
     139: narrative uses a moin header.

well... the diff for this branch is already 799 lines long, so I
did not remove the lint...

[Robert Collins (lifeless)]

 have a functional suggestion for you

currently you have some limits about requiring the thing being sorted on to be in the result set.
But resultset.find(resultset._order_by) will return those expressions (except for desc).

You could handle DecoratedResultSet by doing this:
resultset.find(original_find_expr + _order_by_after_stripping_desc) and creating a lambda to strip the extra results off, then feed them back into the original decoratedresultset row / resultset callbacks.

This could be done in a follow on patch of course.

The following are things I noted reading the patch, I don't think any are mandatory, but they are all pretty shallow so please give them your consideration.

DecoratedResultSet uses pep8 - foo_bar not fooBar, so you could follow its idiom there.

You can add find() to IResultSet via zcml magic if you want; personally I'd fixup storm directly (but after we have this working :)).

DateTimeJSONEncoder might want to live somewhere in the lazr.restful space I guess? What does lazr.restful do for datetime objects? I bet there is an existing encoder.

A decoratedresultset(decoratedresultset(resultset)) will barf atm - may want to loop in either the caller, or the get_plain_resultset method.

I would personally call getSortExpressions getOrderBy - given thats what it docstring says it does.

StormRangeFactory supports only sorting by
would be better english as
StormRangeFactory only supports sorting by

You may need
300 + if isinstance(expression, Desc):
301 + expression = expression.expr
302 + return expression < memo
303 + else:
304 + return expression > memo

to have >= instead of > - I'm not sure about this, but a collection sorted on two keys - say name, someint with rows ('foo', 1) ('foo', 2) ('foo', 3) ... would be a way to test this - make sure that pages work properly.

subclassing assertionerror - StormRangeFactoryError(AssertionError) - is a bit ugly, better to subclass Exception directly

typo: Pytjon

review: Approve

[Abel Deuring (adeuring)]

Hi Robert,

thanks for the review and many goos suggestions.

On 29.07.2011 10:42, Robert Collins wrote:
> Review: Approve
> have a functional suggestion for you
[...]
>
> You may need
> 300 + if isinstance(expression, Desc):
> 301 + expression = expression.expr
> 302 + return expression < memo
> 303 + else:
> 304 + return expression > memo
>
> to have >= instead of > - I'm not sure about this, but a collection
sorted on two keys - say name, someint with rows ('foo', 1) ('foo', 2)
('foo', 3) ... would be a way to test this - make sure that pages work
properly.

Good catch!

Ordering by more that one column makes the approach indeed a bit more
complicated....

We don't want the row with the memo values itself in the result, so, for
a sort expression like "ORDER BY col1, col2, col3" we can use

  SELECT ... FROM ...
  WHERE (original_clause AND col1 >= memo1 AND col2 >= memo2
         AND col3 >= memo3) OFFSET 1

An ugly alternative:

   WHERE (original_clause) AND col1 > memo1
         OR (col1 = memo1 AND col2 > memo2)
         OR (col1 = memo1 AND col2 = memo2 AND col3 > memo3)

Or, since the three WHERE clauses do not overlap:

   (SELECT ... FROM ... WHERE original_clause
       AND col1 = memo1 AND col2 = memo2 AND col3 > memo3 ORDER BY col3)
   UNION ALL
   (SELECT ... FROM ... WHERE original_clause
       AND col1 = memo1 AND col2 > memo2 ORDER BY col2, col3)
   UNION ALL
   (SELECT ... FROM ... WHERE original_clause
   AND col1 > memo1 ORDER BY col1, col2, col3)

that's even more ugly...

[Robert Collins (lifeless)]

I think

  SELECT ... FROM ...
  WHERE (original_clause AND col1 >= memo1 AND col2 >= memo2
         AND col3 > memo3)

is whats needed ?

[Robert Collins (lifeless)]

nvm I see - col2 should be bound differently for col1==memo1.

However
(col1, col2, col3) >= (memo1, memo2, memo3)

should work ?

[Abel Deuring (adeuring)]

On 02.08.2011 11:01, Robert Collins wrote:
> I think
>
>
> SELECT ... FROM ...
> WHERE (original_clause AND col1 >= memo1 AND col2 >= memo2
> AND col3 > memo3)
>
> is whats needed ?
>

no, lets assume two sort columns, with these values in the result set:

1, 1
1, 2
1, 3
2, 1
2, 2
2, 3

If the memo value is (1, 2), we want the rows starting at (1, 3).

So we need the rows where col1 == memo1 and col2 > memo2, and
additinally the rows where col1 > memo1.

See also
https://code.launchpad.net/~adeuring/launchpad/bug-739052-2/+merge/70044

=== modified file 'lib/canonical/launchpad/components/decoratedresultset.py'
--- lib/canonical/launchpad/components/decoratedresultset.py	2011-05-03 07:10:37 +0000
+++ lib/canonical/launchpad/components/decoratedresultset.py	2011-07-28 11:31:31 +0000
@@ -11,7 +11,10 @@
 from lazr.delegates import delegates
 from storm import Undef
 from storm.zope.interfaces import IResultSet
-from zope.security.proxy import removeSecurityProxy
+from zope.security.proxy import (
+    ProxyFactory,
+    removeSecurityProxy,
+    )
 
 
 class DecoratedResultSet(object):
@@ -170,3 +173,22 @@
         return DecoratedResultSet(
             new_result_set, self.result_decorator, self.pre_iter_hook,
             self.slice_info)
+
+    def getPlainResultSet(self):
+        """Return the plain Storm result set."""
+        return self.result_set
+
+    def find(self, *args, **kwargs):
+        """See `IResultSet`.
+
+        :return: The decorated version of the returned result set.
+        """
+        naked_result_set = removeSecurityProxy(self.result_set)
+        if naked_result_set is not self.result_set:
+            naked_new_result_set = naked_result_set.find(*args, **kwargs)
+            new_result_set = ProxyFactory(naked_new_result_set)
+        else:
+            new_result_set = self.result_set.find(*args, **kwargs)
+        return DecoratedResultSet(
+            new_result_set, self.result_decorator, self.pre_iter_hook,
+            self.slice_info)

=== modified file 'lib/canonical/launchpad/components/tests/decoratedresultset.txt'
--- lib/canonical/launchpad/components/tests/decoratedresultset.txt	2010-10-18 22:24:59 +0000
+++ lib/canonical/launchpad/components/tests/decoratedresultset.txt	2011-07-28 11:31:31 +0000
@@ -136,3 +136,19 @@
     >>> isinstance(decorated_result_set[0:3], DecoratedResultSet)
     True
 
+== find() ==
+
+DecoratedResultSet.find() returns another DecoratedResultSet containing
+a refined query.
+
+    >>> result_set = store.find(Distribution)
+    >>> proxied_result_set = ProxyFactory(result_set)
+    >>> decorated_result_set = DecoratedResultSet(
+    ...     proxied_result_set, result_decorator)
+    >>> ubuntu_distros = removeSecurityProxy(decorated_result_set).find(
+    ...     "Distribution.name like 'ubuntu%'")
+    >>> for dist in ubuntu_distros:
+    ...     dist
+    u'Dist name is: ubuntu'
+    u'Dist name is: ubuntutest'
+

=== modified file 'lib/canonical/launchpad/webapp/batching.py'
--- lib/canonical/launchpad/webapp/batching.py	2011-07-13 06:17:19 +0000
+++ lib/canonical/launchpad/webapp/batching.py	2011-07-28 11:31:31 +0000
@@ -3,20 +3,38 @@
 
 __metaclass__ = type
 
+from datetime import datetime
+
 import lazr.batchnavigator
+from lazr.batchnavigator.interfaces import IRangeFactory
+import simplejson
+from storm import Undef
+from storm.expr import Desc
+from storm.properties import PropertyColumn
 from storm.zope.interfaces import IResultSet
 from zope.component import adapts
 from zope.interface import implements
 from zope.interface.common.sequence import IFiniteSequence
+from zope.security.proxy import (
+    isinstance as zope_isinstance,
+    ProxyFactory,
+    removeSecurityProxy,
+    )
 
 from canonical.config import config
-from canonical.launchpad.webapp.interfaces import ITableBatchNavigator
+from canonical.launchpad.components.decoratedresultset import (
+    DecoratedResultSet,
+    )
+from canonical.launchpad.webapp.interfaces import (
+    ITableBatchNavigator,
+    StormRangeFactoryError,
+    )
 from canonical.launchpad.webapp.publisher import LaunchpadView
 
 
 class FiniteSequenceAdapter:
 
-    adapts(IResultSet) # and ISQLObjectResultSet
+    adapts(IResultSet)
     implements(IFiniteSequence)
 
     def __init__(self, context):
@@ -123,3 +141,216 @@
         if columns_to_show:
             for column_to_show in columns_to_show:
                 self.show_column[column_to_show] = True
+
+
+class DateTimeJSONEncoder(simplejson.JSONEncoder):
+    """A JSON encoder that understands datetime objects.
+
+    Datetime objects are formatted according to ISO 1601.
+    """
+    def default(self, obj):
+        if isinstance(obj, datetime):
+            return obj.isoformat()
+        return simplejson.JSONEncoder.default(self, obj)
+
+
+class StormRangeFactory:
+    """A range factory for Storm result sets.
+
+    It creates the endpoint memo values from the expressions used in the
+    ORDER BY clause.
+
+    Limitations:
+
+    - The order_by expressions must be Storm PropertyColumn instances,
+      e.g. Bug.title. Simple strings (e.g., resultset.order_by('Bug.id')
+      or general Storm SQL expression are not supported.
+    - The objects representing rows of the PropertyColumn's table must
+      be contained in the result. I.e.,
+
+          store.find(Bug.id, Bug.id < 10)
+
+      does not work, while
+
+          store.find(Bug, Bug.id < 10)
+
+      works.
+    """
+    implements(IRangeFactory)
+
+    def __init__(self, resultset, error_cb=None):
+        """Create a new StormRangeFactory instance.
+
+        :param resultset: A Storm ResultSet instance or a DecoratedResultSet
+            instance.
+        :param error_cb: A function which takes one string as a parameter.
+            It is called when the parameter endpoint_memo of getSlice()
+            does not match the order settings of a resultset.
+        """
+        self.resultset = resultset
+        if zope_isinstance(resultset, DecoratedResultSet):
+            self.plain_resultset = resultset.getPlainResultSet()
+        else:
+            self.plain_resultset = resultset
+        self.error_cb = error_cb
+
+    def getSortExpressions(self):
+        """Return the order_by expressions of the result set."""
+        return removeSecurityProxy(self.plain_resultset)._order_by
+
+    def getOrderValuesFor(self, row):
+        """Return the values of the order_by expressions for the given row.
+        """
+        sort_values = []
+        if not zope_isinstance(row, tuple):
+            row = (row, )
+        sort_expressions = self.getSortExpressions()
+        if sort_expressions is Undef:
+            raise StormRangeFactoryError(
+                'StormRangeFactory requires a sorted result set.')
+        for expression in sort_expressions:
+            if zope_isinstance(expression, Desc):
+                expression = expression.expr
+            if not zope_isinstance(expression, PropertyColumn):
+                raise StormRangeFactoryError(
+                    'StormRangeFactory supports only sorting by '
+                    'PropertyColumn, not by %r.' % expression)
+            class_instance_found = False
+            for row_part in row:
+                if zope_isinstance(row_part, expression.cls):
+                    sort_values.append(expression.__get__(row_part))
+                    class_instance_found = True
+                    break
+            if not class_instance_found:
+                raise StormRangeFactoryError(
+                    'Instances of %r are not contained in the result set, '
+                    'but are required to retrieve the value of %s.%s.'
+                    % (expression.cls, expression.cls.__name__,
+                       expression.name))
+        return sort_values
+
+    def getEndpointMemos(self, batch):
+        """See `IRangeFactory`."""
+        lower = self.getOrderValuesFor(self.plain_resultset[0])
+        upper = self.getOrderValuesFor(
+            self.plain_resultset[batch.trueSize - 1])
+        return (
+            simplejson.dumps(lower, cls=DateTimeJSONEncoder),
+            simplejson.dumps(upper, cls=DateTimeJSONEncoder),
+            )
+
+    def reportError(self, message):
+        if self.error_cb is not None:
+            self.error_cb(message)
+
+    def parseMemo(self, memo):
+        """Convert the given memo string into a sequence of Python objects.
+
+        memo should be a JSON string as returned by getEndpointMemos().
+
+        Note that memo originates from a URL query parameter and can thus
+        not be trusted to always contain formally valid and consistent
+        data.
+
+        Parsing errors or data not matching the sort parameters of the
+        result set are simply ignored.
+        """
+        if memo == '':
+            return None
+        try:
+            parsed_memo = simplejson.loads(memo)
+        except simplejson.JSONDecodeError:
+            self.reportError('memo is not a valid JSON string.')
+            return None
+        if not isinstance(parsed_memo, list):
+            self.reportError(
+                'memo must be the JSON representation of a list.')
+            return None
+
+        sort_expressions = self.getSortExpressions()
+        if len(sort_expressions) != len(parsed_memo):
+            self.reportError(
+                'Invalid number of elements in memo string. '
+                'Expected: %i, got: %i'
+                % (len(sort_expressions), len(parsed_memo)))
+            return None
+
+        converted_memo = []
+        for expression, value in zip(sort_expressions, parsed_memo):
+            if isinstance(expression, Desc):
+                expression = expression.expr
+            try:
+                expression.variable_factory(value=value)
+            except TypeError, error:
+                # A TypeError is raised when the type of value cannot
+                # be used for expression. All expected types are
+                # properly created by simplejson.loads() above, except
+                # time stamps which are represented as strings in
+                # ISO format. If value is a string and if it can be
+                # converted into a datetime object, we have a valid
+                # value.
+                if (str(error).startswith('Expected datetime') and
+                    isinstance(value, str)):
+                    try:
+                        value = datetime.strptime(
+                            value, '%Y-%m-%dT%H:%M:%S.%f')
+                    except ValueError:
+                        # One more attempt: If the fractions of a second
+                        # are zero, datetime.isoformat() omits the
+                        # entire part '.000000', so we need a different
+                        # format for strptime().
+                        try:
+                            value = datetime.strptime(
+                                value, '%Y-%m-%dT%H:%M:%S')
+                        except ValueError:
+                            self.reportError(
+                                'Invalid datetime value: %r' % value)
+                            return None
+                else:
+                    self.reportError(
+                        'Invalid parameter: %r' % value)
+                    return None
+            converted_memo.append(value)
+        return converted_memo
+
+    def reverseSortOrder(self):
+        """Return a list of reversed sort expressions."""
+        def invert_sort_expression(expr):
+            if isinstance(expression, Desc):
+                return expression.expr
+            else:
+                return Desc(expression)
+
+        return [
+            invert_sort_expression(expression)
+            for expression in self.getSortExpressions()]
+
+    def whereExpressionFromSortExpression(self, expression, memo):
+        """Create a Storm expression to be used in the WHERE clause of the
+        slice query.
+        """
+        if isinstance(expression, Desc):
+            expression = expression.expr
+            return expression < memo
+        else:
+            return expression > memo
+
+    def getSlice(self, size, endpoint_memo='', forwards=True):
+        """See `IRangeFactory`."""
+        if not forwards:
+            self.resultset.order_by(*self.reverseSortOrder())
+        parsed_memo = self.parseMemo(endpoint_memo)
+        if parsed_memo is None:
+            return self.resultset.config(limit=size)
+        else:
+            sort_expressions = self.getSortExpressions()
+            where = [
+                self.whereExpressionFromSortExpression(expression, memo)
+                for expression, memo in zip(sort_expressions, parsed_memo)]
+            # From storm.zope.interfaces.IResultSet.__doc__:
+            #     - C{find()}, C{group_by()} and C{having()} are really
+            #       used to configure result sets, so are mostly intended
+            #       for use on the model side.
+            naked_result = removeSecurityProxy(self.resultset).find(*where)
+            result = ProxyFactory(naked_result)
+            return result.config(limit=size)

=== modified file 'lib/canonical/launchpad/webapp/interfaces.py'
--- lib/canonical/launchpad/webapp/interfaces.py	2011-06-14 15:03:56 +0000
+++ lib/canonical/launchpad/webapp/interfaces.py	2011-07-28 11:31:31 +0000
@@ -878,3 +878,9 @@
 
         def __init__(self, request):
             self.request = request
+
+
+class StormRangeFactoryError(AssertionError):
+    """Raised when a Storm result set cannot be used for slicing by a
+    StormRangeFactory.
+    """

=== modified file 'lib/canonical/launchpad/webapp/tests/test_batching.py'
--- lib/canonical/launchpad/webapp/tests/test_batching.py	2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/webapp/tests/test_batching.py	2011-07-28 11:31:31 +0000
@@ -1,19 +1,430 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
 
-from doctest import (
-    DocTestSuite,
-    ELLIPSIS,
-    NORMALIZE_WHITESPACE,
-    )
+from datetime import datetime
+import simplejson
+from unittest import TestLoader
+
+from lazr.batchnavigator.interfaces import IRangeFactory
+from storm.expr import (
+    Desc,
+    Gt,
+    Lt,
+    )
+from storm.variables import IntVariable
+from zope.security.proxy import isinstance as zope_isinstance
+
+from canonical.launchpad.components.decoratedresultset import (
+    DecoratedResultSet,
+    )
+from canonical.launchpad.database.librarian import LibraryFileAlias
+from canonical.launchpad.webapp.batching import (
+    BatchNavigator,
+    DateTimeJSONEncoder,
+    StormRangeFactory,
+    )
+from canonical.launchpad.webapp.interfaces import StormRangeFactoryError
+from canonical.launchpad.webapp.servers import LaunchpadTestRequest
+from canonical.launchpad.webapp.testing import verifyObject
+from canonical.testing.layers import LaunchpadFunctionalLayer
+from lp.registry.model.person import Person
+from lp.testing import (
+    TestCaseWithFactory,
+    person_logged_in,
+    )
+
+
+class TestStormRangeFactory(TestCaseWithFactory):
+    """Tests for StormRangeFactory."""
+
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super(TestStormRangeFactory, self).setUp()
+        self.error_messages = []
+
+    def makeStormResultSet(self):
+        bug = self.factory.makeBug()
+        for count in range(5):
+            person = self.factory.makePerson()
+            with person_logged_in(person):
+                bug.markUserAffected(person, True)
+        return bug.users_affected
+
+    def makeDecoratedStormResultSet(self):
+        bug = self.factory.makeBug()
+        with person_logged_in(bug.owner):
+            for count in range(5):
+                self.factory.makeBugAttachment(bug=bug, owner=bug.owner)
+        result = bug.attachments
+        self.assertTrue(zope_isinstance(result, DecoratedResultSet))
+        return result
+
+    def test_StormRangeFactory_implements_IRangeFactory(self):
+        resultset = self.makeStormResultSet()
+        range_factory = StormRangeFactory(resultset)
+        self.assertTrue(verifyObject(IRangeFactory, range_factory))
+
+    def test_getOrderValuesFor__one_sort_column(self):
+        # StormRangeFactory.getOrderValuesFor() returns the values
+        # of the fields used in order_by expresssions for a given
+        # result row.
+        resultset = self.makeStormResultSet()
+        resultset.order_by(Person.id)
+        range_factory = StormRangeFactory(resultset)
+        order_values = range_factory.getOrderValuesFor(resultset[0])
+        self.assertEqual([resultset[0].id], order_values)
+
+    def test_getOrderValuesFor__two_sort_columns(self):
+        # Sorting by more than one column is supported.
+        resultset = self.makeStormResultSet()
+        resultset.order_by(Person.displayname, Person.name)
+        range_factory = StormRangeFactory(resultset)
+        order_values = range_factory.getOrderValuesFor(resultset[0])
+        self.assertEqual(
+            [resultset[0].displayname, resultset[0].name], order_values)
+
+    def test_getOrderValuesFor__string_as_sort_expression(self):
+        # Sorting by a string expression is not supported.
+        resultset = self.makeStormResultSet()
+        resultset.order_by('Person.id')
+        range_factory = StormRangeFactory(resultset)
+        exception = self.assertRaises(
+            StormRangeFactoryError, range_factory.getOrderValuesFor,
+            resultset[0])
+        self.assertEqual(
+            "StormRangeFactory supports only sorting by PropertyColumn, "
+            "not by 'Person.id'.",
+            str(exception))
+
+    def test_getOrderValuesFor__generic_storm_expression_as_sort_expr(self):
+        # Sorting by a generic Strom expression is not supported.
+        resultset = self.makeStormResultSet()
+        range_factory = StormRangeFactory(resultset)
+        exception = self.assertRaises(
+            StormRangeFactoryError, range_factory.getOrderValuesFor,
+            resultset[0])
+        self.assertTrue(
+            str(exception).startswith(
+                'StormRangeFactory supports only sorting by PropertyColumn, '
+                'not by <storm.expr.SQL object at'))
+
+    def test_getOrderValuesFor__unordered_result_set(self):
+        # If a result set is not ordered, it cannot be used with a
+        # StormRangeFactory.
+        resultset = self.makeStormResultSet()
+        resultset.order_by()
+        range_factory = StormRangeFactory(resultset)
+        exception = self.assertRaises(
+            StormRangeFactoryError, range_factory.getOrderValuesFor,
+            resultset[0])
+        self.assertEqual(
+            "StormRangeFactory requires a sorted result set.",
+            str(exception))
+
+    def test_getOrderValuesFor__decorated_result_set(self):
+        # getOrderValuesFor() knows how to retrieve SQL sort values
+        # from DecoratedResultSets.
+        resultset = self.makeDecoratedStormResultSet()
+        range_factory = StormRangeFactory(resultset)
+        self.assertEqual(
+            [resultset[0].id], range_factory.getOrderValuesFor(resultset[0]))
+
+    def test_getOrderValuesFor__value_from_second_element_of_result_row(self):
+        # getOrderValuesFor() can retrieve values from attributes
+        # of any Storm table class instance which appear in a result row.
+        resultset = self.makeDecoratedStormResultSet()
+        resultset = resultset.order_by(LibraryFileAlias.id)
+        plain_resultset = resultset.getPlainResultSet()
+        range_factory = StormRangeFactory(resultset)
+        self.assertEqual(
+            [plain_resultset[0][1].id],
+            range_factory.getOrderValuesFor(plain_resultset[0]))
+
+    def test_getOrderValuesFor__descending_sort_order(self):
+        # getOrderValuesFor() can retrieve values from reverse sorted
+        # columns.
+        resultset = self.makeStormResultSet()
+        resultset = resultset.order_by(Desc(Person.id))
+        range_factory = StormRangeFactory(resultset)
+        self.assertEqual(
+            [resultset[0].id], range_factory.getOrderValuesFor(resultset[0]))
+
+    def test_getOrderValuesFor__table_not_included_in_results(self):
+        # Attempts to use a sort by a column which does not appear in the
+        # data returned by the query raise a StormRangeFactoryError.
+        resultset = self.makeStormResultSet()
+        resultset.order_by(LibraryFileAlias.id)
+        range_factory = StormRangeFactory(resultset)
+        exception = self.assertRaises(
+            StormRangeFactoryError, range_factory.getOrderValuesFor,
+            resultset[0])
+        self.assertEqual(
+            "Instances of <class "
+            "'canonical.launchpad.database.librarian.LibraryFileAlias'> are "
+            "not contained in the result set, but are required to retrieve "
+            "the value of LibraryFileAlias.id.",
+            str(exception))
+
+    def test_DatetimeJSONEncoder(self):
+        # DateTimeJSONEncoder represents Pytjon datetime objects as strings
+        # where the value is represented in the ISO time format.
+        self.assertEqual(
+            '"2011-07-25T00:00:00"',
+            simplejson.dumps(datetime(2011, 7, 25), cls=DateTimeJSONEncoder))
+
+        # DateTimeJSONEncoder works for the regular Python types that can
+        # represented as JSON strings.
+        encoded = simplejson.dumps(
+            ('foo', 1, 2.0, [3, 4], {5: 'bar'}, datetime(2011, 7, 24)),
+            cls=DateTimeJSONEncoder)
+        self.assertEqual(
+            '["foo", 1, 2.0, [3, 4], {"5": "bar"}, "2011-07-24T00:00:00"]',
+            encoded
+            )
+
+    def test_getEndpointMemos(self):
+        # getEndpointMemos() returns JSON representations of the
+        # sort fields of the first and last element of a batch.
+        resultset = self.makeStormResultSet()
+        resultset.order_by(Person.name)
+        request = LaunchpadTestRequest()
+        batchnav = BatchNavigator(
+            resultset, request, size=3, range_factory=StormRangeFactory)
+        range_factory = StormRangeFactory(resultset)
+        first, last = range_factory.getEndpointMemos(batchnav.batch)
+        expected_first = simplejson.dumps(
+            [resultset[0].name], cls=DateTimeJSONEncoder)
+        expected_last = simplejson.dumps(
+            [resultset[2].name], cls=DateTimeJSONEncoder)
+        self.assertEqual(expected_first, first)
+        self.assertEqual(expected_last, last)
+
+    def test_getEndpointMemos__decorated_result_set(self):
+        # getEndpointMemos() works for DecoratedResultSet
+        # instances too.
+        resultset = self.makeDecoratedStormResultSet()
+        resultset.order_by(LibraryFileAlias.id)
+        request = LaunchpadTestRequest()
+        batchnav = BatchNavigator(
+            resultset, request, size=3, range_factory=StormRangeFactory)
+        range_factory = StormRangeFactory(resultset)
+        first, last = range_factory.getEndpointMemos(batchnav.batch)
+        expected_first = simplejson.dumps(
+            [resultset.getPlainResultSet()[0][1].id], cls=DateTimeJSONEncoder)
+        expected_last = simplejson.dumps(
+            [resultset.getPlainResultSet()[2][1].id],
+            cls=DateTimeJSONEncoder)
+        self.assertEqual(expected_first, first)
+        self.assertEqual(expected_last, last)
+
+    def logError(self, message):
+        # An error callback for StormResultSet.
+        self.error_messages.append(message)
+
+    def test_parseMemo__empty_value(self):
+        # parseMemo() returns None for an empty memo value.
+        resultset = self.makeStormResultSet()
+        range_factory = StormRangeFactory(resultset, self.logError)
+        self.assertIs(None, range_factory.parseMemo(''))
+        self.assertEqual(0, len(self.error_messages))
+
+    def test_parseMemo__json_error(self):
+        # parseMemo() returns None for formally invalid JSON strings.
+        resultset = self.makeStormResultSet()
+        range_factory = StormRangeFactory(resultset, self.logError)
+        self.assertIs(None, range_factory.parseMemo('foo'))
+        self.assertEqual(
+            ['memo is not a valid JSON string.'], self.error_messages)
+
+    def test_parseMemo__json_no_sequence(self):
+        # parseMemo() accepts only JSON representations of lists.
+        resultset = self.makeStormResultSet()
+        range_factory = StormRangeFactory(resultset, self.logError)
+        self.assertIs(None, range_factory.parseMemo(simplejson.dumps(1)))
+        self.assertEqual(
+            ['memo must be the JSON representation of a list.'],
+            self.error_messages)
+
+    def test_parseMemo__wrong_list_length(self):
+        # parseMemo() accepts only lists which have as many elements
+        # as the number of sort expressions used in the SQL query of
+        # the result set.
+        resultset = self.makeStormResultSet()
+        resultset.order_by(Person.name, Person.id)
+        range_factory = StormRangeFactory(resultset, self.logError)
+        self.assertIs(
+            None, range_factory.parseMemo(simplejson.dumps([1])))
+        expected_message = (
+            'Invalid number of elements in memo string. Expected: 2, got: 1')
+        self.assertEqual([expected_message], self.error_messages)
+
+    def test_parseMemo__memo_type_check(self):
+        # parseMemo() accepts only lists containing values that can
+        # be used in sort expression of the given result set.
+        resultset = self.makeStormResultSet()
+        resultset.order_by(Person.datecreated, Person.name, Person.id)
+        range_factory = StormRangeFactory(resultset, self.logError)
+        invalid_memo = [datetime(2011, 7, 25, 11, 30, 30, 45), 'foo', 'bar']
+        json_data = simplejson.dumps(invalid_memo, cls=DateTimeJSONEncoder)
+        self.assertIs(None, range_factory.parseMemo(json_data))
+        self.assertEqual(["Invalid parameter: 'bar'"], self.error_messages)
+
+    def test_parseMemo__valid_data(self):
+        # If a memo string contains valid data, parseMemo returns this data.
+        resultset = self.makeStormResultSet()
+        resultset.order_by(Person.datecreated, Person.name, Person.id)
+        range_factory = StormRangeFactory(resultset, self.logError)
+        valid_memo = [datetime(2011, 7, 25, 11, 30, 30, 45), 'foo', 1]
+        json_data = simplejson.dumps(valid_memo, cls=DateTimeJSONEncoder)
+        self.assertEqual(valid_memo, range_factory.parseMemo(json_data))
+        self.assertEqual(0, len(self.error_messages))
+
+    def test_parseMemo__short_iso_timestamp(self):
+        # An ISO timestamp without fractions of a second
+        # (YYYY-MM-DDThh:mm:ss) is a valid value for colums which
+        # store datetime values.
+        resultset = self.makeStormResultSet()
+        resultset.order_by(Person.datecreated)
+        range_factory = StormRangeFactory(resultset, self.logError)
+        valid_short_timestamp_json = '["2011-07-25T11:30:30"]'
+        self.assertEqual(
+            [datetime(2011, 7, 25, 11, 30, 30)],
+            range_factory.parseMemo(valid_short_timestamp_json))
+        self.assertEqual(0, len(self.error_messages))
+
+    def test_parseMemo__long_iso_timestamp(self):
+        # An ISO timestamp with fractions of a second
+        # (YYYY-MM-DDThh:mm:ss.ffffff) is a valid value for colums
+        # which store datetime values.
+        resultset = self.makeStormResultSet()
+        resultset.order_by(Person.datecreated)
+        range_factory = StormRangeFactory(resultset, self.logError)
+        valid_long_timestamp_json = '["2011-07-25T11:30:30.123456"]'
+        self.assertEqual(
+            [datetime(2011, 7, 25, 11, 30, 30, 123456)],
+            range_factory.parseMemo(valid_long_timestamp_json))
+        self.assertEqual(0, len(self.error_messages))
+
+    def test_parseMemo__invalid_iso_timestamp_value(self):
+        # An ISO timestamp with an invalid date is rejected as a memo
+        # string.
+        resultset = self.makeStormResultSet()
+        resultset.order_by(Person.datecreated)
+        range_factory = StormRangeFactory(resultset, self.logError)
+        invalid_timestamp_json = '["2011-05-35T11:30:30"]'
+        self.assertIs(
+            None, range_factory.parseMemo(invalid_timestamp_json))
+        self.assertEqual(
+            ["Invalid datetime value: '2011-05-35T11:30:30'"],
+            self.error_messages)
+
+    def test_parseMemo__nonsencial_iso_timestamp_value(self):
+        # A memo string is rejected when an ISO timespamp is expected
+        # but a nonsensical string is provided.
+        resultset = self.makeStormResultSet()
+        resultset.order_by(Person.datecreated)
+        range_factory = StormRangeFactory(resultset, self.logError)
+        nonsensical_timestamp_json = '["bar"]'
+        self.assertIs(
+            None, range_factory.parseMemo(nonsensical_timestamp_json))
+        self.assertEqual(
+            ["Invalid datetime value: 'bar'"],
+            self.error_messages)
+
+    def test_parseMemo__descending_sort_order(self):
+        # Validation of a memo string against a descending sort order works.
+        resultset = self.makeStormResultSet()
+        resultset.order_by(Desc(Person.id))
+        range_factory = StormRangeFactory(resultset, self.logError)
+        self.assertEqual(
+            [1], range_factory.parseMemo(simplejson.dumps([1])))
+
+    def test_reverseSortOrder(self):
+        # reverseSortOrder() wraps a plain PropertyColumn instance into
+        # Desc(), and it returns the plain PropertyCOlumn for a Desc()
+        # expression.
+        resultset = self.makeStormResultSet()
+        resultset.order_by(Person.id, Desc(Person.name))
+        range_factory = StormRangeFactory(resultset, self.logError)
+        reverse_person_id, person_name = range_factory.reverseSortOrder()
+        self.assertTrue(isinstance(reverse_person_id, Desc))
+        self.assertIs(Person.id, reverse_person_id.expr)
+        self.assertIs(Person.name, person_name)
+
+    def test_whereExpressionFromSortExpression__asc(self):
+        """For ascending sort order, whereExpressionFromSortExpression()
+        returns the WHERE clause expression > memo.
+        """
+        resultset = self.makeStormResultSet()
+        range_factory = StormRangeFactory(resultset, self.logError)
+        where_clause = range_factory.whereExpressionFromSortExpression(
+            expression=Person.id, memo=1)
+        self.assertTrue(isinstance(where_clause, Gt))
+        self.assertIs(where_clause.expr1, Person.id)
+        self.assertTrue(where_clause.expr2, IntVariable)
+
+    def test_whereExpressionFromSortExpression_desc(self):
+        """For descending sort order, whereExpressionFromSortExpression()
+        returns the WHERE clause expression < memo.
+        """
+        resultset = self.makeStormResultSet()
+        range_factory = StormRangeFactory(resultset, self.logError)
+        where_clause = range_factory.whereExpressionFromSortExpression(
+            expression=Desc(Person.id), memo=1)
+        self.assertTrue(isinstance(where_clause, Lt))
+        self.assertIs(where_clause.expr1, Person.id)
+        self.assertTrue(where_clause.expr2, IntVariable)
+
+    def test_getSlice__forward_without_memo(self):
+        resultset = self.makeStormResultSet()
+        resultset.order_by(Person.name, Person.id)
+        all_results = list(resultset)
+        range_factory = StormRangeFactory(resultset)
+        sliced_result = range_factory.getSlice(3)
+        self.assertEqual(all_results[:3], list(sliced_result))
+
+    def test_getSlice__forward_with_memo(self):
+        resultset = self.makeStormResultSet()
+        resultset.order_by(Person.name, Person.id)
+        all_results = list(resultset)
+        memo = simplejson.dumps([all_results[0].name, all_results[0].id])
+        range_factory = StormRangeFactory(resultset)
+        sliced_result = range_factory.getSlice(3, memo)
+        self.assertEqual(all_results[1:4], list(sliced_result))
+
+    def test_getSlice__backward_without_memo(self):
+        resultset = self.makeStormResultSet()
+        resultset.order_by(Person.name, Person.id)
+        all_results = list(resultset)
+        expected = all_results[-3:]
+        expected.reverse()
+        range_factory = StormRangeFactory(resultset)
+        sliced_result = range_factory.getSlice(3, forwards=False)
+        self.assertEqual(expected, list(sliced_result))
+
+    def test_getSlice_backward_with_memo(self):
+        resultset = self.makeStormResultSet()
+        resultset.order_by(Person.name, Person.id)
+        all_results = list(resultset)
+        expected = all_results[1:4]
+        expected.reverse()
+        memo = simplejson.dumps([all_results[4].name, all_results[4].id])
+        range_factory = StormRangeFactory(resultset)
+        sliced_result = range_factory.getSlice(3, memo, forwards=False)
+        self.assertEqual(expected, list(sliced_result))
+
+    def test_getSlice__decorated_resultset(self):
+        resultset = self.makeDecoratedStormResultSet()
+        resultset.order_by(LibraryFileAlias.id)
+        all_results = list(resultset)
+        memo = simplejson.dumps([resultset.getPlainResultSet()[0][1].id])
+        range_factory = StormRangeFactory(resultset)
+        sliced_result = range_factory.getSlice(3, memo)
+        self.assertEqual(all_results[1:4], list(sliced_result))
 
 
 def test_suite():
-    suite = DocTestSuite(
-            'canonical.launchpad.webapp.batching',
-            optionflags=NORMALIZE_WHITESPACE | ELLIPSIS
-            )
-    return suite
-
+    return TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/canonical/launchpad/zcml/decoratedresultset.zcml'
--- lib/canonical/launchpad/zcml/decoratedresultset.zcml	2010-08-19 19:52:31 +0000
+++ lib/canonical/launchpad/zcml/decoratedresultset.zcml	2011-07-28 11:31:31 +0000
@@ -10,7 +10,7 @@
 
     <class class="canonical.launchpad.components.decoratedresultset.DecoratedResultSet">
         <allow interface="storm.zope.interfaces.IResultSet" />
-        <allow attributes="__getslice__" />
+        <allow attributes="__getslice__ getPlainResultSet" />
     </class>
 
 </configure>