Merge pull request #7695 from metabase/basic-entity-search

Add basic search on entity names

src/metabase/api/routes.clj

@@ -24,6 +24,7 @@
              [public :as public]
              [pulse :as pulse]
              [revision :as revision]
+             [search :as search]
              [segment :as segment]
              [session :as session]
              [setting :as setting]
@@ -76,6 +77,7 @@
   (context "/public"               [] (+generic-exceptions public/routes))
   (context "/pulse"                [] (+auth pulse/routes))
   (context "/revision"             [] (+auth revision/routes))
+  (context "/search"               [] (+auth search/routes))
   (context "/segment"              [] (+auth segment/routes))
   (context "/session"              [] session/routes)
   (context "/setting"              [] (+auth setting/routes))

src/metabase/api/search.clj

+(ns metabase.api.search
+  (:require [clojure.string :as str]
+            [compojure.core :refer [GET]]
+            [honeysql.helpers :as h]
+            [metabase.api.common :refer [*current-user-permissions-set* check-403 defendpoint define-routes]]
+            [metabase.models
+             [card :refer [Card]]
+             [collection :as coll :refer [Collection]]
+             [dashboard :refer [Dashboard]]
+             [metric :refer [Metric]]
+             [pulse :refer [Pulse]]
+             [segment :refer [Segment]]]
+            [metabase.util
+             [honeysql-extensions :as hx]
+             [schema :as su]]
+            [schema.core :as s]
+            [toucan.db :as db]))
+
+(def ^:private search-columns-without-type
+  "The columns found in search query clauses except type. Type is added automatically"
+  [:name :description :id])
+
+(def ^:private SearchContext
+  "Map with the various allowed search parameters, used to construct the SQL query"
+  {:search-string       su/NonBlankString
+   :archived?           s/Bool
+   :collection          (s/maybe su/IntGreaterThanZero)
+   :visible-collections coll/VisibleCollections})
+
+(defn- merge-search-select
+  "The search query uses a `union-all` which requires that there be the same number of columns in each of the segments
+  of the query. This function will take `entity-columns` and will inject constant `nil` values for any column missing
+  from `entity-columns` but found in `search-columns`"
+  [query-map entity-type entity-columns]
+  (let [entity-column-set (set entity-columns)
+        cols-or-nils      (for [search-col search-columns-without-type]
+                            (if (contains? entity-column-set search-col)
+                              search-col
+                              [nil search-col]))]
+    (apply h/merge-select query-map (concat cols-or-nils [[(hx/literal entity-type) :type]]))))
+
+(s/defn ^:private merge-name-search
+  "Add case-insensitive name query criteria to `query-map`"
+  [query-map {:keys [search-string]} :- SearchContext]
+  (h/merge-where query-map [:like :%lower.name search-string]))
+
+(s/defn ^:private merge-name-and-archived-search
+  "Add name and archived query criteria to `query-map`"
+  [query-map {:keys [search-string archived?] :as search-ctx} :- SearchContext]
+  (-> query-map
+      (merge-name-search search-ctx)
+      (h/merge-where [:= :archived archived?])))
+
+(s/defn ^:private  merge-name-and-active-search
+  [query-map {:keys [search-string archived?] :as search-ctx} :- SearchContext]
+  (-> query-map
+      (merge-name-search search-ctx)
+      ;; archived? = true is the same as is_active = false, so flip it here
+      (h/merge-where [:= :is_active (not archived?)])))
+
+(s/defn ^:private add-collection-criteria
+  "Update the query to only include collections the user has access to"
+  [query-map column-kwd {:keys [visible-collections collection]} :- SearchContext]
+  (cond
+    collection
+    (h/merge-where query-map [:= column-kwd collection])
+
+    (= :all visible-collections)
+    query-map
+
+    :else
+    (do
+      ;; This is validated in the API call, just double checking here
+      (assert (seq visible-collections))
+      (h/merge-where query-map [:in column-kwd visible-collections]))))
+
+(defn- make-honeysql-search-query
+  "Create a HoneySQL query map to search for `entity`, suitable for the UNION ALL used in search."
+  [entity search-type projected-columns]
+  (-> {}
+      (merge-search-select search-type projected-columns)
+      (h/merge-from entity)))
+
+(defmulti ^:private create-search-query (fn [entity search-context] entity))
+
+(s/defmethod ^:private create-search-query :card
+  [_ search-ctx :- SearchContext]
+  (-> (make-honeysql-search-query Card "card" search-columns-without-type)
+      (merge-name-and-archived-search search-ctx)
+      (add-collection-criteria :collection_id search-ctx)))
+
+(s/defmethod ^:private create-search-query :collection
+  [_ {:keys [collection] :as search-ctx} :- SearchContext]
+  ;; If we have a collection, no need to search collections
+  (when-not collection
+    (-> (make-honeysql-search-query Collection "collection" search-columns-without-type)
+        (merge-name-and-archived-search search-ctx)
+        (add-collection-criteria :id search-ctx))))
+
+(s/defmethod ^:private create-search-query :dashboard
+  [_ search-ctx :- SearchContext]
+  (-> (make-honeysql-search-query Dashboard "dashboard" search-columns-without-type)
+      (merge-name-and-archived-search search-ctx)
+      (add-collection-criteria :collection_id search-ctx)))
+
+(s/defmethod ^:private create-search-query :pulse
+  [_ {:keys [archived?] :as search-ctx} :- SearchContext]
+  ;; Pulses don't currently support being archived, omit if archived is true
+  (when-not archived?
+    (-> (make-honeysql-search-query Pulse "pulse" [:name :id])
+        (merge-name-search search-ctx)
+        (add-collection-criteria :collection_id search-ctx))))
+
+(s/defmethod ^:private create-search-query :metric
+  [_ {:keys [collection] :as search-ctx} :- SearchContext]
+  (when-not collection
+    (-> (make-honeysql-search-query Metric "metric" search-columns-without-type)
+        (merge-name-and-active-search search-ctx))))
+
+(s/defmethod ^:private create-search-query :segment
+  [_ {:keys [collection] :as search-ctx} :- SearchContext]
+  (when-not collection
+    (-> (make-honeysql-search-query Segment "segment" search-columns-without-type)
+        (merge-name-and-active-search search-ctx))))
+
+(s/defn ^:private search
+  "Builds a search query that includes all of the searchable entities and runs it"
+  [{:keys [collection visible-collections] :as search-ctx} :- SearchContext]
+  ;; If searching for a collection you don't have access to, no need to run a query
+  (if (and collection
+           (not= :all visible-collections)
+           (not (contains? visible-collections collection)))
+    []
+    (db/query {:union-all (for [entity [:card :collection :dashboard :pulse :segment :metric]
+                                :let [query-map (create-search-query entity search-ctx)]
+                                :when query-map]
+                            query-map)})))
+
+(s/defn ^:private make-search-context :- SearchContext
+  [search-string :- su/NonBlankString
+   archived-string :- (s/maybe su/BooleanString)
+   collection-id :- (s/maybe su/IntGreaterThanZero)]
+  {:search-string       (str "%" (str/lower-case search-string) "%")
+   :archived?           (Boolean/parseBoolean archived-string)
+   :collection          collection-id
+   :visible-collections (coll/permissions-set->visible-collection-ids @*current-user-permissions-set*)})
+
+(defendpoint GET "/"
+  "Search Cards, Dashboards, Collections and Pulses for the substring `q`."
+  [q archived collection_id]
+  {q             su/NonBlankString
+   archived      (s/maybe su/BooleanString)
+   collection_id (s/maybe su/IntGreaterThanZero)}
+  (let [{:keys [visible-collections collection] :as search-ctx} (make-search-context q archived collection_id)]
+    ;; Throw if the user doesn't have access to any collections
+    (check-403 (or (= :all visible-collections)
+                   (seq visible-collections)))
+    (search search-ctx)))
+
+(define-routes)

src/metabase/models/collection.clj

@@ -159,7 +159,11 @@
 ;; 10 and 30, but not 20, we will show them an "effective" location path of `/10/30/`. This is used for things like
 ;; breadcrumbing in the frontend.
 
-(s/defn permissions-set->visible-collection-ids :- (s/cond-pre (s/eq :all) #{su/IntGreaterThanZero})
+(def VisibleCollections
+  "Includes the possible values for visible collections, either `:all` or a set of ids"
+  (s/cond-pre (s/eq :all) #{su/IntGreaterThanZero}))
+
+(s/defn permissions-set->visible-collection-ids :- VisibleCollections
   "Given a `permissions-set` (presumably those of the current user), return a set of IDs of Collections that the
   permissions set allows you to view. For those with *root* permissions (e.g., an admin), this function will return
   `:all`, signifying that you are allowed to view all Collections.

test/metabase/api/search_test.clj

+(ns metabase.api.search-test
+  (:require [expectations :refer :all]
+            [metabase.models
+             [card :refer [Card]]
+             [collection :as coll :refer [Collection]]
+             [dashboard :refer [Dashboard]]
+             [metric :refer [Metric]]
+             [permissions :as perms]
+             [permissions-group :as group]
+             [pulse :refer [Pulse]]
+             [segment :refer [Segment]]]
+            [metabase.test.data.users :refer :all]
+            [metabase.test.util :as tu]
+            [toucan.util.test :as tt]))
+
+(def ^:private default-search-results
+  (set (map #(merge {:description nil :id true} %)
+            [{:name "dashboard foo dashboard", :type "dashboard"}
+             {:name "collection foo collection", :type "collection"}
+             {:name "card foo card", :type "card"}
+             {:name "pulse foo pulse", :type "pulse"}
+             {:name "metric foo metric", :description "Lookin' for a blueberry", :type "metric"}
+             {:name "segment foo segment", :description "Lookin' for a blueberry", :type "segment"}])))
+
+;; Basic search, should find 1 of each entity type
+(expect
+  default-search-results
+  (tt/with-temp* [Card       [_ {:name "card foo card"}]
+                  Dashboard  [_ {:name "dashboard foo dashboard"}]
+                  Collection [_ {:name "collection foo collection"}]
+                  Pulse      [_ {:name "pulse foo pulse"}]
+                  Metric     [_ {:name "metric foo metric"}]
+                  Segment    [_ {:name "segment foo segment"}]]
+    (tu/boolean-ids-and-timestamps (set ((user->client :crowberto) :get 200 "search", :q "foo")))))
+
+;; Basic search should only return substring matches
+(expect
+  default-search-results
+  (tt/with-temp* [Card       [_ {:name "card foo card"}]
+                  Card       [_ {:name "card bar card"}]
+                  Dashboard  [_ {:name "dashboard foo dashboard"}]
+                  Dashboard  [_ {:name "dashboard bar dashboard"}]
+                  Collection [_ {:name "collection foo collection"}]
+                  Collection [_ {:name "collection bar collection"}]
+                  Pulse      [_ {:name "pulse foo pulse"}]
+                  Pulse      [_ {:name "pulse bar pulse"}]
+                  Metric     [_ {:name "metric foo metric"}]
+                  Metric     [_ {:name "metric bar metric"}]
+                  Segment    [_ {:name "segment foo segment"}]
+                  Segment    [_ {:name "segment bar segment"}]]
+    (tu/boolean-ids-and-timestamps (set ((user->client :crowberto) :get 200 "search", :q "foo")))))
+
+(defn- archived [m]
+  (assoc m :archived true))
+
+(defn- inactive [m]
+  (assoc m :is_active false))
+
+;; Should return unarchived results by default
+(expect
+  default-search-results
+  (tt/with-temp* [Card       [_ {:name "card foo card"}]
+                  Card       [_ (archived {:name "card foo card2"})]
+                  Dashboard  [_ {:name "dashboard foo dashboard"}]
+                  Dashboard  [_ (archived {:name "dashboard foo dashboard2"})]
+                  Collection [_ {:name "collection foo collection"}]
+                  Collection [_ (archived  {:name "collection foo collection2"})]
+                  Pulse      [_ {:name "pulse foo pulse"}]
+                  Metric     [_ {:name "metric foo metric"}]
+                  Metric     [_ (inactive {:name "metric foo metric2"})]
+                  Segment    [_ {:name "segment foo segment"}]
+                  Segment    [_ (inactive {:name "segment foo segment2"})]]
+    (tu/boolean-ids-and-timestamps (set ((user->client :crowberto) :get 200 "search", :q "foo")))))
+
+;; Should return archived results when specified
+(expect
+  (set (remove #(= "pulse" (:type %)) default-search-results))
+  (tt/with-temp* [Card       [_ (archived {:name "card foo card"})]
+                  Card       [_ {:name "card foo card2"}]
+                  Dashboard  [_ (archived {:name "dashboard foo dashboard"})]
+                  Dashboard  [_ {:name "dashboard foo dashboard2"}]
+                  Collection [_ (archived {:name "collection foo collection"})]
+                  Collection [_ {:name "collection foo collection2"}]
+                  Pulse      [_ {:name "pulse foo pulse"}]
+                  Metric     [_ (inactive {:name "metric foo metric"})]
+                  Metric     [_ {:name "metric foo metric2"}]
+                  Segment    [_ (inactive {:name "segment foo segment"})]
+                  Segment    [_ {:name "segment foo segment2"}]]
+    (tu/boolean-ids-and-timestamps (set ((user->client :crowberto) :get 200 "search",
+                                         :q "foo", :archived "true")))))
+
+;; Search within a collection will omit the collection, only return cards/dashboards/pulses in the collection
+(expect
+  ;; Metrics and segments don't have a collection, so they shouldn't be included in the results
+  (set (remove #(contains? #{"collection" "metric" "segment"} (:type %)) default-search-results))
+  (tt/with-temp* [Collection [{coll-id :id} {:name "collection foo collection"}]
+                  Card       [_ {:name "card foo card", :collection_id coll-id}]
+                  Card       [_ {:name "card foo card2"}]
+                  Dashboard  [_ {:name "dashboard foo dashboard", :collection_id coll-id}]
+                  Dashboard  [_ {:name "dashboard bar dashboard2"}]
+                  Pulse      [_ {:name "pulse foo pulse", :collection_id coll-id}]
+                  Pulse      [_ {:name "pulse foo pulse2"}]
+                  Metric     [_ {:name "metric foo metric"}]
+                  Segment    [_ {:name "segment foo segment"}]]
+    (tu/boolean-ids-and-timestamps (set ((user->client :crowberto) :get 200 "search", :q "foo", :collection_id coll-id)))))
+
+;; Querying for a collection you don't have access to just returns empty
+(expect
+  []
+  (tt/with-temp* [Collection [coll-1          {:name "collection 1"}]
+                  Collection [{coll-2-id :id} {:name "collection 2"}]]
+    (perms/grant-collection-read-permissions! (group/all-users) coll-1)
+    ((user->client :rasta) :get 200 "search", :q "foo", :collection_id coll-2-id)))
+
+;; Users with access to a collection should be able to search it
+(expect
+  ;; Metrics and segments don't have a collection, so they shouldn't be included in the results
+  (set (remove #(contains? #{"collection" "metric" "segment"} (:type %)) default-search-results))
+  (tt/with-temp* [Collection [{coll-id :id, :as coll} {:name "collection foo collection"}]
+                  Card       [_ {:name "card foo card", :collection_id coll-id}]
+                  Card       [_ {:name "card foo card2"}]
+                  Dashboard  [_ {:name "dashboard foo dashboard", :collection_id coll-id}]
+                  Dashboard  [_ {:name "dashboard bar dashboard2"}]
+                  Pulse      [_ {:name "pulse foo pulse", :collection_id coll-id}]
+                  Pulse      [_ {:name "pulse foo pulse2"}]
+                  Metric     [_ {:name "metric foo metric"}]
+                  Segment    [_ {:name "segment foo segment"}]]
+    (perms/grant-collection-read-permissions! (group/all-users) coll)
+    (tu/boolean-ids-and-timestamps (set ((user->client :rasta) :get 200 "search", :q "foo", :collection_id coll-id)))))
+
+;; Collections a user doesn't have access to are automatically omitted from the results
+(expect
+  default-search-results
+  (tt/with-temp* [Collection [{coll-id-1 :id, :as coll-1} {:name "collection foo collection"}]
+                  Collection [{coll-id-2 :id, :as coll-2} {:name "collection foo collection2"}]
+                  Card       [_ {:name "card foo card", :collection_id coll-id-1}]
+                  Card       [_ {:name "card foo card2", :collection_id coll-id-2}]
+                  Dashboard  [_ {:name "dashboard foo dashboard", :collection_id coll-id-1}]
+                  Dashboard  [_ {:name "dashboard bar dashboard2", :collection_id coll-id-2}]
+                  Pulse      [_ {:name "pulse foo pulse", :collection_id coll-id-1}]
+                  Pulse      [_ {:name "pulse foo pulse2", :collection_id coll-id-2}]
+                  Metric     [_ {:name "metric foo metric"}]
+                  Segment    [_ {:name "segment foo segment"}]]
+    (perms/grant-collection-read-permissions! (group/all-users) coll-1)
+    (tu/boolean-ids-and-timestamps (set ((user->client :rasta) :get 200 "search", :q "foo")))))