Use standard Clojure map for cookies

[samumbach]

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:

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 😃

[dakrone]

Honestly I can't remember why the cookie map would use sorted-map, and I agree that I don't think it's necessary.

[samumbach]

My best guess is that the intent was to put the cookies in a sorted map so they would print sorted by the cookie name, but the code as written puts each cookie into a sorted map (with keys :value, :domain, :path, :secure, etc) but the cookies map itself is a standard map.

[dakrone]

LGTM, thanks!