EE endpoint for deleting CSV uploads (#41293)

enterprise/backend/src/metabase_enterprise/api/routes.clj

@@ -16,6 +16,7 @@
    [metabase-enterprise.llm.api :as llm.api]
    [metabase-enterprise.sandbox.api.routes :as sandbox]
    [metabase-enterprise.serialization.api :as api.serialization]
+   [metabase-enterprise.upload-management.api :as api.uploads]
    [metabase.api.common :refer [context defroutes]]
    [metabase.util.i18n :refer [deferred-tru]]))
 
@@ -47,4 +48,7 @@
     (ee.api.common/+require-premium-feature :serialization (deferred-tru "Serialization") api.serialization/routes))
    (context
     "/autodescribe" []
-    (ee.api.common/+require-premium-feature :llm-autodescription (deferred-tru "LLM Auto-description") llm.api/routes))))
+    (ee.api.common/+require-premium-feature :llm-autodescription (deferred-tru "LLM Auto-description") llm.api/routes))
+   (context
+    "/upload-management" []
+    (ee.api.common/+require-premium-feature :upload-management (deferred-tru "Upload Management") api.uploads/routes))))

enterprise/backend/src/metabase_enterprise/cache/strategies.clj

@@ -48,7 +48,7 @@
 
 ;;; Querying DB
 
-(defn granular-duration-hours
+(defn- granular-duration-hours
   "Returns the granular cache duration (in hours) for a card. On EE, this first checking whether there is a stored value
    for the card, dashboard, or database (in that order of decreasing preference). Returns nil on OSS."
   [card dashboard-id]
@@ -89,7 +89,7 @@
 
 ;;; Strategy execution
 
-(defmulti fetch-cache-stmt-ee*
+(defmulti ^:private fetch-cache-stmt-ee*
   "Generate prepared statement for a db cache backend for a given strategy"
   (fn [strategy _hash _conn] (:type strategy)))
 

enterprise/backend/src/metabase_enterprise/upload_management/api.clj

+(ns metabase-enterprise.upload-management.api
+  (:require
+   [compojure.core :refer [DELETE]]
+   [metabase.api.common :as api]
+   [metabase.api.routes.common :refer [+auth]]
+   [metabase.models.interface :as mi]
+   [metabase.upload :as upload]
+   [metabase.util.i18n :refer [tru]]
+   [metabase.util.malli.schema :as ms]
+   [toucan2.core :as t2]))
+
+(api/defendpoint GET "/tables/"
+  "Get all `Tables` visible to the current user which were created by uploading a file."
+  []
+  (as-> (t2/select :model/Table, :active true, :is_upload true, {:order-by [[:name :asc]]}) tables
+        ;; See https://github.com/metabase/metabase/issues/41023
+        (map #(update % :schema str) tables)
+        (filterv mi/can-read? tables)))
+
+(api/defendpoint DELETE "/tables/:id"
+  "Delete the uploaded table from the database, optionally archiving cards for which it is the primary source."
+  [id archive-cards]
+  {id            ms/PositiveInt
+   archive-cards [:maybe {:default false} ms/BooleanValue]}
+  (try
+    ;; To be idempotent, we do not check whether the table has already been deactivated.
+    (let [table  (api/check-404 (t2/select-one :model/Table id))
+          result (upload/delete-upload! table :archive-cards? archive-cards)]
+      {:status 200
+       :body   (= :done result)})
+    (catch Throwable e
+      {:status (or (-> e ex-data :status-code)
+                   500)
+       :body   {:message (or (ex-message e)
+                             (tru "There was an error deleting the table"))}})))
+
+(api/define-routes +auth)

enterprise/backend/test/metabase_enterprise/upload_management/api_test.clj

+(ns metabase-enterprise.upload-management.api-test
+  (:require
+   [clojure.string :as str]
+   [clojure.test :refer :all]
+   [java-time.api :as t]
+   [metabase.api.table-test :as oss-test]
+   [metabase.test :as mt]
+   [toucan2.tools.with-temp :as t2.with-temp]))
+
+(def list-url "ee/upload-management/tables/")
+
+(deftest list-uploaded-tables-test
+  (testing "GET ee/upload-management/tables"
+    (testing "These should come back in alphabetical order and include relevant metadata"
+      (mt/with-premium-features #{:upload-management}
+        (oss-test/with-tables-as-uploads [:categories :reviews]
+          (t2.with-temp/with-temp [:model/Card {} {:table_id (mt/id :categories)}
+                                   :model/Card {} {:table_id (mt/id :reviews)}
+                                   :model/Card {} {:table_id (mt/id :reviews)}]
+            (let [result (mt/user-http-request :rasta :get 200 list-url)]
+              ;; =? doesn't currently know how to treat sets as literals, only as predicates, so we can't use it.
+              ;; See https://github.com/metabase/hawk/issues/23
+              (is (every? t/offset-date-time? (map :created_at result)))
+              (is (= #{{:name         (mt/format-name "categories")
+                        :display_name "Categories"
+                        :id           (mt/id :categories)
+                        :schema       "PUBLIC"
+                        :entity_type  "entity/GenericTable"}
+                       {:name         (mt/format-name "reviews")
+                        :display_name "Reviews"
+                        :id           (mt/id :reviews)
+                        :schema       "PUBLIC"
+                        :entity_type  "entity/GenericTable"}}
+                     (->> result
+                          (filter #(= (:db_id %) (mt/id)))  ; prevent stray tables from affecting unit test results
+                          (map #(select-keys % [:name :display_name :id :entity_type :schema :usage_count]))
+                          set))))))))))
+
+(defn- delete-url [table-id]
+  (str "ee/upload-management/tables/" table-id))
+
+(defn- listed-table-ids []
+  (into #{} (map :id) (mt/user-http-request :crowberto :get 200 list-url)))
+
+(deftest delete-csv-test
+  (testing "DELETE ee/upload-management/:id"
+    (mt/test-driver :h2
+      (mt/with-empty-db
+       (testing "Behind a feature flag"
+         (is (str/starts-with? (mt/user-http-request :crowberto :delete 402 (delete-url 1))
+                               "Upload Management is a paid feature not currently available to your instance.")))
+
+       (mt/with-premium-features #{:upload-management}
+         (testing "Happy path\n"
+           (let [table-id (:id (oss-test/create-csv!))]
+             (testing "We can see the table in the list"
+               (is (contains? (listed-table-ids) table-id)))
+             (testing "We can make a successful call to delete the table"
+               (is (true? (mt/user-http-request :crowberto :delete 200 (delete-url table-id)))))
+             (testing "The table is gone from the list"
+               (is (not (contains? (listed-table-ids) table-id))))))
+
+         (testing "Uploads may be deleted even when *uploading* has been disabled"
+           (mt/with-temporary-setting-values [uploads-enabled false]
+             (let [table-id (:id (oss-test/create-csv!))]
+               (is (true? (mt/user-http-request :crowberto :delete 200 (delete-url table-id)))))))
+
+         (testing "The table must be uploaded"
+           (mt/with-temp [:model/Table {table-id :id}]
+             (is (= {:message "The table must be an uploaded table."}
+                    (mt/user-http-request :rasta :delete 422 (delete-url table-id))))))
+
+         (testing "Write permissions to the table are required to delete it\n"
+           (let [table-id (:id (oss-test/create-csv!))]
+             (testing "The delete request is rejected"
+               (is (= {:message "You don't have permissions to do that."}
+                      (mt/user-http-request :rasta :delete 403 (delete-url table-id)))))
+             (testing "The table remains in the list"
+               (is (contains? (listed-table-ids) table-id))))))))))

src/metabase/api/table.clj

@@ -53,13 +53,6 @@
                    (map fix-schema))
           tables)))
 
-(api/defendpoint GET "/uploaded"
-  "Get all `Tables` visible to the current user which were created by uploading a file."
-  []
-  (as-> (t2/select Table, :active true, :is_upload true, {:order-by [[:name :asc]]}) tables
-        (map #(update % :schema str) tables)
-        (filterv mi/can-read? tables)))
-
 (api/defendpoint GET "/:id"
   "Get `Table` with ID."
   [id include_editable_data_model]
@@ -520,15 +513,16 @@
   (-> (t2/select-one Table :id id) api/write-check (table/custom-order-fields! field_order)))
 
 (mu/defn ^:private update-csv!
-  "This helper function exists to make testing the POST /api/table/:id/append-csv endpoint easier."
+  "This helper function exists to make testing the POST /api/table/:id/{action}-csv endpoints easier."
   [options :- [:map
                [:table-id ms/PositiveInt]
                [:file (ms/InstanceOfClass java.io.File)]
                [:action upload/update-action-schema]]]
   (try
-    (let [model (upload/update-csv! options)]
+    (let [_result (upload/update-csv! options)]
       {:status 200
-       :body   (:id model)})
+       ;; There is scope to return something more interesting.
+       :body   nil})
     (catch Throwable e
       {:status (or (-> e ex-data :status-code)
                    500)

src/metabase/upload.clj

@@ -609,10 +609,65 @@
   [db table]
   (nil? (can-update-error db table)))
 
+(defn- can-delete-error
+  "Returns an ExceptionInfo object if the user cannot delete the given upload. Returns nil otherwise."
+  [table]
+  (cond
+    (not (:is_upload table))
+    (ex-info (tru "The table must be an uploaded table.")
+             {:status-code 422})
+
+    (not (mi/can-write? table))
+    (ex-info (tru "You don''t have permissions to do that.")
+             {:status-code 403})))
+
+(defn- check-can-delete
+  "Throws an error if the given table is not an upload, or if the user does not have permission to delete it."
+  [table]
+  ;; For now anyone that can update a table is allowed to delete it.
+  (when-let [error (can-delete-error table)]
+    (throw error)))
+
 ;;; +--------------------------------------------------
 ;;; |  public interface for updating an uploaded table
 ;;; +--------------------------------------------------
 
+(defn delete-upload!
+  "Delete the given table from both the app-db and the customer database."
+  [table & {:keys [archive-cards?]}]
+  (let [database   (table/database table)
+        driver     (driver.u/database->driver database)
+        table-name (table-identifier table)]
+    (check-can-delete table)
+
+    ;; Attempt to delete the underlying data from the customer database.
+    ;; We perform this before marking the table as inactive in the app db so that even if it false, the table is still
+    ;; visible to administrators and the operation is easy to retry again later.
+    (driver/drop-table! driver (:id database) table-name)
+
+    ;; We mark the table as inactive synchronously, so that it will no longer shows up in the admin list.
+    (t2/update! :model/Table :id (:id table) {:active false})
+
+    ;; Ideally we would immediately trigger any further clean-up associated with the table being deactivated, but at
+    ;; the time of writing this sync isn't wired up to do anything with explicitly inactive tables, and rather
+    ;; relies on their absence from the tables being described during the database sync itself.
+    ;; TODO update the [[metabase.sync]] module to support direct per-table clean-up
+    ;; Ideally this will also clean up more the metadata which we had created around it, e.g. advanced field values.
+    #_(future (sync/retire-table! (assoc table :active false)))
+
+    ;; Archive the related cards if the customer opted in.
+    ;;
+    ;; For now, this only covers instances where the card has this as its "primary table", i.e.
+    ;; 1. A MBQL question or model that has this table as their first or only data source, or
+    ;; 2. A MBQL question or model that depends on such a model as their first or only data source.
+    ;; Note that this does not include cases where we join to this table, or even native queries which depend .
+    (when archive-cards?
+      (t2/update-returning-pks! :model/Card
+                                {:table_id (:id table) :archived false}
+                                {:archived true}))
+
+    :done))
+
 (def update-action-schema
   "The :action values supported by [[update-csv!]]"
   [:enum ::append ::replace])

test/metabase/api/table_test.clj

@@ -2,7 +2,6 @@
   "Tests for /api/table endpoints."
   (:require
    [clojure.test :refer :all]
-   [java-time.api :as t]
    [medley.core :as m]
    [metabase.api.table :as api.table]
    [metabase.driver :as driver]
@@ -145,31 +144,6 @@
          (finally
            (t2/update! :model/Table where-clause# {:is_upload false}))))))
 
-(deftest list-uploaded-tables-test
-  (testing "GET /api/table/uploaded"
-    (testing "These should come back in alphabetical order and include relevant metadata"
-      (with-tables-as-uploads [:categories :reviews]
-        (t2.with-temp/with-temp [Card {} {:table_id (mt/id :categories)}
-                                 Card {} {:table_id (mt/id :reviews)}
-                                 Card {} {:table_id (mt/id :reviews)}]
-          (let [result (mt/user-http-request :rasta :get 200 "table/uploaded")]
-            ;; =? doesn't seem to allow predicates inside maps, inside a set
-            (is (every? t/offset-date-time? (map :created_at result)))
-            (is (= #{{:name         (mt/format-name "categories")
-                      :display_name "Categories"
-                      :id           (mt/id :categories)
-                      :schema       "PUBLIC"
-                      :entity_type  "entity/GenericTable"}
-                     {:name         (mt/format-name "reviews")
-                      :display_name "Reviews"
-                      :id           (mt/id :reviews)
-                      :schema       "PUBLIC"
-                      :entity_type  "entity/GenericTable"}}
-                   (->> result
-                        (filter #(= (:db_id %) (mt/id)))    ; prevent stray tables from affecting unit test results
-                        (map #(select-keys % [:name :display_name :id :entity_type :schema :usage_count]))
-                        set)))))))))
-
 (deftest get-table-test
   (testing "GET /api/table/:id"
     (is (= (merge
@@ -966,19 +940,22 @@
 ;;; |                                          POST /api/table/:id/append-csv                                        |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
-(defn- append-csv! [options]
-  (@#'api.table/update-csv! (assoc options :action :metabase.upload/append)))
+(defn create-csv! []
+  (mt/with-current-user (mt/user->id :rasta)
+    (upload-test/create-upload-table!)))
+
+(defn- update-csv! [options]
+  (@#'api.table/update-csv! options))
 
-(defn- append-csv-via-api!
+(defn- update-csv-via-api!
   "Upload a small CSV file to the given collection ID. Default args can be overridden"
-  []
-  (mt/with-current-user (mt/user->id :rasta)
-    (mt/with-empty-db
-      (let [file  (upload-test/csv-file-with ["name" "Luke Skywalker" "Darth Vader"] (mt/random-name))
-            table (upload-test/create-upload-table!)]
-        (mt/with-current-user (mt/user->id :crowberto)
-          (append-csv! {:table-id (:id table)
-                        :file     file}))))))
+  [action]
+  (let [table (create-csv!)
+        file  (upload-test/csv-file-with ["name" "Luke Skywalker" "Darth Vader"] (mt/random-name))]
+    (mt/with-current-user (mt/user->id :crowberto)
+      (update-csv! {:action   action
+                    :table-id (:id table)
+                    :file     file}))))
 
 (deftest append-csv-test
   (mt/test-driver :h2
@@ -986,11 +963,11 @@
       (testing "Happy path"
         (mt/with-temporary-setting-values [uploads-enabled true]
           (is (= {:status 200, :body nil}
-                 (append-csv-via-api!)))))
+                 (update-csv-via-api! :metabase.upload/append)))))
       (testing "Failure paths return an appropriate status code and a message in the body"
         (mt/with-temporary-setting-values [uploads-enabled false]
           (is (= {:status 422, :body {:message "Uploads are not enabled."}}
-                 (append-csv-via-api!))))))))
+                 (update-csv-via-api! :metabase.upload/append))))))))
 
 (deftest append-csv-deletes-file-test
   (testing "File gets deleted after appending"
@@ -1002,7 +979,8 @@
                 table (upload-test/create-upload-table!)]
             (is (.exists file) "File should exist before append-csv!")
             (mt/with-current-user (mt/user->id :crowberto)
-              (append-csv! {:table-id (:id table)
+              (update-csv! {:action :metabase.upload/append
+                            :table-id (:id table)
                             :file     file}))
             (is (not (.exists file)) "File should be deleted after append-csv!")))))))
 
@@ -1010,31 +988,17 @@
 ;;; |                                          POST /api/table/:id/replace-csv                                        |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
-(defn- replace-csv! [options]
-  (@#'api.table/update-csv! (assoc options :action :metabase.upload/replace)))
-
-(defn- replace-csv-via-api!
-  "Upload a small CSV file to the given collection ID. Default args can be overridden"
-  []
-  (mt/with-current-user (mt/user->id :rasta)
-                        (mt/with-empty-db
-                         (let [file  (upload-test/csv-file-with ["name" "Luke Skywalker" "Darth Vader"] (mt/random-name))
-                               table (upload-test/create-upload-table!)]
-                           (mt/with-current-user (mt/user->id :crowberto)
-                             (replace-csv! {:table-id (:id table)
-                                            :file     file}))))))
-
 (deftest replace-csv-test
   (mt/test-driver :h2
     (mt/with-empty-db
      (testing "Happy path"
        (mt/with-temporary-setting-values [uploads-enabled true]
          (is (= {:status 200, :body nil}
-                (replace-csv-via-api!)))))
+                (update-csv-via-api! :metabase.upload/replace)))))
      (testing "Failure paths return an appropriate status code and a message in the body"
        (mt/with-temporary-setting-values [uploads-enabled false]
          (is (= {:status 422, :body {:message "Uploads are not enabled."}}
-                (replace-csv-via-api!))))))))
+                (update-csv-via-api! :metabase.upload/replace))))))))
 
 (deftest replace-csv-deletes-file-test
   (testing "File gets deleted after replacing"
@@ -1046,6 +1010,7 @@
                table    (upload-test/create-upload-table!)]
            (is (.exists file) "File should exist before replace-csv!")
            (mt/with-current-user (mt/user->id :crowberto)
-             (replace-csv! {:table-id (:id table)
-                            :file     file}))
+             (update-csv! {:action   :metabase.upload/replace
+                           :table-id (:id table)
+                           :file     file}))
            (is (not (.exists file)) "File should be deleted after replace-csv!")))))))

test/metabase/upload_test.clj

@@ -1752,3 +1752,43 @@
                       (rows-for-table table)))
 
               (io/delete-file file))))))))
+
+(defn- upload-table-exists? [table]
+  ;; we don't need to worry about sql injection here
+  (-> (format "SELECT 1 FROM information_schema.tables WHERE table_name = '%s'" (:name table))
+      ((fn [sql] {:database (:db_id table), :type :native, :native {:query sql}}))
+      qp/process-query
+      :row_count
+      pos?))
+
+(deftest delete-upload!-test
+  (mt/test-drivers (mt/normal-drivers-with-feature :uploads)
+    (doseq [archive-cards? [true false]]
+      (with-upload-table! [table (create-upload-table!
+                                  :col->upload-type (ordered-map/ordered-map
+                                                     :_mb_row_id auto-pk-type
+                                                     :number_1 int-type
+                                                     :number_2 int-type)
+                                  :rows [[1, 1]])]
+
+        (testing "The upload table and the expected application data are created\n"
+          (is (upload-table-exists? table))
+          (is (seq (t2/select :model/Table :id (:id table))))
+          (testing "The expected metadata is synchronously sync'd"
+            (is (seq (t2/select :model/Field :table_id (:id table))))))
+
+        (mt/with-temp [:model/Card {card-id :id} {:table_id (:id table)}]
+          (is (false? (:archived (t2/select-one :model/Card :id card-id))))
+
+          (upload/delete-upload! table :archive-cards? archive-cards?)
+
+          (testing (format "We %s the related cards if archive-cards? is %s"
+                           (if archive-cards? "archive" "do not archive")
+                           archive-cards?)
+            (is (= archive-cards? (:archived (t2/select-one :model/Card :id card-id)))))
+
+          (testing "The upload table and related application data are deleted\n"
+            (is (not (upload-table-exists? table)))
+            (is (= [false] (mapv :active (t2/select :model/Table :id (:id table)))))
+            (testing "We do not clean up any of the child resources synchronously (yet?)"
+              (is (seq (t2/select :model/Field :table_id (:id table)))))))))))