diff --git a/src/metabase/api/database.clj b/src/metabase/api/database.clj
index abb88feaf27946a3a533950859fef1c5135d7ac4..d3613b50e0f77da5566ec19914c0e6b1776c7b8f 100644
--- a/src/metabase/api/database.clj
+++ b/src/metabase/api/database.clj
@@ -63,9 +63,9 @@
(defendpoint POST "/"
"Add a new `Database`."
[:as {{:keys [name engine details is_full_sync]} :body}]
- {name [Required NonEmptyString]
- engine [Required DBEngine]
- details [Required Dict]}
+ {name [Required NonEmptyString]
+ engine [Required DBEngine]
+ details [Required Dict]}
(check-superuser)
;; this function tries connecting over ssl and non-ssl to establish a connection
;; if it succeeds it returns the `details` that worked, otherwise it returns an error
@@ -85,7 +85,8 @@
(if-not (false? (:valid details-or-error))
;; no error, proceed with creation
(let-500 [new-db (db/insert! Database, :name name, :engine engine, :details details-or-error, :is_full_sync is_full_sync)]
- (events/publish-event :database-create new-db))
+ (events/publish-event :database-create new-db)
+ new-db)
;; failed to connect, return error
{:status 400
:body details-or-error})))
diff --git a/test/metabase/api/activity_test.clj b/test/metabase/api/activity_test.clj
index fe3ead213301f8eee02ead31276e008b58abf98f..909ef4ae7fe81e940b3f79bc437a69452fc361b6 100644
--- a/test/metabase/api/activity_test.clj
+++ b/test/metabase/api/activity_test.clj
@@ -9,7 +9,7 @@
[view-log :refer [ViewLog]])
[metabase.test.data :refer :all]
[metabase.test.data.users :refer :all]
- [metabase.test.util :refer [match-$ random-name expect-with-temp resolve-private-fns]]
+ [metabase.test.util :refer [match-$ expect-with-temp resolve-private-fns]]
[metabase.util :as u]))
;; GET /
@@ -99,17 +99,28 @@
:table nil
:custom_id nil
:details $})]
- (->> ((user->client :crowberto) :get 200 "activity")
- (map #(dissoc % :timestamp))))
+ (for [activity ((user->client :crowberto) :get 200 "activity")]
+ (dissoc activity :timestamp)))
-;; GET /recent_views
+;;; GET /recent_views
-; Things we are testing for:
-; 1. ordering is sorted by most recent
-; 2. results are filtered to current user
-; 3. `:model_object` is hydrated in each result
-; 4. we filter out entries where `:model_object` is nil (object doesn't exist)
+;; Things we are testing for:
+;; 1. ordering is sorted by most recent
+;; 2. results are filtered to current user
+;; 3. `:model_object` is hydrated in each result
+;; 4. we filter out entries where `:model_object` is nil (object doesn't exist)
+
+(defn- create-view! [user model model-id]
+ (db/insert! ViewLog
+ :user_id user
+ :model model
+ :model_id model-id
+ :timestamp (u/new-sql-timestamp))
+ ;; we sleep a bit to ensure no events have the same timestamp
+ ;; sadly, MySQL doesn't support milliseconds so we have to wait a second
+ ;; otherwise our records are out of order and this test fails :(
+ (Thread/sleep 1000))
(expect-with-temp [Card [card1 {:name "rand-name"
:creator_id (user->id :crowberto)
@@ -127,47 +138,37 @@
:display "table"
:dataset_query {}
:visualization_settings {}}]]
- [{:cnt 1
- :user_id (user->id :crowberto)
- :model "card"
- :model_id (:id card1)
+ [{:cnt 1
+ :user_id (user->id :crowberto)
+ :model "card"
+ :model_id (:id card1)
:model_object {:id (:id card1)
:name (:name card1)
:description (:description card1)
:display (name (:display card1))}}
- {:cnt 1
- :user_id (user->id :crowberto)
- :model "dashboard"
- :model_id (:id dash1)
+ {:cnt 1
+ :user_id (user->id :crowberto)
+ :model "dashboard"
+ :model_id (:id dash1)
:model_object {:id (:id dash1)
:name (:name dash1)
:description (:description dash1)}}
- {:cnt 1
- :user_id (user->id :crowberto)
- :model "card"
- :model_id (:id card2)
+ {:cnt 1
+ :user_id (user->id :crowberto)
+ :model "card"
+ :model_id (:id card2)
:model_object {:id (:id card2)
:name (:name card2)
:description (:description card2)
:display (name (:display card2))}}]
- (let [create-view (fn [user model model-id]
- (db/insert! ViewLog
- :user_id user
- :model model
- :model_id model-id
- :timestamp (u/new-sql-timestamp))
- ;; we sleep a bit to ensure no events have the same timestamp
- ;; sadly, MySQL doesn't support milliseconds so we have to wait a second
- ;; otherwise our records are out of order and this test fails :(
- (Thread/sleep 1000))]
- (do
- (create-view (user->id :crowberto) "card" (:id card2))
- (create-view (user->id :crowberto) "dashboard" (:id dash1))
- (create-view (user->id :crowberto) "card" (:id card1))
- (create-view (user->id :crowberto) "card" 36478)
- (create-view (user->id :rasta) "card" (:id card1))
- (->> ((user->client :crowberto) :get 200 "activity/recent_views")
- (map #(dissoc % :max_ts))))))
+ (do
+ (create-view! (user->id :crowberto) "card" (:id card2))
+ (create-view! (user->id :crowberto) "dashboard" (:id dash1))
+ (create-view! (user->id :crowberto) "card" (:id card1))
+ (create-view! (user->id :crowberto) "card" 36478)
+ (create-view! (user->id :rasta) "card" (:id card1))
+ (for [recent-view ((user->client :crowberto) :get 200 "activity/recent_views")]
+ (dissoc recent-view :max_ts))))
;;; activities->referenced-objects, referenced-objects->existing-objects, add-model-exists-info
diff --git a/test/metabase/api/database_test.clj b/test/metabase/api/database_test.clj
index 87e6eca2beb10872fe560577dd53580f4a3e6c2b..5f433ff72bd1e16c149cef4ea9705372f0fdc713 100644
--- a/test/metabase/api/database_test.clj
+++ b/test/metabase/api/database_test.clj
@@ -13,22 +13,40 @@
;; HELPER FNS
-(defn create-db
- ([db-name]
- (create-db db-name true))
- ([db-name full-sync?]
- ((user->client :crowberto) :post 200 "database" {:engine :postgres
- :name db-name
- :details {:host "localhost"
- :port 5432
- :dbname "fakedb"
- :user "cam"
- :ssl false}
- :is_full_sync full-sync?})))
+(defn- create-db-via-api! [options]
+ ((user->client :crowberto) :post 200 "database" (merge {:engine :postgres
+ :name (tu/random-name)
+ :details {:host "localhost"
+ :port 5432
+ :dbname "fakedb"
+ :user "cam"
+ :ssl false}
+ :is_full_sync true}
+ options)))
+
+(defn- do-with-temp-db-created-via-api [db-options f]
+ (let [db (create-db-via-api! db-options)]
+ (assert (integer? (:id db)))
+ (try
+ (f db)
+ (finally
+ (db/cascade-delete! Database :id (:id db))))))
+
+(defmacro ^:private expect-with-temp-db-created-via-api {:style/indent 1} [[binding & [options]] expected actual]
+ ;; use `gensym` instead of auto gensym here so we can be sure it's a unique symbol every time. Otherwise since expectations hashes its body
+ ;; to generate function names it will treat every usage this as the same test and only a single one will end up being ran
+ (let [result (gensym "result-")]
+ `(let [~result (delay (do-with-temp-db-created-via-api ~options (fn [~binding]
+ [~expected
+ ~actual])))]
+ (expect
+ (u/ignore-exceptions (first @~result)) ; in case @result# barfs we don't want the test to succeed (Exception == Exception for expectations)
+ (second @~result)))))
+
(defn- db-details
([]
- (db-details (db)))
+ (db-details (db)))
([db]
(match-$ db
{:created_at $
@@ -43,23 +61,6 @@
:description nil
:features (mapv name (driver/features (driver/engine->driver (:engine db))))})))
-(defn- table-details [table]
- (match-$ table
- {:description $
- :entity_type $
- :visibility_type $
- :schema $
- :name $
- :display_name $
- :rows $
- :updated_at $
- :entity_name $
- :active $
- :id $
- :db_id $
- :raw_table_id $
- :created_at $}))
-
;; # DB LIFECYCLE ENDPOINTS
@@ -76,140 +77,142 @@
;; ## POST /api/database
;; Check that we can create a Database
-(let [db-name (random-name)]
- (expect-eval-actual-first
- (match-$ (Database :name db-name)
- {:created_at $
- :engine "postgres" ; string because it's coming back from API instead of DB
- :id $
- :details {:host "localhost", :port 5432, :dbname "fakedb", :user "cam", :ssl true}
- :updated_at $
- :name db-name
- :is_sample false
- :is_full_sync false
- :organization_id nil
- :description nil
- :features (mapv name (driver/features (driver/engine->driver :postgres)))})
- (create-db db-name false)))
+(expect-with-temp-db-created-via-api [db {:is_full_sync false}]
+ (match-$ db
+ {:created_at $
+ :engine :postgres
+ :id $
+ :details {:host "localhost", :port 5432, :dbname "fakedb", :user "cam", :ssl true}
+ :updated_at $
+ :name $
+ :is_sample false
+ :is_full_sync false
+ :organization_id nil
+ :description nil
+ :features (into [] (driver/features (driver/engine->driver :postgres)))})
+ (Database (:id db)))
+
;; ## DELETE /api/database/:id
;; Check that we can delete a Database
-(expect-let [db-name (random-name)
- {db-id :id} (create-db db-name)
- sel-db-name (fn [] (db/select-one-field :name Database, :id db-id))]
- [db-name
- nil]
- [(sel-db-name)
- (do ((user->client :crowberto) :delete 204 (format "database/%d" db-id))
- (sel-db-name))])
+(expect-with-temp-db-created-via-api [db]
+ false
+ (do ((user->client :crowberto) :delete 204 (format "database/%d" (:id db)))
+ (db/exists? 'Database :id (:id db))))
;; ## PUT /api/database/:id
;; Check that we can update fields in a Database
-(expect-let [[old-name new-name] (repeatedly 2 random-name)
- {db-id :id} (create-db old-name)
- sel-db (fn [] (dissoc (into {} (db/select-one [Database :name :engine :details :is_full_sync], :id db-id))
- :features))]
- [{:details {:host "localhost", :port 5432, :dbname "fakedb", :user "cam", :ssl true}
- :engine :postgres
- :name old-name
- :is_full_sync true}
- {:details {:host "localhost", :port 5432, :dbname "fakedb", :user "rastacan"}
- :engine :h2
- :name new-name
- :is_full_sync false}]
- [(sel-db)
- ;; Check that we can update all the fields
- (do ((user->client :crowberto) :put 200 (format "database/%d" db-id) {:name new-name
- :engine "h2"
- :is_full_sync false
- :details {:host "localhost", :port 5432, :dbname "fakedb", :user "rastacan"}})
- (sel-db))])
+(expect-with-temp-db-created-via-api [{db-id :id}]
+ {:details {:host "localhost", :port 5432, :dbname "fakedb", :user "rastacan"}
+ :engine :h2
+ :name "Cam's Awesome Toucan Database"
+ :is_full_sync false}
+ (do ((user->client :crowberto) :put 200 (format "database/%d" db-id) {:name "Cam's Awesome Toucan Database"
+ :engine "h2"
+ :is_full_sync false
+ :details {:host "localhost", :port 5432, :dbname "fakedb", :user "rastacan"}})
+ (dissoc (into {} (db/select-one [Database :name :engine :details :is_full_sync], :id db-id))
+ :features)))
+
+
+(defn- table-details [table]
+ (match-$ table
+ {:description $
+ :entity_type $
+ :visibility_type $
+ :schema $
+ :name $
+ :display_name $
+ :rows $
+ :updated_at $
+ :entity_name $
+ :active $
+ :id $
+ :db_id $
+ :raw_table_id $
+ :created_at $}))
+
+
+;; TODO - this is a test code smell, each test should clean up after itself and this step shouldn't be neccessary. One day we should be able to remove this!
+;; If you're writing a test that needs this, fix your brain and your test
+(defn- ^:deprecated delete-randomly-created-databases!
+ "Delete all the randomly created Databases we've made so far. Optionally specify one or more IDs to SKIP."
+ [& {:keys [skip]}]
+ (db/cascade-delete! Database :id [:not-in (into (set skip)
+ (for [engine datasets/all-valid-engines
+ :let [id (datasets/when-testing-engine engine
+ (:id (get-or-create-test-data-db! (driver/engine->driver engine))))]
+ :when id]
+ id))]))
-;; # DATABASES FOR ORG
;; ## GET /api/database
-;; Test that we can get all the DBs for an Org, ordered by name
+;; Test that we can get all the DBs (ordered by name)
;; Database details *should not* come back for Rasta since she's not a superuser
-(let [db-name (str "A" (random-name))] ; make sure this name comes before "test-data"
- (expect-eval-actual-first
- (set (filter identity (conj (for [engine datasets/all-valid-engines]
- (datasets/when-testing-engine engine
- (match-$ (get-or-create-test-data-db! (driver/engine->driver engine))
- {:created_at $
- :engine (name $engine)
- :id $
- :updated_at $
- :name "test-data"
- :is_sample false
- :is_full_sync true
- :organization_id nil
- :description nil
- :features (mapv name (driver/features (driver/engine->driver engine)))})))
- ;; (?) I don't remember why we have to do this for postgres but not any other of the bonus drivers
- (match-$ (Database :name db-name)
+(expect-with-temp-db-created-via-api [{db-id :id}]
+ (set (filter identity (conj (for [engine datasets/all-valid-engines]
+ (datasets/when-testing-engine engine
+ (match-$ (get-or-create-test-data-db! (driver/engine->driver engine))
{:created_at $
- :engine "postgres"
+ :engine (name $engine)
:id $
:updated_at $
- :name $
+ :name "test-data"
:is_sample false
:is_full_sync true
:organization_id nil
:description nil
- :features (mapv name (driver/features (driver/engine->driver :postgres)))}))))
- (do
- ;; Delete all the randomly created Databases we've made so far
- (db/cascade-delete! Database :id [:not-in (filter identity
- (for [engine datasets/all-valid-engines]
- (datasets/when-testing-engine engine
- (:id (get-or-create-test-data-db! (driver/engine->driver engine))))))])
- ;; Add an extra DB so we have something to fetch besides the Test DB
- (create-db db-name)
- ;; Now hit the endpoint
- (set ((user->client :rasta) :get 200 "database")))))
+ :features (map name (driver/features (driver/engine->driver engine)))})))
+ (match-$ (Database db-id)
+ {:created_at $
+ :engine "postgres"
+ :id $
+ :updated_at $
+ :name $
+ :is_sample false
+ :is_full_sync true
+ :organization_id nil
+ :description nil
+ :features (map name (driver/features (driver/engine->driver :postgres)))}))))
+ (do
+ (delete-randomly-created-databases! :skip [db-id])
+ (set ((user->client :rasta) :get 200 "database"))))
+
+
;; GET /api/databases (include tables)
-(let [db-name (str "A" (random-name))] ; make sure this name comes before "test-data"
- (expect-eval-actual-first
- (set (concat [(match-$ (Database :name db-name)
- {:created_at $
- :engine "postgres"
- :id $
- :updated_at $
- :name $
- :is_sample false
- :is_full_sync true
- :organization_id nil
- :description nil
- :tables []
- :features (mapv name (driver/features (driver/engine->driver :postgres)))})]
- (filter identity (for [engine datasets/all-valid-engines]
- (datasets/when-testing-engine engine
- (let [database (get-or-create-test-data-db! (driver/engine->driver engine))]
- (match-$ database
- {:created_at $
- :engine (name $engine)
- :id $
- :updated_at $
- :name "test-data"
- :is_sample false
- :is_full_sync true
- :organization_id nil
- :description nil
- :tables (->> (db/select Table, :db_id (:id database))
- (mapv table-details)
- (sort-by :name))
- :features (mapv name (driver/features (driver/engine->driver engine)))})))))))
- (do
- ;; Delete all the randomly created Databases we've made so far
- (db/cascade-delete! Database :id [:not-in (set (filter identity
- (for [engine datasets/all-valid-engines]
- (datasets/when-testing-engine engine
- (:id (get-or-create-test-data-db! (driver/engine->driver engine)))))))])
- ;; Add an extra DB so we have something to fetch besides the Test DB
- (create-db db-name)
- ;; Now hit the endpoint
- (set ((user->client :rasta) :get 200 "database" :include_tables true)))))
+(expect-with-temp-db-created-via-api [{db-id :id}]
+ (set (cons (match-$ (Database db-id)
+ {:created_at $
+ :engine "postgres"
+ :id $
+ :updated_at $
+ :name $
+ :is_sample false
+ :is_full_sync true
+ :organization_id nil
+ :description nil
+ :tables []
+ :features (map name (driver/features (driver/engine->driver :postgres)))})
+ (filter identity (for [engine datasets/all-valid-engines]
+ (datasets/when-testing-engine engine
+ (let [database (get-or-create-test-data-db! (driver/engine->driver engine))]
+ (match-$ database
+ {:created_at $
+ :engine (name $engine)
+ :id $
+ :updated_at $
+ :name "test-data"
+ :is_sample false
+ :is_full_sync true
+ :organization_id nil
+ :description nil
+ :tables (sort-by :name (for [table (db/select Table, :db_id (:id database))]
+ (table-details table)))
+ :features (map name (driver/features (driver/engine->driver engine)))})))))))
+ (do
+ (delete-randomly-created-databases! :skip [db-id])
+ (set ((user->client :rasta) :get 200 "database" :include_tables true))))
;; ## GET /api/meta/table/:id/query_metadata
;; TODO - add in example with Field :values
@@ -293,65 +296,65 @@
;; ## GET /api/database/:id/tables
;; These should come back in alphabetical order
(expect
- (let [db-id (id)]
- [(match-$ (Table (id :categories))
- {:description nil
- :entity_type nil
- :visibility_type nil
- :schema "PUBLIC"
- :name "CATEGORIES"
- :rows 75
- :updated_at $
- :entity_name nil
- :active true
- :id $
- :db_id db-id
- :created_at $
- :display_name "Categories"
- :raw_table_id $})
- (match-$ (Table (id :checkins))
- {:description nil
- :entity_type nil
- :visibility_type nil
- :schema "PUBLIC"
- :name "CHECKINS"
- :rows 1000
- :updated_at $
- :entity_name nil
- :active true
- :id $
- :db_id db-id
- :created_at $
- :display_name "Checkins"
- :raw_table_id $})
- (match-$ (Table (id :users))
- {:description nil
- :entity_type nil
- :visibility_type nil
- :schema "PUBLIC"
- :name "USERS"
- :rows 15
- :updated_at $
- :entity_name nil
- :active true
- :id $
- :db_id db-id
- :created_at $
- :display_name "Users"
- :raw_table_id $})
- (match-$ (Table (id :venues))
- {:description nil
- :entity_type nil
- :visibility_type nil
- :schema "PUBLIC"
- :name "VENUES"
- :rows 100
- :updated_at $
- :entity_name nil
- :active true
- :id $
- :db_id db-id
- :created_at $
- :display_name "Venues"
- :raw_table_id $})])
+ (let [db-id (id)]
+ [(match-$ (Table (id :categories))
+ {:description nil
+ :entity_type nil
+ :visibility_type nil
+ :schema "PUBLIC"
+ :name "CATEGORIES"
+ :rows 75
+ :updated_at $
+ :entity_name nil
+ :active true
+ :id $
+ :db_id db-id
+ :created_at $
+ :display_name "Categories"
+ :raw_table_id $})
+ (match-$ (Table (id :checkins))
+ {:description nil
+ :entity_type nil
+ :visibility_type nil
+ :schema "PUBLIC"
+ :name "CHECKINS"
+ :rows 1000
+ :updated_at $
+ :entity_name nil
+ :active true
+ :id $
+ :db_id db-id
+ :created_at $
+ :display_name "Checkins"
+ :raw_table_id $})
+ (match-$ (Table (id :users))
+ {:description nil
+ :entity_type nil
+ :visibility_type nil
+ :schema "PUBLIC"
+ :name "USERS"
+ :rows 15
+ :updated_at $
+ :entity_name nil
+ :active true
+ :id $
+ :db_id db-id
+ :created_at $
+ :display_name "Users"
+ :raw_table_id $})
+ (match-$ (Table (id :venues))
+ {:description nil
+ :entity_type nil
+ :visibility_type nil
+ :schema "PUBLIC"
+ :name "VENUES"
+ :rows 100
+ :updated_at $
+ :entity_name nil
+ :active true
+ :id $
+ :db_id db-id
+ :created_at $
+ :display_name "Venues"
+ :raw_table_id $})])
((user->client :rasta) :get 200 (format "database/%d/tables" (id))))