Skip to content
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

Extend provided SQLx SessionStore implementations to allow for more configurability #1

Open
Hellrespawn opened this issue Oct 6, 2023 · 7 comments
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@Hellrespawn
Copy link

Right now the schema_name for the SQLX stores is hardcoded to be "tower_sessions", ignoring the database specified in the database URL.

My current Docker setup creates a user with access to only one database. It would be useful to to be able to have it use that specific database.

@Hellrespawn Hellrespawn changed the title Support custom schema names? Support custom schema names Oct 6, 2023
@maxcountryman
Copy link
Owner

Thanks for mentioning this.

I would be happy to accept changes to better enable configurability around this.

@maxcountryman maxcountryman added good first issue Good for newcomers enhancement New feature or request help wanted Extra attention is needed labels Oct 6, 2023
@avdb13
Copy link

avdb13 commented Oct 6, 2023

Would with_table_name and with_schema_name suffice or do we want this to be part of the Store::new(pool) signature? Latter is a breaking change but we're still early in development. I vouch for the former.

@maxcountryman
Copy link
Owner

I'm fine with either approach: it's okay to make breaking changes for a better API. 😁

@Hellrespawn
Copy link
Author

Hellrespawn commented Oct 6, 2023

You arguably don't need a schema_name at all. By omitting it you would rely on SQLx determining it from the database URL or returning an error. That's the approach async-sqlx-session takes.

I quickly ripped out schema_name on a local copy of the crate, and store.migrate().await.unwrap() Just Works™ if you specify a database in the database url or gives the following error if you don't:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: 
Database(MySqlDatabaseError { code: Some("3D000"), number: 1046, 
message: "No database selected" })', application/src/cli/serve.rs:86:27

@maxcountryman
Copy link
Owner

I quickly ripped out schema_name on a local copy of the crate, and store.migrate().await.unwrap() Just Works™ if you specify a database in the database url or gives the following error if you don't

You don't need to copy the crate to do this, you can instead implement SessionStore: this is a key aspect of this crate's design. In fact, we fully intend for folks to implement their own stores and expect that our provided implementations will only be used as a convenience.

@maxcountryman maxcountryman changed the title Support custom schema names Extend provided SQLx implementations to allow for more configurability Nov 30, 2023
@maxcountryman maxcountryman changed the title Extend provided SQLx implementations to allow for more configurability Extend provided SQLx SessionStore implementations to allow for more configurability Nov 30, 2023
@maxcountryman
Copy link
Owner

@czocher I think this also relates your recent PR addressing the Postgres store.

@czocher
Copy link

czocher commented Nov 30, 2023

@maxcountryman it does indeed relate. We can possibly remove the schema setting completely and (as @Hellrespawn mentioned) rely on sqlx to select the schema name with the &options=-c search_path=someschemahere URL option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants