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

tower-sessions-core crate rework #218

Open
wants to merge 32 commits into
base: main
Choose a base branch
from

Conversation

carloskiki
Copy link

Let me first start by saying that this crate is awesome. I think its what the tower environment is all about; creating abstraction that compose seamlessly together.

However, I think at its core this crate is much too restrictive, and this is why I decided to do my best at providing what I think this abstraction should look like. It is by no means correct, and what I want is simply to start a discussion.

The Changes

I think the best way to describe my view on the design is to talk about the changes I brought in this PR.

A strongly typed Record

The most flagrant thing that should be added to this crate is the ability to have a strongly typed Record. Clearly, what is idiomatic in Rust is to make all abstractions opt-in, rather than opt-out. I think this crate has it in reversed; It requires us to build an abstraction over a Key-Value store in order to have some type information. The way it should work is that, by default, users should have a concrete type to work with. They can set this type to be HashMap<K, V> if they like, but this runtime cost should not be imposed onto them. This PR solves this by adding a generic type to the SessionStore<R> trait. This generic type R is the record.

Suppose I wanted to implement a SessionStore for some form of storage, and I need the record to be serializable/deserializable, then I as the implementor, am the one to restrict MyStore:

impl<R> SessionStore<R> for MyStore<R>
where
    R: serde::Serialize + serde::Deserializable /* + Some other bounds */
{
// ...
}

The user of MyStore can then use any type they like, as long as it respects the trait bounds.

I think this is a good alternative to what was proposed in this discussion thread.

Removal of async_trait

The new traits now use async functions in traits, which was stabilized in Rust 1.75. The only issue currently is that the user cannot name the output type (the opaque Future returned). This will be solved with Return Type Notation which is hopefully close to stabilization (The only thing left I think is this). Then, we will be able to remove all Send and Sync bounds on the SessionStore trait (which is simply lovely).

It is worth noting that even though this would likely bump up the msrv, version 1.75 has been out for more than 9 months now, and axum have been considering moving away from async_trait towards this for a while now.

Ids are Solely Controlled by the Store

Various nonsense (collisions, lifetime extension, etc.) can arise if we let both the Store and the handler create and change Ids. Therefore, this PR changes the semantics session by only allowing the Store to set and modify Ids. Ids should not be even instantiable by the handler.

This is not fully implemented yet, because right now there is no way of constructing an Id (except from the middleware parsing it from a cookie). What I plan on doing is adding a feature flag that the SessionStore implementor should set in order to have access to Id constructors.

Much, much Simpler Sessions

As a result of all the above, the session type is now much simpler. It is this:

pub struct Session<R, Store> {
    pub store: Store,
    pub id: Option<Id>,
    // ... (PhantomData)
}

You can do as much as you could then by simply calling methods on directly store.

Agnostic over Expiry

This feature is not fully fleshed out yet, but I think it is much better not to impose on users the choice of the time crate.

Instead of having a concrete Expiry type, we could have an Expires trait that should be implemented for the record type R, but I'm not sure yet what it should look like.

What is Missing

This PR is very much an MVP, it does not have tests, does not have very good docs, etc. A bunch of things are missing, and I haven't got to working on the tower-session crate yet. Here are some of the missing features:

  • An ergonomic way to set the cookie in the response from the handler.
  • A middleware.
  • Likely many more things.

As I said previously, I want to discuss these changes, and please do not hesitate to be highly critical of this PR.

@compiler-errors
Copy link

Hey 👋 I saw this cross-linked from one of my rust compiler PRs, so I'm just reaching out:

This will be solved with rust-lang/rust#109417 which is hopefully close to stabilization (The only thing left I think is rust-lang/rust#129629). Then, we will be able to remove all Send and Sync bounds on the SessionStore trait (which is simply lovely).

I encourage you to test this out with RTN locally on rustc nightly and report any errors you encounter.

@carloskiki carloskiki marked this pull request as ready for review September 30, 2024 16:03
@carloskiki
Copy link
Author

@maxcountryman This is still an MVP, but I would just like your input on the new structure. I would like to discuss it with you mainly because I do not want to release a second crate that does the same thing as this one. Let's minimize split efforts and keep the ecosystem clean. Let me know what you don't like about my design.

Obviously this still requires docs, examples, tests, and tracing, but at least every idea I had is now fully implemented. I am not done working on it, I would just like to know what things you would change before I write the docs, examples, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants