Mark crufty tables as such

src/metabase/api/card.clj

@@ -134,7 +134,7 @@
 (defendpoint GET "/"
   "Get all the `Cards`. Option filter param `f` can be used to change the set of Cards that are returned; default is `all`,
    but other options include `mine`, `fav`, `database`, `table`, `recent`, `popular`, and `archived`. See corresponding implementation
-   functions above for the specific behavior of each filter option.
+   functions above for the specific behavior of each filter option. :card_index:
 
    Optionally filter cards by LABEL slug."
   [f model_id label]
@@ -143,7 +143,7 @@
     (checkp (integer? model_id) "id" (format "id is required parameter when filter mode is '%s'" (name f)))
     (case f
       :database (read-check Database model_id)
-      :table    (read-check Database (:db_id (sel :one :fields [Table :db_id] :id model_id)))))
+      :table    (read-check Database (sel :one :field [Table :db_id] :id model_id))))
   (cards-for-filter-option f model_id label))
 
 (defendpoint POST "/"

src/metabase/api/label.clj

@@ -8,19 +8,19 @@
             [metabase.models.label :refer [Label]]))
 
 (defendpoint GET "/"
-  "List all labels."
+  "List all `Labels`. :label:"
   []
   (db/sel :many Label (k/order (k/sqlfn :LOWER :name))))
 
 (defendpoint POST "/"
-  "Create a new label."
+  "Create a new `Label`. :label: "
   [:as {{:keys [name icon]} :body}]
   {name [Required NonEmptyString]
    icon NonEmptyString}
   (db/ins Label, :name name, :icon icon))
 
 (defendpoint PUT "/:id"
-  "Update a label."
+  "Update a `Label`. :label:"
   [id :as {{:keys [name icon], :as body} :body}]
   {name NonEmptyString
    icon NonEmptyString}
@@ -29,7 +29,7 @@
   (Label id)) ; return the updated Label
 
 (defendpoint DELETE "/:id"
-  "Delete a label."
+  "Delete a `Label`. :label:"
   [id]
   (write-check Label id)
   (db/cascade-delete Label :id id))

src/metabase/models/table.clj

@@ -113,11 +113,12 @@
 
 (defn create-table
   "Create `Table` with the data from TABLE-DEF."
-  [database-id {schema-name :schema, table-name :name, raw-table-id :raw-table-id}]
+  [database-id {schema-name :schema, table-name :name, raw-table-id :raw-table-id, visibility-type :visibility-type}]
   (db/ins Table
-    :db_id        database-id
-    :raw_table_id raw-table-id
-    :schema       schema-name
-    :name         table-name
-    :display_name (common/name->human-readable-name table-name)
-    :active       true))
+    :db_id           database-id
+    :raw_table_id    raw-table-id
+    :schema          schema-name
+    :name            table-name
+    :visibility_type visibility-type
+    :display_name    (common/name->human-readable-name table-name)
+    :active          true))

src/metabase/sync_database/sync.clj

@@ -143,16 +143,24 @@
       (catch Throwable t
         (log/error (u/format-color 'red "Unexpected error syncing table") t)))))
 
-(def ^:private ^:const blacklisted-table-names
-  "Names of Tables to skip syncing. These should be lowercased, as we'll compare them with lowercased names of `raw-tables` below.
-   These are Tables that are known to not contain useful data, such as migration or web framework internal tables."
-  #{"_metabase_metadata"
-    ;; Django
+(def ^:private ^:const crufty-table-names
+  "Names of Tables that should automatically given the `visibility-type` of `:cruft`.
+   This means they are automatically hidden to users (but can be unhidden in the admin panel).
+   These `Tables` are known to not contain useful data, such as migration or web framework internal tables."
+  #{;; Django
+    "auth_group"
+    "auth_group_permissions"
+    "auth_permission"
     "django_admin_log"
     "django_content_type"
+    "django_migrations"
     "django_session"
     "django_site"
     "south_migrationhistory"
+    "user_groups"
+    "user_user_permissions"
+    ;; Rails / Active Record
+    "schema_migrations"
     ;; PostGIS
     "spatial_ref_sys"
     ;; nginx
@@ -163,24 +171,26 @@
     ;; Lobos
     "lobos_migrations"})
 
-(defn- non-blacklisted-tables
-  "Filter out Tables that we don't want to sync.
-   This includes the `_metabase_metadata` Table, if present, as well as Tables for frameworks like Rails or Django."
-  [raw-tables]
-  (for [table raw-tables
-        :when (not (contains? blacklisted-table-names (s/lower-case (:name table))))]
-    table))
+(defn- is-crufty-table?
+  "Should we give newly created TABLE a `visibility_type` of `:cruft`?"
+  [table]
+  (contains? crufty-table-names (s/lower-case (:name table))))
 
 (defn- create-and-update-tables!
   "Create/update tables (and their fields)."
   [database existing-tables raw-tables]
-  (doseq [{raw-table-id :id, :as raw-table} (non-blacklisted-tables raw-tables)]
+  (doseq [{raw-table-id :id, :as raw-table} (for [table raw-tables
+                                                  :when (not= "_metabase_metadata" (s/lower-case (:name table)))]
+                                              table)]
     (try
       (save-table-fields! (if-let [existing-table (get existing-tables raw-table-id)]
                             ;; table already exists, update it
                             (table/update-table existing-table raw-table)
                             ;; must be a new table, insert it
-                            (table/create-table (:id database) (assoc raw-table :raw-table-id raw-table-id))))
+                            (table/create-table (:id database) (assoc raw-table
+                                                                      :raw-table-id    raw-table-id
+                                                                      :visibility-type (when (is-crufty-table? raw-table)
+                                                                                         :cruft)))))
       (catch Throwable e
         (log/error (u/format-color 'red "Unexpected error syncing table") e)))))
 

test/metabase/driver/postgres_test.clj

@@ -108,7 +108,7 @@
 
 
 ;;; # Make sure that duplicate column names (e.g. caused by using a FK) still return both columns
-(i/def-database-definition duplicate-names
+(i/def-database-definition ^:const ^:private duplicate-names
   ["birds"
    [{:field-name "name", :base-type :TextField}]
    [["Rasta"]

test/metabase/sync_database/sync_test.clj

@@ -12,6 +12,8 @@
                              [table :refer [Table]])
             (metabase.sync-database [introspect :as introspect]
                                     [sync :refer :all])
+            [metabase.test.data :as data]
+            [metabase.test.data.interface :as i]
             [metabase.test.util :as tu]))
 
 (tu/resolve-private-fns metabase.sync-database.sync
@@ -152,7 +154,7 @@
      :description        nil
      :base_type          :DecimalField
      :visibility_type    :normal
-     :special_type       :id,                 ; existing special types are NOT modified
+     :special_type       :id                  ; existing special types are NOT modified
      :parent_id          false
      :fk_target_field_id false
      :last_analyzed      false
@@ -194,7 +196,7 @@
      :description        nil
      :base_type          :IntegerField
      :visibility_type    :normal
-     :special_type       :category,            ; should be infered from name
+     :special_type       :category             ; should be infered from name
      :parent_id          false
      :fk_target_field_id false
      :last_analyzed      false
@@ -208,7 +210,7 @@
      :display_name       "First"
      :description        nil
      :base_type          :DecimalField
-     :visibility_type    :retired,            ; field retired when RawColumn disabled
+     :visibility_type    :retired             ; field retired when RawColumn disabled
      :special_type       :id
      :parent_id          false
      :fk_target_field_id false
@@ -251,7 +253,7 @@
      :description        nil
      :base_type          :IntegerField
      :visibility_type    :normal
-     :special_type       :category,            ; should be infered from name
+     :special_type       :category             ; should be infered from name
      :parent_id          false
      :fk_target_field_id false
      :last_analyzed      false
@@ -443,3 +445,26 @@
        (do
          (update-data-models-from-raw-tables! db)
          (db-tables database-id))])))
+
+
+;;; ------------------------------------------------------------ Make sure that "crufty" tables are marked as such ------------------------------------------------------------
+(i/def-database-definition ^:const ^:private db-with-some-cruft
+  ["acquired_toucans"
+   [{:field-name "species",              :base-type :CharField}
+    {:field-name "cam_has_acquired_one", :base-type :BooleanField}]
+   [["Toco"               false]
+    ["Chestnut-Mandibled" true]
+    ["Keel-billed"        false]
+    ["Channel-billed"     false]]]
+  ["south_migrationhistory"
+   [{:field-name "app_name",  :base-type :CharField}
+    {:field-name "migration", :base-type :CharField}]
+   [["main" "0001_initial"]
+    ["main" "0002_add_toucans"]]])
+
+;; south_migrationhistory, being a CRUFTY table, should still be synced, but marked as such
+(expect
+  #{{:name "SOUTH_MIGRATIONHISTORY", :visibility_type :cruft}
+    {:name "ACQUIRED_TOUCANS",       :visibility_type nil}}
+  (data/dataset metabase.sync-database.sync-test/db-with-some-cruft
+    (set (db/sel :many :fields [Table :name :visibility_type] :db_id (data/id)))))

test/metabase/test/data.clj

@@ -37,7 +37,7 @@
 
 (defn db
   "Return the current database.
-   Relies on the dynamic variable `*get-db`, which can be rebound with `with-db`."
+   Relies on the dynamic variable `*get-db*`, which can be rebound with `with-db`."
   []
   (*get-db*))
 
@@ -253,17 +253,18 @@
 
 
 (defmacro with-temp-db
-  "Load and sync DATABASE-DEFINITION with DATASET-LOADER and execute BODY with
-   the newly created `Database` bound to DB-BINDING.
-   Add `Database` to `loader->loaded-db-def`, which can be destroyed with `destroy-loaded-temp-dbs!`,
-   which is automatically ran at the end of the test suite.
+  "Load and sync DATABASE-DEFINITION with DATASET-LOADER and execute BODY with the newly created `Database` bound to DB-BINDING,
+   and make it the current database for `metabase.test.data` functions like `id`.
 
      (with-temp-db [db tupac-sightings]
        (driver/process-quiery {:database (:id db)
                                :type     :query
                                :query    {:source_table (:id &events)
                                           :aggregation  [\"count\"]
-                                          :filter       [\"<\" (:id &events.timestamp) \"1765-01-01\"]}}))"
+                                          :filter       [\"<\" (:id &events.timestamp) \"1765-01-01\"]}}))
+
+   A given Database is only created once per run of the test suite, and is automatically destroyed at the conclusion of the suite.
+   (The created Database is added to `loader->loaded-db-def`, which can be destroyed with `destroy-loaded-temp-dbs!`, which is automatically ran at the end of the test suite.)"
   [[db-binding ^DatabaseDefinition database-definition] & body]
   `(do-with-temp-db ~database-definition
      (fn [~db-binding]
@@ -275,7 +276,7 @@
        (throw (Exception. (format "Dataset definition not found: '%s' or 'metabase.test.data.dataset-definitions/%s'" symb symb)))))
 
 (defmacro dataset
-  "Bind temp `Database` for DATASET as the current DB and execute BODY.
+  "Load and sync a temporary `Database` defined by DATASET, make it the current DB (for `metabase.test.data` functions like `id`), and execute BODY.
 
    Like `with-temp-db`, but takes an unquoted symbol naming a `DatabaseDefinition` rather than the dbef itself.
    DATASET is optionally namespace-qualified; if not, `metabase.test.data.dataset-definitions` is assumed.

test/metabase/test/util.clj

@@ -244,6 +244,8 @@
         database-id
         (get-most-recent-database-id))"
   {:style/indent 1}
+  ;; TODO - maybe it makes more sense to have the signature be [with-temp*-form expected & actual] and wrap `actual` in a `do` since it seems like a pretty common use-case.
+  ;; I'm not sure about the readability implications however :scream_cat:
   [with-temp*-form 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 of `expect-with-temp` as the same test and only a single one will end up being ran