From daed46d5b2239af7eb8a2caa7096b0b07a9fa81a Mon Sep 17 00:00:00 2001
From: dpsutton <dan@dpsutton.com>
Date: Mon, 1 Nov 2021 15:44:21 -0500
Subject: [PATCH] Dataset search (#18715)
* Move the multimethods over to strings not classes
datasets are Cards so don't have a good way to intercept. And this kinda
took all of the keywords, turned them into classes, and then didn't
actually use those classes-- they were just opaque keys in the
hierarchy. No bueno.
* Include datasets in search results
* Docstring for the linter. Thank you clj-kondo
* Fix namespace decls
* Update tests
* Clean up ns and make test more resilient
these tests that test global collections can be sensitive to test order
and the CI dbs that continually exist rather than be
recreated. Sometimes "table" shows up in these available models. Kinda
don't care so just check for subset
* Rename for confusion
---
src/metabase/api/search.clj | 118 +++++++++++++++---------------
src/metabase/search/config.clj | 67 +++++++++--------
src/metabase/search/scoring.clj | 7 +-
test/metabase/api/search_test.clj | 48 +++++++-----
4 files changed, 124 insertions(+), 116 deletions(-)
diff --git a/src/metabase/api/search.clj b/src/metabase/api/search.clj
index 27530ba686f..9f2fd473d39 100644
--- a/src/metabase/api/search.clj
+++ b/src/metabase/api/search.clj
@@ -7,16 +7,12 @@
[honeysql.helpers :as h]
[metabase.api.common :as api]
[metabase.db :as mdb]
- [metabase.models.card :refer [Card]]
[metabase.models.card-favorite :refer [CardFavorite]]
[metabase.models.collection :as coll :refer [Collection]]
- [metabase.models.dashboard :refer [Dashboard]]
[metabase.models.dashboard-favorite :refer [DashboardFavorite]]
- [metabase.models.database :refer [Database]]
[metabase.models.interface :as mi]
[metabase.models.metric :refer [Metric]]
[metabase.models.permissions :as perms]
- [metabase.models.pulse :refer [Pulse]]
[metabase.models.segment :refer [Segment]]
[metabase.models.table :refer [Table]]
[metabase.search.config :as search-config]
@@ -39,7 +35,10 @@
(s/optional-key :offset-int) (s/maybe s/Int)})
(def ^:private SearchableModel
- (apply s/enum search-config/searchable-models))
+ (apply s/enum search-config/all-models))
+
+(def ^:private DBModel
+ (apply s/enum search-config/searchable-db-models))
(def ^:private HoneySQLColumn
(s/cond-pre
@@ -65,7 +64,7 @@
are cast to the appropriate type because Postgres will assume `SELECT NULL` is `TEXT` by default and will refuse to
`UNION` two columns of two different types.)"
(ordered-map/ordered-map
- ;; returned for all models
+ ;; returned for all models. Important to be first for changing model for dataset
:model :text
:id :integer
:name :text
@@ -98,7 +97,7 @@
(s/defn ^:private model->alias :- s/Keyword
[model :- SearchableModel]
- (keyword (str/lower-case (name model))))
+ (keyword model))
(s/defn ^:private ->column-alias :- s/Keyword
"Returns the column name. If the column is aliased, i.e. [`:original_name` `:aliased_name`], return the aliased
@@ -117,7 +116,7 @@
:let [maybe-aliased-col (get col-alias->honeysql-clause search-col)]]
(cond
(= search-col :model)
- [(hx/literal (name (model->alias model))) :model]
+ [(hx/literal model) :model]
;; This is an aliased column, no need to include the table alias
(sequential? maybe-aliased-col)
@@ -148,26 +147,27 @@
cols-or-nils (canonical-columns model column-alias->honeysql-clause)]
cols-or-nils))
-(s/defn ^:private from-clause-for-model :- [(s/one [(s/one SearchableModel "model") (s/one s/Keyword "alias")]
+(s/defn ^:private from-clause-for-model :- [(s/one [(s/one DBModel "model") (s/one s/Keyword "alias")]
"from clause")]
[model :- SearchableModel]
- [[model (model->alias model)]])
+ (let [db-model (get search-config/model-to-db-model model)]
+ [[db-model (-> db-model name str/lower-case keyword)]]))
(defmulti ^:private archived-where-clause
{:arglists '([model archived?])}
- (fn [model _] (class model)))
+ (fn [model _] model))
(defmethod archived-where-clause :default
[model archived?]
[:= (hsql/qualify (model->alias model) :archived) archived?])
;; Databases can't be archived
-(defmethod archived-where-clause (class Database)
+(defmethod archived-where-clause "database"
[model archived?]
[:= 1 (if archived? 2 1)])
;; Table has an `:active` flag, but no `:archived` flag; never return inactive Tables
-(defmethod archived-where-clause (class Table)
+(defmethod archived-where-clause "table"
[model archived?]
(if archived?
[:= 1 0] ; No tables should appear in archive searches
@@ -246,11 +246,11 @@
(defmulti ^:private search-query-for-model
{:arglists '([model search-context])}
- (fn [model _] (class model)))
+ (fn [model _] model))
-(s/defmethod search-query-for-model (class Card)
- [_ search-ctx :- SearchContext]
- (-> (base-query-for-model Card search-ctx)
+(s/defn ^:private shared-card-impl [dataset? :- s/Bool search-ctx :- SearchContext]
+ (-> (base-query-for-model "card" search-ctx)
+ (update :where (fn [where] [:and [:= :card.dataset dataset?] where]))
(h/left-join [CardFavorite :fave]
[:and
[:= :card.id :fave.card_id]
@@ -258,48 +258,58 @@
(add-collection-join-and-where-clauses :card.collection_id search-ctx)
(add-card-db-id-clause (:table-db-id search-ctx))))
-(s/defmethod search-query-for-model (class Collection)
- [_ search-ctx :- SearchContext]
- (-> (base-query-for-model Collection search-ctx)
+(s/defmethod search-query-for-model "card"
+ [_model search-ctx :- SearchContext]
+ (shared-card-impl false search-ctx))
+
+(s/defmethod search-query-for-model "dataset"
+ [_model search-ctx :- SearchContext]
+ (-> (shared-card-impl true search-ctx)
+ (update :select (fn [columns]
+ (cons [(hx/literal "dataset") :model] (rest columns))))))
+
+(s/defmethod search-query-for-model "collection"
+ [model search-ctx :- SearchContext]
+ (-> (base-query-for-model model search-ctx)
(add-collection-join-and-where-clauses :collection.id search-ctx)))
-(s/defmethod search-query-for-model (class Database)
- [_ search-ctx :- SearchContext]
- (base-query-for-model Database search-ctx))
+(s/defmethod search-query-for-model "database"
+ [model search-ctx :- SearchContext]
+ (base-query-for-model model search-ctx))
-(s/defmethod search-query-for-model (class Dashboard)
- [_ search-ctx :- SearchContext]
- (-> (base-query-for-model Dashboard search-ctx)
+(s/defmethod search-query-for-model "dashboard"
+ [model search-ctx :- SearchContext]
+ (-> (base-query-for-model model search-ctx)
(h/left-join [DashboardFavorite :fave]
[:and
[:= :dashboard.id :fave.dashboard_id]
[:= :fave.user_id api/*current-user-id*]])
(add-collection-join-and-where-clauses :dashboard.collection_id search-ctx)))
-(s/defmethod search-query-for-model (class Pulse)
- [_ search-ctx :- SearchContext]
+(s/defmethod search-query-for-model "pulse"
+ [model search-ctx :- SearchContext]
;; Pulses don't currently support being archived, omit if archived is true
- (-> (base-query-for-model Pulse search-ctx)
+ (-> (base-query-for-model model search-ctx)
(add-collection-join-and-where-clauses :pulse.collection_id search-ctx)
;; We don't want alerts included in pulse results
(h/merge-where [:and
[:= :alert_condition nil]
[:= :pulse.dashboard_id nil]])))
-(s/defmethod search-query-for-model (class Metric)
- [_ search-ctx :- SearchContext]
- (-> (base-query-for-model Metric search-ctx)
+(s/defmethod search-query-for-model "metric"
+ [model search-ctx :- SearchContext]
+ (-> (base-query-for-model model search-ctx)
(h/left-join [Table :table] [:= :metric.table_id :table.id])))
-(s/defmethod search-query-for-model (class Segment)
- [_ search-ctx :- SearchContext]
- (-> (base-query-for-model Segment search-ctx)
+(s/defmethod search-query-for-model "segment"
+ [model search-ctx :- SearchContext]
+ (-> (base-query-for-model model search-ctx)
(h/left-join [Table :table] [:= :segment.table_id :table.id])))
-(s/defmethod search-query-for-model (class Table)
- [_ {:keys [current-user-perms table-db-id], :as search-ctx} :- SearchContext]
+(s/defmethod search-query-for-model "table"
+ [model {:keys [current-user-perms table-db-id], :as search-ctx} :- SearchContext]
(when (seq current-user-perms)
- (let [base-query (base-query-for-model Table search-ctx)]
+ (let [base-query (base-query-for-model model search-ctx)]
(add-table-db-id-clause
(if (contains? current-user-perms "/")
base-query
@@ -355,18 +365,12 @@
[{:keys [id]}]
(-> id Segment mi/can-read?))
-(defn- models-to-search
- [{:keys [models]} default]
- (if models
- (vec (map search-config/model-name->instance models))
- default))
-
(defn- query-model-set
"Queries all models with respect to query for one result, to see if we get a result or not"
[search-ctx]
(map #((first %) :model)
(filter not-empty
- (for [model search-config/searchable-models]
+ (for [model search-config/all-models]
(let [search-query (search-query-for-model model search-ctx)
query-with-limit (h/limit search-query 1)]
(db/query query-with-limit))))))
@@ -376,7 +380,8 @@
to make the union-all degenerate down to trivial case of one model without errors.
Therefore, we degenerate it down for it"
[search-ctx]
- (let [models (models-to-search search-ctx search-config/searchable-models)
+ (let [models (or (:models search-ctx)
+ search-config/all-models)
sql-alias :alias_is_required_by_sql_but_not_needed_here
order-clause [((fnil order-clause "") (:search-string search-ctx))]]
(if (= (count models) 1)
@@ -388,7 +393,6 @@
query)} sql-alias]]
:order-by order-clause})))
-
(s/defn ^:private search
"Builds a search query that includes all of the searchable entities and runs it"
[search-ctx :- SearchContext]
@@ -410,15 +414,15 @@
;; We get to do this slicing and dicing with the result data because
;; the pagination of search is for UI improvement, not for performance.
;; We intend for the cardinality of the search results to be below the default max before this slicing occurs
- { :total (count total-results)
- :data (cond->> total-results
- (some? (:offset-int search-ctx)) (drop (:offset-int search-ctx))
- (some? (:limit-int search-ctx)) (take (:limit-int search-ctx)))
- :available_models (query-model-set search-ctx)
- :limit (:limit-int search-ctx)
- :offset (:offset-int search-ctx)
- :table_db_id (:table-db-id search-ctx)
- :models (:models search-ctx)})))
+ {:total (count total-results)
+ :data (cond->> total-results
+ (some? (:offset-int search-ctx)) (drop (:offset-int search-ctx))
+ (some? (:limit-int search-ctx)) (take (:limit-int search-ctx)))
+ :available_models (query-model-set search-ctx)
+ :limit (:limit-int search-ctx)
+ :offset (:offset-int search-ctx)
+ :table_db_id (:table-db-id search-ctx)
+ :models (:models search-ctx)})))
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | Endpoint |
@@ -449,7 +453,7 @@
(api/defendpoint GET "/"
"Search within a bunch of models for the substring `q`.
- For the list of models, check `metabase.search.config/searchable-models.
+ For the list of models, check `metabase.search.config/all-models.
To search in archived portions of models, pass in `archived=true`.
If you want, while searching tables, only tables of a certain DB id,
diff --git a/src/metabase/search/config.clj b/src/metabase/search/config.clj
index fb1f421ff5b..d73e989885f 100644
--- a/src/metabase/search/config.clj
+++ b/src/metabase/search/config.clj
@@ -1,6 +1,5 @@
(ns metabase.search.config
(:require [cheshire.core :as json]
- [clojure.string :as str]
[honeysql.core :as hsql]
[metabase.models :refer [Card Collection Dashboard Database Metric Pulse Segment Table]]
[metabase.models.setting :refer [defsetting]]
@@ -37,26 +36,26 @@
"Show this many words of context before/after matches in long search results"
2)
-(def searchable-models
- "Models that can be searched. The order of this list also influences the order of the results: items earlier in the
+(def searchable-db-models
+ "Models that can be searched."
+ #{Dashboard Metric Segment Card Collection Table Pulse Database})
+
+(def model-to-db-model
+ "Mapping from string model to the Toucan model backing it."
+ {"dashboard" Dashboard
+ "metric" Metric
+ "segment" Segment
+ "card" Card
+ "dataset" Card
+ "collection" Collection
+ "table" Table
+ "pulse" Pulse
+ "database" Database})
+
+(def all-models
+ "All valid models to search for. The order of this list also influences the order of the results: items earlier in the
list will be ranked higher."
- [Dashboard Metric Segment Card Collection Table Pulse Database])
-
-(defn model-name->class
- "Given a model name as a string, return its Class."
- [model-name]
- (Class/forName (format "metabase.models.%s.%sInstance" model-name (str/capitalize model-name))))
-
-(defn model-name->instance
- "Given a model name as a string, return the specific instance"
- [model-name]
- (first (filter (fn [x] (= (str/capitalize model-name) (:name x))) searchable-models)))
-
-(defn- ->class
- [class-or-instance]
- (if (class? class-or-instance)
- class-or-instance
- (class class-or-instance)))
+ ["dashboard" "metric" "segment" "card" "dataset" "collection" "table" "pulse" "database"])
(def ^:const displayed-columns
"All of the result components that by default are displayed by the frontend."
@@ -65,29 +64,29 @@
(defmulti searchable-columns-for-model
"The columns that will be searched for the query."
{:arglists '([model])}
- ->class)
+ (fn [model] model))
(defmethod searchable-columns-for-model :default
[_]
[:name])
-(defmethod searchable-columns-for-model (class Card)
+(defmethod searchable-columns-for-model "card"
[_]
[:name
:dataset_query
:description])
-(defmethod searchable-columns-for-model (class Dashboard)
+(defmethod searchable-columns-for-model "dashboard"
[_]
[:name
:description])
-(defmethod searchable-columns-for-model (class Database)
+(defmethod searchable-columns-for-model "database"
[_]
[:name
:description])
-(defmethod searchable-columns-for-model (class Table)
+(defmethod searchable-columns-for-model "table"
[_]
[:name
:display_name])
@@ -118,9 +117,9 @@
(defmulti columns-for-model
"The columns that will be returned by the query for `model`, excluding `:model`, which is added automatically."
{:arglists '([model])}
- ->class)
+ (fn [model] model))
-(defmethod columns-for-model (class Card)
+(defmethod columns-for-model "card"
[_]
(conj default-columns :collection_id :collection_position :dataset_query
[:collection.name :collection_name]
@@ -138,34 +137,34 @@
:moderated_status]
favorite-col dashboardcard-count-col))
-(defmethod columns-for-model (class Dashboard)
+(defmethod columns-for-model "dashboard"
[_]
(conj default-columns :collection_id :collection_position favorite-col
[:collection.name :collection_name]
[:collection.authority_level :collection_authority_level]))
-(defmethod columns-for-model (class Database)
+(defmethod columns-for-model "database"
[_]
[:id :name :description :updated_at])
-(defmethod columns-for-model (class Pulse)
+(defmethod columns-for-model "pulse"
[_]
[:id :name :collection_id [:collection.name :collection_name]])
-(defmethod columns-for-model (class Collection)
+(defmethod columns-for-model "collection"
[_]
(conj (remove #{:updated_at} default-columns) [:id :collection_id] [:name :collection_name]
[:authority_level :collection_authority_level]))
-(defmethod columns-for-model (class Segment)
+(defmethod columns-for-model "segment"
[_]
(into default-columns table-columns))
-(defmethod columns-for-model (class Metric)
+(defmethod columns-for-model "metric"
[_]
(into default-columns table-columns))
-(defmethod columns-for-model (class Table)
+(defmethod columns-for-model "table"
[_]
[:id
:name
diff --git a/src/metabase/search/scoring.clj b/src/metabase/search/scoring.clj
index 58de4bfb08b..85d7cddde0f 100644
--- a/src/metabase/search/scoring.clj
+++ b/src/metabase/search/scoring.clj
@@ -86,7 +86,7 @@
(defn- text-score-with
[weighted-scorers query-tokens search-result]
(let [total-weight (reduce + (map :weight weighted-scorers))
- scores (for [column (search-config/searchable-columns-for-model (search-config/model-name->class (:model search-result)))
+ scores (for [column (search-config/searchable-columns-for-model (:model search-result))
:let [matched-text (-> search-result
(get column)
(search-config/column->string (:model search-result) column))
@@ -157,10 +157,7 @@
:weight 2}])
(def ^:private model->sort-position
- (into {} (map-indexed (fn [i model]
- [(str/lower-case (name model)) i])
- ;; Reverse so that they're in descending order
- (reverse search-config/searchable-models))))
+ (zipmap (reverse search-config/all-models) (range)))
(defn- model-score
[{:keys [model]}]
diff --git a/test/metabase/api/search_test.clj b/test/metabase/api/search_test.clj
index 5d2b3da5272..8e817043b47 100644
--- a/test/metabase/api/search_test.clj
+++ b/test/metabase/api/search_test.clj
@@ -1,24 +1,14 @@
(ns metabase.api.search-test
- (:require [clojure.string :as str]
+ (:require [clojure.set :as set]
+ [clojure.string :as str]
[clojure.test :refer :all]
[honeysql.core :as hsql]
[metabase.api.search :as api.search]
[metabase.models
:refer
- [Card
- CardFavorite
- Collection
- Dashboard
- DashboardCard
- DashboardFavorite
- Database
- Metric
- PermissionsGroup
- PermissionsGroupMembership
- Pulse
- PulseCard
- Segment
- Table]]
+ [Card CardFavorite Collection Dashboard DashboardCard DashboardFavorite
+ Database Metric PermissionsGroup PermissionsGroupMembership Pulse PulseCard
+ Segment Table]]
[metabase.models.permissions :as perms]
[metabase.models.permissions-group :as group]
[metabase.search.config :as search-config]
@@ -77,6 +67,7 @@
[(make-result "dashboard test dashboard", :model "dashboard", :favorite false)
test-collection
(make-result "card test card", :model "card", :favorite false, :dataset_query nil, :dashboardcard_count 0)
+ (make-result "dataset test dataset", :model "dataset", :favorite false, :dataset_query nil, :dashboardcard_count 0)
(make-result "pulse test pulse", :model "pulse", :archived nil, :updated_at false)
(merge
(make-result "metric test metric", :model "metric", :description "Lookin' for a blueberry")
@@ -100,7 +91,7 @@
search-item)))
(defn- default-results-with-collection []
- (on-search-types #{"dashboard" "pulse" "card"}
+ (on-search-types #{"dashboard" "pulse" "card" "dataset"}
#(assoc % :collection {:id true, :name true :authority_level nil})
(default-search-results)))
@@ -113,12 +104,15 @@
{:collection_id (u/the-id collection)})))]
(mt/with-temp* [Collection [coll (data-map "collection %s collection")]
Card [card (coll-data-map "card %s card" coll)]
+ Card [dataset (assoc (coll-data-map "dataset %s dataset" coll)
+ :dataset true)]
Dashboard [dashboard (coll-data-map "dashboard %s dashboard" coll)]
Pulse [pulse (coll-data-map "pulse %s pulse" coll)]
Metric [metric (data-map "metric %s metric")]
Segment [segment (data-map "segment %s segment")]]
(f {:collection coll
:card card
+ :dataset dataset
:dashboard dashboard
:pulse pulse
:metric metric
@@ -151,7 +145,7 @@
(dissoc :scores))))
(defn- make-search-request [user-kwd params]
- (apply (partial mt/user-http-request user-kwd) :get 200 "search" params))
+ (apply mt/user-http-request user-kwd :get 200 "search" params))
(defn- search-request-data-with [xf user-kwd & params]
(let [raw-results-data (:data (make-search-request user-kwd params))
@@ -225,7 +219,7 @@
(is (<= 4 (count (search-request-data :crowberto :q "test" :limit "100" :offset "2"))))))
(testing "It offsets without limit properly"
(with-search-items-in-root-collection "test"
- (is (= 4 (count (search-request-data :crowberto :q "test" :offset "2"))))))
+ (is (= 5 (count (search-request-data :crowberto :q "test" :offset "2"))))))
(testing "It limits without offset properly"
(with-search-items-in-root-collection "test"
(is (= 2 (count (search-request-data :crowberto :q "test" :limit "2"))))))
@@ -233,6 +227,14 @@
(with-search-items-in-root-collection "test"
(is (= 0 (count (search-request-data :crowberto :q "test" :models "database"))))
(is (= 1 (count (search-request-data :crowberto :q "test" :models "database" :models "card"))))))
+ (testing "It distinguishes datasets from cards"
+ (with-search-items-in-root-collection "test"
+ (let [results (search-request-data :crowberto :q "test" :models "dataset")]
+ (is (= 1 (count results)))
+ (is (= "dataset" (-> results first :model))))
+ (let [results (search-request-data :crowberto :q "test" :models "card")]
+ (is (= 1 (count results)))
+ (is (= "card" (-> results first :model))))))
(testing "It returns limit and offset params in return result"
(with-search-items-in-root-collection "test"
(is (= 2 (:limit (search-request :crowberto :q "test" :limit "2" :offset "3"))))
@@ -241,8 +243,11 @@
(deftest query-model-set
(testing "It returns some stuff when you get results"
(with-search-items-in-root-collection "test"
- (is (contains? (apply hash-set (:available_models
- (mt/user-http-request :crowberto :get 200 "search?q=test"))) "dashboard"))))
+ ;; sometimes there is a "table" in these responses. might be do to garbage in CI
+ (is (set/subset? #{"dashboard" "dataset" "segment" "collection" "pulse" "database" "metric" "card"}
+ (-> (mt/user-http-request :crowberto :get 200 "search?q=test")
+ :available_models
+ set)))))
(testing "It returns nothing if there are no results"
(with-search-items-in-root-collection "test"
(is (= [] (:available_models (mt/user-http-request :crowberto :get 200 "search?q=noresults")))))))
@@ -419,6 +424,7 @@
(testing "Should return unarchived results by default"
(with-search-items-in-root-collection "test"
(mt/with-temp* [Card [_ (archived {:name "card test card 2"})]
+ Card [_ (archived {:name "dataset test dataset" :dataset true})]
Dashboard [_ (archived {:name "dashboard test dashboard 2"})]
Collection [_ (archived {:name "collection test collection 2"})]
Metric [_ (archived {:name "metric test metric 2"})]
@@ -430,6 +436,7 @@
(with-search-items-in-root-collection "test2"
(mt/with-temp* [Card [_ (archived {:name "card test card"})]
Card [_ (archived {:name "card that will not appear in results"})]
+ Card [_ (archived {:name "dataset test dataset" :dataset true})]
Dashboard [_ (archived {:name "dashboard test dashboard"})]
Collection [_ (archived {:name "collection test collection"})]
Metric [_ (archived {:name "metric test metric"})]
@@ -439,6 +446,7 @@
(testing "Should return archived results when specified without a search query"
(with-search-items-in-root-collection "test2"
(mt/with-temp* [Card [_ (archived {:name "card test card"})]
+ Card [_ (archived {:name "dataset test dataset" :dataset true})]
Dashboard [_ (archived {:name "dashboard test dashboard"})]
Collection [_ (archived {:name "collection test collection"})]
Metric [_ (archived {:name "metric test metric"})]
--
GitLab