From 70ea4a0ab0d372288409c3738a7cf2f204541ee2 Mon Sep 17 00:00:00 2001
From: Chris Truter <crisptrutski@users.noreply.github.com>
Date: Thu, 14 Nov 2024 11:02:11 +0200
Subject: [PATCH] Fix query building for related updates (#50001)

---
 src/metabase/search/postgres/ingestion.clj    | 17 +++++++++++-
 src/metabase/search/spec.clj                  | 26 ++++++++++++++-----
 .../search/postgres/ingestion_test.clj        | 19 ++++++++++++++
 3 files changed, 54 insertions(+), 8 deletions(-)
 create mode 100644 test/metabase/search/postgres/ingestion_test.clj

diff --git a/src/metabase/search/postgres/ingestion.clj b/src/metabase/search/postgres/ingestion.clj
index afe81fd06143e..9afdf3b00d4fb 100644
--- a/src/metabase/search/postgres/ingestion.clj
+++ b/src/metabase/search/postgres/ingestion.clj
@@ -139,10 +139,25 @@
      (doseq [update updates]
        (queue/put-with-delay! queue delay-ms update)))))
 
+(defn- impossible-condition?
+  "An (incomplete) check where queries will definitely return nothing, to help avoid spurious index update queries."
+  [where]
+  (when (vector? where)
+    (case (first where)
+      :=   (let [[a b] (rest where)]
+             (and (string? a) (string? b) (not= a b)))
+      :!=  (let [[a b] (rest where)]
+             (and (string? a) (string? b) (= a b)))
+      :and (boolean (some impossible-condition? (rest where)))
+      :or  (every? impossible-condition? (rest where))
+      false)))
+
 (defn update-index!
   "Given a new or updated instance, create or update all the corresponding search entries if needed."
   [instance & [always?]]
-  (when-let [updates (seq (search.spec/search-models-to-update instance always?))]
+  (when-let [updates (->> (search.spec/search-models-to-update instance always?)
+                          (remove (comp impossible-condition? second))
+                          seq)]
     ;; We need to delay execution to handle deletes, which alert us *before* updating the database.
     (ingest-maybe-async! updates)
     nil))
diff --git a/src/metabase/search/spec.clj b/src/metabase/search/spec.clj
index 48387e284eb73..973a5366c693f 100644
--- a/src/metabase/search/spec.clj
+++ b/src/metabase/search/spec.clj
@@ -7,7 +7,8 @@
    [malli.error :as me]
    [metabase.config :as config]
    [metabase.util :as u]
-   [toucan2.core :as t2]))
+   [toucan2.core :as t2]
+   [toucan2.tools.transformed :as t2.transformed]))
 
 (def ^:private SearchModel
   [:enum "dashboard" "table" "dataset" "segment" "collection" "database" "action" "indexed-entity" "metric" "card"])
@@ -259,15 +260,26 @@
    (for [[search-model spec-fn] (methods spec)]
      (search-model-hooks (spec-fn search-model)))))
 
+(defn- instance->db-values
+  "Given a transformed toucan map, get back a mapping to the raw db values that we can use in a query."
+  [instance]
+  (let [xforms (#'t2.transformed/in-transforms (t2/model instance))]
+    (reduce-kv
+     (fn [m k v]
+       (assoc m k (if-let [f (get xforms k)] (f v) v)))
+     {}
+     instance)))
+
 (defn search-models-to-update
   "Given an updated or created instance, return a description of which search-models to (re)index."
   [instance & [always?]]
-  (into #{}
-        (keep
-         (fn [{:keys [search-model fields where]}]
-           (when (or always? (and fields (some fields (keys (or (t2/changes instance) instance)))))
-             [search-model (insert-values where :updated instance)])))
-        (get (model-hooks) (t2/model instance))))
+  (let [raw-values (delay (instance->db-values instance))]
+    (into #{}
+          (keep
+           (fn [{:keys [search-model fields where]}]
+             (when (or always? (and fields (some fields (keys (or (t2/changes instance) instance)))))
+               [search-model (insert-values where :updated @raw-values)])))
+          (get (model-hooks) (t2/model instance)))))
 
 (comment
   (doseq [d (descendants :hook/search-index)]
diff --git a/test/metabase/search/postgres/ingestion_test.clj b/test/metabase/search/postgres/ingestion_test.clj
new file mode 100644
index 0000000000000..75564c3433cbe
--- /dev/null
+++ b/test/metabase/search/postgres/ingestion_test.clj
@@ -0,0 +1,19 @@
+(ns metabase.search.postgres.ingestion-test
+  (:require
+   [clojure.test :refer :all]
+   [metabase.search.postgres.ingestion :as search.ingestion]))
+
+(def ^:private impossible-condition? #'search.ingestion/impossible-condition?)
+
+(deftest impossible-condition?-test
+  (is (not (impossible-condition? [:= "card" :this.type])))
+  (is (not (impossible-condition? [:= :that.type :this.type])))
+  (is (impossible-condition? [:= "card" "dashboard"]))
+  (is (not (impossible-condition? [:= "card" "card"])))
+  (is (not (impossible-condition? [:!= "card" "dashboard"])))
+  (is (impossible-condition? [:!= "card" "card"]))
+  (is (not (impossible-condition? [:and [:= 1 :this.id] [:= "card" :this.type]])))
+  (is (impossible-condition? [:and [:= 1 :this.id] [:!= "card" "card"]]))
+  (is (not (impossible-condition? [:and [:= 1 :this.id] [:!= "card" "dashboard"]])))
+  (is (not (impossible-condition? [:or [:= 1 :this.id] [:!= "card" "dashboard"]])))
+  (is (impossible-condition? [:or [:= "oh" "no"] [:= "card" "dashboard"]])))