Discussion: Handle Session Expiry

[No9]

Hey Folks
As mentioned in the code https://github.com/yoshuawuyts/async-mongodb-session/blob/master/src/lib.rs#L49
Sessions do not expire at the moment.

Looking at the mongo docs it seems expiry can be enforced using an index
https://docs.mongodb.com/manual/tutorial/expire-data/#expire-documents-after-a-specified-number-of-seconds

It should be simple enough to code (famous last words when talking about cache expiry :) ), but wanted to clarify a some points before i started.

The approach I am considering is to add an additional parameter called expireAfterSeconds to connect and from_client
Is that ok?

Currently the session object in async-session only has an expiry field https://github.com/http-rs/async-session/blob/main/src/session.rs#L59 .
Using the inbuilt TTL in mongo means that the property in this scenario is not an expiry time but the creation time that will be offset with the expireAfterSeconds value to calculate the expiry time.
I can proceed using the expiry field but would suggest that we change the name of the of the session object to expiry-marker.
If that makes sense I can PR that as part of this.

As the TTL is an index it will have to be redefined on restart in the case where the user changes the value following restarts.
This will be done using collMod https://docs.mongodb.com/manual/reference/command/collMod/#dbcmd.collMod

It would be great if we can finalise #7 as expiry will will need tests and I wouldn't want to do too much using an approach that hasn't landed.

Thanks for your time.

[yoshuawuyts]

Hey @No9; this all sounds great! Some notes:

The approach I am considering is to add an additional parameter called expireAfterSeconds to connect and from_client. Is that ok?

We already pass 3 params to the client. I think at this point it may make sense to create a builder instead. The shape for that would be something like:

impl MongodbSessionStore {
    fn builder(uri: &str, db: &str, coll_name: &str) -> Builder;
}

pub struct Builder{}
impl Builder {
    pub fn expiration(self, dur: Duration) -> Self;
    pub fn build(self) -> MongoDbSessionStore;
}

As part of this it may also be worth seeing if we can cover #6 since the data we're storing will have to change anyway, which may (?) be a semver major.

I've invited you to the repo on both github and crates.io -- feel free to move ahead with this since I can be somewhat slow to reply 😅

Thanks so much!

[No9]

Thanks for the feedback and adding me to the repo @yoshuawuyts
I agree with the builder approach and will take a look at #6

[No9]

@yoshuawuyts while looking at the testing approach for managing the expiry through this library it became clear that the expiry shouldn't be managed here.
In a scenario where there is more than one instance of a service using this library there is too much opportunity for configuration conflict. I've updated the README.md to reflect this reasoning and included steps for the configuration.
https://github.com/yoshuawuyts/async-mongodb-session/pull/9/files#diff-04c6e90faac2675aa89e2176d2eec7d8R49

I've also implemented a fix for #6

As this isn't what we discussed above I thought I would create a PR and get some feedback before pushing.

[No9]

As per #13 and this discussion: expiration based on duration required additional API enhancements and conflicted with expiry based on a defined expiry time.
As expiry based on a specific time offers a better dev experience we went in that direction and removed the duration approach.