diff --git a/src/metabase/api/notify.clj b/src/metabase/api/notify.clj index 4d43fde3107d038025f1d208be111a49d65cad06..a6a830f2b23dafbcc4ba92bef372507c96bc3f6c 100644 --- a/src/metabase/api/notify.clj +++ b/src/metabase/api/notify.clj @@ -3,10 +3,15 @@ (:require [compojure.core :refer [POST]] [metabase.api.common :as api] + [metabase.driver :as driver] + [metabase.driver.util :as driver.u] [metabase.models.database :refer [Database]] [metabase.models.table :refer [Table]] [metabase.sync :as sync] [metabase.sync.sync-metadata :as sync-metadata] + [metabase.sync.sync-metadata.tables :as sync-tables] + [metabase.sync.util :as sync-util] + [metabase.util.i18n :refer [trs]] [metabase.util.schema :as su] [schema.core :as s] [toucan.db :as db])) @@ -36,5 +41,41 @@ synchronous? deref))) {:success true}) +(defn- without-stacktrace [^Throwable throwable] + (doto throwable + (.setStackTrace (make-array StackTraceElement 0)))) + +#_{:clj-kondo/ignore [:deprecated-var]} +(api/defendpoint-schema POST "/db/:id/new-table" + "Sync a new table without running a full database sync. Requires `schema_name` and `table_name`. Will throw an error + if the table already exists in Metabase or cannot be found." + [id :as {{:keys [schema_name table_name]} :body}] + {schema_name su/NonBlankString + table_name su/NonBlankString} + (api/let-404 [database (db/select-one Database :id id)] + (if-not (db/select-one Table :db_id id :name table_name :schema schema_name) + (let [driver (driver.u/database->driver database) + {db-tables :tables} (driver/describe-database driver database)] + (if-let [table (some (fn [table-in-db] + (when (= (dissoc table-in-db :description) + {:schema schema_name :name table_name}) + table-in-db)) + db-tables)] + (let [created (sync-tables/create-or-reactivate-table! database table)] + (doto created + sync/sync-table! + sync-util/set-initial-table-sync-complete!)) + (throw (without-stacktrace + (ex-info (trs "Unable to identify table ''{0}.{1}''" + schema_name table_name) + {:status-code 404 + :schema_name schema_name + :table_name table_name}))))) + (throw (without-stacktrace + (ex-info (trs "Table ''{0}.{1}'' already exists" + schema_name table_name) + {:status-code 400 + :schema_name schema_name + :table_name table_name})))))) (api/define-routes) diff --git a/src/metabase/sync/sync_metadata/tables.clj b/src/metabase/sync/sync_metadata/tables.clj index 7bf3c1e1216b2abb1792e939666a7224e035e232..d905e366999c0a4a619a81d039a52b4f4c666698 100644 --- a/src/metabase/sync/sync_metadata/tables.clj +++ b/src/metabase/sync/sync_metadata/tables.clj @@ -9,7 +9,7 @@ [metabase.models.interface :as mi] [metabase.models.permissions :as perms] [metabase.models.permissions-group :as perms-group] - [metabase.models.table :as table :refer [Table]] + [metabase.models.table :refer [Table]] [metabase.sync.fetch-metadata :as fetch-metadata] [metabase.sync.interface :as i] [metabase.sync.sync-metadata.metabase-metadata :as metabase-metadata] @@ -90,8 +90,31 @@ [database :- i/DatabaseInstance db-metadata :- i/DatabaseMetadata] (log/info (trs "Found new version for DB: {0}" (:version db-metadata))) (db/update! Database (u/the-id database) - :details - (assoc (:details database) :version (:version db-metadata)))) + :details + (assoc (:details database) :version (:version db-metadata)))) + +(defn create-or-reactivate-table! + "Create a single new table in the database, or mark it as active if it already exists." + [database {schema :schema, table-name :name, :as table}] + (if-let [existing-id (db/select-one-id Table + :db_id (u/the-id database) + :schema schema + :name table-name + :active false)] + ;; if the table already exists but is marked *inactive*, mark it as *active* + (db/update! Table existing-id + :active true) + ;; otherwise create a new Table + (let [is-crufty? (is-crufty-table? table)] + (db/insert! Table + :db_id (u/the-id database) + :schema schema + :name table-name + :display_name (humanization/name->human-readable-name table-name) + :active true + :visibility_type (when is-crufty? :cruft) + ;; if this is a crufty table, mark initial sync as complete since we'll skip the subsequent sync steps + :initial_sync_status (if is-crufty? "complete" "incomplete"))))) ;; TODO - should we make this logic case-insensitive like it is for fields? @@ -101,26 +124,8 @@ (log/info (trs "Found new tables:") (for [table new-tables] (sync-util/name-for-logging (mi/instance Table table)))) - (doseq [{schema :schema, table-name :name, :as table} new-tables] - (if-let [existing-id (db/select-one-id Table - :db_id (u/the-id database) - :schema schema - :name table-name - :active false)] - ;; if the table already exists but is marked *inactive*, mark it as *active* - (db/update! Table existing-id - :active true) - ;; otherwise create a new Table - (let [is-crufty? (is-crufty-table? table)] - (db/insert! Table - :db_id (u/the-id database) - :schema schema - :name table-name - :display_name (humanization/name->human-readable-name table-name) - :active true - :visibility_type (when is-crufty? :cruft) - ;; if this is a crufty table, mark initial sync as complete since we'll skip the subsequent sync steps - :initial_sync_status (if is-crufty? "complete" "incomplete")))))) + (doseq [table new-tables] + (create-or-reactivate-table! database table))) (s/defn ^:private retire-tables! diff --git a/test/metabase/api/notify_test.clj b/test/metabase/api/notify_test.clj index e51ef120df11375bcf88afad1aca3d74ac36678f..a532c2b5f7a32853a2a288a9a379a7d491f1e015 100644 --- a/test/metabase/api/notify_test.clj +++ b/test/metabase/api/notify_test.clj @@ -1,16 +1,19 @@ (ns metabase.api.notify-test (:require [clj-http.client :as http] + [clojure.java.jdbc :as jdbc] [clojure.test :refer :all] + [metabase.driver.sql-jdbc.connection :as sql-jdbc.conn] [metabase.http-client :as client] - [metabase.models.database :as database] + [metabase.models.database :as database :refer [Database]] [metabase.server.middleware.auth :as mw.auth] [metabase.server.middleware.util :as mw.util] - [metabase.sync] + [metabase.sync :as sync] [metabase.sync.sync-metadata] [metabase.test :as mt] [metabase.test.fixtures :as fixtures] - [metabase.util :as u])) + [metabase.util :as u] + [toucan.db :as db])) (use-fixtures :once (fixtures/initialize :db :web-server)) @@ -101,3 +104,59 @@ (is (= {:errors {:scan "value may be nil, or if non-nil, value must be one of: `full`, `schema`."}} (post {:scan :unrecognized} 400))))))) + +;; TODO - Consider generalizing this in the future. It was taken from `metabase.driver.postgres-test` +;; Perhaps if there's another instance where it is used put it somewhere common. + +(defn- drop-if-exists-and-create-db! + "Drop a Postgres database named `db-name` if it already exists; then create a new empty one with that name." + [db-name] + (let [spec (sql-jdbc.conn/connection-details->spec :postgres (mt/dbdef->connection-details :postgres :server nil))] + ;; kill any open connections + (jdbc/query spec ["SELECT pg_terminate_backend(pg_stat_activity.pid) + FROM pg_stat_activity + WHERE pg_stat_activity.datname = ?;" db-name]) + ;; create the DB + (jdbc/execute! spec [(format "DROP DATABASE IF EXISTS \"%s\"; + CREATE DATABASE \"%s\";" + db-name db-name)] + {:transaction? false}))) + +(deftest add-new-table-sync-test + (mt/test-driver :postgres + (testing "Ensure we have the ability to add a single new table" + (let [db-name "add_new_table_sync_test_table" + details (mt/dbdef->connection-details :postgres :db {:database-name db-name})] + (drop-if-exists-and-create-db! db-name) + (mt/with-temp* [Database [database {:engine :postgres, :details (assoc details :dbname db-name)}]] + (let [spec (sql-jdbc.conn/connection-details->spec :postgres details) + exec! (fn [spec statements] (doseq [statement statements] (jdbc/execute! spec [statement]))) + tableset #(set (map (fn [{:keys [schema name]}] (format "%s.%s" schema name)) (db/select 'Table :db_id (:id %)))) + post (fn post-api + ([payload] (post-api payload 200)) + ([payload expected-code] + (mt/with-temporary-setting-values [api-key "test-api-key"] + (mt/client-full-response + :post expected-code (format "notify/db/%d/new-table" (:id database)) + {:request-options api-headers} + (merge {:synchronous? true} + payload))))) + sync! #(sync/sync-database! database)] + ;; Create the initial table and sync it. + (exec! spec ["CREATE TABLE public.FOO (val bigint NOT NULL);"]) + (sync!) + (let [tables (tableset database)] + (is (= #{"public.foo"} tables))) + ;; We can't add an existing table + (is (= 400 (:status (post {:schema_name "public" :table_name "foo"} 400)))) + ;; We can't add a nonexistent table + (is (= 404 (:status (post {:schema_name "public" :table_name "bar"} 404)))) + ;; Create two more tables that are not yet synced + (exec! spec ["CREATE TABLE public.BAR (val bigint NOT NULL);" + "CREATE TABLE public.FERN (val bigint NOT NULL);"]) + ;; This will add bar to metabase (but not fern). + (is (= 200 (:status (post {:schema_name "public" :table_name "bar"})))) + ;; Assert that only the synced tables are present. + (let [tables (tableset database)] + (is (= #{"public.foo" "public.bar"} tables)) + (is (false? (contains? tables "public.fern"))))))))))