diff --git a/src/metabase/api/search.clj b/src/metabase/api/search.clj
index 9bbe121174537e2c9b6829ed0d5fe843ee6eb110..7d9b87bd001654699db4e0ab09b0b7ebd5826848 100644
--- a/src/metabase/api/search.clj
+++ b/src/metabase/api/search.clj
@@ -332,7 +332,8 @@
(filter (fn [[_k v]] (= v :text)))
(map first)
(remove #{:collection_authority_level :moderated_status
- :initial_sync_status :pk_ref :location}))
+ :initial_sync_status :pk_ref :location
+ :collection_location}))
case-clauses (as-> columns-to-search <>
(map (fn [col] [:like [:lower col] match]) <>)
(interleave <> (repeat [:inline 0]))
@@ -442,6 +443,47 @@
:last_editor_common_name (get user-id->common-name last_editor_id)))
results)))
+(defn add-dataset-collection-hierarchy
+ "Adds `collection_effective_ancestors` to *datasets* in the search results."
+ [search-results]
+ (let [;; this helper function takes a search result (with `collection_id` and `collection_location`) and returns the
+ ;; effective location of the result.
+ result->loc (fn [{:keys [collection_id collection_location]}]
+ (:effective_location
+ (t2/hydrate
+ (if (nil? collection_id)
+ collection/root-collection
+ {:location collection_location})
+ :effective_location)))
+ ;; a map of collection-ids to collection info
+ col-id->name&loc (into {}
+ (for [item search-results
+ :when (= (:model item) "dataset")]
+ [(:collection_id item)
+ {:name (:collection_name item)
+ :effective_location (result->loc item)}]))
+ ;; the set of all collection IDs where we *don't* know the collection name. For example, if `col-id->name&loc`
+ ;; contained `{1 {:effective_location "/2/" :name "Foo"}}`, we need to look up the name of collection `2`.
+ to-fetch (into #{} (comp (keep :effective_location)
+ (mapcat collection/location-path->ids)
+ ;; already have these names
+ (remove col-id->name&loc))
+ (vals col-id->name&loc))
+ ;; the complete map of collection IDs to names
+ id->name (merge (if (seq to-fetch)
+ (t2/select-pk->fn :name :model/Collection :id [:in to-fetch])
+ {})
+ (update-vals col-id->name&loc :name))
+ annotate (fn [x]
+ (cond-> x
+ (= (:model x) "dataset")
+ (assoc :collection_effective_ancestors
+ (if-let [loc (result->loc x)]
+ (->> (collection/location-path->ids loc)
+ (map (fn [id] {:id id :name (id->name id)})))
+ []))))]
+ (map annotate search-results)))
+
(mu/defn ^:private search
"Builds a search query that includes all the searchable entities and runs it"
[search-ctx :- SearchContext]
@@ -467,8 +509,10 @@
(map #(update % :pk_ref json/parse-string))
(map (partial scoring/score-and-result (:search-string search-ctx)))
(filter #(pos? (:score %))))
- total-results (hydrate-user-metadata
- (scoring/top-results reducible-results search.config/max-filtered-results xf))
+ total-results (cond->> (scoring/top-results reducible-results search.config/max-filtered-results xf)
+ true hydrate-user-metadata
+ (:model-ancestors? search-ctx) (add-dataset-collection-hierarchy)
+ true (map scoring/serialize))
add-perms-for-col (fn [item]
(cond-> item
(mi/instance-of? :model/Collection item)
@@ -502,6 +546,7 @@
filter-items-in-personal-collection
offset
search-string
+ model-ancestors?
table-db-id
search-native-query
verified]} :- [:map {:closed true}
@@ -517,6 +562,7 @@
[:offset {:optional true} [:maybe ms/Int]]
[:table-db-id {:optional true} [:maybe ms/PositiveInt]]
[:search-native-query {:optional true} [:maybe boolean?]]
+ [:model-ancestors? {:optional true} [:maybe boolean?]]
[:verified {:optional true} [:maybe true?]]]] :- SearchContext
(when (some? verified)
(premium-features/assert-has-any-features
@@ -526,7 +572,8 @@
ctx (cond-> {:search-string search-string
:current-user-perms @api/*current-user-permissions-set*
:archived? (boolean archived)
- :models models}
+ :models models
+ :model-ancestors? (boolean model-ancestors?)}
(some? created-at) (assoc :created-at created-at)
(seq created-by) (assoc :created-by created-by)
(some? filter-items-in-personal-collection) (assoc :filter-items-in-personal-collection filter-items-in-personal-collection)
@@ -587,7 +634,7 @@
A search query that has both filters applied will only return models and cards."
[q archived context created_at created_by table_db_id models last_edited_at last_edited_by
- filter_items_in_personal_collection search_native_query verified]
+ filter_items_in_personal_collection model_ancestors search_native_query verified]
{q [:maybe ms/NonBlankString]
archived [:maybe :boolean]
table_db_id [:maybe ms/PositiveInt]
@@ -598,6 +645,7 @@
created_by [:maybe (ms/QueryVectorOf ms/PositiveInt)]
last_edited_at [:maybe ms/NonBlankString]
last_edited_by [:maybe (ms/QueryVectorOf ms/PositiveInt)]
+ model_ancestors [:maybe :boolean]
search_native_query [:maybe true?]
verified [:maybe true?]}
(api/check-valid-page-params mw.offset-paging/*limit* mw.offset-paging/*offset*)
@@ -617,6 +665,7 @@
:models models-set
:limit mw.offset-paging/*limit*
:offset mw.offset-paging/*offset*
+ :model-ancestors? model_ancestors
:search-native-query search_native_query
:verified verified}))
duration (- (System/currentTimeMillis) start-time)
diff --git a/src/metabase/search/config.clj b/src/metabase/search/config.clj
index 2f3a741f3d59a582c811064f8106ae80ad831078..d7f60bc8b54821b0b90caad8ecd3b1aab6f39ae5 100644
--- a/src/metabase/search/config.clj
+++ b/src/metabase/search/config.clj
@@ -99,6 +99,7 @@
[:map {:closed true}
[:search-string [:maybe ms/NonBlankString]]
[:archived? :boolean]
+ [:model-ancestors? :boolean]
[:current-user-perms [:set perms.u/PathSchema]]
[:models [:set SearchableModel]]
[:filter-items-in-personal-collection {:optional true} [:enum "only" "exclude"]]
@@ -138,6 +139,7 @@
:collection_id :integer
:collection_name :text
:collection_type :text
+ :collection_location :text
:collection_authority_level :text
;; returned for Card and Dashboard
:collection_position :integer
@@ -270,6 +272,7 @@
[_]
(conj default-columns :collection_id :collection_position :dataset_query :display :creator_id
[:collection.name :collection_name]
+ [:collection.location :collection_location]
[:collection.authority_level :collection_authority_level]
bookmark-col dashboardcard-count-col))
diff --git a/src/metabase/search/scoring.clj b/src/metabase/search/scoring.clj
index e8e2a79892344e8579da086b82dae69c973ac2f6..111316c9a5770dbfd3ea3d8802aecce2165035e5 100644
--- a/src/metabase/search/scoring.clj
+++ b/src/metabase/search/scoring.clj
@@ -301,12 +301,12 @@
(max (- stale-time days-ago) 0)
stale-time)))
-(defn- serialize
+(defn serialize
"Massage the raw result from the DB and match data into something more useful for the client"
- [result all-scores relevant-scores]
- (let [{:keys [name display_name collection_id collection_name collection_authority_level collection_type]} result
- matching-columns (into #{} (remove nil? (map :column relevant-scores)))
- match-context-thunk (first (keep :match-context-thunk relevant-scores))]
+ [{:as result :keys [all-scores relevant-scores name display_name collection_id collection_name
+ collection_authority_level collection_type collection_effective_ancestors]}]
+ (let [matching-columns (into #{} (remove nil? (map :column relevant-scores)))
+ match-context-thunk (first (keep :match-context-thunk relevant-scores))]
(-> result
(assoc
:name (if (and (contains? matching-columns :display_name) display_name)
@@ -316,14 +316,20 @@
(empty?
(remove matching-columns search.config/displayed-columns)))
(match-context-thunk))
- :collection {:id collection_id
- :name collection_name
- :authority_level collection_authority_level
- :type collection_type}
+ :collection (merge {:id collection_id
+ :name collection_name
+ :authority_level collection_authority_level
+ :type collection_type}
+ (when collection_effective_ancestors
+ {:effective_ancestors collection_effective_ancestors}))
:scores all-scores)
(update :dataset_query #(some-> % json/parse-string mbql.normalize/normalize))
(dissoc
+ :all-scores
+ :relevant-scores
+ :collection_effective_ancestors
:collection_id
+ :collection_location
:collection_name
:collection_type
:display_name))))
@@ -397,7 +403,7 @@
0
text-matches)))
{:score total-score
- :result (serialize result all-scores relevant-scores)}
+ :result (assoc result :all-scores all-scores :relevant-scores relevant-scores)}
{:score 0})))
(defn compare-score
diff --git a/test/metabase/api/search_test.clj b/test/metabase/api/search_test.clj
index 32f5bc6a624b8cfd2514b5c0aad3a1cbdf9ae9a3..a2b84d4540a1622e454de6f7865fc85368e3c800 100644
--- a/test/metabase/api/search_test.clj
+++ b/test/metabase/api/search_test.clj
@@ -141,7 +141,10 @@
(defn- default-results-with-collection []
(on-search-types #{"dashboard" "pulse" "card" "dataset" "action"}
- #(assoc % :collection {:id true, :name (if (= (:model %) "action") nil true) :authority_level nil :type nil})
+ #(assoc % :collection {:id true,
+ :name (if (= (:model %) "action") nil true)
+ :authority_level nil
+ :type nil})
(default-search-results)))
(defn- do-with-search-items [search-string in-root-collection? f]
@@ -919,6 +922,7 @@
:archived? false
:models search.config/all-models
:current-user-perms #{"/"}
+ :model-ancestors? false
:limit-int 100}))]
;; warm it up, in case the DB call depends on the order of test execution and it needs to
;; do some initialization
@@ -1658,3 +1662,43 @@
(->> (mt/user-http-request :lucky :get 200 "search" :archived true :q "test")
:data
(map :name)))))))
+
+(deftest model-ancestors-gets-ancestor-collections
+ (testing "Collection names are correct"
+ (mt/with-temp [Collection {top-col-id :id} {:name "top level col" :location "/"}
+ Collection {mid-col-id :id} {:name "middle level col" :location (str "/" top-col-id "/")}
+ Card {leaf-card-id :id} {:type :model :collection_id mid-col-id :name "leaf model"}
+ Card {top-card-id :id} {:type :model :collection_id top-col-id :name "top model"}]
+ (is (= #{[leaf-card-id [{:name "top level col" :id top-col-id}]]
+ [top-card-id []]}
+ (->> (mt/user-http-request :rasta :get 200 "search" :model_ancestors true :q "model" :models ["dataset"])
+ :data
+ (map (juxt :id #(get-in % [:collection :effective_ancestors])))
+ (into #{}))))))
+ (testing "Models not in a collection work correctly"
+ (mt/with-temp [Card {card-id :id} {:type :model
+ :name "model"
+ :collection_id nil}]
+ (is (= #{[card-id []]}
+ (->> (mt/user-http-request :rasta :get 200 "search" :model_ancestors true :q "model" :models ["dataset"])
+ :data
+ (map (juxt :id #(get-in % [:collection :effective_ancestors])))
+ (into #{}))))))
+ (testing "Non-models don't get collection_ancestors"
+ (mt/with-temp [Card _ {:name "question"
+ :collection_id nil}]
+ (is (not (contains? (->> (mt/user-http-request :rasta :get 200 "search" :model_ancestors true :q "question" :models ["dataset" "card"])
+ :data
+ (filter #(= (:name %) "question"))
+ first
+ :collection)
+ :effective_ancestors)))))
+ (testing "If `model_parents` is not passed, it doesn't get populated"
+ (mt/with-temp [Collection {top-col-id :id} {:name "top level col" :location "/"}
+ Collection {mid-col-id :id} {:name "middle level col" :location (str "/" top-col-id "/")}
+ Card _ {:type :model :collection_id mid-col-id :name "leaf model"}
+ Card _ {:type :model :collection_id top-col-id :name "top model"}]
+ (is (= [nil nil]
+ (->> (mt/user-http-request :rasta :get 200 "search" :model_ancestors false :q "model" :models ["dataset"])
+ :data
+ (map #(get-in % [:collection :effective_ancestors]))))))))
diff --git a/test/metabase/search/filter_test.clj b/test/metabase/search/filter_test.clj
index ac449840ae4b90b19ffbfb408914ae392e7802e5..70bbb3ba3bec3cd5e6181df9a2c5afb434ddea35 100644
--- a/test/metabase/search/filter_test.clj
+++ b/test/metabase/search/filter_test.clj
@@ -11,6 +11,7 @@
{:search-string nil
:archived? false
:models search.config/all-models
+ :model-ancestors? false
:current-user-perms #{"/"}})
(deftest ^:parallel ->applicable-models-test
diff --git a/test/metabase/search/scoring_test.clj b/test/metabase/search/scoring_test.clj
index aaf168aef4d9591f65c6f5e62dd5961ba1d30353..af80593f1745015123b4e101d652a6c88012a567 100644
--- a/test/metabase/search/scoring_test.clj
+++ b/test/metabase/search/scoring_test.clj
@@ -280,11 +280,13 @@
:database 1}
result {:name "card"
:model "card"
- :dataset_query (json/generate-string query)}]
- (is (= query (-> result (#'scoring/serialize {} {}) :dataset_query)))))
+ :dataset_query (json/generate-string query)
+ :all-scores {}
+ :relevant-scores {}}]
+ (is (= query (-> result scoring/serialize :dataset_query)))))
(testing "Doesn't error on other models without a query"
- (is (nil? (-> {:name "dash" :model "dashboard"}
- (#'scoring/serialize {} {})
+ (is (nil? (-> {:name "dash" :model "dashboard" :all-scores {} :relevant-scores {}}
+ (#'scoring/serialize)
:dataset_query)))))
(deftest ^:parallel force-weight-test