Closed sumbach closed 1 year ago
Honestly I can't remember why the cookie map would use sorted-map
, and I agree that I don't think it's necessary.
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.
Clojure
sorted-map
andsorted-set
are useful in some scenarios but can present problems in others:apply-on-form-params
,test-encode-cookies
, andtest-wrap-cookies
get
,contains?
,find
,dissoc
,select-keys
, etc throw ClassCastException when given incompatible keys https://clojure.atlassian.net/browse/CLJ-2693=
andclojure.data/diff
can yield exceptions https://clojure.atlassian.net/browse/CLJ-2325 https://clojure.atlassian.net/browse/CLJ-1983pr
andprint
generate strings indistinguishable from unsorted map/setprint-dup
generates unreadable strings https://clojure.atlassian.net/browse/CLJ-1733I 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:
We expect
get
orfind
to returnnil
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 😃