Fix new Table entries being added if a Table or View is dropped and recreated

src/metabase/models/table.clj

@@ -153,12 +153,10 @@
 
 ;;; ------------------------------------------------------------ Persistence Functions ------------------------------------------------------------
 
-
-
 (defn retire-tables!
   "Retire all `Tables` in the list of TABLE-IDs along with all of each tables `Fields`."
   [table-ids]
-  {:pre [(set? table-ids) (every? integer? table-ids)]}
+  {:pre [(u/maybe? set? table-ids) (every? integer? table-ids)]}
   (when (seq table-ids)
     ;; retire the tables
     (db/update-where! Table {:id [:in table-ids]}
@@ -183,11 +181,16 @@
 (defn create-table-from-tabledef!
   "Create `Table` with the data from TABLE-DEF."
   [database-id {schema-name :schema, table-name :name, raw-table-id :raw-table-id, visibility-type :visibility-type}]
-  (db/insert! Table
-    :db_id           database-id
-    :raw_table_id    raw-table-id
-    :schema          schema-name
-    :name            table-name
-    :visibility_type visibility-type
-    :display_name    (humanization/name->human-readable-name table-name)
-    :active          true))
+  (if-let [existing-id (db/select-one-id Table :db_id database-id, :raw_table_id raw-table-id, :schema schema-name, :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
+    (db/insert! Table
+      :db_id           database-id
+      :raw_table_id    raw-table-id
+      :schema          schema-name
+      :name            table-name
+      :visibility_type visibility-type
+      :display_name    (humanization/name->human-readable-name table-name)
+      :active          true)))

src/metabase/sync_database/sync.clj

@@ -13,6 +13,8 @@
   (:import metabase.models.raw_table.RawTableInstance))
 
 
+;;; ------------------------------------------------------------ FKs ------------------------------------------------------------
+
 (defn- save-fks!
   "Update all of the FK relationships present in DATABASE based on what's captured in the raw schema.
    This will set :special_type :type/FK and :fk_target_field_id <field-id> for each found FK relationship.
@@ -28,6 +30,29 @@
           :special_type       :type/FK
           :fk_target_field_id target-field-id)))))
 
+(defn- set-fk-relationships!
+  "Handle setting any FK relationships for a DATABASE. This must be done after fully syncing the tables/fields because we need all tables/fields in place."
+  [database]
+  (when-let [db-fks (db/select [RawColumn [:id :source-column] [:fk_target_column_id :target-column]]
+                      (mdb/join [RawColumn :raw_table_id] [RawTable :id])
+                      (db/qualify RawTable :database_id) (:id database)
+                      (db/qualify RawColumn :fk_target_column_id) [:not= nil])]
+    (save-fks! db-fks)))
+
+(defn- set-table-fk-relationships!
+  "Handle setting FK relationships for a specific TABLE."
+  [database-id raw-table-id]
+  (when-let [table-fks (db/select [RawColumn [:id :source-column] [:fk_target_column_id :target-column]]
+                         (mdb/join [RawColumn :raw_table_id] [RawTable :id])
+                         (db/qualify RawTable :database_id) database-id
+                         (db/qualify RawTable :id) raw-table-id
+                         (db/qualify RawColumn :fk_target_column_id) [:not= nil])]
+    (save-fks! table-fks)))
+
+
+;;; ------------------------------------------------------------ _metabase_metadata table ------------------------------------------------------------
+
+;; the _metabase_metadata table is a special table that can include Metabase metadata about the rest of the DB. This is used by the sample dataset
 
 (defn sync-metabase-metadata-table!
   "Databases may include a table named `_metabase_metadata` (case-insentive) which includes descriptions or other metadata about the `Tables` and `Fields`
@@ -68,6 +93,22 @@
                (log/error (u/format-color 'red "Error in _metabase_metadata: %s" (.getMessage e)))))))))
 
 
+(defn is-metabase-metadata-table?
+  "Is this TABLE the special `_metabase_metadata` table?"
+  [table]
+  (= "_metabase_metadata" (s/lower-case (:name table))))
+
+
+(defn- maybe-sync-metabase-metadata-table!
+  "Sync the `_metabase_metadata` table, a special table with Metabase metadata, if present.
+   If per chance there were multiple `_metabase_metadata` tables in different schemas, just sync the first one we find."
+  [database raw-tables]
+  (when-let [metadata-table (first (filter is-metabase-metadata-table? raw-tables))]
+    (sync-metabase-metadata-table! (driver/engine->driver (:engine database)) database metadata-table)))
+
+
+;;; ------------------------------------------------------------ Fields ------------------------------------------------------------
+
 (defn- save-table-fields!
   "Refresh all `Fields` in a given `Table` based on what's available in the associated `RawColumns`.
 
@@ -99,43 +140,9 @@
           (field/create-field-from-field-def! table-id (assoc column :raw-column-id raw-column-id)))))))
 
 
-(defn retire-tables!
-  "Retire any `Table` who's `RawTable` has been deactivated.
-  This occurs when a database introspection reveals the table is no longer available."
-  [{database-id :id}]
-  {:pre [(integer? database-id)]}
-  ;; retire tables (and their fields) as needed
-  (when-let [table-ids-to-remove (db/select-ids Table
-                                   (mdb/join [Table :raw_table_id] [RawTable :id])
-                                   :db_id database-id
-                                   (db/qualify Table :active) true
-                                   (db/qualify RawTable :active) false)]
-    (table/retire-tables! table-ids-to-remove)))
-
-
-(defn update-data-models-for-table!
-  "Update the working `Table` and `Field` metadata for a given `Table` based on the latest raw schema information.
-   This function uses the data in `RawTable` and `RawColumn` to update the working data models as needed."
-  [{raw-table-id :raw_table_id, table-id :id, :as existing-table}]
-  (when-let [{database-id :database_id, :as raw-table} (RawTable raw-table-id)]
-    (try
-      (if-not (:active raw-table)
-        ;; looks like the table has been deactivated, so lets retire this Table and its fields
-        (table/retire-tables! #{table-id})
-        ;; otherwise update based on the RawTable/RawColumn information
-        (do
-          (save-table-fields! (table/update-table-from-tabledef! existing-table raw-table))
+;;; ------------------------------------------------------------  "Crufty" Tables ------------------------------------------------------------
 
-          ;; handle setting any fk relationships
-          (when-let [table-fks (db/select [RawColumn [:id :source-column] [:fk_target_column_id :target-column]]
-                                 (mdb/join [RawColumn :raw_table_id] [RawTable :id])
-                                 (db/qualify RawTable :database_id) database-id
-                                 (db/qualify RawTable :id) raw-table-id
-                                 (db/qualify RawColumn :fk_target_column_id) [:not= nil])]
-            (save-fks! table-fks))))
-
-      (catch Throwable t
-        (log/error (u/format-color 'red "Unexpected error syncing table") t)))))
+;; Crufty tables are ones we know are from frameworks like Rails or Django and thus automatically mark as `:cruft`
 
 (def ^:private ^:const crufty-table-patterns
   "Regular expressions that match Tables that should automatically given the `visibility-type` of `:cruft`.
@@ -193,17 +200,49 @@
   [table]
   (boolean (some #(re-find % (s/lower-case (:name table))) crufty-table-patterns)))
 
-(defn is-metabase-metadata-table?
-  "Is this TABLE the special `_metabase_metadata` table?"
-  [table]
-  (= "_metabase_metadata" (s/lower-case (:name table))))
+
+;;; ------------------------------------------------------------ Table Syncing + Saving ------------------------------------------------------------
+
+(defn- table-ids-to-remove
+  "Return a set of active `Table` IDs for Database with DATABASE-ID whose backing RawTable is now inactive."
+  [database-id]
+  (db/select-ids Table
+    (mdb/join [Table :raw_table_id] [RawTable :id])
+    :db_id database-id
+    (db/qualify Table :active) true
+    (db/qualify RawTable :active) false))
+
+(defn retire-tables!
+  "Retire any `Table` who's `RawTable` has been deactivated.
+  This occurs when a database introspection reveals the table is no longer available."
+  [{database-id :id}]
+  {:pre [(integer? database-id)]}
+  ;; retire tables (and their fields) as needed
+  (table/retire-tables! (table-ids-to-remove database-id)))
+
+
+(defn update-data-models-for-table!
+  "Update the working `Table` and `Field` metadata for a given `Table` based on the latest raw schema information.
+   This function uses the data in `RawTable` and `RawColumn` to update the working data models as needed."
+  [{raw-table-id :raw_table_id, table-id :id, :as existing-table}]
+  (when-let [{database-id :database_id, :as raw-table} (RawTable raw-table-id)]
+    (try
+      (if-not (:active raw-table)
+        ;; looks like the table has been deactivated, so lets retire this Table and its fields
+        (table/retire-tables! #{table-id})
+        ;; otherwise update based on the RawTable/RawColumn information
+        (do
+          (save-table-fields! (table/update-table-from-tabledef! existing-table raw-table))
+          (set-table-fk-relationships! database-id raw-table-id)))
+      (catch Throwable t
+        (log/error (u/format-color 'red "Unexpected error syncing table") t)))))
+
 
 (defn- create-and-update-tables!
   "Create/update tables (and their fields)."
   [database existing-tables raw-tables]
-  (doseq [{raw-table-id :id, :as raw-table} (for [table raw-tables
-                                                  :when (not (is-metabase-metadata-table? table))]
-                                              table)]
+  (doseq [{raw-table-id :id, :as raw-table} raw-tables
+          :when                             (not (is-metabase-metadata-table? raw-table))]
     (try
       (save-table-fields! (if-let [existing-table (get existing-tables raw-table-id)]
                             ;; table already exists, update it
@@ -216,35 +255,18 @@
       (catch Throwable e
         (log/error (u/format-color 'red "Unexpected error syncing table") e)))))
 
-(defn- set-fk-relationships!
-  "Handle setting any FK relationships. This must be done after fully syncing the tables/fields because we need all tables/fields in place."
-  [database]
-  (when-let [db-fks (db/select [RawColumn [:id :source-column] [:fk_target_column_id :target-column]]
-                      (mdb/join [RawColumn :raw_table_id] [RawTable :id])
-                      (db/qualify RawTable :database_id) (:id database)
-                      (db/qualify RawColumn :fk_target_column_id) [:not= nil])]
-    (save-fks! db-fks)))
-
-(defn- maybe-sync-metabase-metadata-table!
-  "Sync the `_metabase_metadata` table, a special table with Metabase metadata, if present.
-   If per chance there were multiple `_metabase_metadata` tables in different schemas, just sync the first one we find."
-  [database raw-tables]
-  (when-let [metadata-table (first (filter is-metabase-metadata-table? raw-tables))]
-    (sync-metabase-metadata-table! (driver/engine->driver (:engine database)) database metadata-table)))
 
 (defn update-data-models-from-raw-tables!
   "Update the working `Table` and `Field` metadata for *all* tables in a `Database` based on the latest raw schema information.
    This function uses the data in `RawTable` and `RawColumn` to update the working data models as needed."
   [{database-id :id, :as database}]
   {:pre [(integer? database-id)]}
-
   ;; quick sanity check that this is indeed a :dynamic-schema database
   (when (driver/driver-supports? (driver/engine->driver (:engine database)) :dynamic-schema)
     (throw (IllegalStateException. "This function cannot be called on databases which are :dynamic-schema")))
-
   ;; retire any tables which were disabled
   (retire-tables! database)
-
+  ;; ok, now create new tables as needed and set FK relationships
   (let [raw-tables          (raw-table/active-tables database-id)
         raw-table-id->table (u/key-by :raw_table_id (db/select Table, :db_id database-id, :active true))]
     (create-and-update-tables! database raw-table-id->table raw-tables)

src/metabase/sync_database/sync_dynamic.clj

@@ -6,11 +6,11 @@
             [schema.core :as schema]
             [toucan.db :as db]
             [metabase.driver :as driver]
-            [metabase.models.field :as field]
-            [metabase.models.raw-table :as raw-table]
-            [metabase.models.table :as table]
-            [metabase.sync-database.sync :as sync]
-            [metabase.sync-database.interface :as i]
+            (metabase.models [field :refer [Field], :as field]
+                             [raw-table :refer [RawTable], :as raw-table]
+                             [table :refer [Table], :as table])
+            (metabase.sync-database [interface :as i]
+                                    [sync :as sync])
             [metabase.util :as u]))
 
 
@@ -19,7 +19,7 @@
    All field-defs provided are assumed to be children of the given FIELD."
   [{parent-id :id, table-id :table_id, :as parent-field} nested-field-defs]
   ;; NOTE: remember that we never retire any fields in dynamic-schema tables
-  (let [existing-field-name->field (u/key-by :name (db/select field/Field, :parent_id parent-id))]
+  (let [existing-field-name->field (u/key-by :name (db/select Field, :parent_id parent-id))]
     (u/prog1 (set/difference (set (map :name nested-field-defs)) (set (keys existing-field-name->field)))
       (when (seq <>)
         (log/debug (u/format-color 'blue "Found new nested fields for field '%s': %s" (:name parent-field) <>))))
@@ -43,7 +43,7 @@
   {:pre [(integer? table-id)
          (coll? field-defs)
          (every? map? field-defs)]}
-  (let [field-name->field (u/key-by :name (db/select field/Field, :table_id table-id, :parent_id nil))]
+  (let [field-name->field (u/key-by :name (db/select Field, :table_id table-id, :parent_id nil))]
     ;; NOTE: with dynamic schemas we never disable fields
     ;; create/update the fields
     (doseq [{field-name :name, :keys [nested-fields], :as field-def} field-defs]
@@ -59,7 +59,7 @@
 (defn scan-table-and-update-data-model!
   "Update the working `Table` and `Field` metadata for the given `Table`."
   [driver database {raw-table-id :raw_table_id, table-id :id, :as existing-table}]
-  (when-let [raw-table (raw-table/RawTable raw-table-id)]
+  (when-let [raw-table (RawTable raw-table-id)]
     (try
       (if-not (:active raw-table)
         ;; looks like table was deactivated, so lets retire this Table
@@ -87,7 +87,7 @@
   (sync/retire-tables! database)
 
   (let [raw-tables          (raw-table/active-tables database-id)
-        raw-table-id->table (u/key-by :raw_table_id (db/select table/Table, :db_id database-id, :active true))]
+        raw-table-id->table (u/key-by :raw_table_id (db/select Table, :db_id database-id, :active true))]
     ;; create/update tables (and their fields)
     ;; NOTE: we make sure to skip the _metabase_metadata table here.  it's not a normal table.
     (doseq [{raw-table-id :id, :as raw-table} raw-tables

test/metabase/driver/postgres_test.clj

@@ -7,9 +7,11 @@
             [metabase.driver :as driver]
             [metabase.driver.generic-sql :as sql]
             (metabase.models [database :refer [Database]]
-                             [field :refer [Field]])
+                             [field :refer [Field]]
+                             [table :refer [Table]])
             [metabase.query-processor-test :refer [rows]]
             [metabase.query-processor.expand :as ql]
+            [metabase.sync-database :as sync-db]
             [metabase.test.data :as data]
             (metabase.test.data [datasets :refer [expect-with-engine]]
                                 [interface :as i])
@@ -151,11 +153,27 @@
             (ql/filter (ql/= $ip "192.168.1.1"))))))
 
 
+;;; Util Fns
+
+(defn- drop-if-exists-and-create-db! [db-name]
+  (let [spec (sql/connection-details->spec pg-driver (i/database->connection-details pg-driver :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})))
+
+
 ;; Check that we properly fetch materialized views.
 ;; As discussed in #2355 they don't come back from JDBC `DatabaseMetadata` so we have to fetch them manually.
 (expect-with-engine :postgres
   {:tables #{{:schema "public", :name "test_mview"}}}
   (do
+    (drop-if-exists-and-create-db! "materialized_views_test")
     (jdbc/execute! (sql/connection-details->spec pg-driver (i/database->connection-details pg-driver :server nil))
                    ["DROP DATABASE IF EXISTS materialized_views_test;
                      CREATE DATABASE materialized_views_test;"]
@@ -172,10 +190,7 @@
 (expect-with-engine :postgres
   {:tables #{{:schema "public", :name "foreign_table"} {:schema "public", :name "local_table"}}}
   (do
-    (jdbc/execute! (sql/connection-details->spec pg-driver (i/database->connection-details pg-driver :server nil))
-                   ["DROP DATABASE IF EXISTS fdw_test;
-                     CREATE DATABASE fdw_test;"]
-                   {:transaction? false})
+    (drop-if-exists-and-create-db! "fdw_test")
     (let [details (i/database->connection-details pg-driver :db {:database-name "fdw_test"})]
       (jdbc/execute! (sql/connection-details->spec pg-driver details)
                      [(str "CREATE EXTENSION IF NOT EXISTS postgres_fdw;
@@ -189,7 +204,37 @@
       (tt/with-temp Database [database {:engine :postgres, :details (assoc details :dbname "fdw_test")}]
         (driver/describe-database pg-driver database)))))
 
-;; timezone tests
+;; make sure that if a view is dropped and recreated that the original Table object is marked active rather than a new one being created (#3331)
+(expect
+  [{:name "angry_birds", :active true}]
+  (let [details (i/database->connection-details pg-driver :db {:database-name "dropped_views_test"})
+        spec    (sql/connection-details->spec pg-driver details)
+        exec!   #(doseq [statement %]
+                   (jdbc/execute! spec [statement]))]
+    ;; create the postgres DB
+    (drop-if-exists-and-create-db! "dropped_views_test")
+    ;; create the DB object
+    (tt/with-temp Database [database {:engine :postgres, :details (assoc details :dbname "dropped_views_test")}]
+      (let [sync! #(sync-db/sync-database! database, :full-sync? true)]
+        ;; populate the DB and create a view
+        (exec! ["CREATE table birds (name VARCHAR UNIQUE NOT NULL);"
+                "INSERT INTO birds (name) VALUES ('Rasta'), ('Lucky'), ('Kanye Nest');"
+                "CREATE VIEW angry_birds AS SELECT upper(name) AS name FROM birds;"])
+        ;; now sync the DB
+        (sync!)
+        ;; drop the view
+        (exec! ["DROP VIEW angry_birds;"])
+        ;; sync again
+        (sync!)
+        ;; recreate the view
+        (exec! ["CREATE VIEW angry_birds AS SELECT upper(name) AS name FROM birds;"])
+        ;; sync one last time
+        (sync!)
+        ;; now take a look at the Tables in the database related to the view. THERE SHOULD BE ONLY ONE!
+        (db/select [Table :name :active] :db_id (u/get-id database), :name "angry_birds")))))
+
+
+;;; timezone tests
 
 (tu/resolve-private-vars metabase.driver.generic-sql.query-processor
   run-query-with-timezone)