launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04654
[Merge] lp:~adeuring/launchpad/bug-739052-6 into lp:launchpad
Abel Deuring has proposed merging lp:~adeuring/launchpad/bug-739052-6 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~adeuring/launchpad/bug-739052-6/+merge/71912
[sorry, I use this cover letter for two merge proposals: one for
a Launchpad branch, the other for a lazr.bachnavigator branch.
The changes in both branches rely heavily on each other -- I find
it hard to describe them separately, and they make sense only
together, I think.)
A test of the more or less new class StormRangeFactory with a real
Launchpad view revealed a quite stupid but serious bug in
StormRangeFactory.getEndpointMemos(): This method always returned
the memo values for the second batch...
The reason is simple: It retrieved the memo data from
self.plain_resultset, while it is supposed to look into the
parameter batch. self.plain_resultset is a Storm ResultSet
for the entire query, while batch contains, well, the current
batch of the result set.
The fix for this a bit lenghty, it involves changes to the main
Launchpad branch as well as changes to lazr.batchnavigator: If
we have a DecoratedResultSet, the slice returned by
StormRangeFactory.getSlice() did not contain the data needed by
getEndpointMemos(): the raw, undecorated result rows.
The solutions I proposed in an email to LP development mailing
list were all useless:
lazr.batchnavigator.z3batching.batch._Batch.sliced_list does
some additional slicing for backwards slicing:
sliced = sliced[1:] + sliced[:1]
(line 178), so caching the "raw slice" in StormRangeFactory
would have to track this "shuffling" somehow. Another approach,
returning the memo values in getSlice() would be a bit cumbersome:
We would have to return the memo values the first two and the last
two values (or only the last one?? -- look at -Batch.sliced-list
to find the right answer...); and _Batch.sliced_list would have
become a bit more complicated.
I finally implemented a class ShadowedList, which stores two
sequences as attributes. One of is a slice of the decorated
result set, the other is the corresponding slice of the raw result
set.
This class implements only that part of the sequence protocol
that is needed in the class _Batch. The main idea is that
the slicing related operations (__getslice__(), __add__()) of
this class work both attributes and return new instances of
ShadowedList. This ensures that, when the StormRangeFactory is
used for batching, _Batch.sliced_list always returns ShdowedLists,
which in turn allows getEndpointMemos(batch) to access the raw
Storm result rows from lised_list.shadow_values. Other methods,
like __getitem__, return simply the "main data".
So far, this involves only the Launchpad branch, but
_Batch.sliced_list treats batches which do not have a memo
value in a special way (we don't have the memo value for the
first batch, i.e., for URL like https://launchpad.dev/ubuntu/+builds):
if self.range_memo is None:
# Legacyy mode - use the slice protocol on results.
sliced = list(self.list[self.start:self.end+1])
(self.list is a Storm ResultSet or a DeoratedResultSet)
So we lose again the raw result rows...
I fixed this by adding the method
IBatchNavigator.getSliceByIndex(start, end).
tests:
./bin/test canonical -vvt canonical.launchpad.webapp.tests.test_batching
no lint
--
https://code.launchpad.net/~adeuring/launchpad/bug-739052-6/+merge/71912
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~adeuring/launchpad/bug-739052-6 into lp:launchpad.
=== modified file 'lib/canonical/launchpad/webapp/batching.py'
--- lib/canonical/launchpad/webapp/batching.py 2011-08-15 12:33:21 +0000
+++ lib/canonical/launchpad/webapp/batching.py 2011-08-17 15:46:25 +0000
@@ -11,7 +11,7 @@
)
import lazr.batchnavigator
from lazr.batchnavigator.interfaces import IRangeFactory
-import pytz
+from operator import isSequenceType
import simplejson
from storm import Undef
from storm.expr import (
@@ -166,6 +166,68 @@
return simplejson.JSONEncoder.default(self, obj)
+class ShadowedList:
+ """A (partial) sequence impementation which maintains two
+ sequences: A publicly visible one and a "shadow" sequence.
+
+ Background: StormRangeFactory.getSlice() returns a sequence
+ of records which is used in lazr.batchnavigator.Batchnavigator
+ and in lazr.batchnavigator.z3batching.Batch.
+
+ This slice is passed back by Batch.nextBatch() and Batch.prevBatch()
+ to StormRangeFactory.getEndpointMemos().
+
+ StormRangeFactory can work with DecoratedResultSets, which means
+ that the data required to create the memo values may only be
+ available in the plain, undecorated, result set.
+
+ This class allows to maintain the values needed by BachNavigator
+ and Batch as well as the values needed by
+ StormRangeFactory.getEndpointMemos().
+
+ It implements only those parts of the sequence protocol needed by
+ BatchNavigator and Batch.
+ """
+ def __init__(self, values, shadow_values):
+ if not isSequenceType(values) or not isSequenceType(shadow_values):
+ raise TypeError("values and shadow_values must be sequences.")
+ if len(values) != len(shadow_values):
+ raise ValueError(
+ "values and shadow_values must have the same length.")
+ self.values = values
+ self.shadow_values = shadow_values
+
+ def __len__(self):
+ """See `list`."""
+ return len(self.values)
+
+ def __getslice__(self, start, end):
+ """See `list`."""
+ return ShadowedList(
+ self.values[start:end], self.shadow_values[start:end])
+
+ def __getitem__(self, index):
+ """See `list`."""
+ return self.values[index]
+
+ def __add__(self, other):
+ """See `list`."""
+ if not isinstance(other, ShadowedList):
+ raise TypeError(
+ 'You can only add another ShadowedList to a ShadowedList')
+ return ShadowedList(
+ self.values + other.values,
+ self.shadow_values + other.shadow_values)
+
+ def __iter__(self):
+ """See `list`."""
+ return iter(self.values)
+
+ def reverse(self):
+ self.values.reverse()
+ self.shadow_values.reverse()
+
+
class StormRangeFactory:
"""A range factory for Storm result sets.
@@ -247,9 +309,9 @@
def getEndpointMemos(self, batch):
"""See `IRangeFactory`."""
- lower = self.getOrderValuesFor(self.plain_resultset[0])
- upper = self.getOrderValuesFor(
- self.plain_resultset[batch.trueSize - 1])
+ plain_slice = batch.sliced_list.shadow_values
+ lower = self.getOrderValuesFor(plain_slice[0])
+ upper = self.getOrderValuesFor(plain_slice[batch.trueSize - 1])
return (
simplejson.dumps(lower, cls=DateTimeJSONEncoder),
simplejson.dumps(upper, cls=DateTimeJSONEncoder),
@@ -475,6 +537,17 @@
# Note that lazr.batchnavigator calls len(slice), so we can't
# return the plain result set.
if parsed_memo is None:
- return list(self.resultset.config(limit=size))
- else:
- return list(self.getSliceFromMemo(size, parsed_memo))
+ result = self.resultset.config(limit=size)
+ else:
+ result = self.getSliceFromMemo(size, parsed_memo)
+ real_result = list(result)
+ if zope_isinstance(result, DecoratedResultSet):
+ shadow_result = list(result.get_plain_result_set())
+ else:
+ shadow_result = real_result
+ return ShadowedList(real_result, shadow_result)
+
+ def getSliceByIndex(self, start, end):
+ """See `IRangeFactory."""
+ sliced = self.resultset[start:end]
+ return ShadowedList(list(sliced), list(sliced.get_plain_result_set()))
=== modified file 'lib/canonical/launchpad/webapp/tests/test_batching.py'
--- lib/canonical/launchpad/webapp/tests/test_batching.py 2011-08-15 13:11:05 +0000
+++ lib/canonical/launchpad/webapp/tests/test_batching.py 2011-08-17 15:46:25 +0000
@@ -21,6 +21,7 @@
from canonical.launchpad.webapp.batching import (
BatchNavigator,
DateTimeJSONEncoder,
+ ShadowedList,
StormRangeFactory,
)
from canonical.launchpad.webapp.interfaces import StormRangeFactoryError
@@ -188,15 +189,17 @@
# sort fields of the first and last element of a batch.
resultset = self.makeStormResultSet()
resultset.order_by(Person.name)
- request = LaunchpadTestRequest()
+ range_factory = StormRangeFactory(resultset)
+ memo_value = range_factory.getOrderValuesFor(resultset[0])
+ request = LaunchpadTestRequest(
+ QUERY_STRING='memo=%s' % simplejson.dumps(memo_value))
batchnav = BatchNavigator(
- resultset, request, size=3, range_factory=StormRangeFactory)
- range_factory = StormRangeFactory(resultset)
+ resultset, request, size=3, range_factory=range_factory)
first, last = range_factory.getEndpointMemos(batchnav.batch)
expected_first = simplejson.dumps(
- [resultset[0].name], cls=DateTimeJSONEncoder)
+ [resultset[1].name], cls=DateTimeJSONEncoder)
expected_last = simplejson.dumps(
- [resultset[2].name], cls=DateTimeJSONEncoder)
+ [resultset[3].name], cls=DateTimeJSONEncoder)
self.assertEqual(expected_first, first)
self.assertEqual(expected_last, last)
@@ -205,10 +208,10 @@
# instances too.
resultset = self.makeDecoratedStormResultSet()
resultset.order_by(LibraryFileAlias.id)
+ range_factory = StormRangeFactory(resultset)
request = LaunchpadTestRequest()
batchnav = BatchNavigator(
- resultset, request, size=3, range_factory=StormRangeFactory)
- range_factory = StormRangeFactory(resultset)
+ resultset, request, size=3, range_factory=range_factory)
first, last = range_factory.getEndpointMemos(batchnav.batch)
expected_first = simplejson.dumps(
[resultset.get_plain_result_set()[0][1].id],
@@ -518,7 +521,7 @@
all_results = list(resultset)
range_factory = StormRangeFactory(resultset)
sliced_result = range_factory.getSlice(3)
- self.assertEqual(all_results[:3], sliced_result)
+ self.assertEqual(all_results[:3], list(sliced_result))
def test_getSlice__forward_with_memo(self):
resultset = self.makeStormResultSet()
@@ -527,7 +530,7 @@
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], sliced_result)
+ self.assertEqual(all_results[1:4], list(sliced_result))
def test_getSlice__backward_without_memo(self):
resultset = self.makeStormResultSet()
@@ -537,7 +540,7 @@
expected.reverse()
range_factory = StormRangeFactory(resultset)
sliced_result = range_factory.getSlice(3, forwards=False)
- self.assertEqual(expected, sliced_result)
+ self.assertEqual(expected, list(sliced_result))
def test_getSlice_backward_with_memo(self):
resultset = self.makeStormResultSet()
@@ -548,7 +551,7 @@
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, sliced_result)
+ self.assertEqual(expected, list(sliced_result))
def makeResultSetWithPartiallyIdenticalSortData(self):
# Create a result set, where each value of
@@ -585,25 +588,102 @@
[memo_lfa.mimetype, memo_lfa.filename, memo_lfa.id])
range_factory = StormRangeFactory(resultset)
sliced_result = range_factory.getSlice(3, memo)
- self.assertEqual(all_results[3:6], sliced_result)
+ self.assertEqual(all_results[3:6], list(sliced_result))
def test_getSlice__decorated_resultset(self):
resultset = self.makeDecoratedStormResultSet()
resultset.order_by(LibraryFileAlias.id)
all_results = list(resultset)
+ plain_results = list(resultset.get_plain_result_set())
memo = simplejson.dumps([resultset.get_plain_result_set()[0][1].id])
range_factory = StormRangeFactory(resultset)
sliced_result = range_factory.getSlice(3, memo)
- self.assertEqual(all_results[1:4], sliced_result)
+ self.assertEqual(all_results[1:4], list(sliced_result))
+ self.assertEqual(plain_results[1:4], sliced_result.shadow_values)
- def test_getSlice__returns_list(self):
+ def test_getSlice__returns_ShadowedList(self):
# getSlice() returns lists.
resultset = self.makeStormResultSet()
resultset.order_by(Person.id)
- all_results = list(resultset)
range_factory = StormRangeFactory(resultset)
sliced_result = range_factory.getSlice(3)
- self.assertIsInstance(sliced_result, list)
- memo = simplejson.dumps([all_results[0].name])
- sliced_result = range_factory.getSlice(3, memo)
- self.assertIsInstance(sliced_result, list)
+ self.assertIsInstance(sliced_result, ShadowedList)
+
+ def makeStringSequence(self, sequence):
+ return [str(elem) for elem in sequence]
+
+ def test_ShadowedList__init(self):
+ # ShadowedList instances need two lists as constructor parametrs.
+ list1 = range(3)
+ list2 = self.makeStringSequence(list1)
+ shadowed_list = ShadowedList(list1, list2)
+ self.assertEqual(shadowed_list.values, list1)
+ self.assertEqual(shadowed_list.shadow_values, list2)
+
+ def test_ShadowedList__init__non_sequence_parameter(self):
+ # values and shadow_values must be sequences.
+ self.assertRaises(TypeError, ShadowedList, 1, range(3))
+ self.assertRaises(TypeError, ShadowedList, range(3), 1)
+
+ def test_ShadowedList__init__different_list_lengths(self):
+ # values and shadow_values must have the same length.
+ self.assertRaises(ValueError, ShadowedList, range(2), range(3))
+
+ def test_ShadowedList__len(self):
+ # The length of a ShadowedList ist the same as the list of
+ # the sequences it stores.
+ list1 = range(3)
+ list2 = self.makeStringSequence(list1)
+ self.assertEqual(len(list1), len(ShadowedList(list1, list2)))
+
+ def test_ShadowedList__slice(self):
+ # A slice of a ShadowedList contains the slices of its
+ # values and shaow_values.
+ list1 = range(5)
+ list2 = self.makeStringSequence(list1)
+ shadowed_list = ShadowedList(list1, list2)
+ self.assertEqual(list1[2:4], shadowed_list[2:4].values)
+ self.assertEqual(list2[2:4], shadowed_list[2:4].shadow_values)
+
+ def test_ShadowedList__getitem(self):
+ # Accessing a single element of a ShadowedList is equivalent to
+ # accessig an element of its values attribute.
+ list1 = range(3)
+ list2 = self.makeStringSequence(list1)
+ shadowed_list = ShadowedList(list1, list2)
+ self.assertEqual(list1[1], shadowed_list[1])
+
+ def test_ShadowedList__add(self):
+ # Two shadowedLists can be added, yielding another ShadowedList.
+ list1 = range(3)
+ list2 = self.makeStringSequence(list1)
+ shadow_list1 = ShadowedList(list1, list2)
+ list3 = range(4)
+ list4 = self.makeStringSequence(list3)
+ shadow_list2 = ShadowedList(list3, list4)
+ list_sum = shadow_list1 + shadow_list2
+ self.assertIsInstance(list_sum, ShadowedList)
+ self.assertEqual(list1 + list3, list_sum.values)
+ self.assertEqual(list2 + list4, list_sum.shadow_values)
+
+ def test_ShadowedList__iterator(self):
+ # Iterating over a ShadowedList yields if values elements.
+ list1 = range(3)
+ list2 = self.makeStringSequence(list1)
+ shadow_list = ShadowedList(list1, list2)
+ self.assertEqual(list1, list(iter(shadow_list)))
+
+ def test_ShadowedList__reverse(self):
+ # ShadowList.reverse() reverses its elements.
+ list1 = range(3)
+ list2 = self.makeStringSequence(list1)
+ first1 = list1[0]
+ last1 = list1[-1]
+ first2 = list2[0]
+ last2 = list2[-1]
+ shadow_list = ShadowedList(list1, list2)
+ shadow_list.reverse()
+ self.assertEqual(first1, shadow_list[-1])
+ self.assertEqual(last1, shadow_list[0])
+ self.assertEqual(first2, shadow_list.shadow_values[-1])
+ self.assertEqual(last2, shadow_list.shadow_values[0])
=== modified file 'versions.cfg'
--- versions.cfg 2011-08-17 08:09:43 +0000
+++ versions.cfg 2011-08-17 15:46:25 +0000
@@ -31,7 +31,7 @@
launchpadlib = 1.9.9
lazr.amqp = 0.1
lazr.authentication = 0.1.1
-lazr.batchnavigator = 1.2.6
+lazr.batchnavigator = 1.2.7
lazr.config = 1.1.3
lazr.delegates = 1.2.0
lazr.enum = 1.1.3
