Repository: wilkerlucio/pathom
Pull request #88 (2019-04-14 21:56:23)

feat: consider params in request caching
This commit introduces (naive) parameter consideration to request
caching - meaning that parameters will be considered when deciding
whether to use an existing cached value.

The approach is naive in that it does not consider defaults, so
additional work might be performed if a user requests something w/ the
same parameters as the defaults. That being said, in a "real-world"
workload, this seems extremely unlikely. Furthermore, with the existing API, I cannot imagine an ergonomic way of introducing the functionality.

All tests in connect are reported as passing, with the exception of an assertion that was already broken because it is using a single argument to `=` in the assertion.

rschmukler: Upon a closer look of the diff, it seems my autoformatter might have been a bit aggressive. If this is too much, let me know and I will work to cleanup the formattings in the diff. 

Patch:
diff --git a/src/com/wsscode/pathom/connect.cljc b/src/com/wsscode/pathom/connect.cljc
index fcc325f8..6a2d494d 100644
--- a/src/com/wsscode/pathom/connect.cljc
+++ b/src/com/wsscode/pathom/connect.cljc
@@ -272,7 +272,7 @@
 
       ; leaf / branches
       (reduce
-        (fn [idx {:keys [key children] :as item}]
+        (fn [idx {:keys [key children]}]
           (cond-> idx
             key
             (update key (partial merge-with merge-grow)
@@ -580,8 +580,9 @@
        (every? m input)))
 
 (defn- cache-batch [env resolver-sym linked-results]
-  (doseq [[input value] linked-results]
-    (p/cached env [resolver-sym input] value)))
+  (let [params (-> env :ast :params)]
+    (doseq [[input value] linked-results]
+      (p/cached env [resolver-sym input params] value))))
 
 ;; resolve plan
 
@@ -783,7 +784,8 @@
   the requested attribute."
   [{::keys   [indexes] :as env
     ::p/keys [processing-sequence]}]
-  (let [k (-> env :ast :key)]
+  (let [k (-> env :ast :key)
+        p (-> env :ast :params)]
     (if (get-in indexes [::index-oir k])
       (if-let [{:keys [e s]} (pick-resolver env)]
         (let [{::keys [cache? batch? input] :or {cache? true} :as resolver}
@@ -791,7 +793,7 @@
               env      (assoc env ::resolver-data resolver)
               response (if cache?
                          (p.async/throw-err
-                           (p/cached env [s e]
+                           (p/cached env [s e p]
                              (if (and batch? processing-sequence)
                                (let [items          (->> processing-sequence
                                                          (mapv #(entity-select-keys env % input))
@@ -855,12 +857,13 @@
                 output     (resolver->output env resolver-sym)
                 env        (assoc env ::resolver-data resolver)
                 e          (select-keys (p/entity env) input)
+                p          (-> env :ast :params)
                 trace-data {:key         key
                             ::sym        resolver-sym
                             ::input-data e}
                 response   (if cache?
                              (p.async/throw-err
-                               (p/cached env [resolver-sym e]
+                               (p/cached env [resolver-sym e p]
                                  (if (and batch? processing-sequence)
                                    (pt/tracing env (assoc trace-data ::pt/event ::call-resolver-batch)
                                      (let [_              (pt/trace env (assoc trace-data ::pt/event ::call-resolver-with-cache))
@@ -935,7 +938,8 @@
   Like reader, but supports async values on resolver return."
   [{::keys   [indexes] :as env
     ::p/keys [processing-sequence]}]
-  (let [k (-> env :ast :key)]
+  (let [k (-> env :ast :key)
+        p (-> env :ast :params)]
     (if (get-in indexes [::index-oir k])
       (go-catch
         (if-let [{:keys [e s]} (<? (async-pick-resolver env))]
@@ -944,7 +948,7 @@
                 env      (assoc env ::resolver-data resolver)
                 response (if cache?
                            (<?maybe
-                             (p/cached-async env [s e]
+                             (p/cached-async env [s e p]
                                (fn []
                                  (go-catch
                                    (if (and batch? processing-sequence)
@@ -978,25 +982,26 @@
 
 (defn- async-read-cache-read
   [env resolver-sym e batch? processing-sequence trace-data input]
-  (p/cached-async env [resolver-sym e]
-    (fn []
-      (go-catch
-        (if (and batch? processing-sequence)
-          (pt/tracing env (assoc trace-data ::pt/event ::call-resolver-batch)
-            (let [_              (pt/trace env (assoc trace-data ::pt/event ::call-resolver-with-cache))
-                  items          (->> processing-sequence
-                                      (map-async-serial #(entity-select-keys env % input)) <?
-                                      (filterv #(all-values-valid? % input))
-                                      (distinct))
-                  _              (pt/trace env {::pt/event ::batch-items-ready
-                                                ::items    items})
-                  batch-result   (<?maybe (call-resolver env items))
-                  _              (pt/trace env {::pt/event    ::batch-result-ready
-                                                ::items-count (count batch-result)})
-                  linked-results (zipmap items batch-result)]
-              (cache-batch env resolver-sym linked-results)
-              (get linked-results e)))
-          (<?maybe (call-resolver env e)))))))
+  (let [params (-> env :ast :params)]
+    (p/cached-async env [resolver-sym e params]
+      (fn []
+        (go-catch
+          (if (and batch? processing-sequence)
+            (pt/tracing env (assoc trace-data ::pt/event ::call-resolver-batch)
+              (let [_              (pt/trace env (assoc trace-data ::pt/event ::call-resolver-with-cache))
+                    items          (->> processing-sequence
+                                        (map-async-serial #(entity-select-keys env % input)) <?
+                                        (filterv #(all-values-valid? % input))
+                                        (distinct))
+                    _              (pt/trace env {::pt/event ::batch-items-ready
+                                                  ::items    items})
+                    batch-result   (<?maybe (call-resolver env items))
+                    _              (pt/trace env {::pt/event    ::batch-result-ready
+                                                  ::items-count (count batch-result)})
+                    linked-results (zipmap items batch-result)]
+                (cache-batch env resolver-sym linked-results)
+                (get linked-results e)))
+            (<?maybe (call-resolver env e))))))))
 
 (defn async-reader2
   "Like reader2, but supports async values on resolver return."
@@ -1006,8 +1011,8 @@
     :as      env}]
   (if-let [[plan out] (reader-compute-plan env #{})]
     (go-catch
-      (let [key (-> env :ast :key)]
-        (loop [[step & tail] plan
+     (let [key (-> env :ast :key)]
+       (loop [[step & tail] plan
                failed-resolvers {}
                out-left         out]
           (if step
@@ -1090,24 +1095,25 @@
            resolver-sym ::sym} (-> env ::resolver-data)
           e          (select-keys (p/entity env) input)
           key        (-> env :ast :key)
+          params     (-> env :ast :params)
           trace-data {:key         key
                       ::sym        resolver-sym
                       ::input-data e}]
       (pt/tracing env (assoc trace-data ::pt/event ::call-resolver-batch)
-        (if (p/cache-contains? env [resolver-sym e])
-          (<! (p/cache-read env [resolver-sym e]))
+        (if (p/cache-contains? env [resolver-sym e params])
+          (<! (p/cache-read env [resolver-sym e params]))
           (let [items-map      (->> (<? (map-async-serial #(entity-select-keys env % input) processing-sequence))
                                     (into [] (comp
                                                (map-indexed vector)
                                                (filter #(all-values-valid? (second %) input))
-                                               (remove #(p/cache-contains? env [resolver-sym (second %)]))))
+                                               (remove #(p/cache-contains? env [resolver-sym (second %) params]))))
                                     (group-input-indexes))
                 items          (keys items-map)
                 _              (pt/trace env {::pt/event ::batch-items-ready
                                               ::items    items})
                 channels       (into [] (map (fn [resolver-input]
                                                (let [ch (async/promise-chan)]
-                                                 (p/cache-hit env [resolver-sym resolver-input] ch)
+                                                 (p/cache-hit env [resolver-sym resolver-input params] ch)
                                                  ch))) items)
 
                 batch-result   (try
@@ -1155,6 +1161,7 @@
      ::pp/response-stream
      (let [ch  (async/chan 10)
            key (-> env :ast :key)
+           params (-> env :ast :params)
            env (assoc env ::plan plan)]
        (go
          (loop [[step & tail] plan
@@ -1183,7 +1190,7 @@
                                   (do
                                     (pt/trace env (assoc trace-data ::pt/event ::call-resolver-with-cache))
                                     (<!
-                                      (p/cached-async env [resolver-sym e]
+                                      (p/cached-async env [resolver-sym e params]
                                         #(go-catch (or (<!maybe (call-resolver env e)) {}))))))
 
                                 :else


Test patch:
diff --git a/test/com/wsscode/pathom/connect_test.cljc b/test/com/wsscode/pathom/connect_test.cljc
index 2c11421c..7d2cd8bc 100644
--- a/test/com/wsscode/pathom/connect_test.cljc
+++ b/test/com/wsscode/pathom/connect_test.cljc
@@ -852,10 +852,10 @@
     (is (= (parser {::p/entity (atom {:user/id 1 :user/foo "bar"})}
              [:user/name :cache])
            {:user/name "Mel"
-            :cache     {[`user-by-id {:user/id 1}] {:user/age   26
-                                                    :user/id    1
-                                                    :user/login "meel"
-                                                    :user/name  "Mel"}}})))
+            :cache     {[`user-by-id {:user/id 1} nil] {:user/age   26
+                                                        :user/id    1
+                                                        :user/login "meel"
+                                                        :user/name  "Mel"}}})))
 
   (testing "doesn't cache if asked to cache? is false"
     (is (= (parser {} [:value :cache])
@@ -1017,10 +1017,10 @@
     (is (= (parser2 {::p/entity (atom {:user/id 1 :user/foo "bar"})}
              [:user/name :cache])
            {:user/name "Mel"
-            :cache     {[`user-by-id {:user/id 1}] {:user/age   26
-                                                    :user/id    1
-                                                    :user/login "meel"
-                                                    :user/name  "Mel"}}})))
+            :cache     {[`user-by-id {:user/id 1} nil] {:user/age   26
+                                                        :user/id    1
+                                                        :user/login "meel"
+                                                        :user/name  "Mel"}}})))
 
   (testing "doesn't cache if asked to cache? is false"
     (is (= (parser2 {} [:value :cache])
@@ -2511,7 +2511,7 @@
                    :com.wsscode.pathom.trace/event     :com.wsscode.pathom.connect/compute-plan}]))))
 
      (testing "use cache when available"
-       (is (= (call-parallel-reader {::p/request-cache (atom {['a {}] (go-promise {:a 3})})} :b)
+       (is (= (call-parallel-reader {::p/request-cache (atom {['a {} nil] (go-promise {:a 3})})} :b)
               #:com.wsscode.pathom.parser{:provides        #{:a :b}
                                           :response-stream [#:com.wsscode.pathom.parser{:provides       #{:a}
                                                                                         :response-value {:a 3}}
@@ -2537,7 +2537,8 @@
                  :com.wsscode.pathom.trace/event        :com.wsscode.pathom.connect/call-resolver-with-cache
                  :key                                   :b}
                 {:com.wsscode.pathom.core/cache-key [a
-                                                     {}]
+                                                     {}
+                                                     nil]
                  :com.wsscode.pathom.core/path      [:b]
                  :com.wsscode.pathom.trace/event    :com.wsscode.pathom.core/cache-hit}
                 {:com.wsscode.pathom.connect/sym a
@@ -2550,7 +2551,8 @@
                  :com.wsscode.pathom.trace/event        :com.wsscode.pathom.connect/call-resolver-with-cache
                  :key                                   :b}
                 {:com.wsscode.pathom.core/cache-key [a->b
-                                                     {:a 3}]
+                                                     {:a 3}
+                                                     nil]
                  :com.wsscode.pathom.core/path      [:b]
                  :com.wsscode.pathom.trace/event    :com.wsscode.pathom.core/cache-miss}
                 {:com.wsscode.pathom.connect/input-data {:a 3}
@@ -2578,11 +2580,14 @@
                                                                                           :response-value {:l "a"}}]}))
          (is (= (into {} (map (fn [[k v]] [k (async/<!! v)])) @cache)
                 '{[i->l
-                   {:i 1}] {:l "a"}
+                   {:i 1}
+                   nil] {:l "a"}
                   [i->l
-                   {:i 2}] {:l "b"}
+                   {:i 2}
+                   nil] {:l "b"}
                   [i->l
-                   {:i 3}] {:l "c"}}))
+                   {:i 3}
+                   nil] {:l "c"}}))
 
          (is (= (comparable-trace @trace)
                 '[{:com.wsscode.pathom.core/path       [:l]
@@ -2606,15 +2611,18 @@
                    :com.wsscode.pathom.core/path     [:l]
                    :com.wsscode.pathom.trace/event   :com.wsscode.pathom.connect/batch-items-ready}
                   {:com.wsscode.pathom.core/cache-key [i->l
-                                                       {:i 1}]
+                                                       {:i 1}
+                                                       nil]
                    :com.wsscode.pathom.core/path      [:l]
                    :com.wsscode.pathom.trace/event    :com.wsscode.pathom.core/cache-miss}
                   {:com.wsscode.pathom.core/cache-key [i->l
-                                                       {:i 2}]
+                                                       {:i 2}
+                                                       nil]
                    :com.wsscode.pathom.core/path      [:l]
                    :com.wsscode.pathom.trace/event    :com.wsscode.pathom.core/cache-miss}
                   {:com.wsscode.pathom.core/cache-key [i->l
-                                                       {:i 3}]
+                                                       {:i 3}
+                                                       nil]
                    :com.wsscode.pathom.core/path      [:l]
                    :com.wsscode.pathom.trace/event    :com.wsscode.pathom.core/cache-miss}
                   {:com.wsscode.pathom.connect/input-data [{:i 1}
@@ -2849,11 +2857,14 @@
                                                                                           :response-value {:error-batch :com.wsscode.pathom.core/reader-error}}]}))
          (is (= (into {} (map (fn [[k v]] [k (async/<!! v)])) @cache)
                 '{[error-batch
-                   {:i 1}] {:error-batch :com.wsscode.pathom.core/reader-error}
+                   {:i 1}
+                   nil] {:error-batch :com.wsscode.pathom.core/reader-error}
                   [error-batch
-                   {:i 2}] {:error-batch :com.wsscode.pathom.core/reader-error}
+                   {:i 2}
+                   nil] {:error-batch :com.wsscode.pathom.core/reader-error}
                   [error-batch
-                   {:i 3}] {:error-batch :com.wsscode.pathom.core/reader-error}}))
+                   {:i 3}
+                   nil] {:error-batch :com.wsscode.pathom.core/reader-error}}))
 
          (is (= @errors {[:list
                           0
@@ -2895,19 +2906,22 @@
                                                       :error-batch]
                    :com.wsscode.pathom.trace/event   :com.wsscode.pathom.connect/batch-items-ready}
                   {:com.wsscode.pathom.core/cache-key [error-batch
-                                                       {:i 1}]
+                                                       {:i 1}
+                                                       nil]
                    :com.wsscode.pathom.core/path      [:list
                                                        0
                                                        :error-batch]
                    :com.wsscode.pathom.trace/event    :com.wsscode.pathom.core/cache-miss}
                   {:com.wsscode.pathom.core/cache-key [error-batch
-                                                       {:i 2}]
+                                                       {:i 2}
+                                                       nil]
                    :com.wsscode.pathom.core/path      [:list
                                                        0
                                                        :error-batch]
                    :com.wsscode.pathom.trace/event    :com.wsscode.pathom.core/cache-miss}
                   {:com.wsscode.pathom.core/cache-key [error-batch
-                                                       {:i 3}]
+                                                       {:i 3}
+                                                       nil]
                    :com.wsscode.pathom.core/path      [:list
                                                        0
                                                        :error-batch]