search

ring-clojure / ring

Clojure HTTP server abstraction
MIT License
3.75k stars 519 forks source link

Added support for cookie options #392

Closed Elknar closed 1 year ago

Elknar commented 4 years ago

... as defined by ring.middleware.cookies/wrap-cookies

weavejester commented 4 years ago

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?

Elknar commented 4 years ago

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.

weavejester commented 4 years ago

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.

Elknar commented 4 years ago

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

weavejester commented 4 years ago

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.