Skip to content
New issue

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

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

Already on GitHub? # to your account

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

Open
wants to merge 6 commits into
base: development
Choose a base branch
from

Conversation

jlgoolsbee
Copy link

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.

@jlgoolsbee jlgoolsbee force-pushed the optional-table-creation branch from e03e557 to e77975e Compare April 16, 2024 16:41
@jlgoolsbee jlgoolsbee force-pushed the optional-table-creation branch from b087de8 to 2d9ebc2 Compare April 17, 2024 16:40
@Lxstr
Copy link
Contributor

Lxstr commented Apr 18, 2024

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 jlgoolsbee force-pushed the optional-table-creation branch from 2d9ebc2 to beb9f34 Compare April 24, 2024 22:30
@jlgoolsbee
Copy link
Author

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

@MauriceBrg
Copy link
Contributor

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.

If you set ``table_exists`` to True, you're responsible for creating a table with this config:

@jlgoolsbee
Copy link
Author

@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?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants