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

HTTP Client suggestion for cookies and multiple headers #501

Closed
g23 opened this issue Dec 13, 2022 · 2 comments
Closed

HTTP Client suggestion for cookies and multiple headers #501

g23 opened this issue Dec 13, 2022 · 2 comments

Comments

@g23
Copy link

g23 commented Dec 13, 2022

Hi, I recently wanted to switch to http-kit over clj-http since it seems to have better performance, but I noticed handling cookies is tricky. The resulting :set-cookie header has all of the cookies concatenated with , but for example in a cookie's Expires field there can also be ,s so it makes it tricky to parse.

My suggestion is to instead concatenate multiple headers with \n instead so then the end user can easily split on \n to get the multiple values.

I was going to try to fork and make a PR but when I run lein test locally on the unchanged code I get 8 test fails. So I'm not sure about my setup.

The change I'm suggesting is java/org/httpkit/HttpUtils.java in the splitAndAddHeader method, line 378 replace the "," for "\n"

This then results in 3 new test failures that can be fixed by changing test/org/httpkit/client_test.clj tests: test-header-multiple-values and test-headers-stringified to use \n instead of , on lines 457, 458, and 465.

@ptaoussanis
Copy link
Member

Hi Greg! PR welcome, I'll look at getting any issues with the tests sorted out when I'm working on the next release 👍

@ptaoussanis
Copy link
Member

@g23 Your PR has been merged, thanks Greg! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants