taoensso / carmine

Redis client + message queue for Clojure
https://www.taoensso.com/carmine
Eclipse Public License 1.0
1.16k stars 131 forks source link

Unexpected behavior with `(parse-map ... :keywordize)` #303

Closed carrete closed 5 months ago

carrete commented 5 months ago

Per https://github.com/taoensso/nippy/issues/170

This:

(ns titletk.carmine-issue
  (:require
   [taoensso.carmine :as car])
  (:gen-class))

(defn -main
  [& args]
  (let [opts {:host "localhost"
              :port 6379}
        hset (fn [scope k v]
               (car/wcar
                opts
                (car/hset scope k v)))
        hget (fn [scope k]
               (car/wcar
                opts
                (car/hget scope k)))
        hget-keywordize (fn [scope k]
                          (car/wcar
                           opts
                           (car/parse-map (car/hget scope k) :keywordize)))
        hgetall (fn [scope]
                  (car/wcar
                   opts
                   (car/hgetall scope)))
        hgetall-keywordize (fn [scope]
                             (car/wcar
                              opts
                              (car/parse-map (car/hgetall scope) :keywordize)))
        uuid1 (random-uuid)
        uuid2 (random-uuid)]
    (hset "foobar" uuid1 "thing 1")
    (hset "foobar" uuid2 "thing-2")
    (hset "foobar" "some-key" "some-value")
    (println "1:" (hget "foobar" uuid1))
    (println "2:" (hget "foobar" uuid2))
    (println "3:" (hget "foobar" "some-key"))
    (println "4:" (hget-keywordize "foobar" uuid1))
    (println "5:" (hget-keywordize "foobar" uuid2))
    (println "6:" (hget-keywordize "foobar" "some-key"))
    (println "7:" (hgetall "foobar"))
    (println "8:" (hgetall-keywordize "foobar"))))

Outputs:

1: thing 1
2: thing-2
3: some-value
4: {nil nil}
5: {nil nil}
6: {nil e}
7: [#uuid "f424cfc7-a1a6-4c44-b94d-9ea7a36e44e6" thing 1 #uuid "800bcecb-a084-41c2-8e70-54e6850ea65b" thing-2 some-key some-value]
8: {nil thing-2, :some-key some-value}

I don't completely understand lines 4-6 above, but my issue is with the conversion of the uuid to nil (because (keyword uuid) returns nil) on lines 4, 5, and 8. On line 8, the two uuids are converted to nil which means they're collapsed with their associated values under the same nil key so the map that's returned just includes one of them, whichever is added last.

When I brought this issue up in the nippy repo (link above) I thought I remembered this problem had to do with with the built-in #uuid and #inst reader conditionals in EDN (because that's where the data that's saved to redis comes from). This is not the case though. The problem is with anything that can't be turned into a keyword. I don't know what the right, idiomatic solution is, but my preference would be that anything that can't be converted to be a keyword should be left as-is.

ptaoussanis commented 5 months ago

So here's your example, written out as individual wcar calls that you can easily run at the REPL:

(require '[taoensso.carmine :as car :refer [wcar]])
(def opts {:host "localhost"
           :port 6379})

(def uuid1 (random-uuid))
(def uuid2 (random-uuid))

(wcar opts (car/hset "foobar" uuid1 "thing 1")) ; => 1
(wcar opts (car/hget "foobar" uuid1)) ; => "thing 1"

(wcar opts (car/hset "foobar" uuid2 "thing-2")) ; => 1
(wcar opts (car/hget "foobar" uuid2)) ; => "thing-2"

(wcar opts (car/hset "foobar" "some-key" "some-value")) ; => 1
(wcar opts (car/hget "foobar" "some-key")) ; => "some-value"

(wcar opts (car/parse-map (car/hget "foobar" uuid1)      :keywordize)) ; => {nil nil} *
(wcar opts (car/parse-map (car/hget "foobar" uuid2)      :keywordize)) ; => {nil nil} *
(wcar opts (car/parse-map (car/hget "foobar" "some-key") :keywordize)) ; => {nil \e}  *

(wcar opts                (car/hgetall "foobar")) ; [<uuid1> "thing 1" <uuid2> "thing-2"]
(wcar opts (car/parse-map (car/hgetall "foobar") :keywordize)) ; => {nil "thing2", :some-key "some-value"} *

There's 4 calls with unexpected results, marked with a *.

These 3 share the same problem:

(wcar opts (car/parse-map (car/hget "foobar" uuid1)      :keywordize)) ; => {nil nil} *
(wcar opts (car/parse-map (car/hget "foobar" uuid2)      :keywordize)) ; => {nil nil} *
(wcar opts (car/parse-map (car/hget "foobar" "some-key") :keywordize)) ; => {nil \e}  *

In all these cases the inner hget returns a single value, as you can see from the earlier examples:

(wcar opts (car/hget "foobar" uuid1))      ; => "thing 1" (key <uuid1>)
(wcar opts (car/hget "foobar" uuid2))      ; => "thing-2" (key <uuid2>)
(wcar opts (car/hget "foobar" "some-key")) ; => "some-value" (key "some-key")

parse-map expects a seq of kvs like you'd see from (wcar opts (car/hgetall "foobar")), not a single value like this. So it's treating "thing 1", "thing-2" and "some-value" as seqs of kvs. (This should be documented, sorry about that).

It's not obvious to me what you're actually trying to do here, but if you want something like {<uuid1> "thing 1"} then you could just do {uuid1 (wcar opts (car/hget "foobar" uuid1))} since you already know the key.

For the last call with an unexpected result:

(wcar opts (car/parse-map (car/hgetall "foobar") :keywordize)) ; => {nil "thing2", :some-key "some-value"} *

The problem that you correctly identified is that you're asking Carmine to parse a UUID as a keyword - which doesn't work as you'd like (returns nil).

You'd see the same behaviour trying to do the same thing in plain Clojure:

(keyword (random-uuid)) => nil

:keywordize just causes keyword to be called on keys. In this case 2 of your 3 keys are uuids, for which keyword will return nil.

If you'd prefer keyword to only be called on string keys, you could use a custom fn instead of :keywordize, e.g.:

(wcar opts
  (car/parse-map (car/hgetall "foobar")
    (fn my-key-fn [k _v] (if (string? k) (keyword k) k))))

=>  {<uuid1> "thing 1", <uuid2> "thing-2", :some-key "some-value"}

Hope that helps?

carrete commented 5 months ago

It's not obvious to me what you're actually trying to do here

I have some hardware devices that I want to group together, for example (hset :devices device1-uuid {:device/name ... :device/pin ...}). Then I want to be able to get either an individual device (hget :devices device1-uuid), or all devices (hgetall :devices).

parse-map expects a seq of kvs

Ah, ok. That makes sense. I'm actually working with maps (as above), and only noticed this for the first time when I created this issue (and didn't look at it closely). Thanks.

you could use a custom fn ... Hope that helps?

Yes, it does. Thanks!

ptaoussanis commented 5 months ago

You're welcome, cheers :-)