From 486950219e504c8922b283e0f483822f7049e9c6 Mon Sep 17 00:00:00 2001
From: Cam Saul <1455846+camsaul@users.noreply.github.com>
Date: Mon, 2 Mar 2020 16:37:40 -0800
Subject: [PATCH] Implement for GET /api/database?include=tables and
?saved=true (#12027)
---
src/metabase/api/database.clj | 68 +++++--
test/metabase/api/database_test.clj | 285 +++++++++++++++-------------
2 files changed, 199 insertions(+), 154 deletions(-)
diff --git a/src/metabase/api/database.clj b/src/metabase/api/database.clj
index 67d83853998..13bbcaa3466 100644
--- a/src/metabase/api/database.clj
+++ b/src/metabase/api/database.clj
@@ -4,6 +4,7 @@
[clojure.tools.logging :as log]
[compojure.core :refer [DELETE GET POST PUT]]
[metabase
+ [config :as config]
[driver :as driver]
[events :as events]
[public-settings :as public-settings]
@@ -134,37 +135,64 @@
(for [card (source-query-cards)]
(table-api/card->virtual-table card :include-fields? include-fields?)))
-(defn- saved-cards-virtual-db-metadata [& {:keys [include-fields?]}]
+(defn- saved-cards-virtual-db-metadata [& {:keys [include-tables? include-fields?]}]
(when (public-settings/enable-nested-queries)
- (when-let [virtual-tables (seq (cards-virtual-tables :include-fields? include-fields?))]
- {:name "Saved Questions"
- :id mbql.s/saved-questions-virtual-database-id
- :features #{:basic-aggregations}
- :tables virtual-tables
- :is_saved_questions true})))
+ (cond-> {:name "Saved Questions"
+ :id mbql.s/saved-questions-virtual-database-id
+ :features #{:basic-aggregations}
+ :is_saved_questions true}
+ include-tables? (assoc :tables (cards-virtual-tables :include-fields? include-fields?)))))
;; "Virtual" tables for saved cards simulate the db->schema->table hierarchy by doing fake-db->collection->card
-(defn- add-virtual-tables-for-saved-cards [dbs]
- (if-let [virtual-db-metadata (saved-cards-virtual-db-metadata)]
+(defn- add-saved-questions-virtual-database [dbs & options]
+ (if-let [virtual-db-metadata (apply saved-cards-virtual-db-metadata options)]
;; only add the 'Saved Questions' DB if there are Cards that can be used
(conj (vec dbs) virtual-db-metadata)
dbs))
-(defn- dbs-list [include-tables? include-cards?]
+(defn- dbs-list [& {:keys [include-tables? include-saved-questions-db? include-saved-questions-tables?]}]
(when-let [dbs (seq (filter mi/can-read? (db/select Database {:order-by [:%lower.name :%lower.engine]})))]
(cond-> (add-native-perms-info dbs)
- include-tables? add-tables
- include-cards? add-virtual-tables-for-saved-cards)))
+ include-tables? add-tables
+ include-saved-questions-db? (add-saved-questions-virtual-database :include-tables? include-saved-questions-tables?))))
(api/defendpoint GET "/"
- "Fetch all `Databases`. `include_tables` means we should hydrate the Tables belonging to each DB. `include_cards` here
- means we should also include virtual Table entries for saved Questions, e.g. so we can easily use them as source
- Tables in queries. Default for both is `false`."
- [include_tables include_cards]
+ "Fetch all `Databases`.
+
+ * `include=tables` means we should hydrate the Tables belonging to each DB. Default: `false`.
+
+ * `saved` means we should include the saved questions virtual database. Default: `false`.
+
+ * `include_tables` is a legacy alias for `include=tables`, but should be considered deprecated as of 0.35.0, and will
+ be removed in a future release.
+
+ * `include_cards` here means we should also include virtual Table entries for saved Questions, e.g. so we can easily
+ use them as source Tables in queries. This is a deprecated alias for `saved=true` + `include=tables` (for the saved
+ questions virtual DB). Prefer using `include` and `saved` instead. "
+ [include_tables include_cards include saved]
{include_tables (s/maybe su/BooleanString)
- include_cards (s/maybe su/BooleanString)}
- (or (dbs-list (Boolean/parseBoolean include_tables) (Boolean/parseBoolean include_cards))
- []))
+ include_cards (s/maybe su/BooleanString)
+ include (s/maybe (s/eq "tables"))
+ saved (s/maybe su/BooleanString)}
+ (when (and config/is-dev?
+ (or include_tables include_cards))
+ ;; don't need to i18n since this is dev-facing only
+ (log/warn "GET /api/database?include_tables and ?include_cards are deprecated."
+ "Prefer using ?include=tables and ?saved=true instead."))
+ (let [include-tables? (cond
+ (seq include) (= include "tables")
+ (seq include_tables) (Boolean/parseBoolean include_tables))
+ include-saved-questions-db? (cond
+ (seq saved) (Boolean/parseBoolean saved)
+ (seq include_cards) (Boolean/parseBoolean include_cards))
+ include-saved-questions-tables? (when include-saved-questions-db?
+ (if (seq include_cards)
+ true
+ include-tables?))]
+ (or (dbs-list :include-tables? include-tables?
+ :include-saved-questions-db? include-saved-questions-db?
+ :include-saved-questions-tables? include-saved-questions-tables?)
+ [])))
;;; --------------------------------------------- GET /api/database/:id ----------------------------------------------
@@ -220,7 +248,7 @@
"Endpoint that provides metadata for the Saved Questions 'virtual' database. Used for fooling the frontend
and allowing it to treat the Saved Questions virtual DB just like any other database."
[]
- (saved-cards-virtual-db-metadata :include-fields? true))
+ (saved-cards-virtual-db-metadata :include-tables? true, :include-fields? true))
(defn- db-metadata [id]
(-> (api/read-check Database id)
diff --git a/test/metabase/api/database_test.clj b/test/metabase/api/database_test.clj
index 7ad8c8437e8..a43ddd49bf4 100644
--- a/test/metabase/api/database_test.clj
+++ b/test/metabase/api/database_test.clj
@@ -112,24 +112,6 @@
field
[:updated_at :id :created_at :last_analyzed :fingerprint :fingerprint_version :fk_target_field_id :position])))
-(deftest databases-list-test
- (testing "GET /api/database"
- (testing "Test that we can get all the DBs (ordered by name, then driver)"
- (testing "Database details *should not* come back for Rasta since she's not a superuser"
- (let [expected-keys (-> (into #{:features :native_permissions} (keys (Database (mt/id))))
- (disj :details))]
- (doseq [db ((mt/user->client :rasta) :get 200 "database")]
- (testing (format "Database %s %d %s" (:engine db) (u/get-id db) (pr-str (:name db)))
- (is (= expected-keys
- (set (keys db)))))))))
-
- (testing "?include_tables=true"
- (mt/with-temp Database [{db-id :id, db-name :name} {:engine (u/qualified-name ::test-driver)}]
- (doseq [db ((mt/user->client :rasta) :get 200 "database" :include_tables true)]
- (testing (format "Database %s %d %s" (:engine db) (u/get-id db) (pr-str (:name db)))
- (is (= (expected-tables db)
- (:tables db)))))))))
-
(defn- add-schedules [db]
(assoc db :schedules {:cache_field_values {:schedule_day nil
:schedule_frame nil
@@ -344,11 +326,6 @@
:description nil}
kvs))
-(defn- fetch-virtual-database []
- (some #(when (= (:name %) "Saved Questions")
- %)
- ((mt/user->client :crowberto) :get 200 "database" :include_cards true)))
-
(driver/register! ::no-nested-query-support
:parent :sql-jdbc
:abstract? true)
@@ -360,82 +337,116 @@
:source-table (mt/id :checkins))
:result_metadata [{:name "num_toucans"}]))
+(deftest databases-list-test
+ (testing "GET /api/database"
+ (testing "Test that we can get all the DBs (ordered by name, then driver)"
+ (testing "Database details *should not* come back for Rasta since she's not a superuser"
+ (let [expected-keys (-> (into #{:features :native_permissions} (keys (Database (mt/id))))
+ (disj :details))]
+ (doseq [db ((mt/user->client :rasta) :get 200 "database")]
+ (testing (format "Database %s %d %s" (:engine db) (u/get-id db) (pr-str (:name db)))
+ (is (= expected-keys
+ (set (keys db)))))))))
+
+ ;; ?include=tables and ?include_tables=true mean the same thing so test them both the same way
+ (doseq [query-param ["?include_tables=true"
+ "?include=tables"]]
+ (testing query-param
+ (mt/with-temp Database [{db-id :id, db-name :name} {:engine (u/qualified-name ::test-driver)}]
+ (doseq [db ((mt/user->client :rasta) :get 200 (str "database" query-param))]
+ (testing (format "Database %s %d %s" (:engine db) (u/get-id db) (pr-str (:name db)))
+ (is (= (expected-tables db)
+ (:tables db))))))))))
+
(deftest databases-list-include-saved-questions-test
- (testing "GET /api/database?include_cards=true"
- (testing "Check that we get back 'virtual' tables for Saved Questions"
- (testing "The saved questions virtual DB should be the last DB in the list"
- (mt/with-temp* [Card [card (card-with-native-query "Kanye West Quote Views Per Month")]]
- ;; run the Card which will populate its result_metadata column
- ((mt/user->client :crowberto) :post 202 (format "card/%d/query" (u/get-id card)))
- ;; Now fetch the database list. The 'Saved Questions' DB should be last on the list
- (is (= (-> card
- virtual-table-for-card
- saved-questions-virtual-db)
- (last ((mt/user->client :crowberto) :get 200 "database" :include_cards true))))))
-
- (testing "Make sure saved questions are NOT included if the setting is disabled"
- (mt/with-temp Card [card (card-with-native-query "Kanye West Quote Views Per Month")]
- (mt/with-temporary-setting-values [enable-nested-queries false]
- ;; run the Card which will populate its result_metadata column
- ((mt/user->client :crowberto) :post 202 (format "card/%d/query" (u/get-id card)))
- ;; Now fetch the database list. The 'Saved Questions' DB should NOT be in the list
- (is (= nil
- (some (fn [database]
- (when (= (u/get-id database) mbql.s/saved-questions-virtual-database-id)
- database))
- ((mt/user->client :crowberto) :get 200 "database" :include_cards true))))))))
-
- (testing "should pretend Collections are schemas"
- (mt/with-temp* [Collection [stamp-collection {:name "Stamps"}]
- Collection [coin-collection {:name "Coins"}]
- Card [stamp-card (card-with-native-query "Total Stamp Count", :collection_id (u/get-id stamp-collection))]
- Card [coin-card (card-with-native-query "Total Coin Count", :collection_id (u/get-id coin-collection))]]
- ;; run the Cards which will populate their result_metadata columns
- (doseq [card [stamp-card coin-card]]
- ((mt/user->client :crowberto) :post 202 (format "card/%d/query" (u/get-id card))))
- ;; Now fetch the database list. The 'Saved Questions' DB should be last on the list. Cards should have their
- ;; Collection name as their Schema
- (is (= (saved-questions-virtual-db
- (virtual-table-for-card coin-card :schema "Coins")
- (virtual-table-for-card stamp-card :schema "Stamps"))
- (last ((mt/user->client :crowberto) :get 200 "database" :include_cards true))))))
-
- (testing "should remove Cards that have ambiguous columns"
- (mt/with-temp* [Card [ok-card (assoc (card-with-native-query "OK Card") :result_metadata [{:name "cam"}])]
- Card [cambiguous-card (assoc (card-with-native-query "Cambiguous Card") :result_metadata [{:name "cam"} {:name "cam_2"}])]]
- (is (= (-> ok-card
- virtual-table-for-card
- saved-questions-virtual-db)
- (fetch-virtual-database)))))
-
- (testing "should remove Cards that belong to a driver that doesn't support nested queries"
- (mt/with-temp* [Database [bad-db {:engine ::no-nested-query-support, :details {}}]
- Card [bad-card {:name "Bad Card"
- :dataset_query {:database (u/get-id bad-db)
- :type :native
- :native {:query "[QUERY GOES HERE]"}}
- :result_metadata [{:name "sparrows"}]
- :database_id (u/get-id bad-db)}]
- Card [ok-card (assoc (card-with-native-query "OK Card")
- :result_metadata [{:name "finches"}])]]
- (is (= (-> ok-card
- virtual-table-for-card
- saved-questions-virtual-db)
- (fetch-virtual-database)))))
-
- (testing "should remove Cards that use cumulative-sum and cumulative-count aggregations"
- (mt/with-temp* [Card [ok-card (ok-mbql-card)]
- Card [_ (merge
- (mt/$ids checkins
- (card-with-mbql-query "Cum Count Card"
- :source-table $$checkins
- :aggregation [[:cum-count]]
- :breakout [!month.date]))
- {:result_metadata [{:name "num_toucans"}]})]]
- (is (= (-> ok-card
- virtual-table-for-card
- saved-questions-virtual-db)
- (fetch-virtual-database)))))))
+ (testing "GET /api/database?saved=true"
+ (testing "We should be able to include the saved questions virtual DB (without Tables) with the param ?saved=true"
+ (is (= {:name "Saved Questions"
+ :id mbql.s/saved-questions-virtual-database-id
+ :features ["basic-aggregations"]
+ :is_saved_questions true}
+ (last ((mt/user->client :lucky) :get 200 "database?saved=true")))))))
+
+(deftest databases-list-include-saved-questions-tables-test
+ ;; `?saved=true&include=tables` and `?include_cards=true` mean the same thing, so test them both
+ (doseq [params ["?saved=true&include=tables"
+ "?include_cards=true"]]
+ (testing (str "GET /api/database" params)
+ (letfn [(fetch-virtual-database []
+ (some #(when (= (:name %) "Saved Questions")
+ %)
+ ((mt/user->client :crowberto) :get 200 (str "database" params))))]
+ (testing "Check that we get back 'virtual' tables for Saved Questions"
+ (testing "The saved questions virtual DB should be the last DB in the list"
+ (mt/with-temp* [Card [card (card-with-native-query "Kanye West Quote Views Per Month")]]
+ ;; run the Card which will populate its result_metadata column
+ ((mt/user->client :crowberto) :post 202 (format "card/%d/query" (u/get-id card)))
+ ;; Now fetch the database list. The 'Saved Questions' DB should be last on the list
+ (is (= (-> card
+ virtual-table-for-card
+ saved-questions-virtual-db)
+ (last ((mt/user->client :crowberto) :get 200 (str "database" params)))))))
+
+ (testing "Make sure saved questions are NOT included if the setting is disabled"
+ (mt/with-temp Card [card (card-with-native-query "Kanye West Quote Views Per Month")]
+ (mt/with-temporary-setting-values [enable-nested-queries false]
+ ;; run the Card which will populate its result_metadata column
+ ((mt/user->client :crowberto) :post 202 (format "card/%d/query" (u/get-id card)))
+ ;; Now fetch the database list. The 'Saved Questions' DB should NOT be in the list
+ (is (= nil
+ (fetch-virtual-database)))))))
+
+ (testing "should pretend Collections are schemas"
+ (mt/with-temp* [Collection [stamp-collection {:name "Stamps"}]
+ Collection [coin-collection {:name "Coins"}]
+ Card [stamp-card (card-with-native-query "Total Stamp Count", :collection_id (u/get-id stamp-collection))]
+ Card [coin-card (card-with-native-query "Total Coin Count", :collection_id (u/get-id coin-collection))]]
+ ;; run the Cards which will populate their result_metadata columns
+ (doseq [card [stamp-card coin-card]]
+ ((mt/user->client :crowberto) :post 202 (format "card/%d/query" (u/get-id card))))
+ ;; Now fetch the database list. The 'Saved Questions' DB should be last on the list. Cards should have their
+ ;; Collection name as their Schema
+ (is (= (saved-questions-virtual-db
+ (virtual-table-for-card coin-card :schema "Coins")
+ (virtual-table-for-card stamp-card :schema "Stamps"))
+ (last ((mt/user->client :crowberto) :get 200 (str "database" params)))))))
+
+ (testing "should remove Cards that have ambiguous columns"
+ (mt/with-temp* [Card [ok-card (assoc (card-with-native-query "OK Card") :result_metadata [{:name "cam"}])]
+ Card [cambiguous-card (assoc (card-with-native-query "Cambiguous Card") :result_metadata [{:name "cam"} {:name "cam_2"}])]]
+ (is (= (-> ok-card
+ virtual-table-for-card
+ saved-questions-virtual-db)
+ (fetch-virtual-database)))))
+
+ (testing "should remove Cards that belong to a driver that doesn't support nested queries"
+ (mt/with-temp* [Database [bad-db {:engine ::no-nested-query-support, :details {}}]
+ Card [bad-card {:name "Bad Card"
+ :dataset_query {:database (u/get-id bad-db)
+ :type :native
+ :native {:query "[QUERY GOES HERE]"}}
+ :result_metadata [{:name "sparrows"}]
+ :database_id (u/get-id bad-db)}]
+ Card [ok-card (assoc (card-with-native-query "OK Card")
+ :result_metadata [{:name "finches"}])]]
+ (is (= (-> ok-card
+ virtual-table-for-card
+ saved-questions-virtual-db)
+ (fetch-virtual-database)))))
+
+ (testing "should remove Cards that use cumulative-sum and cumulative-count aggregations"
+ (mt/with-temp* [Card [ok-card (ok-mbql-card)]
+ Card [_ (merge
+ (mt/$ids checkins
+ (card-with-mbql-query "Cum Count Card"
+ :source-table $$checkins
+ :aggregation [[:cum-count]]
+ :breakout [!month.date]))
+ {:result_metadata [{:name "num_toucans"}]})]]
+ (is (= (-> ok-card
+ virtual-table-for-card
+ saved-questions-virtual-db)
+ (fetch-virtual-database)))))))))
(deftest db-metadata-saved-questions-db-test
(testing "GET /api/database/:id/metadata works for the Saved Questions 'virtual' database"
@@ -453,9 +464,13 @@
((mt/user->client :crowberto) :get 200
(format "database/%d/metadata" mbql.s/saved-questions-virtual-database-id)))))
- (testing "if no eligible Saved Questions exist the virtual DB metadata endpoint should just return `nil`"
- (is (= nil
- ((mt/user->client :crowberto) :get 204
+ (testing "if no eligible Saved Questions exist the endpoint should return empty tables"
+ (is (= {:name "Saved Questions"
+ :id mbql.s/saved-questions-virtual-database-id
+ :features ["basic-aggregations"]
+ :is_saved_questions true
+ :tables []}
+ ((mt/user->client :crowberto) :get 200
(format "database/%d/metadata" mbql.s/saved-questions-virtual-database-id)))))))
@@ -537,46 +552,48 @@
(is (= true
(deref analyze-called? long-timeout :analyze-never-called)))))))))
-;; (Non-admins should not be allowed to trigger sync)
(deftest non-admins-cant-trigger-sync
- (is (= "You don't have permissions to do that."
- ((mt/user->client :rasta) :post 403 (format "database/%d/sync_schema" (mt/id))))))
+ (testing "Non-admins should not be allowed to trigger sync"
+ (is (= "You don't have permissions to do that."
+ ((mt/user->client :rasta) :post 403 (format "database/%d/sync_schema" (mt/id)))))))
-;; Can we RESCAN all the FieldValues for a DB?
(deftest can-rescan-fieldvalues-for-a-db
- (is (= :sync-called
- (let [update-field-values-called? (promise)]
- (mt/with-temp Database [db {:engine "h2", :details (:details (mt/db))}]
- (with-redefs [field-values/update-field-values! (fn [synced-db]
- (when (= (u/get-id synced-db) (u/get-id db))
- (deliver update-field-values-called? :sync-called)))]
- ((mt/user->client :crowberto) :post 200 (format "database/%d/rescan_values" (u/get-id db)))
- (deref update-field-values-called? long-timeout :sync-never-called)))))))
-
-;; (Non-admins should not be allowed to trigger re-scan)
+ (testing "Can we RESCAN all the FieldValues for a DB?"
+ (let [update-field-values-called? (promise)]
+ (mt/with-temp Database [db {:engine "h2", :details (:details (mt/db))}]
+ (with-redefs [field-values/update-field-values! (fn [synced-db]
+ (when (= (u/get-id synced-db) (u/get-id db))
+ (deliver update-field-values-called? :sync-called)))]
+ ((mt/user->client :crowberto) :post 200 (format "database/%d/rescan_values" (u/get-id db)))
+ (is (= :sync-called
+ (deref update-field-values-called? long-timeout :sync-never-called))))))))
+
(deftest nonadmins-cant-trigger-rescan
- (is (= "You don't have permissions to do that."
- ((mt/user->client :rasta) :post 403 (format "database/%d/rescan_values" (mt/id))))))
+ (testing "Non-admins should not be allowed to trigger re-scan"
+ (is (= "You don't have permissions to do that."
+ ((mt/user->client :rasta) :post 403 (format "database/%d/rescan_values" (mt/id)))))))
-;; Can we DISCARD all the FieldValues for a DB?
(deftest discard-db-fieldvalues
- (is (= {:values-1-still-exists? false
- :values-2-still-exists? false}
- (mt/with-temp* [Database [db {:engine "h2", :details (:details (mt/db))}]
- Table [table-1 {:db_id (u/get-id db)}]
- Table [table-2 {:db_id (u/get-id db)}]
- Field [field-1 {:table_id (u/get-id table-1)}]
- Field [field-2 {:table_id (u/get-id table-2)}]
- FieldValues [values-1 {:field_id (u/get-id field-1), :values [1 2 3 4]}]
- FieldValues [values-2 {:field_id (u/get-id field-2), :values [1 2 3 4]}]]
- ((mt/user->client :crowberto) :post 200 (format "database/%d/discard_values" (u/get-id db)))
- {:values-1-still-exists? (db/exists? FieldValues :id (u/get-id values-1))
- :values-2-still-exists? (db/exists? FieldValues :id (u/get-id values-2))}))))
-
-;; (Non-admins should not be allowed to discard all FieldValues)
+ (testing "Can we DISCARD all the FieldValues for a DB?"
+ (mt/with-temp* [Database [db {:engine "h2", :details (:details (mt/db))}]
+ Table [table-1 {:db_id (u/get-id db)}]
+ Table [table-2 {:db_id (u/get-id db)}]
+ Field [field-1 {:table_id (u/get-id table-1)}]
+ Field [field-2 {:table_id (u/get-id table-2)}]
+ FieldValues [values-1 {:field_id (u/get-id field-1), :values [1 2 3 4]}]
+ FieldValues [values-2 {:field_id (u/get-id field-2), :values [1 2 3 4]}]]
+ ((mt/user->client :crowberto) :post 200 (format "database/%d/discard_values" (u/get-id db)))
+ (testing "values-1 still exists?"
+ (is (= false
+ (db/exists? FieldValues :id (u/get-id values-1)))))
+ (testing "values-2 still exists?"
+ (is (= false
+ (db/exists? FieldValues :id (u/get-id values-2))))))))
+
(deftest nonadmins-cant-discard-all-fieldvalues
- (is (= "You don't have permissions to do that."
- ((mt/user->client :rasta) :post 403 (format "database/%d/discard_values" (mt/id))))))
+ (testing "Non-admins should not be allowed to discard all FieldValues"
+ (is (= "You don't have permissions to do that."
+ ((mt/user->client :rasta) :post 403 (format "database/%d/discard_values" (mt/id)))))))
;; For some stupid reason the *real* version of `test-database-connection` is set up to do nothing for tests. I'm
--
GitLab