Hi Free, Am 28.06.2013 um 11:16 schrieb Free Ekanayaka <[email protected]>:
> [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 > <[email protected]> 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 <[email protected]>: >> >>> Good catch Markus, >>> >>> I applied your one-liner plus test. >>> >>> Thanks, >>> >>> Free >>> >>> On Thu, Jun 27, 2013 at 4:46 PM, Markus Kemmerling >>> <[email protected]> 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 <[email protected]>: >>>> >>>>> 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 >>>>> [email protected] >>>>> Modify settings or unsubscribe at: >>>>> https://lists.ubuntu.com/mailman/listinfo/storm >>>>> >>>> >>> >> > -- storm mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/storm
