Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add a type to collections returned by search #42535

Merged
merged 1 commit into from
May 17, 2024
Merged

Add a type to collections returned by search #42535

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 40 additions & 36 deletions src/metabase/api/search.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,44 +468,46 @@
[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)))
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 (-> {:name (:collection_name item)
:id (:collection_id item)
:type (:collection_type item)}
collection/maybe-localize-trash-name
:name)
: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`
col-id->info (into {}
(for [item search-results
:when (= (:model item) "dataset")]
[(:collection_id item)
{:id (:collection_id item)
:name (-> {:name (:collection_name item)
:id (:collection_id item)
:type (:collection_type item)}
collection/maybe-localize-trash-name
:name)
:type (:collection_type 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->info`
;; 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)})))
[]))))]
to-fetch (into #{} (comp (keep :effective_location)
(mapcat collection/location-path->ids)
;; already have these names
(remove col-id->info))
(vals col-id->info))
;; the now COMPLETE map of collection IDs to info
col-id->info (merge (if (seq to-fetch)
(t2/select-pk->fn #(select-keys % [:name :type :id]) :model/Collection :id [:in to-fetch])
{})
(update-vals col-id->info #(dissoc % :effective_location)))
annotate (fn [x]
(cond-> x
(= (:model x) "dataset")
(assoc :collection_effective_ancestors
(if-let [loc (result->loc x)]
(->> (collection/location-path->ids loc)
(map col-id->info))
[]))))]
(map annotate search-results)))

(defn- add-can-write [row]
Expand Down Expand Up @@ -533,6 +535,8 @@
(map #(if (t2/instance-of? :model/Collection %)
(t2/hydrate % :effective_location)
(assoc % :effective_location nil)))
(map #(cond-> %
(t2/instance-of? :model/Collection %) (assoc :type (:collection_type %))))
(map #(cond-> % (t2/instance-of? :model/Collection %) collection/maybe-localize-trash-name))
(filter (partial check-permissions-for-model (:archived? search-ctx)))
;; MySQL returns `:bookmark` and `:archived` as `1` or `0` so convert those to boolean as
Expand Down
45 changes: 44 additions & 1 deletion test/metabase/api/search_test.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@
:effective_location "/"
:location "/"
:updated_at false
:type nil
:can_write true))

(def ^:private action-model-params {:name "ActionModel", :type :model})
Expand Down Expand Up @@ -1731,7 +1732,7 @@
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}]]
(is (= #{[leaf-card-id [{:name "top level col" :type nil :id top-col-id}]]
[top-card-id []]}
(->> (mt/user-http-request :rasta :get 200 "search" :model_ancestors true :q "model" :models ["dataset"])
:data
Expand Down Expand Up @@ -1764,3 +1765,45 @@
(->> (mt/user-http-request :rasta :get 200 "search" :model_ancestors false :q "model" :models ["dataset"])
:data
(map #(get-in % [:collection :effective_ancestors]))))))))

(deftest collection-type-is-returned
(testing "Users without perms for a collection can't see search results that were trashed from that collection"
(let [search-name (random-uuid)
named #(str search-name "-" %)]
(mt/with-temp [:model/Collection {parent-id :id :as parent} {}
:model/Collection _ {:location (collection/children-location parent)
:name (named "collection")
:type "meow mix"}
:model/Dashboard _ {:collection_id parent-id :name (named "dashboard")}
:model/Card _ {:collection_id parent-id :name (named "card")}
:model/Card _ {:collection_id parent-id :type :model :name (named "model")}]
(testing "the collection data includes the type under `item.type` for collections"
(is (every? #(contains? % :type)
(->> (mt/user-http-request :crowberto :get 200 "/search" :q search-name)
:data
(filter #(= (:model %) "collection")))))
(is (not-any? #(contains? % :type)
(->> (mt/user-http-request :crowberto :get 200 "/search" :q search-name)
:data
(remove #(= (:model %) "collection"))))))
(testing "`item.type` is correct for collections"
(is (= #{"meow mix"} (->> (mt/user-http-request :crowberto :get 200 "/search" :q search-name)
:data
(keep :type)
set)))))
(testing "Type is on both `item.collection.type` and `item.collection.effective_ancestors`"
(mt/with-temp [Collection {top-col-id :id} {:name "top level col" :location "/" :type "foo"}
Collection {mid-col-id :id} {:name "middle level col" :type "bar" :location (str "/" top-col-id "/")}
Card {leaf-card-id :id} {:type :model :collection_id mid-col-id :name "leaf model"}]
(let [leaf-card-response (->> (mt/user-http-request :rasta :get 200 "search" :model_ancestors true :q "model" :models ["dataset"])
:data
(filter #(= (:id %) leaf-card-id))
first)]
(is (= {:id mid-col-id
:name "middle level col"
:type "bar"
:authority_level nil
:effective_ancestors [{:id top-col-id
:name "top level col"
:type "foo"}]}
(:collection leaf-card-response)))))))))
Loading