Skip to content

Conversation

JonathanWoollett-Light
Copy link

@JonathanWoollett-Light JonathanWoollett-Light commented Mar 31, 2025

Updates the oauth example to use a better approach to session management.

Motivation

The current approach to sessions within the oauth example is not fit for any circumstance, and if anyone where to take it at face value and use async_sessions their application would encounter severe performance issues. The current approach not only fails to lead users down the right path but actively leads them down the wrong path.

Solution

By updating the example to use a viable approach it helps people understand the right way to solve the problem, rather than potentially misleading them.

@JonathanWoollett-Light JonathanWoollett-Light force-pushed the oauth-sessions branch 3 times, most recently from 1b352f7 to 612f2e4 Compare March 31, 2025 19:03
@mladedav
Copy link
Collaborator

mladedav commented Apr 1, 2025

Would you mind expanding a bit on the motivation? Is the only issue performance?

@JonathanWoollett-Light
Copy link
Author

Would you mind expanding a bit on the motivation? Is the only issue performance?

The main motivation is that the current example while simpler doesn't really give an example of what someone needs to know.

Personally I used this example in testing, but recently discovered that this is not usable in production and thus needed to figure out what was usable. It would've saved me a lot of time if the example was usable to begin with.

I think we can save a lot of people time by simply making the example include what they will need rather than forcing them to look at this example, then realise it can't be used, then find a different example that can be used.

Copy link
Collaborator

@mladedav mladedav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry it takes this long, but there are a lot of unrelated changes, why did you make all of the changes like removing the AsRef impls, the whole User extractor and so on?

I guess the relevant changes are just using an SQL backing storage and changing the library from async-session to tower-sessions?

Cargo.lock Outdated
@@ -54,12 +54,12 @@ dependencies = [

[[package]]
name = "ahash"
version = "0.8.11"
version = "0.8.12"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove all the unrelated updates from the PR please.

Copy link
Author

@JonathanWoollett-Light JonathanWoollett-Light Aug 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running cargo test --no-run changes the lock file. I didn't change it manually. Cargo thinks these changes are related.

match user {
async fn index(
State(AppState { store, .. }): State<AppState>,
TypedHeader(cookies): TypedHeader<headers::Cookie>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this change the cookie is not optional anymore.

Please leave the User extractor as is unless you have a reason to delete it.

Copy link
Author

@JonathanWoollett-Light JonathanWoollett-Light Aug 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find a way to make it work without changing it.

Updates the oauth example to use a practical approach to session management.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants