"Phillip J. Eby" wrote:
At 08:58 PM 6/19/00 +0800, mike wrote:
http://www.zope.org/Members/RainDog/ZSession/ZSession-0.0.2.tar.gz/view
Comments?
Now that I've had a chance to really look at this (while tracking down one of the bugs you found), I do have a few comments.
First, nice job... It's a good adaptation use of ZPatterns. I do have
Thanks.
some suggestions for fine tuning, though.
* Rather than putting property manipulation functions right into the base Session class, I would simply let users derive ZClasses of their own from Session. They could then actually give the Session a UI of its own, for
I put those get/set/has stuff just for make easier life of some lazy people (like me :-)
example. Think of someone creating a Session subclass called "Shopping Cart", with methods for viewing, checking out, adding/deleting items, etc. Or, if they have many subsystems which want to share the same Session objects, they could do this by having each subsystem use a different propertysheet of the same Session object.
Well, well, well. As for me, beter I've set up a Client abstraction which have access to CartManager and store cart_id in the session object. Mixing Session and Cart is not a good idea just because Client can have several carts, for example. The composition is better than inheritance in this case, Phillip :-)
IMHO, the basicSheet stuff you've done, while convenient, encourages developers to take the quick-and-dirty route of just using the Session as a place to store variables - and that throws away a lot of the usefulness of ZPatterns. For one thing, you've guaranteed that Sessions have to be stored as persistent objects in the ZODB, or at least that the basicSheet has to support arbitrary properties.
But without this stuff people will have to code ZClasses each time they want sessions. Hmm... I guess the best way is splitting current session class into two - simple Session and LazySession.
* Make dead session removal something that doesn't happen automatically, or at least have an option to do so. Your default deletion algorithm would be very expensive when executed against a non-ZODB database, and would be more efficiently done a few times a day in large batches by a cron job.
Moreover, I found a bug which causes infinite loop :-( You're right, automatic removal should be optional. Will be fixed in 0.0.3 release.
* Delegate determining which sessions are dead to the Rack, rather than doing it in the Specialist. This allows an SQL implementation to make use of indexes. Specifically, make removeDead call:
self._getSource().findIdleSessions(self.session_ttl)
And then provide a default implementation of findIdleSessions() in the Specialist. (Btw, please note that your current implementation absolutely requires that at least *some* session data be stored persistently in the ZODB, which if you're using a FileStorage, is a bad place for it to be.) Now, the whole thing works the same way when first installed, but if someone implements the session ID and lastAccessed attributes in an SQL database, they can substitute a different implementation for findIdleSessions() by putting it in the Rack.
Agree with you completely. Thanks, Mike