[storm] Releasing 0.20 - take two

[Markus Kemmerling]

Hi Free,

thanks a lot for your effort!

Back in January you came up with a fix for Bug #620369, "RuntimeError when using Desc in ReferenceSet order_by", open since storm 0.17, see:

https://bugs.launchpad.net/storm/+bug/620369

https://lists.ubuntu.com/archives/storm/2013-January/001466.html

This fix works great for me, but I think there is another small issue related to this bug caused by 'resolving' the 'order_by' expression since storm 0.17.

The problem emerges if you query an ordered reference set and try to retrieve the last item of the returned result set. This fails since the 'find()' method of a reference set passes the '_order_by' expression as a single argument to the 'order_by()' method of the result set. The 'ResultSet.last()' method then checks if the '_order_by' expression is a 'Desc' or 'Asc' expression, but does not recognize a tuple. Since it does not recognize the tuple, it wraps it in another 'Desc' expression and you end up with a SQL statement including 'DESC DESC' (or 'DESC ASC').

Complicated error description, but I think the fix is easy: The '_order_by' expression should probably be passed as an argument list to the 'find()' method of a reference set now that it is a tuple.

I included this additional change in the change set of your fix for bug #620369 (http://bazaar.launchpad.net/~storm/storm/trunk/revision/455):

$ diff -Naur storm/references.py.orig storm/references.py
--- storm/references.py.orig	2011-09-25 20:45:14.000000000 +0200
+++ storm/references.py	2013-06-27 14:07:23.000000000 +0200
@@ -25,7 +25,7 @@
 from storm.store import Store, get_where_for_args, LostObjectError
 from storm.variables import LazyValue
 from storm.expr import (
-    Select, Column, Exists, ComparableExpr, LeftJoin, Not, SQLRaw,
+    Select, Column, Exists, ComparableExpr, SuffixExpr, LeftJoin, Not, SQLRaw,
     compare_columns, compile)
 from storm.info import get_cls_info, get_obj_info

@@ -281,7 +281,7 @@
         where = self._get_where_clause()
         result = store.find(self._target_cls, where, *args, **kwargs)
         if self._order_by is not None:
-            result.order_by(self._order_by)
+            result.order_by(*self._order_by)
         return result

     def __iter__(self):
@@ -912,6 +912,12 @@
             return self.resolve(property)
         elif isinstance(property, basestring):
             return self._resolve_string(property)
+        elif isinstance(property, SuffixExpr):
+            # XXX This covers cases like order_by=Desc("Bar.id"), see #620369.
+            # Eventually we might want to add support for other types of
+            # expressions
+            property.expr = self.resolve(property.expr)
+            return property
         elif not isinstance(property, Column):
             return _find_descriptor_obj(self._used_cls, property)
         return property

and also added two lines to your new 'test_reference_set_order_by_desc_id' test:

$ diff -Naur tests/store/base.py.orig tests/store/base.py
--- tests/store/base.py.orig	2011-09-25 20:45:14.000000000 +0200
+++ tests/store/base.py	2013-06-27 14:09:02.000000000 +0200
@@ -4055,10 +4055,23 @@
         foo = self.store.get(FooRefSetOrderID, 20)

         values = list(foo.bars.values(Bar.id, Bar.foo_id, Bar.title))
-        self.assertEquals(values, [
-                          (200, 20, "Title 200"),
-                          (400, 20, "Title 100"),
-                         ])
+        self.assertEquals(values,
+                          [(200, 20, "Title 200"), (400, 20, "Title 100")])
+
+    def test_reference_set_order_by_desc_id(self):
+        self.add_reference_set_bar_400()
+
+        class FooRefSetOrderByDescID(Foo):
+            bars = ReferenceSet(Foo.id, Bar.foo_id, order_by=Desc(Bar.id))
+
+        foo = self.store.get(FooRefSetOrderByDescID, 20)
+
+        values = list(foo.bars.values(Bar.id, Bar.foo_id, Bar.title))
+        self.assertEquals(values,
+                          [(400, 20, "Title 100"), (200, 20, "Title 200")])
+
+        self.assertEquals(foo.bars.first().id, 400)
+        self.assertEquals(foo.bars.last().id, 200)

     def test_indirect_reference_set(self):
         foo = self.store.get(FooIndRefSet, 20)
@@ -4068,10 +4081,7 @@
             items.append((bar.id, bar.title))
         items.sort()

-        self.assertEquals(items, [
-                          (100, "Title 300"),
-                          (200, "Title 200"),
-                         ])
+        self.assertEquals(items, [(100, "Title 300"), (200, "Title 200")])

     def test_indirect_reference_set_with_added(self):
         bar1 = Bar()

Sorry that I didn't diff the trunk, but simply downloaded storm 0.19 to test this. I hope I don't come up too late with this issue.

Best regards,
Markus Kemmerling

Am 26.06.2013 um 14:45 schrieb Free Ekanayaka <free.ekanayaka at gmail.com>:

> Hi folks,
> 
> I didn't really go on with the 0.20 release, see:
> 
> https://lists.ubuntu.com/archives/storm/2013-January/001464.html
> 
> But I'd like to it now. Please step up if you think there's something we
> should block on, otherwise I'll prepare the tarball in the next few days.
> 
> Cheers,
> 
> Free
> 
> -- 
> storm mailing list
> storm at lists.canonical.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/storm
>