From f3ddcb66a700d8104b01c0b605383bcde285be19 Mon Sep 17 00:00:00 2001
From: Cal Herries <39073188+calherries@users.noreply.github.com>
Date: Mon, 27 May 2024 11:18:46 -0600
Subject: [PATCH] Migrate uploads settings to the db-level behind the scenes,
so the uploads DB can be set by the config file (#42869)
---
.clj-kondo/hooks/metabase/models/setting.clj | 1 -
.../scenarios/collections/uploads.cy.spec.js | 9 +-
.../advanced_permissions/common_test.clj | 49 +++--
.../upload_management/api_test.clj | 74 +++----
.../test/metabase_enterprise/upload_test.clj | 85 +++++----
frontend/src/metabase-types/api/database.ts | 3 +
.../src/metabase-types/api/mocks/database.ts | 3 +
.../src/metabase-types/api/mocks/settings.ts | 9 +-
frontend/src/metabase-types/api/settings.ts | 11 +-
.../UploadSettings.unit.spec.tsx | 180 +++++++++---------
.../UploadSettings/UploadSettingsForm.tsx | 56 +++---
.../src/metabase/admin/settings/selectors.js | 27 +--
.../CollectionContent/CollectionContent.tsx | 8 +-
.../CollectionContentView.tsx | 5 +-
.../use-setting/use-setting.unit.spec.tsx | 2 +-
frontend/src/metabase/lib/settings.ts | 2 +-
.../src/metabase/query_builder/selectors.js | 17 +-
.../FileUploadStatus.unit.spec.tsx | 7 +-
.../migrations/001_update_migrations.yaml | 50 +++++
src/metabase/api/card.clj | 9 +-
src/metabase/api/database.clj | 17 +-
src/metabase/db/custom_migrations.clj | 29 +++
src/metabase/models/database.clj | 13 +-
src/metabase/public_settings.clj | 139 ++++++++------
src/metabase/upload.clj | 18 +-
test/metabase/api/card_test.clj | 50 ++---
test/metabase/api/collection_test.clj | 2 +-
test/metabase/api/database_test.clj | 29 +--
test/metabase/api/table_test.clj | 11 +-
test/metabase/db/custom_migrations_test.clj | 75 ++++++++
test/metabase/upload_test.clj | 129 +++++++------
31 files changed, 643 insertions(+), 476 deletions(-)
diff --git a/.clj-kondo/hooks/metabase/models/setting.clj b/.clj-kondo/hooks/metabase/models/setting.clj
index 32eb3266aec..9e65a8093ae 100644
--- a/.clj-kondo/hooks/metabase/models/setting.clj
+++ b/.clj-kondo/hooks/metabase/models/setting.clj
@@ -162,7 +162,6 @@
token-status
toucan-name
uncached-setting
- uploads-table-prefix
user-visibility
version
version-info
diff --git a/e2e/test/scenarios/collections/uploads.cy.spec.js b/e2e/test/scenarios/collections/uploads.cy.spec.js
index a4dac07e17f..b1dad35e4cd 100644
--- a/e2e/test/scenarios/collections/uploads.cy.spec.js
+++ b/e2e/test/scenarios/collections/uploads.cy.spec.js
@@ -504,10 +504,11 @@ function headlessUpload(file) {
function enableUploads(dialect) {
const settings = {
- "uploads-enabled": true,
- "uploads-database-id": WRITABLE_DB_ID,
- "uploads-schema-name": dialect === "postgres" ? "public" : null,
- "uploads-table-prefix": dialect === "mysql" ? "upload_" : null,
+ "uploads-settings": {
+ db_id: WRITABLE_DB_ID,
+ schema_name: dialect === "postgres" ? "public" : null,
+ table_prefix: dialect === "mysql" ? "upload_" : null,
+ },
};
cy.request("PUT", "/api/setting", settings);
diff --git a/enterprise/backend/test/metabase_enterprise/advanced_permissions/common_test.clj b/enterprise/backend/test/metabase_enterprise/advanced_permissions/common_test.clj
index 73beff846cc..32d7dda8a8a 100644
--- a/enterprise/backend/test/metabase_enterprise/advanced_permissions/common_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/advanced_permissions/common_test.clj
@@ -770,10 +770,10 @@
(perms/grant-application-permissions! (perms-group/all-users) :setting)
(testing "Upload DB can be set with the right permission"
(mt/with-all-users-data-perms-graph! {(mt/id) {:details :yes}}
- (mt/user-http-request :rasta :put 204 "setting/" {:uploads-database-id (mt/id)})))
+ (mt/user-http-request :rasta :put 204 "setting/" {:uploads-settings {:db_id (mt/id) :schema_name nil :table_prefix nil}})))
(testing "Upload DB cannot be set without the right permission"
(mt/with-all-users-data-perms-graph! {(mt/id) {:details :no}}
- (mt/user-http-request :rasta :put 403 "setting/" {:uploads-database-id (mt/id)})))
+ (mt/user-http-request :rasta :put 403 "setting/" {:uploads-settings {:db_id (mt/id) :schema_name nil :table_prefix nil}})))
(perms/revoke-application-permissions! (perms-group/all-users) :setting))
(deftest upload-csv-test
@@ -858,29 +858,24 @@
(mt/test-drivers (mt/normal-drivers-with-feature :uploads :schemas)
(testing "GET /api/database and GET /api/database/:id responses should include can_upload depending on unrestricted data access to the upload schema"
(mt/with-model-cleanup [:model/Table]
- (let [schema-name (sql.tx/session-schema driver/*driver*)]
+ (let [schema-name (sql.tx/session-schema driver/*driver*)
+ db-id (u/the-id (mt/db))]
(upload-test/with-upload-table! [table (upload-test/create-upload-table! :schema-name schema-name)]
- (let [db-id (u/the-id (mt/db))]
- (mt/with-temp [:model/Table {} {:db_id db-id :schema "some_schema"}]
- (mt/with-temporary-setting-values [uploads-enabled true
- uploads-database-id db-id
- uploads-schema-name schema-name
- uploads-table-prefix "uploaded_magic_"]
- (doseq [[schema-perms can-upload?] {:query-builder true
- :no false
- {(:id table) :query-builder} false}]
- (testing (format "can_upload should be %s if the user has %s access to the upload schema"
- can-upload? schema-perms)
- (mt/with-all-users-data-perms-graph! {db-id {:view-data :unrestricted
- :create-queries {"some_schema" :query-builder
- schema-name schema-perms}}}
- (testing "GET /api/database"
- (let [result (->> (mt/user-http-request :rasta :get 200 "database")
- :data
- (filter #(= (:id %) db-id))
- first)]
- (def res (mt/user-http-request :rasta :get 200 "database"))
- (is (= can-upload? (:can_upload result)))))
- (testing "GET /api/database/:id"
- (let [result (mt/user-http-request :rasta :get 200 (format "database/%d" db-id))]
- (is (= can-upload? (:can_upload result)))))))))))))))))
+ (mt/with-temp [:model/Table {} {:db_id db-id :schema "some_schema"}]
+ (doseq [[schema-perms can-upload?] {:query-builder true
+ :no false
+ {(:id table) :query-builder} false}]
+ (testing (format "can_upload should be %s if the user has %s access to the upload schema"
+ can-upload? schema-perms)
+ (mt/with-all-users-data-perms-graph! {db-id {:view-data :unrestricted
+ :create-queries {"some_schema" :query-builder
+ schema-name schema-perms}}}
+ (testing "GET /api/database"
+ (let [result (->> (mt/user-http-request :rasta :get 200 "database")
+ :data
+ (filter #(= (:id %) db-id))
+ first)]
+ (is (= can-upload? (:can_upload result)))))
+ (testing "GET /api/database/:id"
+ (let [result (mt/user-http-request :rasta :get 200 (format "database/%d" db-id))]
+ (is (= can-upload? (:can_upload result)))))))))))))))
diff --git a/enterprise/backend/test/metabase_enterprise/upload_management/api_test.clj b/enterprise/backend/test/metabase_enterprise/upload_management/api_test.clj
index 2b46eb55808..cbde41ceb1e 100644
--- a/enterprise/backend/test/metabase_enterprise/upload_management/api_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/upload_management/api_test.clj
@@ -6,6 +6,7 @@
[metabase.api.table-test :as oss-test]
[metabase.test :as mt]
[metabase.upload :as upload]
+ [metabase.upload-test :as upload-test]
[toucan2.tools.with-temp :as t2.with-temp]))
(def list-url "ee/upload-management/tables")
@@ -47,44 +48,45 @@
(testing "DELETE ee/upload-management/:id"
(mt/test-driver :h2
(mt/with-empty-db
- (testing "Behind a feature flag"
- (mt/with-premium-features #{} ;; not :upload-management
- (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."))))
+ (upload-test/with-uploads-enabled
+ (testing "Behind a feature flag"
+ (mt/with-premium-features #{} ;; not :upload-management
+ (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]
+ (mt/with-premium-features #{:upload-management}
+ (testing "Happy path\n"
(let [table-id (:id (oss-test/create-csv!))]
- (is (true? (mt/user-http-request :crowberto :delete 200 (delete-url table-id)))))))
+ (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 "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 "Uploads may be deleted even when *uploading* has been disabled"
+ (upload-test/with-uploads-disabled
+ (let [table-id (:id (oss-test/create-csv!))]
+ (is (true? (mt/user-http-request :crowberto :delete 200 (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)))))
+ (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 "The archive_cards argument is passed through"
- (let [passed-value (atom nil)]
- (mt/with-dynamic-redefs [upload/delete-upload! (fn [_ & {:keys [archive-cards?]}]
- (reset! passed-value archive-cards?)
- :done)]
- (let [table-id (:id (oss-test/create-csv!))]
- (is (mt/user-http-request :crowberto :delete 200 (delete-url table-id) :archive-cards true))
- (is (true? @passed-value)))))))))))
+ (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)))))
+
+ (testing "The archive_cards argument is passed through"
+ (let [passed-value (atom nil)]
+ (mt/with-dynamic-redefs [upload/delete-upload! (fn [_ & {:keys [archive-cards?]}]
+ (reset! passed-value archive-cards?)
+ :done)]
+ (let [table-id (:id (oss-test/create-csv!))]
+ (is (mt/user-http-request :crowberto :delete 200 (delete-url table-id) :archive-cards true))
+ (is (true? @passed-value))))))))))))
diff --git a/enterprise/backend/test/metabase_enterprise/upload_test.clj b/enterprise/backend/test/metabase_enterprise/upload_test.clj
index 5aa059daffb..da5d7650a69 100644
--- a/enterprise/backend/test/metabase_enterprise/upload_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/upload_test.clj
@@ -2,6 +2,7 @@
(:require
[clojure.test :refer :all]
[metabase-enterprise.test :as met]
+ [metabase.driver :as driver]
[metabase.test :as mt]
[metabase.test.fixtures :as fixtures]
[metabase.upload-test :as upload-test]))
@@ -10,51 +11,53 @@
(use-fixtures :once (fixtures/initialize :db :test-users))
-(deftest create-disabled-for-sandboxed-user-test
+(deftest uploads-disabled-for-sandboxed-user-test
(mt/test-drivers (mt/normal-drivers-with-feature :uploads)
(met/with-gtaps-for-user! :rasta {:gtaps {:venues {}}}
- (is (thrown-with-msg? Exception #"Uploads are not permitted for sandboxed users\."
- (upload-test/upload-example-csv! {:grant-permission? false}))))))
-
-(deftest update-disabled-for-sandboxed-user-test
- (mt/test-drivers (mt/normal-drivers-with-feature :uploads)
- (doseq [verb [:metabase.upload/append :metabase.upload/replace]]
- (met/with-gtaps-for-user! :rasta {:gtaps {:venues {}}}
- (is (thrown-with-msg? Exception #"Uploads are not permitted for sandboxed users\."
- (upload-test/update-csv-with-defaults! verb :user-id (mt/user->id :rasta))))))))
+ (mt/with-temp [:model/Database db {:engine driver/*driver* :details (:details (mt/db))}]
+ (mt/with-db db
+ (testing "If the user is sandboxed, creating a new upload should fail"
+ (upload-test/with-uploads-enabled
+ (is (thrown-with-msg? Exception #"Uploads are not permitted for sandboxed users\."
+ (upload-test/upload-example-csv! {:grant-permission? false})))))
+ (upload-test/with-uploads-enabled
+ (doseq [verb [:metabase.upload/append :metabase.upload/replace]]
+ (testing (format "If the user is sandboxed, %s should fail" (name verb))
+ (is (thrown-with-msg? Exception #"Uploads are not permitted for sandboxed users\."
+ (upload-test/update-csv-with-defaults! verb :user-id (mt/user->id :rasta))))))))))))
(deftest based-on-upload-for-sandboxed-user-test
- (mt/with-temporary-setting-values [uploads-enabled true]
;; FIXME: Redshift is flaking on `mt/dataset` and I don't know why, so I'm excluding it temporarily
(mt/test-drivers (disj (mt/normal-drivers-with-feature :uploads) :redshift)
- (mt/dataset (mt/dataset-definition
- (mt/random-name)
- ["venues"
- [{:field-name "name" :base-type :type/Text}]
- [["something"]]])
- (mt/with-temp [:model/Collection collection {}
- :model/Database {db-id :id} {:engine "h2"}
- :model/Table {table-id :id} {:db_id db-id
- :is_upload true}
- :model/Card {card-id :id
- :as card} {:collection_id (:id collection)
- :type :model
- :dataset_query {:type :query
- :database db-id
- :query {:source-table table-id}}}]
- (let [get-card (fn [] (mt/user-http-request :rasta :get 200 (str "card/" card-id)))
- get-collection-item (fn []
- (->> (mt/user-http-request :rasta :get 200 (str "collection/" (:collection_id card) "/items?models=dataset"))
- :data
- (filter (fn [item]
- (= (:id item) (:id card))))
- first))]
- (testing "Sanity check: if the user is not sandboxed, based_on_upload is non-nil"
- (is (= table-id
- (:based_on_upload (get-card))
- (:based_on_upload (get-collection-item)))))
- (testing "If the user is sandboxed, based_on_upload is nil"
- (met/with-gtaps-for-user! :rasta {:gtaps {:venues {}}}
- (is (= nil
+ (upload-test/with-uploads-enabled
+ (mt/dataset (mt/dataset-definition
+ (mt/random-name)
+ ["venues"
+ [{:field-name "name" :base-type :type/Text}]
+ [["something"]]])
+ (mt/with-temp [:model/Collection collection {}
+ :model/Database {db-id :id} {:engine driver/*driver* :details (:details (mt/db))}
+ :model/Table {table-id :id} {:db_id db-id
+ :is_upload true}
+ :model/Card {card-id :id
+ :as card} {:collection_id (:id collection)
+ :type :model
+ :dataset_query {:type :query
+ :database db-id
+ :query {:source-table table-id}}}]
+ (let [get-card (fn [] (mt/user-http-request :rasta :get 200 (str "card/" card-id)))
+ get-collection-item (fn []
+ (->> (mt/user-http-request :rasta :get 200 (str "collection/" (:collection_id card) "/items?models=dataset"))
+ :data
+ (filter (fn [item]
+ (= (:id item) (:id card))))
+ first))]
+ (testing "Sanity check: if the user is not sandboxed, based_on_upload is non-nil"
+ (is (= table-id
(:based_on_upload (get-card))
- (:based_on_upload (get-collection-item))))))))))))
+ (:based_on_upload (get-collection-item)))))
+ (testing "If the user is sandboxed, based_on_upload is nil"
+ (met/with-gtaps-for-user! :rasta {:gtaps {:venues {}}}
+ (is (= nil
+ (:based_on_upload (get-card))
+ (:based_on_upload (get-collection-item))))))))))))
diff --git a/frontend/src/metabase-types/api/database.ts b/frontend/src/metabase-types/api/database.ts
index dc3c491f869..01e19d169a1 100644
--- a/frontend/src/metabase-types/api/database.ts
+++ b/frontend/src/metabase-types/api/database.ts
@@ -51,6 +51,9 @@ export interface Database extends DatabaseData {
created_at: ISO8601Time;
updated_at: ISO8601Time;
can_upload: boolean;
+ uploads_enabled: boolean;
+ uploads_schema_name: string | null;
+ uploads_table_prefix: string | null;
// Only appears in GET /api/database/:id
"can-manage"?: boolean;
diff --git a/frontend/src/metabase-types/api/mocks/database.ts b/frontend/src/metabase-types/api/mocks/database.ts
index b218dd3b930..0a48abba4db 100644
--- a/frontend/src/metabase-types/api/mocks/database.ts
+++ b/frontend/src/metabase-types/api/mocks/database.ts
@@ -37,6 +37,9 @@ export const createMockDatabase = (opts?: Partial<Database>): Database => ({
native_permissions: "write",
initial_sync_status: "complete",
features: COMMON_DATABASE_FEATURES,
+ uploads_enabled: false,
+ uploads_schema_name: null,
+ uploads_table_prefix: null,
...opts,
});
diff --git a/frontend/src/metabase-types/api/mocks/settings.ts b/frontend/src/metabase-types/api/mocks/settings.ts
index 51cbcb2f774..198b72434ad 100644
--- a/frontend/src/metabase-types/api/mocks/settings.ts
+++ b/frontend/src/metabase-types/api/mocks/settings.ts
@@ -231,10 +231,11 @@ export const createMockSettings = (
version: createMockVersion(),
"version-info": createMockVersionInfo(),
"version-info-last-checked": null,
- "uploads-enabled": false,
- "uploads-database-id": null,
- "uploads-table-prefix": null,
- "uploads-schema-name": null,
+ "uploads-settings": {
+ db_id: null,
+ schema_name: null,
+ table_prefix: null,
+ },
"user-visibility": null,
"last-acknowledged-version": "v1",
"last-used-native-database-id": 1,
diff --git a/frontend/src/metabase-types/api/settings.ts b/frontend/src/metabase-types/api/settings.ts
index 376cd51ccfb..d597a4f52ea 100644
--- a/frontend/src/metabase-types/api/settings.ts
+++ b/frontend/src/metabase-types/api/settings.ts
@@ -190,6 +190,12 @@ export interface OpenAiModel {
export type HelpLinkSetting = "metabase" | "hidden" | "custom";
+export interface UploadsSettings {
+ db_id: number | null;
+ schema_name: string | null;
+ table_prefix: string | null;
+}
+
interface InstanceSettings {
"admin-email": string;
"email-smtp-host": string | null;
@@ -210,10 +216,7 @@ interface InstanceSettings {
"show-homepage-xrays": boolean;
"site-uuid": string;
"subscription-allowed-domains": string | null;
- "uploads-enabled": boolean;
- "uploads-database-id": number | null;
- "uploads-schema-name": string | null;
- "uploads-table-prefix": string | null;
+ "uploads-settings": UploadsSettings;
"user-visibility": string | null;
}
diff --git a/frontend/src/metabase/admin/settings/components/UploadSettings/UploadSettings.unit.spec.tsx b/frontend/src/metabase/admin/settings/components/UploadSettings/UploadSettings.unit.spec.tsx
index b7549a043c6..c1a73211e76 100644
--- a/frontend/src/metabase/admin/settings/components/UploadSettings/UploadSettings.unit.spec.tsx
+++ b/frontend/src/metabase/admin/settings/components/UploadSettings/UploadSettings.unit.spec.tsx
@@ -7,9 +7,9 @@ import { checkNotNull } from "metabase/lib/types";
import { getMetadata } from "metabase/selectors/metadata";
import type { Database } from "metabase-types/api";
import { createMockDatabase, createMockTable } from "metabase-types/api/mocks";
+import type { UploadsSettings } from "metabase-types/api/settings";
import { createMockState } from "metabase-types/store/mocks";
-import type { UploadSettings } from "./UploadSettingsForm";
import { UploadSettingsFormView } from "./UploadSettingsForm";
const TEST_DATABASES = [
@@ -52,16 +52,15 @@ const TEST_DATABASES = [
interface SetupOpts {
databases?: Database[];
- settings?: UploadSettings;
+ uploadsSettings?: UploadsSettings;
}
function setup({
databases = TEST_DATABASES,
- settings = {
- uploads_enabled: false,
- uploads_database_id: null,
- uploads_schema_name: null,
- uploads_table_prefix: null,
+ uploadsSettings = {
+ db_id: null,
+ schema_name: null,
+ table_prefix: null,
},
}: SetupOpts = {}) {
const state = createMockState({
@@ -81,7 +80,7 @@ function setup({
renderWithProviders(
<UploadSettingsFormView
databases={databases.map(({ id }) => checkNotNull(metadata.database(id)))}
- settings={settings}
+ uploadsSettings={uploadsSettings}
updateSettings={updateSpy}
saveStatusRef={{
current: {
@@ -157,10 +156,11 @@ describe("Admin > Settings > UploadSetting", () => {
);
expect(updateSpy).toHaveBeenCalledWith({
- "uploads-enabled": true,
- "uploads-database-id": 1,
- "uploads-schema-name": "uploads",
- "uploads-table-prefix": null,
+ "uploads-settings": {
+ db_id: 1,
+ schema_name: "uploads",
+ table_prefix: null,
+ },
});
});
@@ -181,10 +181,11 @@ describe("Admin > Settings > UploadSetting", () => {
);
expect(updateSpy).toHaveBeenCalledWith({
- "uploads-enabled": true,
- "uploads-database-id": 2,
- "uploads-schema-name": null,
- "uploads-table-prefix": "my_prefix_",
+ "uploads-settings": {
+ db_id: 2,
+ schema_name: null,
+ table_prefix: "my_prefix_",
+ },
});
});
@@ -210,10 +211,11 @@ describe("Admin > Settings > UploadSetting", () => {
);
expect(updateSpy).toHaveBeenCalledWith({
- "uploads-enabled": true,
- "uploads-database-id": 1,
- "uploads-schema-name": "uploads",
- "uploads-table-prefix": "my_prefix_",
+ "uploads-settings": {
+ db_id: 1,
+ schema_name: "uploads",
+ table_prefix: "my_prefix_",
+ },
});
});
@@ -250,10 +252,11 @@ describe("Admin > Settings > UploadSetting", () => {
);
expect(updateSpy).toHaveBeenCalledWith({
- "uploads-enabled": true,
- "uploads-database-id": 2,
- "uploads-schema-name": null,
- "uploads-table-prefix": "upload_",
+ "uploads-settings": {
+ db_id: 2,
+ schema_name: null,
+ table_prefix: "upload_",
+ },
});
expect(await screen.findByText(/There was a problem/i)).toBeInTheDocument();
@@ -261,11 +264,10 @@ describe("Admin > Settings > UploadSetting", () => {
it("should be able to disable uploads", async () => {
const { updateSpy } = setup({
- settings: {
- uploads_enabled: true,
- uploads_database_id: 2,
- uploads_schema_name: null,
- uploads_table_prefix: null,
+ uploadsSettings: {
+ db_id: 2,
+ schema_name: null,
+ table_prefix: null,
},
});
await userEvent.click(
@@ -273,20 +275,20 @@ describe("Admin > Settings > UploadSetting", () => {
);
expect(updateSpy).toHaveBeenCalledWith({
- "uploads-enabled": false,
- "uploads-database-id": null,
- "uploads-schema-name": null,
- "uploads-table-prefix": null,
+ "uploads-settings": {
+ db_id: null,
+ schema_name: null,
+ table_prefix: null,
+ },
});
});
it("should show an error if disabling fails", async () => {
const { updateSpy, savingSpy, clearSpy, savedSpy } = setup({
- settings: {
- uploads_enabled: true,
- uploads_database_id: 2,
- uploads_schema_name: null,
- uploads_table_prefix: null,
+ uploadsSettings: {
+ db_id: 2,
+ schema_name: null,
+ table_prefix: null,
},
});
updateSpy.mockImplementation(() => Promise.reject(new Error("Oh no!")));
@@ -295,10 +297,11 @@ describe("Admin > Settings > UploadSetting", () => {
);
expect(updateSpy).toHaveBeenCalledWith({
- "uploads-enabled": false,
- "uploads-database-id": null,
- "uploads-schema-name": null,
- "uploads-table-prefix": null,
+ "uploads-settings": {
+ db_id: null,
+ schema_name: null,
+ table_prefix: null,
+ },
});
expect(await screen.findByText(/There was a problem/i)).toBeInTheDocument();
@@ -309,11 +312,10 @@ describe("Admin > Settings > UploadSetting", () => {
it("should populate db and schema from existing settings", async () => {
setup({
- settings: {
- uploads_enabled: true,
- uploads_database_id: 1,
- uploads_schema_name: "top_secret",
- uploads_table_prefix: null,
+ uploadsSettings: {
+ db_id: 1,
+ schema_name: "top_secret",
+ table_prefix: null,
},
});
@@ -323,11 +325,10 @@ describe("Admin > Settings > UploadSetting", () => {
it("should populate db and stable prefix from existing settings", async () => {
setup({
- settings: {
- uploads_enabled: true,
- uploads_database_id: 2,
- uploads_schema_name: null,
- uploads_table_prefix: "my_uploads_",
+ uploadsSettings: {
+ db_id: 2,
+ schema_name: null,
+ table_prefix: "my_uploads_",
},
});
@@ -337,13 +338,15 @@ describe("Admin > Settings > UploadSetting", () => {
it("should show a message if there are no schema for the selected db", async () => {
setup({
- settings: {
- uploads_enabled: false,
- uploads_database_id: 5,
- uploads_schema_name: null,
- uploads_table_prefix: null,
+ uploadsSettings: {
+ db_id: null,
+ schema_name: null,
+ table_prefix: null,
},
});
+ const dbItem = await screen.findByText("Select a database");
+ await userEvent.click(dbItem);
+ await userEvent.click(await screen.findByText("Db Cinco"));
expect(
await screen.findByText(/We couldn't find any schema/i),
@@ -355,11 +358,10 @@ describe("Admin > Settings > UploadSetting", () => {
it("should be able to update db settings", async () => {
const { updateSpy } = setup({
- settings: {
- uploads_enabled: true,
- uploads_database_id: 2,
- uploads_schema_name: null,
- uploads_table_prefix: null,
+ uploadsSettings: {
+ db_id: 2,
+ schema_name: null,
+ table_prefix: null,
},
});
await userEvent.click(await screen.findByText("Db Dos"));
@@ -387,10 +389,11 @@ describe("Admin > Settings > UploadSetting", () => {
);
expect(updateSpy).toHaveBeenCalledWith({
- "uploads-enabled": true,
- "uploads-database-id": 1,
- "uploads-schema-name": "uploads",
- "uploads-table-prefix": null,
+ "uploads-settings": {
+ db_id: 1,
+ schema_name: "uploads",
+ table_prefix: null,
+ },
});
});
@@ -416,11 +419,10 @@ describe("Admin > Settings > UploadSetting", () => {
it("should show enabled disable button when a db is populated", async () => {
setup({
- settings: {
- uploads_enabled: true,
- uploads_database_id: 2,
- uploads_schema_name: null,
- uploads_table_prefix: null,
+ uploadsSettings: {
+ db_id: 2,
+ schema_name: null,
+ table_prefix: null,
},
});
expect(
@@ -442,11 +444,10 @@ describe("Admin > Settings > UploadSetting", () => {
it("should show the only the update button when a db is changed", async () => {
setup({
- settings: {
- uploads_enabled: true,
- uploads_database_id: 2,
- uploads_schema_name: null,
- uploads_table_prefix: null,
+ uploadsSettings: {
+ db_id: 2,
+ schema_name: null,
+ table_prefix: null,
},
});
await userEvent.click(await screen.findByText("Db Dos"));
@@ -477,11 +478,10 @@ describe("Admin > Settings > UploadSetting", () => {
it("should show the update button when a table prefix is changed", async () => {
setup({
- settings: {
- uploads_enabled: true,
- uploads_database_id: 2,
- uploads_schema_name: null,
- uploads_table_prefix: "up_",
+ uploadsSettings: {
+ db_id: 2,
+ schema_name: null,
+ table_prefix: "up_",
},
});
@@ -496,11 +496,10 @@ describe("Admin > Settings > UploadSetting", () => {
it("should show a loading spinner on submit", async () => {
const { updateSpy } = setup({
- settings: {
- uploads_enabled: true,
- uploads_database_id: 2,
- uploads_schema_name: null,
- uploads_table_prefix: "up_",
+ uploadsSettings: {
+ db_id: 2,
+ schema_name: null,
+ table_prefix: "up_",
},
});
updateSpy.mockImplementation(
@@ -523,11 +522,10 @@ describe("Admin > Settings > UploadSetting", () => {
it("should reset button loading state on input change", async () => {
const { updateSpy } = setup({
- settings: {
- uploads_enabled: true,
- uploads_database_id: 2,
- uploads_schema_name: null,
- uploads_table_prefix: "up_",
+ uploadsSettings: {
+ db_id: 2,
+ schema_name: null,
+ table_prefix: "up_",
},
});
updateSpy.mockImplementation(
diff --git a/frontend/src/metabase/admin/settings/components/UploadSettings/UploadSettingsForm.tsx b/frontend/src/metabase/admin/settings/components/UploadSettings/UploadSettingsForm.tsx
index e91559d3163..c74ee243976 100644
--- a/frontend/src/metabase/admin/settings/components/UploadSettings/UploadSettingsForm.tsx
+++ b/frontend/src/metabase/admin/settings/components/UploadSettings/UploadSettingsForm.tsx
@@ -20,6 +20,7 @@ import { getSetting } from "metabase/selectors/settings";
import { Stack, Group, Text } from "metabase/ui";
import type Database from "metabase-lib/v1/metadata/Database";
import type Schema from "metabase-lib/v1/metadata/Schema";
+import type { UploadsSettings } from "metabase-types/api/settings";
import type { State } from "metabase-types/store";
import SettingHeader from "../SettingHeader";
@@ -31,18 +32,14 @@ const FEEDBACK_TIMEOUT = 5000;
const enableErrorMessage = t`There was a problem enabling uploads. Please try again shortly.`;
const disableErrorMessage = t`There was a problem disabling uploads. Please try again shortly.`;
-export interface UploadSettings {
- uploads_enabled: boolean;
- uploads_database_id: number | null;
- uploads_schema_name: string | null;
- uploads_table_prefix: string | null;
-}
-
interface UploadSettingProps {
databases: Database[];
- settings: UploadSettings;
+ uploadsSettings: UploadsSettings;
updateSettings: (
- settings: Record<string, string | number | boolean | null>,
+ settings: Record<
+ string,
+ string | number | boolean | UploadsSettings | null
+ >,
) => Promise<void>;
saveStatusRef: React.RefObject<{
setSaving: () => void;
@@ -53,12 +50,7 @@ interface UploadSettingProps {
}
const mapStateToProps = (state: State) => ({
- settings: {
- uploads_enabled: getSetting(state, "uploads-enabled"),
- uploads_database_id: getSetting(state, "uploads-database-id"),
- uploads_schema_name: getSetting(state, "uploads-schema-name"),
- uploads_table_prefix: getSetting(state, "uploads-table-prefix"),
- },
+ uploadsSettings: getSetting(state, "uploads-settings"),
});
const mapDispatchToProps = {
@@ -83,18 +75,18 @@ const Header = () => (
export function UploadSettingsFormView({
databases,
- settings,
+ uploadsSettings,
updateSettings,
saveStatusRef,
}: UploadSettingProps) {
const [dbId, setDbId] = useState<number | null>(
- settings.uploads_database_id ?? null,
+ uploadsSettings.db_id ?? null,
);
const [schemaName, setSchemaName] = useState<string | null>(
- settings.uploads_schema_name ?? null,
+ uploadsSettings.schema_name ?? null,
);
const [tablePrefix, setTablePrefix] = useState<string | null>(
- settings.uploads_table_prefix ?? null,
+ uploadsSettings.table_prefix ?? null,
);
const [errorMessage, setErrorMessage] = useState<null | string>(null);
const dispatch = useDispatch();
@@ -125,10 +117,11 @@ export function UploadSettingsFormView({
const handleEnableUploads = async () => {
showSaving();
return updateSettings({
- "uploads-enabled": true,
- "uploads-database-id": dbId,
- "uploads-schema-name": schemaName,
- "uploads-table-prefix": tablePrefix,
+ "uploads-settings": {
+ db_id: dbId,
+ schema_name: schemaName,
+ table_prefix: tablePrefix,
+ },
})
.then(() => {
setSchemaName(schemaName);
@@ -142,10 +135,11 @@ export function UploadSettingsFormView({
const handleDisableUploads = () => {
showSaving();
return updateSettings({
- "uploads-enabled": false,
- "uploads-database-id": null,
- "uploads-schema-name": null,
- "uploads-table-prefix": null,
+ "uploads-settings": {
+ db_id: null,
+ schema_name: null,
+ table_prefix: null,
+ },
})
.then(() => {
setDbId(null);
@@ -159,9 +153,9 @@ export function UploadSettingsFormView({
const showPrefix = !!dbId;
const hasValidSettings = Boolean(dbId && (!showSchema || schemaName));
const settingsChanged =
- dbId !== settings.uploads_database_id ||
- schemaName !== settings.uploads_schema_name ||
- tablePrefix !== settings.uploads_table_prefix;
+ dbId !== uploadsSettings.db_id ||
+ schemaName !== uploadsSettings.schema_name ||
+ tablePrefix !== uploadsSettings.table_prefix;
const hasValidDatabases = databaseOptions.length > 0;
const isH2db = Boolean(
@@ -229,7 +223,7 @@ export function UploadSettingsFormView({
)}
</Group>
<Group mt="lg">
- {settings.uploads_enabled ? (
+ {uploadsSettings.db_id ? (
settingsChanged ? (
<ActionButton
ref={updateButtonRef}
diff --git a/frontend/src/metabase/admin/settings/selectors.js b/frontend/src/metabase/admin/settings/selectors.js
index f6dc6ce7008..49a68c836d2 100644
--- a/frontend/src/metabase/admin/settings/selectors.js
+++ b/frontend/src/metabase/admin/settings/selectors.js
@@ -372,32 +372,7 @@ export const ADMIN_SETTINGS_SECTIONS = {
component: UploadSettings,
settings: [
{
- key: "uploads-enabled",
- display_name: t`Data Uploads`,
- description: t`Enable admins to upload data to new database tables from CSV files.`,
- type: "boolean",
- },
- {
- key: "uploads-database-id",
- getHidden: settings => !settings["uploads-enabled"],
- display_name: t`Database`,
- description: t`Identify a database where upload tables will be created.`,
- placeholder: t`Select a database`,
- },
- {
- key: "uploads-schema-name",
- display_name: t`Schema name`,
- description: t`Identify a database schema where data upload tables will be created.`,
- type: "string",
- placeholder: "uploads",
- },
- {
- key: "uploads-table-prefix",
- display_name: t`Table prefix`,
- description: t`Identify a table prefix for tables created by data uploads.`,
- placeholder: "uploaded_",
- type: "string",
- required: false,
+ key: "uploads-settings",
},
],
},
diff --git a/frontend/src/metabase/collections/components/CollectionContent/CollectionContent.tsx b/frontend/src/metabase/collections/components/CollectionContent/CollectionContent.tsx
index 0ad28634ef4..b4e03baf221 100644
--- a/frontend/src/metabase/collections/components/CollectionContent/CollectionContent.tsx
+++ b/frontend/src/metabase/collections/components/CollectionContent/CollectionContent.tsx
@@ -40,12 +40,10 @@ export function CollectionContent({
id: collectionId,
});
- const uploadDbId = useSelector(state =>
- getSetting(state, "uploads-database-id"),
- );
- const uploadsEnabled = useSelector(state =>
- getSetting(state, "uploads-enabled"),
+ const uploadDbId = useSelector(
+ state => getSetting(state, "uploads-settings")?.db_id,
);
+ const uploadsEnabled = !!uploadDbId;
const canUploadToDb = useSelector(
state =>
diff --git a/frontend/src/metabase/collections/components/CollectionContent/CollectionContentView.tsx b/frontend/src/metabase/collections/components/CollectionContent/CollectionContentView.tsx
index 5e5d1529862..3b70162dec1 100644
--- a/frontend/src/metabase/collections/components/CollectionContent/CollectionContentView.tsx
+++ b/frontend/src/metabase/collections/components/CollectionContent/CollectionContentView.tsx
@@ -204,10 +204,7 @@ export const CollectionContentView = ({
};
const canUpload =
- uploadsEnabled &&
- canUploadToDb &&
- collection.can_write &&
- !isTrashedCollection(collection);
+ canUploadToDb && collection.can_write && !isTrashedCollection(collection);
const dropzoneProps = canUpload ? getComposedDragProps(getRootProps()) : {};
diff --git a/frontend/src/metabase/common/hooks/use-setting/use-setting.unit.spec.tsx b/frontend/src/metabase/common/hooks/use-setting/use-setting.unit.spec.tsx
index fde11bda3b9..9c75a177719 100644
--- a/frontend/src/metabase/common/hooks/use-setting/use-setting.unit.spec.tsx
+++ b/frontend/src/metabase/common/hooks/use-setting/use-setting.unit.spec.tsx
@@ -51,7 +51,7 @@ describe("useTableListQuery", () => {
});
it("should get an empty setting", async () => {
- renderWithProviders(<TestComponent settingName={"uploads-schema-name"} />);
+ renderWithProviders(<TestComponent settingName={"email-smtp-host"} />);
expect(screen.getByText("null")).toBeInTheDocument();
expect(screen.getByText("object")).toBeInTheDocument();
expect(screen.getByText("isNull")).toBeInTheDocument();
diff --git a/frontend/src/metabase/lib/settings.ts b/frontend/src/metabase/lib/settings.ts
index 69711849603..6acca460c25 100644
--- a/frontend/src/metabase/lib/settings.ts
+++ b/frontend/src/metabase/lib/settings.ts
@@ -217,7 +217,7 @@ class MetabaseSettings {
* @deprecated use getSetting(state, "anon-tracking-enabled")
*/
uploadsEnabled() {
- return !!(this.get("uploads-enabled") && this.get("uploads-database-id"));
+ return !!this.get("uploads-settings")?.db_id;
}
/**
diff --git a/frontend/src/metabase/query_builder/selectors.js b/frontend/src/metabase/query_builder/selectors.js
index 23e9f72ea4d..d6a6c27d72d 100644
--- a/frontend/src/metabase/query_builder/selectors.js
+++ b/frontend/src/metabase/query_builder/selectors.js
@@ -1006,18 +1006,11 @@ export const getDashboard = state => {
};
export const canUploadToQuestion = question => state => {
- const uploadsEnabled = getSetting(state, "uploads-enabled");
- if (!uploadsEnabled) {
- return false;
- }
- const uploadsDbId = getSetting(state, "uploads-database-id");
- const canUploadToDb =
- uploadsDbId === question.databaseId() &&
- Databases.selectors
- .getObject(state, {
- entityId: uploadsDbId,
- })
- ?.canUpload();
+ const canUploadToDb = Databases.selectors
+ .getObject(state, {
+ entityId: question.databaseId(),
+ })
+ ?.canUpload();
return canUploadToDb;
};
diff --git a/frontend/src/metabase/status/components/FileUploadStatus/FileUploadStatus.unit.spec.tsx b/frontend/src/metabase/status/components/FileUploadStatus/FileUploadStatus.unit.spec.tsx
index 3316538eb0a..364f536b283 100644
--- a/frontend/src/metabase/status/components/FileUploadStatus/FileUploadStatus.unit.spec.tsx
+++ b/frontend/src/metabase/status/components/FileUploadStatus/FileUploadStatus.unit.spec.tsx
@@ -65,8 +65,11 @@ async function setupCollectionContent(overrides = {}) {
setupBookmarksEndpoints([]);
const settings = createMockSettingsState({
- "uploads-enabled": true,
- "uploads-database-id": 1,
+ "uploads-settings": {
+ db_id: 1,
+ schema_name: null,
+ table_prefix: null,
+ },
});
renderWithProviders(
diff --git a/resources/migrations/001_update_migrations.yaml b/resources/migrations/001_update_migrations.yaml
index dc10c1d6dc1..dba7b62dd17 100644
--- a/resources/migrations/001_update_migrations.yaml
+++ b/resources/migrations/001_update_migrations.yaml
@@ -7364,6 +7364,56 @@ databaseChangeLog:
- customChange:
class: "metabase.db.custom_migrations.MigrateStackedAreaBarComboDisplaySettings"
+ - changeSet:
+ id: v50.2024-05-17T19:54:23
+ author: calherries
+ comment: Add metabase_database.uploads_enabled column
+ changes:
+ - addColumn:
+ tableName: metabase_database
+ columns:
+ - column:
+ name: uploads_enabled
+ type: ${boolean.type}
+ defaultValueBoolean: false
+ remarks: Whether uploads are enabled for this database
+ constraints:
+ nullable: false
+
+ - changeSet:
+ id: v50.2024-05-17T19:54:24
+ author: calherries
+ comment: Add metabase_database.uploads_schema_name column
+ changes:
+ - addColumn:
+ tableName: metabase_database
+ columns:
+ - column:
+ name: uploads_schema_name
+ type: ${text.type}
+ remarks: The schema name for uploads
+
+ - changeSet:
+ id: v50.2024-05-17T19:54:25
+ author: calherries
+ comment: Add metabase_database.uploads_table_prefix column
+ changes:
+ - addColumn:
+ tableName: metabase_database
+ columns:
+ - column:
+ name: uploads_table_prefix
+ type: ${text.type}
+ remarks: The prefix for upload table names
+
+ - changeSet:
+ id: v50.2024-05-17T19:54:26
+ author: calherries
+ comment: Update metabase_database.uploads_enabled value
+ changes:
+ - customChange:
+ class: "metabase.db.custom_migrations.MigrateUploadsSettings"
+
- changeSet:
id: v51.2024-05-13T15:30:57
author: metamben
diff --git a/src/metabase/api/card.clj b/src/metabase/api/card.clj
index 58d10d38d47..d25f8a3ec5c 100644
--- a/src/metabase/api/card.clj
+++ b/src/metabase/api/card.clj
@@ -865,12 +865,13 @@
"This helper function exists to make testing the POST /api/card/from-csv endpoint easier."
[{:keys [collection-id filename file]}]
(try
- (let [model (upload/create-csv-upload! {:collection-id collection-id
+ (let [uploads-db-settings (public-settings/uploads-settings)
+ model (upload/create-csv-upload! {:collection-id collection-id
:filename filename
:file file
- :schema-name (public-settings/uploads-schema-name)
- :table-prefix (public-settings/uploads-table-prefix)
- :db-id (or (public-settings/uploads-database-id)
+ :schema-name (:schema_name uploads-db-settings)
+ :table-prefix (:table_prefix uploads-db-settings)
+ :db-id (or (:db_id uploads-db-settings)
(throw (ex-info (tru "The uploads database is not configured.")
{:status-code 422})))})]
{:status 200
diff --git a/src/metabase/api/database.clj b/src/metabase/api/database.clj
index 3384841ac6e..cd0d6c5505f 100644
--- a/src/metabase/api/database.clj
+++ b/src/metabase/api/database.clj
@@ -244,13 +244,16 @@
[db]
(driver/database-supports? (driver.u/database->driver db) :uploads db))
+(defn- add-can-upload
+ "Adds :can_upload boolean, which is true if the user can create a new upload to this DB."
+ [db]
+ (assoc db :can_upload (upload/can-create-upload? db (:uploads_schema_name db))))
+
(defn- add-can-upload-to-dbs
"Add an entry to each DB about whether the user can upload to it."
[dbs]
- (let [uploads-db-id (public-settings/uploads-database-id)]
- (for [db dbs]
- (assoc db :can_upload (and (= (:id db) uploads-db-id)
- (upload/can-create-upload? db (public-settings/uploads-schema-name)))))))
+ (for [db dbs]
+ (add-can-upload db)))
(defn- dbs-list
[& {:keys [include-tables?
@@ -344,12 +347,6 @@
; filter hidden fields
(= include "tables.fields") (map #(update % :fields filter-sensitive-fields))))))))
-(defn- add-can-upload
- "Add an entry about whether the user can upload to this DB."
- [db]
- (assoc db :can_upload (and (= (u/the-id db) (public-settings/uploads-database-id))
- (upload/can-create-upload? db (public-settings/uploads-schema-name)))))
-
(api/defendpoint GET "/:id"
"Get a single Database with `id`. Optionally pass `?include=tables` or `?include=tables.fields` to include the Tables
belonging to this database, or the Tables and Fields, respectively. If the requestor has write permissions for the DB
diff --git a/src/metabase/db/custom_migrations.clj b/src/metabase/db/custom_migrations.clj
index 39bfd24f8e1..acf0770c404 100644
--- a/src/metabase/db/custom_migrations.clj
+++ b/src/metabase/db/custom_migrations.clj
@@ -1396,3 +1396,32 @@
(define-reversible-migration MigrateMetricsToV2
(metrics-v2/migrate-up!)
(metrics-v2/migrate-down!))
+
+(defn- raw-setting-value [key]
+ (some-> (t2/query-one {:select [:value], :from :setting, :where [:= :key key]})
+ :value
+ encryption/maybe-decrypt))
+
+(define-reversible-migration MigrateUploadsSettings
+ (do (when (some-> (raw-setting-value "uploads-enabled") parse-boolean)
+ (when-let [db-id (some-> (raw-setting-value "uploads-database-id") parse-long)]
+ (let [uploads-table-prefix (raw-setting-value "uploads-table-prefix")
+ uploads-schema-name (raw-setting-value "uploads-schema-name")]
+ (t2/query {:update :metabase_database
+ :set {:uploads_enabled true
+ :uploads_table_prefix uploads-table-prefix
+ :uploads_schema_name uploads-schema-name}
+ :where [:= :id db-id]}))))
+ (t2/query {:delete-from :setting
+ :where [:in :key ["uploads-enabled"
+ "uploads-database-id"
+ "uploads-schema-name"
+ "uploads-table-prefix"]]}))
+ (when-let [db (t2/query-one {:select [:*], :from :metabase_database, :where :uploads_enabled})]
+ (let [settings [{:key "uploads-database-id", :value (encryption/maybe-encrypt (str (:id db)))}
+ {:key "uploads-enabled", :value (encryption/maybe-encrypt "true")}
+ {:key "uploads-table-prefix", :value (encryption/maybe-encrypt (:uploads_table_prefix db))}
+ {:key "uploads-schema-name", :value (encryption/maybe-encrypt (:uploads_schema_name db))}]]
+ (->> settings
+ (filter :value)
+ (t2/insert! :setting)))))
diff --git a/src/metabase/models/database.clj b/src/metabase/models/database.clj
index 6d67114706d..0b22db2a445 100644
--- a/src/metabase/models/database.clj
+++ b/src/metabase/models/database.clj
@@ -256,6 +256,13 @@
(partial handle-db-details-secret-prop! database))]
(assoc database :details updated-details)))
+(defn- handle-uploads-enabled!
+ "This function maintains the invariant that only one database can have uploads_enabled=true."
+ [db]
+ (when (:uploads_enabled db)
+ (t2/update! :model/Database :uploads_enabled true {:uploads_enabled false :uploads_table_prefix nil :uploads_schema_name nil}))
+ db)
+
(t2/define-before-update :model/Database
[database]
(let [changes (t2/changes database)
@@ -288,7 +295,10 @@
infer-db-schedules
(some? (:details changes))
- handle-secrets-changes)
+ handle-secrets-changes
+
+ (:uploads_enabled changes)
+ handle-uploads-enabled!)
;; This maintains a constraint that if a driver doesn't support actions, it can never be enabled
;; If we drop support for actions for a driver, we'd need to add a migration to disable actions for all databases
(when (and (:database-enable-actions (or new-settings existing-settings))
@@ -311,6 +321,7 @@
(not details) (assoc :details {})
(not initial_sync_status) (assoc :initial_sync_status "incomplete"))
handle-secrets-changes
+ handle-uploads-enabled!
infer-db-schedules))
(defmethod serdes/hash-fields :model/Database
diff --git a/src/metabase/public_settings.clj b/src/metabase/public_settings.clj
index 0cab2bb3d05..40aed5b1b50 100644
--- a/src/metabase/public_settings.clj
+++ b/src/metabase/public_settings.clj
@@ -94,7 +94,7 @@
(defsetting site-name
(deferred-tru "The name used for this instance of {0}."
- (application-name-for-setting-descriptions))
+ (application-name-for-setting-descriptions))
:default "Metabase"
:audit :getter
:visibility :settings-manager
@@ -145,7 +145,7 @@
:base setting/uuid-nonce-base)
(defn- normalize-site-url [^String s]
- (let [ ;; remove trailing slashes
+ (let [;; remove trailing slashes
s (str/replace s #"/$" "")
;; add protocol if missing
s (if (str/starts-with? s "http")
@@ -162,8 +162,8 @@
;; It will also prepend `http://` to the URL if there's no protocol when it comes in
(defsetting site-url
(deferred-tru
- (str "This URL is used for things like creating links in emails, auth redirects, and in some embedding scenarios, "
- "so changing it could break functionality or get you locked out of this instance."))
+ (str "This URL is used for things like creating links in emails, auth redirects, and in some embedding scenarios, "
+ "so changing it could break functionality or get you locked out of this instance."))
:visibility :public
:audit :getter
:getter (fn []
@@ -208,7 +208,7 @@
(defsetting anon-tracking-enabled
(deferred-tru "Enable the collection of anonymous usage data in order to help {0} improve."
- (application-name-for-setting-descriptions))
+ (application-name-for-setting-descriptions))
:type :boolean
:default true
:visibility :public
@@ -380,10 +380,10 @@ To change the chart colors:
:feature :whitelabel
:audit :getter
:setter (fn [new-value]
- (when new-value
- (when-not (u.fonts/available-font? new-value)
- (throw (ex-info (tru "Invalid font {0}" (pr-str new-value)) {:status-code 400}))))
- (setting/set-value-of-type! :string :application-font new-value)))
+ (when new-value
+ (when-not (u.fonts/available-font? new-value)
+ (throw (ex-info (tru "Invalid font {0}" (pr-str new-value)) {:status-code 400}))))
+ (setting/set-value-of-type! :string :application-font new-value)))
(defsetting application-font-files
(deferred-tru "Tell us where to find the file for each font weight. You don’t need to include all of them, but it’ll look better if you do.")
@@ -522,10 +522,10 @@ See [fonts](../configuring-metabase/fonts.md).")
(defsetting help-link
(deferred-tru
- (str
- "Keyword setting to control whitelabeling of the help link. Valid values are `:metabase`, `:hidden`, and "
- "`:custom`. If `:custom` is set, the help link will use the URL specified in the `help-link-custom-destination`, "
- "or be hidden if it is not set."))
+ (str
+ "Keyword setting to control whitelabeling of the help link. Valid values are `:metabase`, `:hidden`, and "
+ "`:custom`. If `:custom` is set, the help link will use the URL specified in the `help-link-custom-destination`, "
+ "or be hidden if it is not set."))
:type :keyword
:audit :getter
:visibility :public
@@ -544,11 +544,11 @@ See [fonts](../configuring-metabase/fonts.md).")
[url]
(let [validation-exception (ex-info (tru "Please make sure this is a valid URL")
{:url url})]
- (if-let [matches (re-matches #"^mailto:(.*)" url)]
- (when-not (u/email? (second matches))
- (throw validation-exception))
- (when-not (u/url? url)
- (throw validation-exception)))))
+ (if-let [matches (re-matches #"^mailto:(.*)" url)]
+ (when-not (u/email? (second matches))
+ (throw validation-exception))
+ (when-not (u/url? url)
+ (throw validation-exception)))))
(defsetting help-link-custom-destination
(deferred-tru "Custom URL for the help link.")
@@ -558,8 +558,8 @@ See [fonts](../configuring-metabase/fonts.md).")
:feature :whitelabel
:setter (fn [new-value]
(let [new-value-string (str new-value)]
- (validate-help-url new-value-string)
- (setting/set-value-of-type! :string :help-link-custom-destination new-value-string))))
+ (validate-help-url new-value-string)
+ (setting/set-value-of-type! :string :help-link-custom-destination new-value-string))))
(defsetting show-metabase-links
(deferred-tru (str "Whether or not to display Metabase links outside admin settings."))
@@ -596,8 +596,8 @@ See [fonts](../configuring-metabase/fonts.md).")
(defsetting breakout-bin-width
(deferred-tru
- (str "When using the default binning strategy for a field of type Coordinate (such as Latitude and Longitude), "
- "this number will be used as the default bin width (in degrees)."))
+ (str "When using the default binning strategy for a field of type Coordinate (such as Latitude and Longitude), "
+ "this number will be used as the default bin width (in degrees)."))
:type :double
:default 10.0
:audit :getter)
@@ -620,8 +620,8 @@ See [fonts](../configuring-metabase/fonts.md).")
(defsetting show-homepage-data
(deferred-tru
- (str "Whether or not to display data on the homepage. "
- "Admins might turn this off in order to direct users to better content than raw data"))
+ (str "Whether or not to display data on the homepage. "
+ "Admins might turn this off in order to direct users to better content than raw data"))
:type :boolean
:default true
:visibility :authenticated
@@ -640,8 +640,8 @@ See [fonts](../configuring-metabase/fonts.md).")
(defsetting show-homepage-pin-message
(deferred-tru
- (str "Whether or not to display a message about pinning dashboards. It will also be hidden if any dashboards are "
- "pinned. Admins might hide this to direct users to better content than raw data"))
+ (str "Whether or not to display a message about pinning dashboards. It will also be hidden if any dashboards are "
+ "pinned. Admins might hide this to direct users to better content than raw data"))
:type :boolean
:default true
:visibility :authenticated
@@ -809,46 +809,79 @@ See [fonts](../configuring-metabase/fonts.md).")
;; frontend should set this value to `true` after the modal has been shown once
v))))
-(defsetting uploads-enabled
- (deferred-tru "Whether or not uploads are enabled")
- :visibility :authenticated
- :export? true
- :type :boolean
- :audit :getter
- :default false)
-
(defn- not-handling-api-request?
[]
(nil? @api/*current-user*))
-(defn set-uploads-database-id!
- "Sets the :uploads-database-id setting, with an appropriate permission check."
- [new-id]
- (if (or (not-handling-api-request?)
- (mi/can-write? :model/Database new-id))
- (setting/set-value-of-type! :integer :uploads-database-id new-id)
- (api/throw-403)))
+;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+;;; Deprecated uploads settings begin
+;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
-(defsetting uploads-database-id
- (deferred-tru "Database ID for uploads")
+;; These settings were removed in 50.0 and will be erased from the code in 53.0. They are kept here for backwards compatibility
+;; to avoid breaking existing installations that may have set these values with environment variables, or via the config
+;; file.
+
+(defsetting ^{:deprecated "0.50.0"} uploads-enabled
+ (deferred-tru "Whether or not uploads are enabled")
:visibility :authenticated
- :export? true
+ :export? false
+ :type :boolean
+ :default false
+ :getter (fn [] (log/warn "'uploads-enabled' has been removed; use 'uploads_enabled' on the database instead"))
+ :setter (fn [_] (log/warn "'uploads-enabled' has been removed; use 'uploads_enabled' on the database instead")))
+
+(defsetting ^{:deprecated "0.50.0"} uploads-database-id
+ (deferred-tru "Database ID for uploads")
+ :visibility :internal
+ :export? false
:type :integer
- :audit :getter
- :setter set-uploads-database-id!)
+ :getter (fn [] (log/warn "'uploads-database-id' has been removed; use 'uploads_enabled' on the database instead"))
+ :setter (fn [_] (log/warn "'uploads-database-id' has been removed; use 'uploads_enabled' on the database instead")))
-(defsetting uploads-schema-name
+(defsetting ^{:deprecated "0.50.0"} uploads-schema-name
(deferred-tru "Schema name for uploads")
- :visibility :authenticated
- :export? true
+ :visibility :internal
+ :export? false
:type :string
- :audit :getter)
+ :getter (fn [] (log/warn "'uploads-schema-name' has been removed; use 'uploads_schema_name' on the database instead"))
+ :setter (fn [_] (log/warn "'uploads-schema-name' has been removed; use 'uploads_schema_name' on the database instead")))
-(defsetting uploads-table-prefix
+(defsetting ^{:deprecated "0.50.0"} uploads-table-prefix
(deferred-tru "Prefix for upload table names")
- :visibility :authenticated
+ :visibility :internal
+ :export? false
:type :string
- :audit :getter)
+ :getter (fn [] (log/warn "'uploads-table-prefix' has been removed; use 'uploads_table_prefix' on the database instead"))
+ :setter (fn [_] (log/warn "'uploads-table-prefix' has been removed; use 'uploads_table_prefix' on the database instead")))
+
+;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+;;; Deprecated uploads settings end
+;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+
+(defsetting uploads-settings
+ (deferred-tru "Upload settings")
+ :visibility :authenticated
+ :export? false ; the data is exported with a database export, so we don't need to export a setting
+ :type :json
+ :audit :getter
+ :getter (fn []
+ (let [db (t2/select-one :model/Database :uploads_enabled true)]
+ {:db_id (:id db)
+ :schema_name (:uploads_schema_name db)
+ :table_prefix (:uploads_table_prefix db)}))
+ :setter (fn [{:keys [db_id schema_name table_prefix]}]
+ (cond
+ (nil? db_id)
+ (t2/update! :model/Database :uploads_enabled true {:uploads_enabled false
+ :uploads_schema_name nil
+ :uploads_table_prefix nil})
+ (or (not-handling-api-request?)
+ (mi/can-write? :model/Database db_id))
+ (t2/update! :model/Database db_id {:uploads_enabled true
+ :uploads_schema_name schema_name
+ :uploads_table_prefix table_prefix})
+ :else
+ (api/throw-403))))
(defsetting attachment-table-row-limit
(deferred-tru "Maximum number of rows to render in an alert or subscription image.")
diff --git a/src/metabase/upload.clj b/src/metabase/upload.clj
index 87361547290..3ef25824dd6 100644
--- a/src/metabase/upload.clj
+++ b/src/metabase/upload.clj
@@ -120,9 +120,9 @@
(update-vals col->upload-type (partial defaulting-database-type driver)))
(defn current-database
- "The database being used for uploads (as per the `uploads-database-id` setting)."
+ "The database being used for uploads."
[]
- (t2/select-one Database :id (public-settings/uploads-database-id)))
+ (t2/select-one Database :uploads_enabled true))
(mu/defn table-identifier
"Returns a string that can be used as a table identifier in SQL, including a schema if provided."
@@ -284,13 +284,16 @@
:synchronous (sync/sync-table! table)
:never nil))
+(defn- uploads-enabled? []
+ (some? (:db_id (public-settings/uploads-settings))))
+
(defn- can-use-uploads-error
"Returns an ExceptionInfo object if the user cannot upload to the given database for the subset of reasons common to all uploads
entry points. Returns nil otherwise."
[db]
(let [driver (driver.u/database->driver db)]
(cond
- (not (public-settings/uploads-enabled))
+ (not (uploads-enabled?))
(ex-info (tru "Uploads are not enabled.")
{:status-code 422})
@@ -305,8 +308,10 @@
(defn- can-create-upload-error
"Returns an ExceptionInfo object if the user cannot upload to the given database and schema. Returns nil otherwise."
[db schema-name]
- (or (can-use-uploads-error db)
- (cond
+ (or (cond
+ (not (:uploads_enabled db))
+ (ex-info (tru "Uploads are not enabled.")
+ {:status-code 422})
(and (str/blank? schema-name)
(driver/database-supports? (driver.u/database->driver db) :schemas db))
(ex-info (tru "A schema has not been set.")
@@ -328,7 +333,8 @@
(and (some? schema-name)
(not (driver.s/include-schema? db schema-name)))
(ex-info (tru "The schema {0} is not syncable." schema-name)
- {:status-code 422}))))
+ {:status-code 422}))
+ (can-use-uploads-error db)))
(defn- check-can-create-upload
"Throws an error if the user cannot upload to the given database and schema."
diff --git a/test/metabase/api/card_test.clj b/test/metabase/api/card_test.clj
index 0fcfbf04543..7720f231f50 100644
--- a/test/metabase/api/card_test.clj
+++ b/test/metabase/api/card_test.clj
@@ -13,6 +13,7 @@
[metabase.api.card :as api.card]
[metabase.api.pivots :as api.pivots]
[metabase.config :as config]
+ [metabase.driver :as driver]
[metabase.driver.sql-jdbc.execute :as sql-jdbc.execute]
[metabase.http-client :as client]
[metabase.lib.core :as lib]
@@ -3374,30 +3375,12 @@
(mt/test-driver :h2
(mt/with-empty-db
(testing "Happy path"
- (mt/with-temporary-setting-values [uploads-enabled true
- uploads-database-id (mt/id)
- uploads-table-prefix nil
- uploads-schema-name "PUBLIC"]
- (let [{:keys [status body]} (upload-example-csv-via-api!)]
- (is (= 200
- status))
- (is (= body
- (t2/select-one-pk :model/Card :database_id (mt/id)))))))
- (testing "Failure paths return an appropriate status code and a message in the body"
- (mt/with-temporary-setting-values [uploads-enabled true
- uploads-database-id nil
- uploads-table-prefix nil
- uploads-schema-name "PUBLIC"]
- (is (= {:body {:message "The uploads database is not configured."},
- :status 422}
- (upload-example-csv-via-api!))))
- (mt/with-temporary-setting-values [uploads-enabled true
- uploads-database-id Integer/MAX_VALUE
- uploads-table-prefix nil
- uploads-schema-name "PUBLIC"]
- (is (= {:body {:message "The uploads database does not exist."},
- :status 422}
- (upload-example-csv-via-api!))))))))
+ (t2/update! :model/Database (mt/id) {:uploads_enabled true :uploads_schema_name "PUBLIC" :uploads_table_prefix nil})
+ (let [{:keys [status body]} (upload-example-csv-via-api!)]
+ (is (= 200
+ status))
+ (is (= body
+ (t2/select-one-pk :model/Card :database_id (mt/id)))))))))
(deftest pivot-from-model-test
(testing "Pivot options should match fields through models (#35319)"
@@ -3445,9 +3428,10 @@
This function exists to deduplicate test logic for all API endpoints that must return `based_on_upload`,
including GET /api/collection/:id/items and GET /api/card/:id"
[request]
- (mt/with-driver :h2 ; just test on H2 because failure should be independent of drivers
- (mt/with-temporary-setting-values [uploads-enabled true]
- (mt/with-temp [:model/Database {db-id :id} {:engine "h2"}
+ (mt/test-drivers (mt/normal-drivers-with-feature :uploads)
+ (mt/with-discard-model-updates [:model/Database] ; to restore any existing metabase_database.uploads_enabled=true
+ (mt/with-temp [:model/Database {db-id :id} {:engine driver/*driver*}
+ :model/Database {other-db-id :id} {:engine driver/*driver* :uploads_enabled true}
:model/Table {table-id :id} {:db_id db-id, :is_upload true}
:model/Collection {collection-id :id} {}]
(let [card-defaults {:collection_id collection-id
@@ -3481,22 +3465,22 @@
(t2/update! :model/Table table-id {:is_upload false})
(is (nil? (:based_on_upload (request card))))
(t2/update! :model/Table table-id {:is_upload true}))
- (testing "\nIf uploads are disabled, based_on_upload should be nil"
+ (testing "\nIf the user doesn't have data perms for the database, based_on_upload should be nil"
(mt/with-temp-copy-of-db
(mt/with-no-data-perms-for-all-users!
(is (nil? (:based_on_upload (mt/user-http-request :rasta :get 200 (str "card/" card-id))))))))
- (testing "\nIf uploads are disabled, based_on_upload should be nil"
- (mt/with-temporary-setting-values [uploads-enabled false]
- (is (nil? (:based_on_upload (request card))))))
(testing "\nIf the card is not a model, based_on_upload should be nil"
(mt/with-temp [:model/Card card' (assoc card-defaults :type :question)]
(is (nil? (:based_on_upload (request card'))))))
(testing "\nIf the card is a native query, based_on_upload should be nil"
(mt/with-temp [:model/Card card' (assoc card-defaults
:dataset_query (mt/native-query {:query "select 1"}))]
- (is (nil? (:based_on_upload (request card')))))))))))))
+ (is (nil? (:based_on_upload (request card'))))))
+ (testing "\nIf uploads are disabled on all databases, based_on_upload should be nil"
+ (t2/update! :model/Database other-db-id {:uploads_enabled false})
+ (is (nil? (:based_on_upload (request card))))))))))))
-(deftest based-on-upload-test
+(deftest ^:mb/once based-on-upload-test
(run-based-on-upload-test!
(fn [card]
(mt/user-http-request :crowberto :get 200 (str "card/" (:id card))))))
diff --git a/test/metabase/api/collection_test.clj b/test/metabase/api/collection_test.clj
index c61a0d45c22..c2e3cd40a79 100644
--- a/test/metabase/api/collection_test.clj
+++ b/test/metabase/api/collection_test.clj
@@ -655,7 +655,7 @@
(:data (mt/user-http-request :crowberto :get 200
(str "collection/" (u/the-id collection) "/items"))))))))))
-(deftest collection-items-based-on-upload-test
+(deftest ^:mb/once collection-items-based-on-upload-test
(testing "GET /api/collection/:id/items"
(testing "check that based_on_upload is returned for cards correctly"
(api.card-test/run-based-on-upload-test!
diff --git a/test/metabase/api/database_test.clj b/test/metabase/api/database_test.clj
index 99bad309c50..aa799afad49 100644
--- a/test/metabase/api/database_test.clj
+++ b/test/metabase/api/database_test.clj
@@ -195,12 +195,13 @@
(deftest get-database-can-upload-test
(testing "GET /api/database"
- (t2.with-temp/with-temp [Database {db-id :id} {:engine :postgres :name "The Chosen One"}]
+ (mt/with-discard-model-updates [:model/Database] ; to restore any existing metabase_database.uploads_enabled=true
(doseq [uploads-enabled? [true false]]
- (testing (format "The database with uploads enabled for the public schema has can_upload=%s" uploads-enabled?)
- (mt/with-temporary-setting-values [uploads-enabled uploads-enabled?
- uploads-schema-name "public"
- uploads-database-id db-id]
+ (mt/with-temp [Database {db-id :id} {:engine :postgres
+ :name "The Chosen One"
+ :uploads_enabled uploads-enabled?
+ :uploads_schema_name "public"}]
+ (testing (format "The database with uploads enabled for the public schema has can_upload=%s" uploads-enabled?)
(let [result (mt/user-http-request :crowberto :get 200 (format "database/%d" db-id))]
(is (= uploads-enabled? (:can_upload result))))))))))
@@ -882,15 +883,15 @@
(deftest databases-list-can-upload-test
(testing "GET /api/database"
(let [old-ids (t2/select-pks-set Database)]
- (t2.with-temp/with-temp [Database {db-id :id} {:engine :postgres :name "The Chosen One"}]
- (doseq [uploads-enabled? [true false]]
- (testing (format "The database with uploads enabled for the public schema has can_upload=%s" uploads-enabled?)
- (mt/with-temporary-setting-values [uploads-enabled uploads-enabled?
- uploads-schema-name "public"
- uploads-database-id db-id]
- (let [result (get-all :crowberto "database" old-ids)]
- (is (= (:total result) 1))
- (is (= uploads-enabled? (-> result :data first :can_upload)))))))))))
+ (doseq [uploads-enabled? [true false]]
+ (testing (format "The database with uploads enabled for the public schema has can_upload=%s" uploads-enabled?)
+ (mt/with-temp [Database _ {:engine :postgres
+ :name "The Chosen One"
+ :uploads_enabled uploads-enabled?
+ :uploads_schema_name "public"}]
+ (let [result (get-all :crowberto "database" old-ids)]
+ (is (= (:total result) 1))
+ (is (= uploads-enabled? (-> result :data first :can_upload))))))))))
(deftest databases-list-include-saved-questions-test
(testing "GET /api/database?saved=true"
diff --git a/test/metabase/api/table_test.clj b/test/metabase/api/table_test.clj
index cc10d1fabcd..501936aef2c 100644
--- a/test/metabase/api/table_test.clj
+++ b/test/metabase/api/table_test.clj
@@ -34,7 +34,8 @@
(defn- db-details []
(merge
(select-keys (mt/db) [:id :created_at :updated_at :timezone :creator_id :initial_sync_status :dbms_version
- :cache_field_values_schedule :metadata_sync_schedule])
+ :cache_field_values_schedule :metadata_sync_schedule :uploads_enabled :uploads_schema_name
+ :uploads_table_prefix])
{:engine "h2"
:name "test-data"
:is_sample false
@@ -991,11 +992,11 @@
(mt/test-driver :h2
(mt/with-empty-db
(testing "Happy path"
- (mt/with-temporary-setting-values [uploads-enabled true]
+ (upload-test/with-uploads-enabled
(is (= {:status 200, :body nil}
(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]
+ (upload-test/with-uploads-disabled
(is (= {:status 422, :body {:message "Uploads are not enabled."}}
(update-csv-via-api! :metabase.upload/append))))))))
@@ -1022,11 +1023,11 @@
(mt/test-driver :h2
(mt/with-empty-db
(testing "Happy path"
- (mt/with-temporary-setting-values [uploads-enabled true]
+ (upload-test/with-uploads-enabled
(is (= {:status 200, :body nil}
(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]
+ (upload-test/with-uploads-disabled
(is (= {:status 422, :body {:message "Uploads are not enabled."}}
(update-csv-via-api! :metabase.upload/replace))))))))
diff --git a/test/metabase/db/custom_migrations_test.clj b/test/metabase/db/custom_migrations_test.clj
index 3c546103b1c..fea93318ae8 100644
--- a/test/metabase/db/custom_migrations_test.clj
+++ b/test/metabase/db/custom_migrations_test.clj
@@ -1867,3 +1867,78 @@
(dissoc :name))
(-> card
(dissoc :name))))))))))))
+
+(def ^:private migrate-uploads-default-db
+ {:name "DB"
+ :engine "h2"
+ :created_at :%now
+ :updated_at :%now
+ :details "{}"})
+
+(deftest migrate-uploads-settings-test-1
+ (testing "MigrateUploadsSettings with valid settings state works as expected."
+ (encryption-test/with-secret-key "dont-tell-anyone-about-this"
+ (impl/test-migrations ["v50.2024-05-17T19:54:26"] [migrate!]
+ (let [uploads-db-id (t2/insert-returning-pk! :metabase_database (assoc migrate-uploads-default-db :name "DB 1"))
+ not-uploads-db-id (t2/insert-returning-pk! :metabase_database (assoc migrate-uploads-default-db :name "DB 2"))]
+ (let [settings [{:key "uploads-database-id", :value (encryption/maybe-encrypt (str uploads-db-id))}
+ {:key "uploads-enabled", :value (encryption/maybe-encrypt "true")}
+ {:key "uploads-table-prefix", :value (encryption/maybe-encrypt "uploads_")}
+ {:key "uploads-schema-name", :value (encryption/maybe-encrypt "uploads")}]
+ _ (t2/insert! :setting settings)
+ get-settings #(t2/query {:select [:key :value], :from :setting, :where [:in :key (map :key settings)]})
+ settings-before (get-settings)]
+ (testing "make sure the settings are encrypted before the migrations"
+ (is (not-empty settings-before))
+ (is (every? encryption/possibly-encrypted-string?
+ (map :value settings-before))))
+ (migrate!)
+ (testing "make sure the settings are removed after the migrations"
+ (is (empty? (get-settings))))
+ (is (=? {uploads-db-id {:uploads_enabled true, :uploads_schema_name "uploads", :uploads_table_prefix "uploads_"}
+ not-uploads-db-id {:uploads_enabled false, :uploads_schema_name nil, :uploads_table_prefix nil}}
+ (m/index-by :id (t2/select :metabase_database))))
+ ;; TODO: delete the rest of the test after merging because down-migrations flake with MySQL:
+ (migrate! :down 49)
+ (testing "make sure the settings contain the same decrypted values after the migrations"
+ (let [settings-after (get-settings)]
+ (is (not-empty settings-after))
+ (is (every? encryption/possibly-encrypted-string?
+ (map :value settings-after)))
+ (is (= (set (map #(update % :value encryption/maybe-decrypt) settings-before))
+ (set (map #(update % :value encryption/maybe-decrypt) settings-after))))))))))))
+
+(deftest migrate-uploads-settings-test-2
+ (testing "MigrateUploadsSettings with invalid settings state (missing uploads-database-id) doesn't fail."
+ (encryption-test/with-secret-key "dont-tell-anyone-about-this"
+ (impl/test-migrations ["v50.2024-05-17T19:54:26"] [migrate!]
+ (let [uploads-db-id (t2/insert-returning-pk! :metabase_database migrate-uploads-default-db)
+ settings [;; no uploads-database-id and uploads-schema-name
+ {:key "uploads-enabled", :value (encryption/maybe-encrypt "true")}
+ {:key "uploads-table-prefix", :value (encryption/maybe-encrypt "uploads_")}]
+ _ (t2/insert! :setting settings)
+ get-settings #(t2/query {:select [:key :value], :from :setting, :where [:in :key (map :key settings)]})]
+ (migrate!)
+ (testing "make sure the settings are removed after the migrations"
+ (is (empty? (get-settings))))
+ (is (=? {uploads-db-id {:uploads_enabled false
+ :uploads_schema_name nil
+ :uploads_table_prefix nil}}
+ (m/index-by :id (t2/select :metabase_database)))))))))
+
+(deftest migrate-uploads-settings-test-3
+ (testing "MigrateUploadsSettings with invalid settings state (missing uploads-enabled) doesn't set uploads_enabled on the database."
+ (encryption-test/with-secret-key "dont-tell-anyone-about-this"
+ (impl/test-migrations ["v50.2024-05-17T19:54:26"] [migrate!]
+ (let [uploads-db-id (t2/insert-returning-pk! :metabase_database migrate-uploads-default-db)
+ settings [;; no uploads-enabled
+ {:key "uploads-database-id", :value (encryption/maybe-encrypt "uploads_")}]
+ _ (t2/insert! :setting settings)
+ get-settings #(t2/query {:select [:key :value], :from :setting, :where [:in :key (map :key settings)]})]
+ (migrate!)
+ (testing "make sure the settings are removed after the migrations"
+ (is (empty? (get-settings))))
+ (is (=? {uploads-db-id {:uploads_enabled false
+ :uploads_schema_name nil
+ :uploads_table_prefix nil}}
+ (m/index-by :id (t2/select :metabase_database)))))))))
diff --git a/test/metabase/upload_test.clj b/test/metabase/upload_test.clj
index e15f02cb82d..da807fca2e9 100644
--- a/test/metabase/upload_test.clj
+++ b/test/metabase/upload_test.clj
@@ -286,7 +286,9 @@
auxiliary-sync-steps :never
csv-file-prefix "example csv file"}
:as args}]
- (mt/with-temporary-setting-values [uploads-enabled uploads-enabled]
+ (mt/with-discard-model-updates [:model/Database]
+ (t2/update! :model/Database :uploads_enabled true {:uploads_enabled false})
+ (t2/update! :model/Database db-id {:uploads_enabled uploads-enabled})
(mt/with-current-user user-id
(let [file (or file (csv-file-with
["id, name"
@@ -313,15 +315,28 @@
:schema-name schema-name
:table-prefix table-prefix})))))))
-(defn do-with-uploads-allowed
- "Set uploads-enabled to true, and uses an admin user, run the thunk"
+(defn do-with-uploads-enabled
+ "Set uploads_enabled to true the current database, and as an admin user, run the thunk"
[thunk]
- (mt/with-temporary-setting-values [uploads-enabled true]
+ (mt/with-discard-model-updates [:model/Database]
+ (t2/update! :model/Database (mt/id) {:uploads_enabled true
+ :uploads_schema_name (sql.tx/session-schema driver/*driver*)})
(mt/with-current-user (mt/user->id :crowberto)
(thunk))))
-(defmacro with-uploads-allowed [& body]
- `(do-with-uploads-allowed (fn [] ~@body)))
+(defmacro with-uploads-enabled [& body]
+ `(do-with-uploads-enabled (fn [] ~@body)))
+
+(defn do-with-uploads-disabled
+ "Set uploads_enabled to false the current database, and as an admin user, run the thunk"
+ [thunk]
+ (mt/with-discard-model-updates [:model/Database]
+ (t2/update! :model/Database :uploads_enabled true {:uploads_enabled false})
+ (mt/with-current-user (mt/user->id :crowberto)
+ (thunk))))
+
+(defmacro with-uploads-disabled [& body]
+ `(do-with-uploads-disabled (fn [] ~@body)))
(defn do-with-upload-table! [table thunk]
(try (thunk table)
@@ -346,7 +361,7 @@
...)"
{:style/indent 1}
[[table-binding create-table-expr] & body]
- `(with-uploads-allowed
+ `(with-uploads-enabled
(mt/with-model-cleanup [:model/Table]
(do-with-upload-table! ~create-table-expr (fn [~table-binding] ~@body)))))
@@ -970,11 +985,10 @@
(mt/test-drivers (mt/normal-drivers-with-feature :uploads)
(mt/with-empty-db
(testing "Uploads must be enabled"
- (doseq [uploads-enabled-value [false nil]]
- (is (thrown-with-msg?
- java.lang.Exception
- #"^Uploads are not enabled\.$"
- (upload-example-csv! :uploads-enabled uploads-enabled-value :schema-name "public", :table-prefix "uploaded_magic_")))))
+ (is (thrown-with-msg?
+ java.lang.Exception
+ #"^Uploads are not enabled\.$"
+ (upload-example-csv! :uploads-enabled false :schema-name "public", :table-prefix "uploaded_magic_"))))
(testing "Database ID must be valid"
(is (thrown-with-msg?
java.lang.Exception
@@ -1099,62 +1113,59 @@
(defn update-csv-with-defaults!
"Upload a small CSV file to a newly created default table, or an existing table if `table-id` is provided. Default args can be overridden."
- [action & {:keys [uploads-enabled user-id file table-id is-upload]
- :or {uploads-enabled true
- user-id (mt/user->id :crowberto)
+ [action & {:keys [user-id file table-id is-upload]
+ :or {user-id (mt/user->id :crowberto)
file (csv-file-with
["name"
"Luke Skywalker"
"Darth Vader"])
is-upload true}}]
- (mt/with-temporary-setting-values [uploads-enabled uploads-enabled]
- (mt/with-current-user user-id
- (mt/with-model-cleanup [:model/Table]
- (let [new-table (when (nil? table-id)
- (create-upload-table!))
- table-id (or table-id (:id new-table))]
- (t2/update! :model/Table table-id {:is_upload is-upload})
- (try (update-csv! action {:table-id table-id, :file file})
- (finally
+ (mt/with-current-user user-id
+ (mt/with-model-cleanup [:model/Table]
+ (let [new-table (when (nil? table-id)
+ (create-upload-table!))
+ table-id (or table-id (:id new-table))]
+ (t2/update! :model/Table table-id {:is_upload is-upload})
+ (try (update-csv! action {:table-id table-id, :file file})
+ (finally
;; Drop the table in the testdb if a new one was created.
- (when (and new-table (not= driver/*driver* :redshift)) ; redshift tests flake when tables are dropped
- (driver/drop-table! driver/*driver*
- (mt/id)
- (#'upload/table-identifier new-table))))))))))
+ (when (and new-table (not= driver/*driver* :redshift)) ; redshift tests flake when tables are dropped
+ (driver/drop-table! driver/*driver*
+ (mt/id)
+ (#'upload/table-identifier new-table)))))))))
(deftest can-update-test
(mt/test-drivers (mt/normal-drivers-with-feature :uploads)
(doseq [action (actions-to-test driver/*driver*)]
(testing (action-testing-str action)
- (testing "Happy path"
- (is (= {:row-count 2}
- (update-csv-with-defaults! action))))
- (testing "Even if the uploads database, schema and table prefix are not set, appends succeed"
- (mt/with-temporary-setting-values [uploads-database-id nil
- uploads-schema-name nil
- uploads-table-prefix nil]
- (is (some? (update-csv-with-defaults! action)))))
- (testing "Uploads must be enabled"
- (is (= {:message "Uploads are not enabled."
- :data {:status-code 422}}
- (catch-ex-info (update-csv-with-defaults! action :uploads-enabled false)))))
- (testing "The table must exist"
- (is (= {:message "Not found."
- :data {:status-code 404}}
- (catch-ex-info (update-csv-with-defaults! action :table-id Integer/MAX_VALUE)))))
- (testing "The table must be an uploaded table"
- (is (= {:message "The table must be an uploaded table."
- :data {:status-code 422}}
- (catch-ex-info (update-csv-with-defaults! action :is-upload false)))))
- (testing "The CSV file must not be empty"
- (is (= {:message "The CSV file is missing columns that are in the table:\n- name",
- :data {:status-code 422}}
- (catch-ex-info (update-csv-with-defaults! action :file (csv-file-with []))))))
- (testing "Uploads must be supported"
- (mt/with-dynamic-redefs [driver/database-supports? (constantly false)]
- (is (= {:message (format "Uploads are not supported on %s databases." (str/capitalize (name driver/*driver*)))
+ (mt/with-discard-model-updates [:model/Database]
+ ;; start with uploads disabled for all databases
+ (t2/update! :model/Database :uploads_enabled true {:uploads_enabled false})
+ (testing "Updates fail if uploads are disabled for all databases."
+ (is (= {:message "Uploads are not enabled."
:data {:status-code 422}}
- (catch-ex-info (update-csv-with-defaults! action))))))))))
+ (catch-ex-info (update-csv-with-defaults! action)))))
+ (mt/with-temp [:model/Database _ {:uploads_enabled true}]
+ (testing "Updates succeed if uploads are enabled for one database, even if it is not the current one."
+ (is (= {:row-count 2}
+ (update-csv-with-defaults! action))))
+ (testing "The table must exist"
+ (is (= {:message "Not found."
+ :data {:status-code 404}}
+ (catch-ex-info (update-csv-with-defaults! action :table-id Integer/MAX_VALUE)))))
+ (testing "The table must be an uploaded table"
+ (is (= {:message "The table must be an uploaded table."
+ :data {:status-code 422}}
+ (catch-ex-info (update-csv-with-defaults! action :is-upload false)))))
+ (testing "The CSV file must not be empty"
+ (is (= {:message "The CSV file is missing columns that are in the table:\n- name",
+ :data {:status-code 422}}
+ (catch-ex-info (update-csv-with-defaults! action :file (csv-file-with []))))))
+ (testing "Uploads must be supported"
+ (mt/with-dynamic-redefs [driver/database-supports? (constantly false)]
+ (is (= {:message (format "Uploads are not supported on %s databases." (str/capitalize (name driver/*driver*)))
+ :data {:status-code 422}}
+ (catch-ex-info (update-csv-with-defaults! action))))))))))))
(deftest update-column-match-test
(mt/test-drivers (mt/normal-drivers-with-feature :uploads)
@@ -1189,7 +1200,7 @@
(mt/test-drivers (mt/normal-drivers-with-feature :uploads)
(doseq [action (actions-to-test driver/*driver*)]
(testing (action-testing-str action)
- (with-uploads-allowed
+ (with-uploads-enabled
(testing "Append should fail only if there are missing columns in the CSV file"
(doseq [[csv-rows error-message]
{[""]
@@ -1309,7 +1320,7 @@
(mt/test-drivers (mt/normal-drivers-with-feature :uploads)
(doseq [action (actions-to-test driver/*driver*)]
(testing (action-testing-str action)
- (with-uploads-allowed
+ (with-uploads-enabled
(testing "Append should succeed with a CSV with only the header"
(let [csv-rows ["name"]]
(with-upload-table!
@@ -1651,7 +1662,7 @@
(mt/test-drivers (mt/normal-drivers-with-feature :uploads)
(doseq [action (actions-to-test driver/*driver*)]
(testing (action-testing-str action)
- (with-uploads-allowed
+ (with-uploads-enabled
(testing "Append should handle new columns being added in the latest CSV"
(with-upload-table! [table (create-upload-table!)]
;; Reorder as well for good measure
--
GitLab