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

(maint) add support for multiple Set-Cookie headers in a response #104

Merged
merged 2 commits into from
Feb 2, 2024

Conversation

jonathannewman
Copy link
Contributor

The design of the client does not allow multiple entries for the
same header name, as headers are returned in a map, rather than
an array of them. As a result, headers like "Set-Cookie" would only
return the last one seen if they were encountered. This alters the
behavior to concatenate any of the "Set-Cookie" entries together,
separated by a newline character. This seems to be a fairly common
convention for clients that use headers with maps.

Tests were updated to include multiple cookies, and demonstrate
the behavior.

The design of the client does not allow multiple entries for the
same header name, as headers are returned in a map, rather than
an array of them.  As a result, headers like "Set-Cookie" would only
return the last one seen if they were encountered. This alters the
behavior to concatenate any of the "Set-Cookie" entries together,
separated by a newline character. This seems to be a fairly common
convention for clients that use headers with maps.

Tests were updated to include multiple cookies, and demonstrate
the behavior.
In leiningen 2.11.0, composite profiles became a warning. This removes
the use of them in favor of explicit profiles to remove the warning.
@jonathannewman jonathannewman requested a review from a team as a code owner February 2, 2024 00:25
@jonathannewman
Copy link
Contributor Author

The eastwood failure is related to a lein 2.11.1 and eastwood incompatibility. When run locally with an older version of lein, it all passes:

➜  clj-http-client git:(maint/main/multiple-headers) ✗ /usr/local/bin/lein-2-10-0 eastwood
== Eastwood 1.2.2 Clojure 1.11.1 JVM 17.0.8 ==
Directories scanned for source files: src/clj test
== Linting puppetlabs.http.client.common ==
== Linting puppetlabs.http.client.metrics ==
== Linting puppetlabs.http.client.async ==
== Linting puppetlabs.http.client.async-unbuffered-test ==
== Linting puppetlabs.http.client.sync ==
== Linting puppetlabs.http.client.metrics-test ==
== Linting puppetlabs.http.client.test-common ==
== Linting puppetlabs.http.client.gzip-request-test ==
== Linting com.puppetlabs.http.client.impl.java-client-test ==
== Linting puppetlabs.http.client.sync-plaintext-test ==
== Linting puppetlabs.http.client.sync-ssl-test ==
== Linting puppetlabs.http.client.async-plaintext-test ==
== Linting com.puppetlabs.http.client.impl.metrics-unit-test ==
== Linting done in 7718 ms ==
== Warnings: 0. Exceptions thrown: 0
➜  clj-http-client git:(maint/main/multiple-headers) ✗ 

Copy link
Contributor

@steveax steveax left a comment

Choose a reason for hiding this comment

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

cookeis!
LGTM!

@steveax steveax merged commit be66ef3 into puppetlabs:main Feb 2, 2024
4 of 5 checks passed
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