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

Add ring session middleware #316

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

dawranliou
Copy link
Contributor

This PR addresses issue #205

Problem:

wrap-session instantiates the default memory store per instance. This means that every route will have their own memory-store. This is a surprising default and should be resolved.

Solution: The reitit.ring.middleware.session ns shares one single store instance when the session store isn't specified from the user.

Please let me know if there's anything that I can improve for this PR. Thank you!

@dawranliou dawranliou force-pushed the issue-205-ring-middleware-session branch from 6c71b6b to cc073be Compare October 1, 2019 20:15
@dawranliou
Copy link
Contributor Author

I might have misunderstood the usage of :spec because I don't see any error when the :session route data is invalid. For example, the :store cannot be nil:

(def app
  (ring/ring-handler
   (ring/router
    ["/api"
     {:session    {:store nil}
      :middleware [mw/session-middleware]}
     ["/ping" handler]
     ["/pong" handler]])))

@dawranliou
Copy link
Contributor Author

I might have misunderstood the usage of :spec because I don't see any error when the :session route data is invalid.

I forgot about the :validate key.

@ikitommi
Copy link
Member

ikitommi commented Oct 1, 2019

Route data validation is not on by default, here's the guide: https://cljdoc.org/d/metosin/reitit/0.3.9/doc/ring/route-data-validation

Document specs for the :session entity map.
Fix the default session options.
Add test to validate spec.
@dawranliou dawranliou force-pushed the issue-205-ring-middleware-session branch from cc073be to f46244c Compare October 1, 2019 21:09
@dawranliou
Copy link
Contributor Author

Hi @ikitommi @miikka, thanks for helping. I did another force push to fix another spec issue and include the test for the spec validation. Would you review it again when you have the time? Thanks.

Copy link
Contributor

@miikka miikka left a comment

Choose a reason for hiding this comment

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

Looks good to me. Please remove the stray .nrepl-port file and then I'd be happy to merge this.

modules/reitit-middleware/.nrepl-port Outdated Show resolved Hide resolved
Recursively ignore .nrepl-port in the sub-directories.
@dawranliou dawranliou requested a review from miikka October 3, 2019 14:37
@ikitommi
Copy link
Member

ikitommi commented Oct 3, 2019

One thing that comes to mind is that how should the middleware be configured. In reitit, there are options:

  1. via middleware options
    pass the store etc. options for a create function, that is used to create the middleware. Interceptors use this is a lot and I think reitit 1.0.0 should do this too: it adds a clear phase to initialize one middleware once per application. Not the best option per today.
["/routes"
  {:middleware [(session/session-middleware {:store xyz})]}
 ["/with-session!" ...]]
  1. via route-data, on by default
    if mw is mounted, it's on. Mounting a session-mw into top-level means there is no way to disable it for a certain route. Not optimal.
["/routes"
  {:middleware [session/session-middleware]}
 ["/with-session!" ...]]
  1. via route-data, off by default
    mounting mw add just capabilities for a route. Only after adding a :session route data, enables it for certain routes. Most reitit-internal mw work like this, for example, content-negotiation & coercion.
["/routes"
  {:middleware [session/session-middleware]}
 ["/with-session!"  {:session xyz} ...]
 ["/without-session!" ...]]

would be good to have some coherence here. Just not sure what is the best way to do this. Comments?

@dawranliou
Copy link
Contributor Author

I agree that option 2 is not optimal, so this PR will to be fixed.

My personal preference is option 3. I value the consistancy with other internal mw (option 3) more than the clarity from option 1.

When `:session` key is absent in the route data, the session middleware will not
be attached to the route. To enable the middleware, the user at least need to
use an empty map `{}` for the `:session`, which uses the default options.
@dawranliou dawranliou force-pushed the issue-205-ring-middleware-session branch from 5152e46 to a3c64ba Compare October 4, 2019 18:02
@dawranliou
Copy link
Contributor Author

The commit a3c64ba changes from option 2 to option 3. I think we need some more feedbacks to make a decision.

I think another reason to support option 3 (in general) is that the router code would be cleaner if anyone wants to have different configurations on routes. Say:

["/routes"
  {:middleware [session/session-middleware]}
 ["/with-session-mem!"  {:session {:store mem-store}} ...]
 ["/with-session-cookie!"  {:session {:store cookie-store}} ...]
 ["/without-session!" ...]]

Versus

["/routes"
 ["/with-session-mem!"  {:middleware [(session/session-middleware {:store mem-store})]} ...]
 ["/with-session-cookie!"  {:middleware [(session/session-middleware {:store cookie-store})]} ...]
 ["/without-session!" ...]]

@dawranliou
Copy link
Contributor Author

I finally see this today: #264. Perhaps put this PR on hold till #264 makes a decision.

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.

3 participants