lazr.restful and batching cause a FeatureError

Bug #627442 reported by Curtis Hovey on 2010-08-31
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Launchpad itself
High
Gary Poster

Bug Description

accessing https://api.edge.launchpad.net/beta/%7Elocoteams/members appears to cause OOPS-1703EB1759. The team code is several years old so it appears lazr.restful, batching, or storm changed.

 FeatureError: __contains__() does not yet support set expressions that combine ORDER BY with LIMIT/OFFSET

Module zope.publisher.publish, line 109, in mapply
    return debug_call(obj, args)
   - __traceback_info__: <security proxied lazr.restful._resource.CollectionResource instance at 0x2af4892a6050>
  Module zope.publisher.publish, line 115, in debug_call
    return obj(*args)
  Module lazr.restful._resource, line 871, in __call__
    result = self.do_GET()
  Module lazr.restful._resource, line 1696, in do_GET
    result = self.batch(entries)
  Module lazr.restful._resource, line 1711, in batch
    result = super(CollectionResource, self).batch(entries, request)
  Module lazr.restful._resource, line 649, in batch
    total_size = navigator.batch.total()
  Module lazr.batchnavigator.z3batching.batch, line 212, in total
    return self.listlength
  Module lazr.batchnavigator.z3batching.batch, line 52, in listlength
    self._listlength = self._get_length(self.list)
  Module lazr.batchnavigator.z3batching.batch, line 47, in _get_length
    return len(results)
  Module canonical.launchpad.webapp.batching, line 53, in __len__
    return self.context.count()
  Module storm.sqlobject, line 570, in count
    return result_set.count()
  Module storm.store, line 1241, in count
    return int(self._aggregate(lambda expr: Count(expr, distinct), expr))
  Module storm.store, line 1228, in _aggregate
    subquery = replace_columns(self._get_select(), columns)
  Module storm.store, line 1771, in replace_columns
    "__contains__() does not yet support set "
FeatureError: __contains__() does not yet support set expressions that combine ORDER BY with LIMIT/OFFSET

Related branches

Gary Poster (gary) on 2010-08-31
Changed in launchpad-foundations:
status: New → Triaged
importance: Undecided → High
Gary Poster (gary) wrote :

Leonard reports that problem is known and should be addressed already. If it is happening, the test suite should catch it. The underlying storm issue is bug 620508.

Gary Poster (gary) wrote :

To be clear, I duped the problem; I'm not questioning whether it currently occurs. It does.

Gary Poster (gary) wrote :

Leonard adds: look at r11452, which includes a patch that's supposed to fix the problem.

http://bazaar.launchpad.net/~launchpad-pqm/launchpad/devel/revision/11452

The difference between the OOPS that Leonard quotes and this OOPS is that the current OOPS is for the sqlobject compatibility layer. I suspect that the sqlobject code path needs a slightly different approach.

Gary Poster (gary) wrote :

The sqlobject code that failed was in this method:

    def count(self):
        result_set = self._without_prejoins()._result_set
        return result_set.count()

The fix for non-sqlobject storm was in lib/canonical/launchpad/webapp/batching.py :

 49 resultset = removeSecurityProxy(self.context)
 50 if hasattr(resultset, '_select'):
 51 resultset._select.limit = Undef
 52 resultset._select.offset = Undef
 53 return self.context.count()

Therefore, naively, assuming the _select hack was the best we could do, until the Storm bug is fixed I think we need to sniff for sqlobject. Something on the order of the following might work.

# XXX The following code hacks around Storm bug 620508.
resultset = removeSecurityProxy(self.context)
if isinstance(resultset, storm.sqlobject.SQLObjectResultSet):
    # It is particularly annoying to handle the Storm bug here
    # because it requires fixing up a wrapped object.
    if zope.proxy.queryProxy(self.context, zope.security.proxy.Proxy):
        # This is a security-proxied object.
        # We need to manually check whether it is OK to access count,
        # because we are going to bypass the whole shebang.
        checker = zope.security.checker.getChecker(self.context)
        # This will raise the appropriate error if the checker would not
        # have allowed access.
        checker.checker.check_getattr(self.context, 'count')
    context = resultset._without_prejoins() # This is not proxied.
    resultset = context
else:
    context = self.context # This is proxied.
if getattr(resultset, '_select', None) is None: # hasattr swallows exceptions.
    resultset._select.limit = Undef
    resultset._select.offset = Undef
return context.count()

Now the questions are, (a) does anyone agree with me, and (b) how do we test this?

Leonard Richardson (leonardr) wrote :

I think it would be easier to fix Storm. The fix is pretty obvious once you know the problem. The difficult part is writing a good test without having much knowledge of Storm.

Gary Poster (gary) wrote :

I have asked on the bug and on IRC for assistance with the Storm bug. If we don't get it soon, say within 24 hours, we should proceed on the work-around hack.

I'll also ask Stuart if he is comfortable making this Storm fix.

Robert Collins (lifeless) wrote :

therve's storm/resultset-select-copy looks like it may have a fix in it. I'm crashing v soon, but I'd look there, and chat with niemeyer/jkakar in #storm.

Gary Poster (gary) on 2010-09-06
Changed in launchpad-foundations:
assignee: nobody → Gary Poster (gary)
status: Triaged → In Progress
Gary Poster (gary) on 2010-09-08
Changed in launchpad-foundations:
status: In Progress → Fix Committed
milestone: none → 10.09
tags: added: qa-ok
tags: added: qa-needstesting
removed: qa-ok
Ursula Junque (ursinha) on 2010-09-08
tags: added: qa-ok
removed: qa-needstesting
Curtis Hovey (sinzui) on 2010-09-09
Changed in launchpad-foundations:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers