maxcountryman / tower-sessions

🥠 Sessions as a `tower` and `axum` middleware.
MIT License
253 stars 42 forks source link

only delete from stores when we have a cookie #90

Closed maxcountryman closed 12 months ago

maxcountryman commented 12 months ago

If we don't have a cookie, the session is new and therefore has never been saved. Given this, we should not invoke the overhead of talking to the store. Likewise, we don't need to set a removal cookie as we have never set the cookie.

This should address the original issue in #89.

maxcountryman commented 12 months ago

This could also check that we have a loaded persisted session, either from the store or the in-memory cache that's used for safe concurrent data access; this might be more correct because a malicious user agent could send along a bogus session ID, which would invoke the overhead of talking to the store.

codecov[bot] commented 12 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (2749adb) 75.91% compared to head (bd8b5a6) 76.06%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #90 +/- ## ========================================== + Coverage 75.91% 76.06% +0.15% ========================================== Files 11 11 Lines 465 468 +3 ========================================== + Hits 353 356 +3 Misses 112 112 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

bikeshedder commented 12 months ago

I can confirm that this fixes the superfluous Storage::delete calls. It still does log "created new session" and "deleted" though.

DominicWrege commented 12 months ago

Can't wait to get this merged :)

maxcountryman commented 12 months ago

Thanks for taking a look. I'll cut a new release shortly.