[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, 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 <[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
