Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

global cache leads to collisions #21

Closed
jasonjckn opened this issue Jul 22, 2023 · 1 comment · Fixed by #23
Closed

global cache leads to collisions #21

jasonjckn opened this issue Jul 22, 2023 · 1 comment · Fixed by #23

Comments

@jasonjckn
Copy link

jasonjckn commented Jul 22, 2023

I'm using multiple independent configs, that have similar source/target values, and your global cache causes bugs
e.g. /~https://github.com/esuomi/muotti/blob/master/src/main/clj/muotti/core.clj#L97

Instead it makes more senes to create a new localized cache when ->transformer is called, tied to that specific configuration.

(defn ->transformer
  "Creates a new [[Transformer]] instance from given map of adjacencies and related configuration. By default uses
  [[default-adjacencies]], but the map of adjacency lists to validation and transformation functions can be overridden.

  ```clojure
  ; create new transformer with built-in defaults:
  (def defaults-t (->transformer))

  ; create new transformer with custom transformation configuration:
  (def custom-t (-> transformer {:transformations [:keyword :string] {:validator   keyword?
                                                                      :transformer name}
                                                  [:string :number]  {:validator   string?
                                                                      :transformer parse-long}})
  ```"
  ([] ->transformer default-config)
  ([{:keys [transformations]
     :as   config}]
   (let [resolved-paths (atom {})
         graph          (adjacencies-as-graph transformations)]
     (reify Transformer
       (transform [_ from to value]
         (if-let [cached-chain (get @resolved-paths [from to])]
           (do
             (log/tracef "Reusing cached chain %s for transformation [%s -> %s]" cached-chain from to)
             (resolve-chain value cached-chain))
           (let [chains (resolve-transformer-chains graph from to)]
             (if (= ::unknown-path chains)
               chains
               (reduce
                 (fn [_ chain]
                   (let [result (resolve-chain value chain)]
                     (if-not (= ::failed-resolve result)
                       (do
                         (log/debugf "Path %s produces usable result for transformation [%s -> %s], caching path for future lookups" chain from to)
                         (swap! resolved-paths assoc [from to] chain)
                         (reduced result))
                       ::invalid-value)))
                 ::invalid-value
                 chains)))))
       (visualize-dot [_]
         (lio/dot-str graph))
       (visualize-mermaid [_]
         (->mermaid graph))
       (config [_]
         config)))))

Also, as a side note, if you're interested in optimal performance, that cache will perform better (both lookups & insertions) if it's a java.util.HashMap or ConcurrentHashMap, malli does the same thing.

@esuomi
Copy link
Owner

esuomi commented Jul 25, 2023

Hi and thank you for the report! This makes sense since the global cache was added as a sort of quick&dirty solution, so it isn't the most thought out feature. I'm on vacation currently and I don't have access to my devbox (on purpose, I maintain quite strict work/personal life balance) so if you're up for it, patches are welcome :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants