search

pallets-eco / flask-session

Server side session extension for Flask
https://flask-session.readthedocs.io
BSD 3-Clause "New" or "Revised" License
488 stars 236 forks source link

Allow for table creation to be optional to allow for table/schema management by other tooling #239

Open jlgoolsbee opened 2 months ago

jlgoolsbee commented 2 months ago

In previous versions of Flask-Session, table creation was intentionally removed to allow other tooling (e.g., Flask-Migrate) to manage table creation and schema updates, among other reasons.

It seems that in the shuffle of merging in changes from Flask-Session2, automatic table creation was re-added to Flask-Session, thus breaking application init and migrations for folks who manage their own tables and schemas.

This PR simply gates that behavior behind a configuration key to allow users to choose the behavior they prefer; the config key defaults to True to reflect the current state.

Lxstr commented 2 months ago

Thanks for the PR! @MauriceBrg has a similar PR for dynamo in #237. I'm thinking let's align the terminology with ...TABLE_EXISTS? And a similar addition to the docstring maybe

jlgoolsbee commented 2 months ago

@MauriceBrg Happy to align on terminology; I think I've folded in the requested updates? Let me know if there's anything else.

MauriceBrg commented 2 months ago

Looks good to me in terms of naming, the DynamoDB version is SESSION_DYNAMODB_TABLE_EXISTS, so they appear to align.

For DynamoDB I added a bit more information about what the table needs to look like if you want to use an existing one. I'm not too familiar with SQL Alchemy so I don't know if it's worth adding something like that. https://github.com/pallets-eco/flask-session/blob/bc2fe67958bff5e46023c4807b5e75ca350554eb/src/flask_session/dynamodb/dynamodb.py#L39

jlgoolsbee commented 2 months ago

@MauriceBrg lol; just realized I tagged you in my response that was meant for @Lxstr 😅

I'm all for better docs, but I'm not sure it's necessary to document the model schema in the docstring since the model definition exists just a few lines north of the interface class itself (create_session_model method), so I've added a blurb about the table management options available which point folks to that method and calls out the need to align any manually-created table with the SqlAlchemy-specific configuration options available.

@Lxstr I think this is ready to go?