From 83f948ff6052841ef1f72832f7e17bb44ed60f97 Mon Sep 17 00:00:00 2001
From: dpsutton <dan@dpsutton.com>
Date: Tue, 22 Jun 2021 17:28:44 -0500
Subject: [PATCH] Move *ignore-cached-results* to middleware (#16695)
* Move *ignore-cached-results* to middleware
Had tried to adjust the binding to work, but the binding ended before
all of the bound functions and such were submitted to work, so when
that code actually executed the binding was no longer in
place. Annoying and it should be in middleware anyways
* Remove now unused import (cache became keyword in middleware not binding)
---
.dir-locals.el | 1 +
src/metabase/api/card.clj | 50 ++++++++++---------
.../query_processor/middleware/cache.clj | 14 ++----
.../query_processor/middleware/cache_test.clj | 20 +++-----
4 files changed, 39 insertions(+), 46 deletions(-)
diff --git a/.dir-locals.el b/.dir-locals.el
index 009b7d29608..72cadc8d07d 100644
--- a/.dir-locals.el
+++ b/.dir-locals.el
@@ -36,6 +36,7 @@
(p.types/deftype+ '(2 nil nil (:defn)))
(p/def-map-type '(2 nil nil (:defn)))
(p.types/defrecord+ '(2 nil nil (:defn)))
+ (qp.streaming/streaming-response 1)
(prop/for-all 1)
(tools.macro/macrolet '(1 (:defn))))))
(clojure-indent-style . always-align)
diff --git a/src/metabase/api/card.clj b/src/metabase/api/card.clj
index 0fb644ee563..1bdf01f69cf 100644
--- a/src/metabase/api/card.clj
+++ b/src/metabase/api/card.clj
@@ -25,7 +25,6 @@
[metabase.public-settings :as public-settings]
[metabase.query-processor :as qp]
[metabase.query-processor.async :as qp.async]
- [metabase.query-processor.middleware.cache :as cache]
[metabase.query-processor.middleware.constraints :as constraints]
[metabase.query-processor.middleware.permissions :as qp.perms]
[metabase.query-processor.middleware.results-metadata :as results-metadata]
@@ -615,21 +614,23 @@
be returned as the result of an API endpoint fn. Will throw an Exception if preconditions (such as read perms) are
not met before returning the `StreamingResponse`."
[card-id export-format
- & {:keys [parameters constraints context dashboard-id middleware qp-runner run]
+ & {:keys [parameters constraints context dashboard-id middleware qp-runner run ignore_cache]
:or {constraints constraints/default-query-constraints
context :question
- qp-runner qp/process-query-and-save-execution!
- ;; param `run` can be used to control how the query is ran, e.g. if you need to
- ;; customize the `context` passed to the QP
- run (^:once fn* [query info]
- (qp.streaming/streaming-response [context export-format]
- (binding [qp.perms/*card-id* card-id]
- (qp-runner query info context))))}}]
+ qp-runner qp/process-query-and-save-execution!}}]
{:pre [(u/maybe? sequential? parameters)]}
- (let [card (api/read-check (Card card-id))
+ (let [run (or run
+ ;; param `run` can be used to control how the query is ran, e.g. if you need to
+ ;; customize the `context` passed to the QP
+ (^:once fn* [query info]
+ (qp.streaming/streaming-response [context export-format]
+ (binding [qp.perms/*card-id* card-id]
+ (qp-runner query info context)))))
+ card (api/read-check (Card card-id))
query (-> (assoc (query-for-card card parameters constraints middleware)
:async? true)
- (assoc-in [:middleware :js-int-to-string?] true))
+ (update :middleware merge {:js-int-to-string? true
+ :ignore-cached-results? ignore_cache}))
info {:executed-by api/*current-user-id*
:context context
:card-id card-id
@@ -641,8 +642,7 @@
"Run the query associated with a Card."
[card-id :as {{:keys [parameters ignore_cache], :or {ignore_cache false}} :body}]
{ignore_cache (s/maybe s/Bool)}
- (binding [cache/*ignore-cached-results* ignore_cache]
- (run-query-for-card-async card-id :api, :parameters parameters)))
+ (run-query-for-card-async card-id :api, :parameters parameters, :ignore_cache ignore_cache))
(api/defendpoint ^:streaming POST "/:card-id/query/:export-format"
"Run the query associated with a Card, and return its results as a file in the specified format. Note that this
@@ -650,15 +650,15 @@
[card-id export-format :as {{:keys [parameters]} :params}]
{parameters (s/maybe su/JSONString)
export-format dataset-api/ExportFormat}
- (binding [cache/*ignore-cached-results* true]
- (run-query-for-card-async
- card-id export-format
- :parameters (json/parse-string parameters keyword)
- :constraints nil
- :context (dataset-api/export-format->context export-format)
- :middleware {:skip-results-metadata? true
- :format-rows? false
- :js-int-to-string? false})))
+ (run-query-for-card-async
+ card-id export-format
+ :parameters (json/parse-string parameters keyword)
+ :constraints nil
+ :context (dataset-api/export-format->context export-format)
+ :middleware {:skip-results-metadata? true
+ :ignore-cached-results? true
+ :format-rows? false
+ :js-int-to-string? false}))
;;; ----------------------------------------------- Sharing is Caring ------------------------------------------------
@@ -718,7 +718,9 @@
[card-id :as {{:keys [parameters ignore_cache]
:or {ignore_cache false}} :body}]
{ignore_cache (s/maybe s/Bool)}
- (binding [cache/*ignore-cached-results* ignore_cache]
- (run-query-for-card-async card-id :api, :parameters parameters, :qp-runner qp.pivot/run-pivot-query)))
+ (run-query-for-card-async card-id :api
+ :parameters parameters,
+ :qp-runner qp.pivot/run-pivot-query
+ :ignore_cache ignore_cache))
(api/define-routes)
diff --git a/src/metabase/query_processor/middleware/cache.clj b/src/metabase/query_processor/middleware/cache.clj
index 07b63000251..d0ada61d46c 100644
--- a/src/metabase/query_processor/middleware/cache.clj
+++ b/src/metabase/query_processor/middleware/cache.clj
@@ -33,12 +33,6 @@
[initial-metadata row-1 row-2 ... row-n final-metadata]"
3)
-;; TODO - Why not make this an option in the query itself? :confused:
-(def ^:dynamic ^Boolean *ignore-cached-results*
- "Should we force the query to run, ignoring cached results even if they're available?
- Setting this to `true` will run the query again and will still save the updated results."
- false)
-
(def ^:dynamic *backend*
"Current cache backend. Dynamically rebindable primary for test purposes."
(i/cache-backend (config/config-kw :mb-qp-cache-backend)))
@@ -145,9 +139,9 @@
(defn- maybe-reduce-cached-results
"Reduces cached results if there is a hit. Otherwise, returns `::miss` directly."
- [query-hash max-age-seconds rff context]
+ [ignore-cache? query-hash max-age-seconds rff context]
(try
- (or (when-not *ignore-cached-results*
+ (or (when-not ignore-cache?
(log/tracef "Looking for cached results for query with hash %s younger than %s\n"
(pr-str (i/short-hex-hash query-hash)) (u/format-seconds max-age-seconds))
(i/with-cached-results *backend* query-hash max-age-seconds [is]
@@ -173,11 +167,11 @@
;;; --------------------------------------------------- Middleware ---------------------------------------------------
(defn- run-query-with-cache
- [qp {:keys [cache-ttl], :as query} rff context]
+ [qp {:keys [cache-ttl middleware], :as query} rff context]
;; TODO - Query will already have `info.hash` if it's a userland query. I'm not 100% sure it will be the same hash,
;; because this is calculated after normalization, instead of before
(let [query-hash (qputil/query-hash query)
- result (maybe-reduce-cached-results query-hash cache-ttl rff context)]
+ result (maybe-reduce-cached-results (:ignore-cached-results? middleware) query-hash cache-ttl rff context)]
(when (= result ::miss)
(let [start-time-ms (System/currentTimeMillis)]
(log/trace "Running query and saving cached results (if eligible)...")
diff --git a/test/metabase/query_processor/middleware/cache_test.clj b/test/metabase/query_processor/middleware/cache_test.clj
index e5ba388a007..0d8e2bd7f4f 100644
--- a/test/metabase/query_processor/middleware/cache_test.clj
+++ b/test/metabase/query_processor/middleware/cache_test.clj
@@ -223,24 +223,20 @@
(run-query)))))))
(deftest ignore-cached-results-test
- (testing "check that *ignore-cached-results* is respected when returning results..."
+ (testing "check that :ignore-cached-results? in middleware is respected when returning results..."
(with-mock-cache [save-chan]
- (run-query)
+ (run-query :middleware {:ignore-cached-results? false})
(mt/wait-for-result save-chan)
- (binding [cache/*ignore-cached-results* true]
- (is (= :not-cached
- (run-query)))))))
+ (is (= :not-cached
+ (run-query :middleware {:ignore-cached-results? true}))))))
(deftest ignore-cached-results-should-still-save-test
(testing "...but if it's set those results should still be cached for next time."
(with-mock-cache [save-chan]
- (binding [cache/*ignore-cached-results* true]
- (is (= true
- (cacheable?)))
- (run-query)
- (mt/wait-for-result save-chan))
- (is (= :cached
- (run-query))))))
+ (is (= true (cacheable?)))
+ (run-query :middleware {:ignore-cached-results? true})
+ (mt/wait-for-result save-chan)
+ (is (= :cached (run-query))))))
(deftest min-ttl-test
(testing "if the cache takes less than the min TTL to execute, it shouldn't be cached"
--
GitLab