ring
ring copied to clipboard
ring-clojure
Added support for cookie options
Thanks for the patch. However, I'm not sure what this is meant to do, or what functionality this is supposed to add. Can you explain?
Yeah, sorry.
ring.middleware.cookies/wrap-cookies provides the ability to add custom cookie encoders/decoders (which, apart from the expected en-/de-coding allows for sanity checks on the cookies)
This was for some reason missing in the session wrapper, despite using the cookie wrapper.
Ah, I see. Yes, that is a problem, well spotted. It's often useful to report things like this an an issue first, even if you intend to open a PR to solve it.
Regarding the PR, I'm not sure why you're doing things with :cookie-attrs and not just passing :cookie-opts directly to the cookie middleware functions. Can you explain?
Also commits need to adhere to the contributing guidelines.
Sorry about that, new to this. Can close and open an issue instead...
As for :cookie-attrs stuff - i didn't want to modify the inputs of session-response as it's not a private fn, and I don't know where else it might be used
Keep the PR open for now. Usually an issue is opened first and then a PR, but now that we've had this discussion here, we might as well continue so that the bug report is all in one place. If it turns out that you don't have the time to make the necessary changes to get this PR merged, we can always add an issue as well. I may do that anyway.
session-request and session-response should take the same options map as wrap-session. I'm still not exactly sure what you were trying to do with :cookie-attrs, but I think it's unnecessary. I believe you just need:
(cookies/cookies-request (:cookie-opts options))
And:
(cookies/cookies-response (:cookie-opts options))
It should be a two line change. However, this issue brings up the more thorny issue of whether wrap-session should be calling wrap-cookies at all! In my view it really shouldn't, but that functionality was put in pre-1.0 and we can't remove existing features.
What we can do is make it easier to use wrap-cookies and wrap-session together, as right now the heart of this issue is that the cookie middleware included in the session overrides the functionality of an outer wrap-cookies. Let me think about this a little, as I think there's a better way of doing this.