WebHelpers conflict #63 and #59
Mike Orr
On Sat, Mar 19, 2011 at 9:36 PM, Bitbucket <issues-...@bitbucket.org> wrote:
> New issue 63: Regression: paginate loads whole table when given SqlAlchemy object
> https://bitbucket.org/bbangert/webhelpers/issue/63/regression-paginate-loads-whole-table-when
>
> nh2 on Sun, 20 Mar 2011 05:36:05 +0100:
>
> Description:
> The changeset <<changeset b82b9f9942bb>> replaces
> {{{
> list(obj[slice])
> }}}
> by
> {{{
> list(obj)[slice]
> }}}
>
> which results in **all** rows being loaded on e.g. Page(Session.query(MyModel),filter(...)).
>
> Therefore, webhelpers 1.3b1 completely kills performance.
>
> Responsible:
> bbangert
This was done in order to solve this big report:
> #59, by dennysonique:
> While using TurboGears 2 which relies on Webhelpers (sprox forms) I
> found this bug:
> Line 434 has an error causing:
> Module webhelpers.paginate:434 in init view
> self.items = list(self.collection[self.first_item-1:self.last_item])
> TypeError: unhashable type
> The solution to fix this is to change:
> -- self.items = list(self.collection[self.first_item-1:self.last_item])
> ++ self.items = list(self.collection)[self.first_item-1:self.last_item]
In other words, different users have contradictory desires. Should I
revert #59 and change it to WONTFIX, or is there a way to reconcile
these two? Or is this a reason to overhaul how paginate handles lists
vs queries overall? If so, what would be a better algorithm?
--
Mike Orr <slugg...@gmail.com>
danj...@gmail.com
Check if items is a sqlalchemy query would be an immediate fix.
This can seriously kill a web server so paginate should be considered unusable with sqlalchemy until this is resolved.
--
You received this message because you are subscribed to the Google Groups "pylons-devel" group.
To post to this group, send email to pylons...@googlegroups.com.
To unsubscribe from this group, send email to pylons-devel...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/pylons-devel?hl=en.
Wichert Akkerman
Yaiks - that change will effectively kill any site which does pagination
over a large SQL table, so reverting it would be a good start. I don't
see why the original line would raise that TypeError though. Did the bug
submitter contribute a test to demonstrate that? It looks like he is
strangely behaving collection type.
Wichert.
Mike Orr
>>> New issue 63: Regression: paginate loads whole table when given
>>> SqlAlchemy object
>>>
> Yaiks - that change will effectively kill any site which does pagination
> over a large SQL table, so reverting it would be a good start. I don't see
> why the original line would raise that TypeError though. Did the bug
> submitter contribute a test to demonstrate that? It looks like he is
> strangely behaving collection type.
I reverted the patch in 556b55e75dd6, and implemented Marcin
Kuzminski's slice test posted to the bug report. Can somebody try
checking out WebHelpers dev and make sure it works in your
application, especially if you have some large tables to try it
against? I'll release 1.3b2 in a day or two after people have had time
to test it.
(https://bitbucket.org/bbangert/webhelpers/issue/59/webhelperspaginate-error-line-434)
Sprox objects appear to be unsliceable and therefore incompatible with
paginate. I also discovered that SQLAlchemy queries and selects also
don't have .__getslice__, but that's OK because their wrapper class
emulates it. So i had to move the slice check below the SQLAlchemy
check.
I wrote some unittests for various collection types, and discovered
there seems to be a bug with SQLAlchemy select objects.
https://bitbucket.org/bbangert/webhelpers/issue/64/paginate-sqlalchemy-select-test-fails
The wrapper's .__len__ returns the query's rowcount directly, which is
-1, which makes Python choke because len() isn't supposed to return a
negative number. The test is commented out but is
tests/test_paginate.py#TestSQLAlchemyCollectionTypes.test_sqlalchemy_select
. Christoph, can you try the test and see if the wrapper is buggy or
the test is wrong? I don't know how long it's been this way; maybe
nobody uses paginate with select.
https://bitbucket.org/bbangert/webhelpers/issue/64/paginate-sqlalchemy-select-test-fails
In the long term I think paginate should be refactored to eliminate
the pseudo-list layer. It makes it hard to tell what the backend is
doing, and how Python's special methods are interacting with it, and
how the collection's special methods may be complicating it further.
I'd like to replace the list-like wrappers with straightforward helper
classes that do their business directly and return a Page or the data
to construct a Page. The Page could remain a list subclass but it
would be fully static, not collection-aware. Furthermore I'd like
users to be able to provide their own helper classes for third-party
collection types. There have been patches to add PyMongo, CouchDB, and
now Sprox to paginate, but we can't add everything. Better to go with
autonomous helper classes. But this is a longer-term project, it's not
something I can do right now. Christoph, what do you think?
https://bitbucket.org/bbangert/webhelpers/issue/65/refactor-paginates-pseudo-lists
--
Mike Orr <slugg...@gmail.com>
Marius Gedminas
> Sprox objects appear to be unsliceable and therefore incompatible with
> paginate. I also discovered that SQLAlchemy queries and selects also
> don't have .__getslice__, but that's OK because their wrapper class
> emulates it. So i had to move the slice check below the SQLAlchemy
> check.
I'll just add a note that defining __getslice__ is not necessary to support
slicing (in fact it's been deprecated since Python 2.0). The modern way
is to have your __getitem__ check if the argument passed to it is an
instance of the builtin `slice` type.
(You probably knew that already, but your wording above is a bit ambiguous.)
Incidentally, what's that '__slice__' thing you're checking for in
https://bitbucket.org/bbangert/webhelpers/changeset/556b55e75dd6#chg_webhelpers/paginate.py_newline215
? docs.python.org has no knowledge of it.
Marius Gedminas
--
It is a mess, pure and simple. Sucks to be away from Unix, huh?
-- man perlfaq3
Mike Orr
> I'll just add a note that defining __getslice__ is not necessary to support
> slicing (in fact it's been deprecated since Python 2.0). The modern way
> is to have your __getitem__ check if the argument passed to it is an
> instance of the builtin `slice` type.
>
> (You probably knew that already, but your wording above is a bit ambiguous.)
No, I didn't know that. I've never dealt with slices, just individual items.
Actually, I don't know if I can check for that. I can check for the
presence of a method, but not its semantics. Unless I call the method
and see what it does with a slice, but that would be silly and
potentially disruptive.
> Incidentally, what's that '__slice__' thing you're checking for in
> https://bitbucket.org/bbangert/webhelpers/changeset/556b55e75dd6#chg_webhelpers/paginate.py_newline215
> ? docs.python.org has no knowledge of it.
Oh, that should be .__getslice__ . Maybe that's causing some of the
SQLAlchemy complications (but not all of them).
--
Mike Orr <slugg...@gmail.com>
Mike Orr
SQLAlchemy and sliceable cases. I think it will let some non-sliceable
cases through which would cause the original Sprox error, but i don't
think there's anything we can do about that because we can't check
whether .__getitem__ accepts a slice without calling it and guessing
what the exception means, and calling it may cause side effects (like
a db query).
So if y'all can make sure you're satisfied with it, I'll release beta
2 in a day or two.
--
Mike Orr <slugg...@gmail.com>
Mike Orr
was common to the Sprox failure and other unsliceable failures
("TypeError: unhashable type"). So I can at least raise the
incompatible message so users will know what's going on.
--
Mike Orr <slugg...@gmail.com>