[storm] Releasing 0.20 - take two

Fri Jun 28 09:16:37 UTC 2013

[Sorry, I forgot to keep the list in CC, adding it now. Previous replies below]

On Fri, Jun 28, 2013 at 10:47 AM, Markus Kemmerling
<markus.kemmerling at meduniwien.ac.at> wrote:
> Hi Free,
>
> Thanks a lot!
>
> FYI: I just stumbled over another problem with storm and PostgreSQL after upgrading psycopg2 to 2.5 :-(
>
> psycopg2 as of version 2.5 doesn't seem to allow to set storm.exceptions.Error as base class of psycopg2.Error any more:
>
> Traceback (most recent call last):
> [...]
>  File "/Users/markus/zope/packages/eggs-cache/storm-0.19-py2.6-macosx-10.8-x86_64.egg/storm/databases/postgres.py", line 54, in <module>
>     install_exceptions(psycopg2)
>   File "/Users/markus/zope/packages/eggs-cache/storm-0.19-py2.6-macosx-10.8-x86_64.egg/storm/exceptions.py", line 145, in install_exceptions
>     module_exception.__bases__ += (exception,)
> TypeError: can't set attributes of built-in/extension type 'psycopg2.Error'
>
> Since storm.exceptions.Error seems to be catched by storm.database.Connection.rollback() only I worked around this problem by modifying the except clause in rollback() to simply catch a general Exception instead of storm.exceptions.Error (similar to commits) and not setting the latter as a base class of psycopg2.Error in storm.exceptions.install_exceptions. But I didn't run the storm tests for PostgreSQL after that change.
>
Unfortunately this doesn't seem like an acceptable general solution,
since there is user code out there actually relying on those
exceptions. This needs a broader fix, but I'd postpone it after the
0.20 release. Please would you file a bug for this issue?

Thanks,

Free

> Am 28.06.2013 um 10:13 schrieb Free Ekanayaka <free.ekanayaka at gmail.com>:
>
>> Good catch Markus,
>>
>> I applied your one-liner plus test.
>>
>> Thanks,
>>
>> Free
>>
>> On Thu, Jun 27, 2013 at 4:46 PM, Markus Kemmerling
>> <markus.kemmerling at meduniwien.ac.at> wrote:
>>> 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
>>>>
>>>
>>
>