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

use clj-http's wrap-cookies instead of ring's because formats have diverged #15

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bmaddy
Copy link

@bmaddy bmaddy commented Oct 23, 2015

This switches to use clj-http's wrap-cookies instead of ring's because their formats have diverged. clj-http returns cookies something like this:

{:discard true, :expires , :path /, :version 0, :value ...}

but ring no longer accepts :discard and :version as shown here: ring-clojure/ring@c05004f#diff-c055e0aeef031084c68bf6da304ed4f7L25

This leads to the following error when there is cookie info in the response:

java.lang.AssertionError: Assert failed: (every? valid-attr? attrs)

@webmonarch
Copy link

👍 my understanding of all of the cookie fields is a little rusty, but I do think for a proxy server, being a bit "looser" with spec adherence makes sense.

Alternatively, one could delete the keys in the tailrecursion.ring-proxy/prepare-cookies function. (which I also did successfully for my use-cases).

@ghost
Copy link

ghost commented Apr 18, 2016

Just curious @webmonarch is there some way to override this implementation easily as you mentioned above? I want to make sure I'm not missing a neat piece of clojure. The only thing I can think of is a fork in my own repo to implement the desired cookie behavior.

@webmonarch
Copy link

hey @aft-luke, yeah, nothing neat that I know of. I forked for our internal projects. The code is pretty small so a good ol' copy + paste would probably work too.

Good luck!

@ghost
Copy link

ghost commented Apr 21, 2016

Thanks @webmonarch. I ended up writing my own handler that's 80% copy/paste as well.

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.

2 participants