🤖 backported "Add collection.effective_ancestors to model search results"

src/metabase/api/search.clj

@@ -361,7 +361,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]))
@@ -446,6 +447,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]
@@ -471,7 +513,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))]
     ;; 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
@@ -500,6 +545,7 @@
            filter-items-in-personal-collection
            offset
            search-string
+           model-ancestors?
            table-db-id
            search-native-query
            verified]}      :- [:map {:closed true}
@@ -515,6 +561,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
@@ -524,7 +571,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)
@@ -585,7 +633,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]
@@ -596,6 +644,7 @@
    created_by                          [:maybe [:or ms/PositiveInt [:sequential ms/PositiveInt]]]
    last_edited_at                      [:maybe ms/NonBlankString]
    last_edited_by                      [:maybe [:or ms/PositiveInt [:sequential 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*)
@@ -616,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)

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"]]
@@ -139,6 +140,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))
 

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

test/metabase/api/search_test.clj

@@ -138,7 +138,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]
@@ -899,6 +902,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
@@ -1638,3 +1642,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]))))))))

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

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