New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PostgresqlSession is incomplete, broken, and too specific #748

Closed
bb-migration opened this Issue Oct 30, 2007 · 5 comments

Comments

Projects
None yet
2 participants
@bb-migration

bb-migration commented Oct 30, 2007

Originally reported by: Robert Brewer (Bitbucket: fumanchu, GitHub: fumanchu)


The current !PostgresqlSession class:

  1. Is incomplete without a get_db function. At the least, it should try to import pypgsql and psycopg.
  2. Is broken, since it doesn't even INSERT new rows.
  3. Is too specific. It should be transformed into a generic DBAPISession class (which would make it incomplete again, but why limit ourselves to a single provider for such a simple need?).

@bb-migration

This comment has been minimized.

Show comment
Hide comment
@bb-migration

bb-migration Mar 20, 2008

Original comment by guest (Bitbucket: guest, GitHub: guest):


  1. There should be a way to specify the postgresql schema. Current implementation assumes that table will be created in default public schema.[[BR]]
  2. I think we should create a generic DBSession class and then derive PGSession class from DBSession class. This will allow us to plugin other databases in future. Why limit to postgresql for db session storage ?[[BR]]
  3. DBSession class (if created) should be able to use ORMs (which people use even to our dismay).[[BR]]
    [[BR]]
    ---[[BR]]
    Vivek Khurana

bb-migration commented Mar 20, 2008

Original comment by guest (Bitbucket: guest, GitHub: guest):


  1. There should be a way to specify the postgresql schema. Current implementation assumes that table will be created in default public schema.[[BR]]
  2. I think we should create a generic DBSession class and then derive PGSession class from DBSession class. This will allow us to plugin other databases in future. Why limit to postgresql for db session storage ?[[BR]]
  3. DBSession class (if created) should be able to use ORMs (which people use even to our dismay).[[BR]]
    [[BR]]
    ---[[BR]]
    Vivek Khurana
@bb-migration

This comment has been minimized.

Show comment
Hide comment
@bb-migration

bb-migration Jul 9, 2008

Original comment by Nick Kamper (Bitbucket: nick125, GitHub: nick125):


I'm not particularly sure what a DBSession class would provide, especially if you are talking about allowing ORMs.

As for the entire get_db() function, I'm not sure how else you could implement it, as I think adding database initialization code would be too complex and inflexible. Maybe you could allow the database object/cursor to be passed to the initializer, although I'm not too sure on the implementation specifics.

I think that PostgresqlSession should either be scrapped or replaced with a much more generic implementation, like fumanchu said in the initial report. It would be hard to do a generic implementation, especially with varying schemas. We could allow the users to specify the query strings and pass an already initialized database cursor, which would solve some of the issues.

bb-migration commented Jul 9, 2008

Original comment by Nick Kamper (Bitbucket: nick125, GitHub: nick125):


I'm not particularly sure what a DBSession class would provide, especially if you are talking about allowing ORMs.

As for the entire get_db() function, I'm not sure how else you could implement it, as I think adding database initialization code would be too complex and inflexible. Maybe you could allow the database object/cursor to be passed to the initializer, although I'm not too sure on the implementation specifics.

I think that PostgresqlSession should either be scrapped or replaced with a much more generic implementation, like fumanchu said in the initial report. It would be hard to do a generic implementation, especially with varying schemas. We could allow the users to specify the query strings and pass an already initialized database cursor, which would solve some of the issues.

@bb-migration

This comment has been minimized.

Show comment
Hide comment
@bb-migration

bb-migration Sep 19, 2013

Original comment by Sylvain Hellegouarch (Bitbucket: Lawouach, GitHub: Lawouach):


I believe the PostgreSQL backend session ought to be dropped but mostly because the whole backend design is contrived and complicated anyway.

bb-migration commented Sep 19, 2013

Original comment by Sylvain Hellegouarch (Bitbucket: Lawouach, GitHub: Lawouach):


I believe the PostgreSQL backend session ought to be dropped but mostly because the whole backend design is contrived and complicated anyway.

@bb-migration

This comment has been minimized.

Show comment
Hide comment
@bb-migration

bb-migration Dec 31, 2013

Original comment by CleanCut (Bitbucket: CleanCut, GitHub: CleanCut):


Wow, this sure has languished. The docs led me to believe this was supported, but when I tried it out it crashed and burned.

  1. The first two lines in init() must be reversed, since Session.init() calls _exists() which requires the cursor to be already present:
#!python

    def __init__(self, id=None, **kwargs):
        self.cursor = self.db.cursor()
        Session.__init__(self, id, **kwargs)
  1. The last line in setup() has to be fixed. It's trying to access "self" in a class method instead of "cls", which is another instant crash.
#!python

        cls.db = cls.get_db()
  1. As was mentioned in 2007, even when you fix the obvious crashing problems, the object never inserts a session entry into the database -- so sessions are never, ever saved. :-(

  2. It was very not obvious when, where and/or how to add the get_db() function to the class. The following example could help:

#!python

# The following code could be added near the top of your main cherrypy application.

# Give each thread its own database connection
def db_connect(thread_index):
    # Create a connection to your database for your thread named "db"
    # db = ... (right here) ...
    # Then make it available to this thread
    cherrypy.thread_data.db = db

# Tell cherrypy to actually call db_connect() for each thread
cherrypy.engine.subscribe('start_thread', db_connect)

# Make it so that the PostgresqlSession handler can find the db connection
def get_db():
    return cherrypy.thread_data.db
cherrypy.lib.sessions.PostgresqlSession.get_db = get_db

bb-migration commented Dec 31, 2013

Original comment by CleanCut (Bitbucket: CleanCut, GitHub: CleanCut):


Wow, this sure has languished. The docs led me to believe this was supported, but when I tried it out it crashed and burned.

  1. The first two lines in init() must be reversed, since Session.init() calls _exists() which requires the cursor to be already present:
#!python

    def __init__(self, id=None, **kwargs):
        self.cursor = self.db.cursor()
        Session.__init__(self, id, **kwargs)
  1. The last line in setup() has to be fixed. It's trying to access "self" in a class method instead of "cls", which is another instant crash.
#!python

        cls.db = cls.get_db()
  1. As was mentioned in 2007, even when you fix the obvious crashing problems, the object never inserts a session entry into the database -- so sessions are never, ever saved. :-(

  2. It was very not obvious when, where and/or how to add the get_db() function to the class. The following example could help:

#!python

# The following code could be added near the top of your main cherrypy application.

# Give each thread its own database connection
def db_connect(thread_index):
    # Create a connection to your database for your thread named "db"
    # db = ... (right here) ...
    # Then make it available to this thread
    cherrypy.thread_data.db = db

# Tell cherrypy to actually call db_connect() for each thread
cherrypy.engine.subscribe('start_thread', db_connect)

# Make it so that the PostgresqlSession handler can find the db connection
def get_db():
    return cherrypy.thread_data.db
cherrypy.lib.sessions.PostgresqlSession.get_db = get_db

@jaraco jaraco removed the 3.3 label May 1, 2016

@jaraco jaraco closed this in defa24d May 1, 2016

@jaraco

This comment has been minimized.

Show comment
Hide comment
@jaraco

jaraco May 1, 2016

Member

I've stripped out the code, though it's always available in the parent of the aforementioned commit for posterity and in case anyone wants to revive the implementation.

For what it's worth, jaraco.mongodb provides a sessions implementation in MongoDB, and third-party packages may be a good model for implementing functionality in other backends.

Member

jaraco commented May 1, 2016

I've stripped out the code, though it's always available in the parent of the aforementioned commit for posterity and in case anyone wants to revive the implementation.

For what it's worth, jaraco.mongodb provides a sessions implementation in MongoDB, and third-party packages may be a good model for implementing functionality in other backends.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment