Skip to content

Commit 63a5341

Browse files
authored
Use standard Clojure map for cookies (#621)
Clojure `sorted-map` and `sorted-set` are useful in some scenarios but can present problems in others: * good: predictable iteration order, used to great effect in `apply-on-form-params`, `test-encode-cookies`, and `test-wrap-cookies` * bad: `get`, `contains?`, `find`, `dissoc`, `select-keys`, etc throw ClassCastException when given incompatible keys https://clojure.atlassian.net/browse/CLJ-2693 * bad: `=` and `clojure.data/diff` can yield exceptions https://clojure.atlassian.net/browse/CLJ-2325 https://clojure.atlassian.net/browse/CLJ-1983 * bad: `pr` and `print` generate strings indistinguishable from unsorted map/set * bad: `print-dup` generates unreadable strings https://clojure.atlassian.net/browse/CLJ-1733 I propose to avoid `sorted-map` for values in the clj-http `:cookies` map. Looking at the commit history, I'm unable to determine why a sorted-map was used initially. The change away from sorted-map doesn't break any tests and doesn't appear to have any substantive effect on clj-http behavior, but perhaps there are some use cases I'm not familiar with. Of course, I probably should provide a justification or example of obvious downside to this use of sorted-map 😉 so I'll attempt to explain: ```clj user> (-> resp pr-str clojure.edn/read-string (get-in [:cookies "someCookie" "value"]) nil user> (-> resp pr-str clojure.edn/read-string (get-in [:cookies "someCookie" :value]) "someValue" user> (-> resp (get-in [:cookies "someCookie" "value"]) ClassCastException clojure.lang.Keyword cannot be cast to java.lang.String java.lang.String.compareTo (String.java:111) user> (-> resp (get-in [:cookies "someCookie" :value]) "someValue" ``` We expect `get` or `find` to return `nil` when a key isn't present in a map, and that's what we observe when serializing the response map from service1 to EDN and reading this EDN in service2. We were surprised when the same code threw ClassCastException when executed within service1. While I wouldn't expect another clj-http user to run into exactly this set of circumstances, the potential for inconsistent behavior, this inconvenient behavior of sorted-map, the lack of obvious upside to using sorted-map for cookie values, and the minimal clj-http code change lead me to believe this is a worthwhile change. In the meantime, we've adjusted our code to coerce these sorted-maps to standard Clojure maps, so I won't claim that we're blocked on this clj-http change. My goal here is to document the concern and attempt to make a case for this change, but remain open-minded about the approach and probably learn something in the process 😄 Happy to discuss alternatives, concerns, etc. Thanks your ongoing maintenance and stewardship of clj-http @dakrone 😃
1 parent 6fbec42 commit 63a5341

File tree

1 file changed

+1
-1
lines changed

1 file changed

+1
-1
lines changed

src/clj_http/cookies.clj

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@
2222
(if (not (nil? (get m k)))
2323
(assoc newm k (get m k))
2424
newm))
25-
(sorted-map) (sort (keys m))))
25+
{} (keys m)))
2626

2727
(defn to-cookie
2828
"Converts a ClientCookie object into a tuple where the first item is

0 commit comments

Comments
 (0)