[storm] Releasing 0.20 - take two
Markus Kemmerling
markus.kemmerling at meduniwien.ac.at
Fri Jun 28 09:31:08 UTC 2013
Hi Free,
Am 28.06.2013 um 11:16 schrieb Free Ekanayaka <free.ekanayaka at gmail.com>:
> [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,
No, its only a quick workaround.
> 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?
Ah, I should have checked earlier, sorry! The bug has already been reported:
https://bugs.launchpad.net/storm/+bug/1170063
Thanks agin,
Markus
> 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
>>>>>
>>>>
>>>
>>
>
More information about the storm
mailing list