session: Add Sqlite session backend #298
Conversation
|
I fixed the doctest errors in sqlite.rs (SqliteSessionStore), but I still have these errors locally and I'm not sure what am I missing: Any suggestions please ? |
| [dev-dependencies] | ||
| actix-session = { path = ".", features = ["cookie-session", "redis-actor-session", "redis-rs-session"] } | ||
| actix-session = { path = ".", features = ["cookie-session", "sqlite-session"] } |
There was a problem hiding this comment.
| actix-session = { path = ".", features = ["cookie-session", "sqlite-session"] } | |
| actix-session = { path = ".", features = ["cookie-session", "redis-actor-session", "sqlite-session"] } |
We should keep enabling all features for the test suite, that's where your error is coming from.
| required-features = ["redis-actor-session"] | ||
| required-features = ["sqlite-session"] |
There was a problem hiding this comment.
You should add a new example for sqlite instead of modifying the existing one for Redis.
| /// executes before every insert on the `sessions` table. | ||
| pub fn new( | ||
| pool: Pool<SqliteConnectionManager>, | ||
| garbage_collect: bool, |
There was a problem hiding this comment.
It's almost never a good idea to use a boolean in a public API - let's add an enum with two variants using self-describing names.
| conn.execute( | ||
| "CREATE TABLE IF NOT EXISTS sessions ( | ||
| id INTEGER NOT NULL, | ||
| session_key TEXT NOT NULL, | ||
| session_state TEXT NOT NULL, | ||
| expiry INTEGER NOT NULL, | ||
| PRIMARY KEY(id AUTOINCREMENT) | ||
| )", | ||
| [], | ||
| ) | ||
| .map_err(Into::into) | ||
| .map_err(LoadError::Other)?; | ||
|
|
||
| // in order to garbage collect stale sessions, we will use a trigger | ||
| if garbage_collect { | ||
| conn.execute( | ||
| " | ||
| CREATE TRIGGER IF NOT EXISTS garbage_collect | ||
| BEFORE INSERT | ||
| ON sessions | ||
| BEGIN | ||
| DELETE FROM sessions WHERE expiry < STRFTIME('%s'); | ||
| END; | ||
| ", | ||
| [], | ||
| ) | ||
| .map_err(Into::into) | ||
| .map_err(LoadError::Other)?; | ||
| } else { | ||
| conn.execute("DROP TRIGGER IF EXISTS garbage_collect", []) | ||
| .map_err(Into::into) | ||
| .map_err(LoadError::Other)?; | ||
| } |
There was a problem hiding this comment.
This is a one-off setup action that should not be executed every single time somebody tries to use Sqlite as a session backend in their applications.
It should live in a separate routine, new should assume that the initialization has already taken place.
| async fn load(&self, session_key: &SessionKey) -> Result<Option<SessionState>, LoadError> { | ||
| let cache_key = (self.configuration.cache_keygen)(session_key.as_ref()); | ||
|
|
||
| let conn = self.pool.get().unwrap(); |
There was a problem hiding this comment.
You don't want to unwrap here.
| session_key TEXT NOT NULL, | ||
| session_state TEXT NOT NULL, | ||
| expiry INTEGER NOT NULL, | ||
| PRIMARY KEY(id AUTOINCREMENT) |
There was a problem hiding this comment.
This allows for multiple rows sharing the same session_key - that's not what we want. I'd suggest using the session_key as primary key or, at the very least, add a UNIQUE constraint on it.
| .unwrap() | ||
| .unix_timestamp(); | ||
|
|
||
| let conn = self.pool.get().unwrap(); |
There was a problem hiding this comment.
We do not want to unwrap here.
| let mut stmt = match statement { | ||
| Ok(v) => v, | ||
| Err(e) => return Err(SaveError::Other(anyhow::anyhow!(e))), | ||
| }; |
There was a problem hiding this comment.
| let mut stmt = match statement { | |
| Ok(v) => v, | |
| Err(e) => return Err(SaveError::Other(anyhow::anyhow!(e))), | |
| }; | |
| let mut stmt = statement.map_err(Into::into).map_err(SaveError::Other)?; |
| async fn delete(&self, session_key: &SessionKey) -> Result<(), anyhow::Error> { | ||
| let cache_key = (self.configuration.cache_keygen)(session_key.as_ref()); | ||
|
|
||
| let conn = self.pool.get().unwrap(); |
There was a problem hiding this comment.
We do not want to unwrap here.
|
Left a bunch of comments - sorry for the long delay! Another issue that needs to be addressed: |
|
@bbogdan95 , will you be able to complete this? |
robjtede
changed the title
robjtede
changed the title
PR Type
Feature
PR Checklist
cargo +nightly fmt).Overview
Implemented Sqlite session backend. The database/table will get created automatically if they don't exist.
Default garbage collection of stale sessions happens through a database trigger. (it can be turned off and the trigger will be dropped, and garbage collection can be implemented outside if needed.)