djblue / portal

A clojure tool to navigate through your data.
https://djblue.github.io/portal/
MIT License
872 stars 81 forks source link

Portal confuses values that are equal but of different types #218

Open mrkam2 opened 5 months ago

mrkam2 commented 5 months ago

I was able to reproduce the problem I reported in this slack discussion using plain clojure types.

(tap> {:a [[1]]})
(tap> {:a ['(1)]})

Here one object contains a vector [1] and another contains a list (1). However, the portal shows both as vectors. image

If I clear it and tap them in the opposite order, both will be shown as lists: image

This time I wasn't able to observe diferrent values by accessing portal selection, but tap-list clearly shows two different types:

@#'portal.runtime/tap-list ; => #<Atom@aeae4fd: ({:a [(1)]} {:a [[1]]})>

When types are important this becomes extremely confusing since you're used to trust what you see in portal.

djblue commented 4 months ago

Thanks for the bug report, should be fixed in 0.55.0.

mrkam2 commented 4 months ago

Thanks for the bug report, should be fixed in 0.55.0.

Awesome, thanks a lot for a quick fix!

mrkam2 commented 4 months ago

Unfortunately I just found that this didn't address my original problem.

mrkam2 commented 4 months ago

So if I'm reading the fix correctly, it takes hashcode and meta into account but it doesn't take type into account. I'm pretty sure adding type to the hash would address my problem. As a workaround I could also force my custom objects to generate a hashcode that's different from the one that regular Clojure collections produce.

In general, I think we should expect key clashes when storing data in a cache. So when two or more different objects share the same key, we should still be able to distinguish them in the cache.

Thoughts?

mrkam2 commented 4 months ago

Redefining hashCode function didn't work. hash function in Clojure ignores different values as can be seen here:

; Here is what I see after tapping two of my objects:
(map #(-> % :metrics first hash) @@#'portal.runtime/tap-list)
(-724974908 -724974908) ; identical values here

(map #(-> % :metrics first type) @@#'portal.runtime/tap-list)
(clojure.lang.PersistentArrayMap cash.Metric) ; different types here

(map #(-> % :metrics first .hashCode) @@#'portal.runtime/tap-list)
(1655286045 1655286051) ; different values here
djblue commented 4 months ago

With the example provided above:

(tap> {:a [[1]]})
(tap> {:a ['(1)]})

I get:

image

The inner list / vector types are now correctly sent to the client.

Is the other example you are trying out something I can do locally?

The solution involves adding the additional hash-like number computed by:

(defn- hash+ [x]
  (cond
    (map? x)
    (reduce-kv
     (fn [out k v]
       (+ out (hash+ k) (hash+ v)))
     (+ 1 (if (sorted? x) 1 0) (hash+ (meta x)))
     x)

    (coll? x)
    (reduce
     (fn [out v]
       (+ out (hash+ v)))
     (+ (cond
          (list? x)   3
          (set? x)    (if (sorted? x) 4 5)
          (vector? x) 6
          :else       7)
        (hash+ (meta x)))
     x)

    :else
    (cond-> (hash x)
      (can-meta? x)
      (+ (hash+ (meta x))))))

Which assigns different values to different shapes based on various Clojure type predicates. This will easily produce collisions which is why the value is still part of the cache key. The idea being that there is a minimal chance of both hash and hash+ producing a collision at the same time.

here are the current tests.