From ce5f60b7cae904247959ae4f8bbc931bbc5a9a1d Mon Sep 17 00:00:00 2001
From: Chris Truter <crisptrutski@users.noreply.github.com>
Date: Tue, 26 Nov 2024 16:50:12 +0200
Subject: [PATCH] Enable New Search tests in CI (#50386)
---
src/metabase/api/search.clj | 4 ++-
src/metabase/cmd/load_from_h2.clj | 8 +++--
src/metabase/public_settings.clj | 2 +-
src/metabase/search/impl.clj | 5 ++--
src/metabase/search/postgres/index.clj | 31 +++++++++++++++-----
src/metabase/search/postgres/ingestion.clj | 13 +++++---
test/metabase/api/search_test.clj | 18 ++++++++----
test/metabase/cmd/load_from_h2_test.clj | 4 ++-
test/metabase/db/custom_migrations_test.clj | 6 ++++
test/metabase/db/schema_migrations_test.clj | 6 ++++
test/metabase/search/postgres/index_test.clj | 20 ++++++++-----
11 files changed, 85 insertions(+), 32 deletions(-)
diff --git a/src/metabase/api/search.clj b/src/metabase/api/search.clj
index c7e785147ff..9c2ee04945b 100644
--- a/src/metabase/api/search.clj
+++ b/src/metabase/api/search.clj
@@ -6,6 +6,7 @@
[compojure.core :refer [GET]]
[java-time.api :as t]
[metabase.api.common :as api]
+ [metabase.config :as config]
[metabase.public-settings :as public-settings]
[metabase.public-settings.premium-features :as premium-features]
[metabase.search :as search]
@@ -68,7 +69,8 @@
(throw (ex-info "Search index is not enabled." {:status-code 501}))
(search/supports-index?)
- (if (task/job-exists? task.search-index/reindex-job-key)
+ ;; The job appears to wait on the main thread when run from tests, so, unfortunately, testing this branch is hard.
+ (if (and (task/job-exists? task.search-index/reindex-job-key) (not config/is-test?))
(do (task/trigger-now! task.search-index/reindex-job-key) {:message "task triggered"})
(do (task.search-index/reindex!) {:message "done"}))
diff --git a/src/metabase/cmd/load_from_h2.clj b/src/metabase/cmd/load_from_h2.clj
index 2fc32182fc5..0577b170bc2 100644
--- a/src/metabase/cmd/load_from_h2.clj
+++ b/src/metabase/cmd/load_from_h2.clj
@@ -17,10 +17,13 @@
# MySQL
mysql -u root -e 'DROP DATABASE IF EXISTS metabase; CREATE DATABASE metabase;'
MB_DB_TYPE=mysql MB_DB_HOST=localhost MB_DB_PORT=3305 MB_DB_USER=root MB_DB_DBNAME=metabase clojure -M:run load-from-h2"
+ ;; TODO pending a reorganization of the search namespaces, to expose this properly
+ #_{:clj-kondo/ignore [:metabase/ns-module-checker]}
(:require
[metabase.cmd.copy :as copy]
[metabase.cmd.copy.h2 :as copy.h2]
- [metabase.db :as mdb]))
+ [metabase.db :as mdb]
+ [metabase.search.postgres.index :as search.index]))
(defn load-from-h2!
"Transfer data from existing H2 database to a newly created (presumably MySQL or Postgres) DB. Intended as a tool for
@@ -32,4 +35,5 @@
([h2-filename]
(let [h2-filename (str h2-filename ";IFEXISTS=TRUE")
h2-data-source (copy.h2/h2-data-source h2-filename)]
- (copy/copy! :h2 h2-data-source (mdb/db-type) (mdb/data-source)))))
+ (copy/copy! :h2 h2-data-source (mdb/db-type) (mdb/data-source))
+ (search.index/reset-tracking!))))
diff --git a/src/metabase/public_settings.clj b/src/metabase/public_settings.clj
index d58aeb7c268..903060c7437 100644
--- a/src/metabase/public_settings.clj
+++ b/src/metabase/public_settings.clj
@@ -1068,7 +1068,7 @@ See [fonts](../configuring-metabase/fonts.md).")
(deferred-tru "Enables search engines which are still in the experimental stage")
:visibility :internal
:export? false
- :default false
+ :default (not config/is-prod?)
:type :boolean)
(defsetting experimental-search-weight-overrides
diff --git a/src/metabase/search/impl.clj b/src/metabase/search/impl.clj
index cfc6a65a0b6..1fdba4171fa 100644
--- a/src/metabase/search/impl.clj
+++ b/src/metabase/search/impl.clj
@@ -2,6 +2,7 @@
(:require
[cheshire.core :as json]
[clojure.string :as str]
+ [metabase.config :as config]
[metabase.db :as mdb]
[metabase.legacy-mbql.normalize :as mbql.normalize]
[metabase.lib.core :as lib]
@@ -11,7 +12,6 @@
[metabase.models.database :as database]
[metabase.models.interface :as mi]
[metabase.permissions.util :as perms.u]
- [metabase.public-settings :as public-settings]
[metabase.public-settings.premium-features :as premium-features]
[metabase.search.api :as search.api]
[metabase.search.config
@@ -212,7 +212,8 @@
(defmethod supported-engine? :search.engine/fulltext [_] (search.fulltext/supported-db? (mdb/db-type)))
(defn- default-engine []
- (if (public-settings/experimental-fulltext-search-enabled)
+ ;; The API tests have not yet been ported to reflect the new search's results.
+ (if (and (supported-engine? :search.engine/fulltext) (not config/is-test?))
:search.engine/fulltext
:search.engine/in-place))
diff --git a/src/metabase/search/postgres/index.clj b/src/metabase/search/postgres/index.clj
index 2b6ac10ac40..335964385e5 100644
--- a/src/metabase/search/postgres/index.clj
+++ b/src/metabase/search/postgres/index.clj
@@ -5,7 +5,8 @@
[honey.sql.helpers :as sql.helpers]
[metabase.search.spec :as search.spec]
[metabase.util :as u]
- [toucan2.core :as t2]))
+ [toucan2.core :as t2])
+ (:import (org.postgresql.util PSQLException)))
(def ^:dynamic *active-table*
"The table against which we should currently make search queries. Dynamic for testing."
@@ -19,6 +20,13 @@
(defonce ^:private reindexing? (atom false))
+(defn reset-tracking!
+ "Remove assumptions about search indexing state. Not particularly safe, but only used in tests."
+ []
+ ;; TODO we'll make this safe when we switch to the metadata table.
+ (reset! initialized? false)
+ (reset! reindexing? false))
+
(def ^:private tsv-language "english")
(defn- random-prefix []
@@ -244,15 +252,24 @@
(str/join " | ")
maybe-complete)))))
+(defn- safe-batch-upsert! [table-name entries]
+ (try
+ (batch-upsert! table-name entries)
+ (catch Exception e
+ ;; ignore database errors, the table likely doesn't exist, or has a stale schema.
+ (when-not (instance? PSQLException (ex-cause e))
+ (throw e)))))
+
(defn batch-update!
"Create the given search index entries in bulk"
[entities]
- (let [entries (map entity->entry entities)]
- (when @initialized?
- (batch-upsert! *active-table* entries))
- (when @reindexing?
- (batch-upsert! pending-table entries))
- (->> entries (map :model) frequencies)))
+ (let [entries (map entity->entry entities)
+ ;; Ideally, we would reset these atoms if the corresponding tables don't exist.
+ ;; We're about to rework this area, so just leaving this as a note for now.
+ active-updated? (when @initialized? (safe-batch-upsert! *active-table* entries))
+ pending-updated? (when @reindexing? (safe-batch-upsert! pending-table entries))]
+ (when (or active-updated? pending-updated?)
+ (->> entries (map :model) frequencies))))
(defn search-query
"Query fragment for all models corresponding to a query parameter `:search-term`."
diff --git a/src/metabase/search/postgres/ingestion.clj b/src/metabase/search/postgres/ingestion.clj
index 8a412899abe..8d3bb69e082 100644
--- a/src/metabase/search/postgres/ingestion.clj
+++ b/src/metabase/search/postgres/ingestion.clj
@@ -101,6 +101,10 @@
"Force ingestion to happen immediately, on the same thread."
false)
+(def ^:dynamic *disable-updates*
+ "Used by tests to disable updates, for example when testing migrations, where the schema is wrong."
+ false)
+
(defn- bulk-ingest! [updates]
(->> (for [[search-model where-clauses] (u/group-by first second updates)]
(spec-index-reducible search-model (into [:or] (distinct where-clauses))))
@@ -126,10 +130,11 @@
([updates]
(ingest-maybe-async! updates (or *force-sync* (not (index-worker-exists?)))))
([updates sync?]
- (if sync?
- (bulk-ingest! updates)
- (doseq [update updates]
- (queue/put-with-delay! queue delay-ms update)))))
+ (when-not *disable-updates*
+ (if sync?
+ (bulk-ingest! updates)
+ (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."
diff --git a/test/metabase/api/search_test.clj b/test/metabase/api/search_test.clj
index 3745c4cb50a..418a2318ac9 100644
--- a/test/metabase/api/search_test.clj
+++ b/test/metabase/api/search_test.clj
@@ -25,6 +25,7 @@
[metabase.search.config :as search.config]
[metabase.search.fulltext :as search.fulltext]
[metabase.search.in-place.scoring :as scoring]
+ [metabase.search.postgres.index :as search.index]
[metabase.test :as mt]
[metabase.util :as u]
[toucan2.core :as t2]
@@ -312,6 +313,7 @@
(deftest custom-engine-test
(when (search/supports-index?)
(testing "It can use an alternate search engine"
+ (is (search.index/ensure-ready! false))
(with-search-items-in-root-collection "test"
(let [resp (search-request :crowberto :q "test" :search_engine "fulltext" :limit 1)]
;; The index is not populated here, so there's not much interesting to assert.
@@ -1579,14 +1581,18 @@
(deftest force-reindex-test
(when (search/supports-index?)
(mt/with-temp [Card {id :id} {:name "It boggles the mind!"}]
- (let [search-results #(:data (mt/user-http-request :rasta :get 200 "search" :q "boggle" :search_engine "fulltext"))]
- (try
- (t2/delete! :search_index)
- (catch Exception _))
- (is (empty? (search-results)))
+ (mt/user-http-request :crowberto :post 200 "search/re-init")
+ (let [search-results #(:data (mt/user-http-request :rasta :get % "search" :q "boggle" :search_engine "fulltext"))]
+ (is (try
+ (t2/delete! :search_index)
+ (catch Exception _
+ :already-deleted)))
+ (is (empty? (search-results 200)))
(mt/user-http-request :crowberto :post 200 "search/force-reindex")
(is (loop [attempts-left 5]
- (if (some (comp #{id} :id) (search-results))
+ (if (and (#'search.index/exists? :search_index)
+ (pos? (t2/count :search_index))
+ (some (comp #{id} :id) (search-results 200)))
::success
(when (pos? attempts-left)
(Thread/sleep 200)
diff --git a/test/metabase/cmd/load_from_h2_test.clj b/test/metabase/cmd/load_from_h2_test.clj
index 23c74a7ec93..448bdbdbd0b 100644
--- a/test/metabase/cmd/load_from_h2_test.clj
+++ b/test/metabase/cmd/load_from_h2_test.clj
@@ -12,6 +12,7 @@
[metabase.driver :as driver]
[metabase.driver.sql-jdbc.connection :as sql-jdbc.conn]
[metabase.models :refer [Table]]
+ [metabase.search.postgres.index :as search.index]
[metabase.test :as mt]
[metabase.test.data.interface :as tx]
[metabase.util.log :as log]
@@ -68,7 +69,8 @@
[db-type db-def data-source]
(tx/destroy-db! db-type db-def)
(tx/create-db! db-type db-def)
- (mdb.setup/setup-db! db-type data-source true false))
+ (mdb.setup/setup-db! db-type data-source true false)
+ (search.index/reset-tracking!))
(defn- dump-filename
[h2-filename version]
diff --git a/test/metabase/db/custom_migrations_test.clj b/test/metabase/db/custom_migrations_test.clj
index bc836c6db2a..a254c4f82b4 100644
--- a/test/metabase/db/custom_migrations_test.clj
+++ b/test/metabase/db/custom_migrations_test.clj
@@ -24,6 +24,7 @@
[metabase.models.permissions-group :as perms-group]
[metabase.models.pulse-channel-test :as pulse-channel-test]
[metabase.models.setting :as setting]
+ [metabase.search.postgres.ingestion :as search.ingestion]
[metabase.task :as task]
[metabase.task.send-pulses :as task.send-pulses]
[metabase.task.sync-databases-test :as task.sync-databases-test]
@@ -40,6 +41,11 @@
(use-fixtures :once (fixtures/initialize :db))
+;; Disable the search index, as older schemas may not be compatible with ingestion.
+(use-fixtures :each (fn [thunk]
+ (binding [search.ingestion/*disable-updates* true]
+ (thunk))))
+
(jobs/defjob AbandonmentEmail [_] :default)
(defn- table-default [table]
diff --git a/test/metabase/db/schema_migrations_test.clj b/test/metabase/db/schema_migrations_test.clj
index 204b8e09d4d..d081696d530 100644
--- a/test/metabase/db/schema_migrations_test.clj
+++ b/test/metabase/db/schema_migrations_test.clj
@@ -40,6 +40,7 @@
[metabase.models.collection :as collection]
[metabase.models.permissions :as perms]
[metabase.models.permissions-group :as perms-group]
+ [metabase.search.postgres.ingestion :as search.ingestion]
[metabase.test :as mt]
[metabase.test.data.env :as tx.env]
[metabase.test.fixtures :as fixtures]
@@ -52,6 +53,11 @@
(use-fixtures :once (fixtures/initialize :db))
+;; Disable the search index, as older schemas may not be compatible with ingestion.
+(use-fixtures :each (fn [thunk]
+ (binding [search.ingestion/*disable-updates* true]
+ (thunk))))
+
(deftest rollback-test
(testing "Migrating to latest version, rolling back to v44, and then migrating up again"
;; using test-migrations to excercise all drivers
diff --git a/test/metabase/search/postgres/index_test.clj b/test/metabase/search/postgres/index_test.clj
index f41c12dc514..148390a5959 100644
--- a/test/metabase/search/postgres/index_test.clj
+++ b/test/metabase/search/postgres/index_test.clj
@@ -15,6 +15,10 @@
(defn- index-hits [term]
(count (search.index/search term)))
+(defn- now []
+ ;; Truncate to milliseconds as precision may be lost when roundtripping to the database.
+ (t/truncate-to (t/offset-date-time) :millis))
+
;; These helpers only mutate the temp local AppDb.
#_{:clj-kondo/ignore [:metabase/test-helpers-use-non-thread-safe-functions]}
(defmacro with-index
@@ -211,7 +215,7 @@
(doseq [[model-type card-type] [["card" "question"] ["dataset" "model"] ["metric" "metric"]]]
(testing (format "simple %s" model-type)
(let [card-name (mt/random-name)
- yesterday (t/- (t/offset-date-time) (t/days 1))]
+ yesterday (t/- (now) (t/days 1))]
(mt/with-temp [:model/Card {card-id :id} {:name card-name
:type card-type
:created_at yesterday
@@ -237,7 +241,7 @@
(testing (format "everything %s" model-type)
(let [card-name (mt/random-name)
- yesterday (t/- (t/offset-date-time) (t/days 1))
+ yesterday (t/- (now) (t/days 1))
two-days-ago (t/- yesterday (t/days 1))]
(mt/with-temp
[:model/Collection {coll-id :id} {:name "My collection"
@@ -301,7 +305,7 @@
(deftest database-ingestion-test
(search.tu/with-temp-index-table
(let [db-name (mt/random-name)
- yesterday (t/- (t/offset-date-time) (t/days 1))]
+ yesterday (t/- (now) (t/days 1))]
(mt/with-temp [:model/Database {db-id :id} {:name db-name
:created_at yesterday
:updated_at yesterday}]
@@ -316,7 +320,7 @@
(deftest table-ingestion-test
(search.tu/with-temp-index-table
(let [table-name (mt/random-name)
- yesterday (t/- (t/offset-date-time) (t/days 1))]
+ yesterday (t/- (now) (t/days 1))]
(mt/with-temp [:model/Database {db-id :id} {}
:model/Table {table-id :id} {:name table-name
;; :view_count = 42
@@ -340,7 +344,7 @@
(deftest collection-ingestion-test
(search.tu/with-temp-index-table
(let [collection-name (mt/random-name)
- yesterday (t/- (t/offset-date-time) (t/days 1))]
+ yesterday (t/- (now) (t/days 1))]
(mt/with-temp [:model/Collection {coll-id :id} {:name collection-name
;; :authority_level = "official"
:authority_level "official"
@@ -362,7 +366,7 @@
(deftest action-ingestion-test
(search.tu/with-temp-index-table
(let [action-name (mt/random-name)
- yesterday (t/- (t/offset-date-time) (t/days 1))]
+ yesterday (t/- (now) (t/days 1))]
(mt/with-temp [:model/Database {db-id :id} {}
:model/Collection {coll-id :id} {}
:model/Card {model-id :id} {:type "model"
@@ -396,7 +400,7 @@
(deftest dashboard-ingestion-test
(search.tu/with-temp-index-table
(let [dashboard-name (mt/random-name)
- yesterday (t/- (t/offset-date-time) (t/days 1))
+ yesterday (t/- (now) (t/days 1))
two-days-ago (t/- yesterday (t/days 1))]
(mt/with-temp [:model/Dashboard {dashboard-id :id} {:name dashboard-name
;; :model_created_at = yesterday
@@ -434,7 +438,7 @@
(deftest segment-ingestion-test
(search.tu/with-temp-index-table
(let [segment-name (mt/random-name)
- yesterday (t/- (t/offset-date-time) (t/days 1))]
+ yesterday (t/- (now) (t/days 1))]
(mt/with-temp [:model/Database {db-id :id} {}
:model/Table {table-id :id} {:db_id db-id}
:model/Segment {segment-id :id} {:name segment-name
--
GitLab