Skip to content

Commit

Permalink
Add a type to collections returned by search
Browse files Browse the repository at this point in the history 
Under:

- `item.type`

- `item.collection.type`, and

- `item.collection.effective_ancestors.*.type`
  • Loading branch information
@johnswanson
johnswanson committed May 14, 2024
1 parent 83c317d commit c9864f5
Show file tree
Hide file tree
Showing 2 changed files with 84 additions and 37 deletions.
76 changes: 40 additions & 36 deletions src/metabase/api/search.clj
View file
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
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)))))))))

0 comments on commit c9864f5

Please sign in to comment.