Add pavex_session_redis lib for Redis-based session management

[joehasson]

Hey, I'm working on #466 following the patterns used in the postgres-based session store. I have a working implementation and there's still some work to do polish this (mainly tests + documentation) but I'd like to get some pointers around architectural patterns beforehand, any guidance is much appreciated.

1. Exposing redis-rs as a dependency: this new lib defines a RedisSessionStore constructor with a new method accepting a redis::aio::ConnectionManager. This is analogous to the situation with the PostgresSessionStore constructor in the postgres implementation which accepts a sqlx::PgPool. This means that the user of these libraries is forced to construct these types and hence to depend directly on redis-rs / sqlx. I'm wondering whether it would be better, at least in the redis case, if the dependency was instead abstracted away? We could instead just require the user to supply connection strings rather than pre-constructed clients.
2. Injecting configuration: it would be nice to let users configure how redis keys are generated, like actix-session does , so that they can implement namespacing and use existing redis stores to store their session data. It's not clear to me how to do that with the current approach, however, since dependency injection happens at compile time.

One way to address both of these points might be to have the RedisSessionStore::new function instead accept a struct of the following type, which we would require the user to instantiate:

pub struct RedisSessionStoreConfig {
    pub connection_string: String,
    pub key_generator: Option<Box<dyn Fn(&str) -> String>>,
}

Does this seem reasonable? (If we definitely want to expose redis-rs as a dependency this struct could store a ConnectionManager instead of a connection string).

3. Testing: as mentioned in Sqlite session store #497, if you have any particular thoughts on how to test this stuff I'm keen to hear them, otherwise I am happy to crack on.

Many thanks for any input on the above questions and any other comments on the PR.

[LukeMathWalker]

First of all, thanks for picking this up!

Hey, I'm working on #466 following the patterns used in the postgres-based session store. I have a working implementation and there's still some work to do polish this (mainly tests + documentation) but I'd like to get some pointers around architectural patterns beforehand, any guidance is much appreciated.

1. Exposing redis-rs as a dependency: this new lib defines a RedisSessionStore constructor with a new method accepting a redis::aio::ConnectionManager. This is analogous to the situation with the PostgresSessionStore constructor in the postgres implementation which accepts a sqlx::PgPool. This means that the user of these libraries is forced to construct these types and hence to depend directly on redis-rs / sqlx. I'm wondering whether it would be better, at least in the redis case, if the dependency was instead abstracted away? We could instead just require the user to supply connection strings rather than pre-constructed clients.

I intend to abstract the underlying library by giving users pre-packaged constructors that go from a configuration type (e.g. PostgresPoolConfig) to the library-specific type (i.e. sqlx::PgPool). This will happen in a dedicated pavex_sqlx/pavex_redis crate.
Taking a raw connection string in the session crate would lock users into having a dedicated pool for sessions, which may or may not be what they want.
For the sake of sessions, it's OK to assume that you'll get a ConnectionManager directly.

2. Injecting configuration: it would be nice to let users configure how redis keys are generated, like actix-session does , so that they can implement namespacing and use existing redis stores to store their session data. It's not clear to me how to do that with the current approach, however, since dependency injection happens at compile time.

One way to address both of these points might be to have the RedisSessionStore::new function instead accept a struct of the following type, which we would require the user to instantiate:

pub struct RedisSessionStoreConfig {
    pub connection_string: String,
    pub key_generator: Option<Box<dyn Fn(&str) -> String>>,
}

A dedicated configuration type is ideal, but it shouldn't include the connection configuration. As discussed above, the connection config will live elsewhere.
We should design the configuration type to be deserialization-friendly—e.g. imagine that it'll come from a YAML file through a serde operation. We should also have a Default implementation for it.
In turn, this forces us to decide if a fully-general key_generator is needed or a simpler key_prefix: Option<String> is enough.

3. Testing: as mentioned in Sqlite session store #497, if you have any particular thoughts on how to test this stuff I'm keen to hear them, otherwise I am happy to crack on.

Let's add unit tests. In particular, we should cover:

• TTL expiry
• Round-trip tests ensuring that we can read back what we wrote, including updates.

[LukeMathWalker]

I would prefer to avoid this helper for now, you can get the raw byte representation via .inner().

[LukeMathWalker]

Let's convert it into a workspace dependency, for easier version management.

[LukeMathWalker]

We shouldn't panic.

[LukeMathWalker]

This may leak the stored data, which might contain sensitive information.
We should limit ourselves to stating which value type we got, rather than logging the value itself.

[LukeMathWalker]

Same as below, regarding privacy.

[joehasson]

Hi @LukeMathWalker, thanks very much for the feedback above. I believe these commits address everything we've discussed. Worth calling out on this point regarding the dedicated configuration type

In turn, this forces us to decide if a fully-general key_generator is needed or a simpler key_prefix: Option is enough.

Given that the only use case I had in mind here was namespacing, a fully general key_generator seems on reflection like overkill and an unnecessary footgun, so I went ahead and implemented the Option<String> alternative.

For the tests, I have mostly copy-pasted from @oliverbarnes' work on #497 since the relevant tests carry over with the appropriate changes to the create_test_store helper. It strikes me that it would be helpful to create a single test suite which was generic over the SessionStorageBackend trait as more backends are implemented, to prevent this kind of duplication.

Happy to squash these commits if you'd prefer a cleaner history when its time to merge.

[oliverbarnes]

Hey @joehasson, just a heads-up that I've made some changes to the tests today, based on Luca's feedback. Namely I've moved them to a separate file, and cleaned up a couple of superfluous assertions.

I'm curious about your idea to abstract the session storage test suite, specially given I'm now implementing a very similar suite for MySQL on #499.

...though I have to say, coming from Rails/Rspec where sometimes people went a little overboard with test abstractions, that these days I have a lot more tolerance for test duplication =D

(tangentially: "footgun" is a great word, thanks - first time I see it)

[LukeMathWalker]

For the tests, I have mostly copy-pasted from @oliverbarnes' work on #497 since the relevant tests carry over with the appropriate changes to the create_test_store helper. It strikes me that it would be helpful to create a single test suite which was generic over the SessionStorageBackend trait as more backends are implemented, to prevent this kind of duplication.

Generics unfortunately won't be enough, since the default test runner for Rust is quite limited and requires #[test]-annotated function definitions. One could go with a custom test harness, but that's a non-trivial amount of work.
A more pragmatic solution would be to create a macro that takes as input a function that returns a store and the name of the database to generate all the <store>_* test functions.

[joehasson]

Point well taken from both of you regarding test deduplication - seems more effort than it's worth on reflection.
So I have brought my branch up to date in light of subsequent discussion on #497. To be completely explicit:

• Refactored tests in line with comments on Sqlite session store #497 addressed in a0faea8
• Added unhappy path tests, broadly in line with those in c6bf07b. Code coverage is now in the high 80s as measured by cargo llvm-cov. I have omitted a counterpart to the test_database_unavailable_error test since Redis ConnectionManager doesn't have an equivalent to SQLite's pool.close() method, and simulating network failures would require external dependencies or system-level changes that seem like overkill for a unit test of a relatively simple code-path.
• Moved tests to their own folder in line with 316634a

I'll convert this from draft once I have updated and added documentation as mentioned before, unless there's any further comments. Thanks very much!

[joehasson]

Documentation added as requested and rebased on latest main. Ready for review - thanks for your patience!

[LukeMathWalker]

Let's add crate-level documentation, the one that will show up on crates.io when you look up this crate.

You can use the one for pavex_session_sqlx as a reference.

[LukeMathWalker]

Let's include an example, showing the structure of a namespaced key (i.e. that : is used as separator).

[LukeMathWalker]

There's no longer a need for kits. It's enough to add a couple of attributes to get thing working with the new DI system.
You can use either pavex.dev/docs or the MySQL PR as a reference on this matter.

[LukeMathWalker]

I've resolved the remaining issues in #533, which has now been merged. Thanks for all your work!