diff --git a/modules/drivers/redshift/src/metabase/driver/redshift.clj b/modules/drivers/redshift/src/metabase/driver/redshift.clj index 983298687320211afb3140b4f2fe608ce04aab31..c0b63ed63c5430ca0bc0e826526e62de6246c986 100644 --- a/modules/drivers/redshift/src/metabase/driver/redshift.clj +++ b/modules/drivers/redshift/src/metabase/driver/redshift.clj @@ -440,7 +440,7 @@ ::upload/int [:bigint] ;; identity(1, 1) defines an auto-increment column starting from 1 ::upload/auto-incrementing-int-pk [:bigint [:identity 1 1]] - ::upload/float [:float] + ::upload/float [(keyword "double precision")] ::upload/boolean [:boolean] ::upload/date [:date] ::upload/datetime [:timestamp] @@ -504,3 +504,17 @@ (defmethod driver.sql/default-database-role :redshift [_ _] "DEFAULT") + +(defmethod driver/add-columns! :redshift + [driver db-id table-name column-definitions & {:as settings}] + ;; Redshift doesn't support adding multiple columns at a time, so we break it up + (let [f (get-method driver/add-columns! :postgres)] + (doseq [[k v] column-definitions] + (f driver db-id table-name {k v} settings)))) + +(defmethod driver/alter-columns! :redshift + [_driver _db-id _table-name column-definitions] + ;; TODO: redshift doesn't allow promotion of ints to floats using ALTER TABLE. + (let [[column-name type-and-constraints] (first column-definitions) + type (first type-and-constraints)] + (throw (ex-info (format "There's a value with the wrong type ('%s') in the '%s' column" (name type) (name column-name)) {})))) diff --git a/modules/drivers/redshift/test/metabase/test/data/redshift.clj b/modules/drivers/redshift/test/metabase/test/data/redshift.clj index d8c836a2baca882e70ac2e5324d0131e5a3b422a..19fd6901ac744cf07ed936abf765adbb69d8b39a 100644 --- a/modules/drivers/redshift/test/metabase/test/data/redshift.clj +++ b/modules/drivers/redshift/test/metabase/test/data/redshift.clj @@ -13,6 +13,7 @@ [clojure.string :as str] [java-time.api :as t] [metabase.driver :as driver] + [metabase.driver.ddl.interface :as ddl.i] [metabase.driver.sql-jdbc.connection :as sql-jdbc.conn] [metabase.driver.sql-jdbc.execute :as sql-jdbc.execute] [metabase.driver.sql.test-util.unique-prefix :as sql.tu.unique-prefix] @@ -226,3 +227,9 @@ (= (:name %) "extsales"))) tables)))) (original-describe-database driver database))) + +(defmethod ddl.i/format-name :redshift + [_driver s] + ;; Redshift is case-insensitive for identifiers and returns them in lower-case by default from system tables, even if + ;; you create the tables with upper-case characters. + (u/lower-case-en s)) diff --git a/src/metabase/driver/postgres.clj b/src/metabase/driver/postgres.clj index bb295d2e6c0628933ffba6060910b08a03245b36..7e64ec68570118fb1fd95d355e9981e7335f96b1 100644 --- a/src/metabase/driver/postgres.clj +++ b/src/metabase/driver/postgres.clj @@ -67,6 +67,7 @@ :now true :persist-models true :schemas true + :uploads true :sql/window-functions.order-by-output-column-numbers false}] (defmethod driver/database-supports? [:postgres feature] [_driver _feature _db] supported?)) @@ -78,7 +79,6 @@ (doseq [feature [:actions :actions/custom :table-privileges - :uploads :index-info]] (defmethod driver/database-supports? [:postgres feature] [driver _feat _db] @@ -274,7 +274,7 @@ (defmethod driver/describe-database :postgres [_driver database] ;; TODO: we should figure out how to sync tables using transducer, this way we don't have to hold 100k tables in - ;; memrory in a set like this + ;; memory in a set like this {:tables (into #{} (describe-syncable-tables database))}) ;; Describe the Fields present in a `table`. This just hands off to the normal SQL driver implementation of the same @@ -706,7 +706,7 @@ (keyword "time without time zone") :type/Time ;; TODO postgres also supports `timestamp(p) with time zone` where p is the precision ;; maybe we should switch this to use `sql-jdbc.sync/pattern-based-database-type->base-type` - (keyword "timestamp with time zone") :type/DateTimeWithTZ + (keyword "timestamp with time zone") :type/DateTimeWithLocalTZ (keyword "timestamp without time zone") :type/DateTime}) (defmethod sql-jdbc.sync/database-type->base-type :postgres diff --git a/src/metabase/upload.clj b/src/metabase/upload.clj index b0925505307cc8f152ce415dd12855abe6b5b250..342fdbc3ee798fdb16a10e242f7827f6454189b3 100644 --- a/src/metabase/upload.clj +++ b/src/metabase/upload.clj @@ -10,6 +10,7 @@ [metabase.analytics.snowplow :as snowplow] [metabase.api.common :as api] [metabase.driver :as driver] + [metabase.driver.ddl.interface :as ddl.i] [metabase.driver.sync :as driver.s] [metabase.driver.util :as driver.u] [metabase.events :as events] @@ -124,7 +125,7 @@ [] (t2/select-one Database :id (public-settings/uploads-database-id))) -(mu/defn ^:private table-identifier +(mu/defn table-identifier "Returns a string that can be used as a table identifier in SQL, including a schema if provided." [{:keys [schema name] :as _table} :- [:map @@ -257,18 +258,18 @@ ;;;; | Create upload ;;;; +------------------+ -(def ^:dynamic *sync-synchronously?* - "For testing purposes, often we'd like to sync synchronously so that we can test the results immediately and avoid - race conditions." - false) +(def ^:dynamic *auxiliary-sync-steps* + "For testing purposes, we'd like to control whether the analyze and field values steps of sync are run synchronously, or not at all. + In production this should always be asynchronous, so users can use the table earlier." + :asynchronous) (defn- scan-and-sync-table! [database table] (sync-fields/sync-fields-for-table! database table) - (if *sync-synchronously?* - (sync/sync-table! table) - (future - (sync/sync-table! table)))) + (case *auxiliary-sync-steps* + :asynchronous (future (sync/sync-table! table)) + :synchronous (sync/sync-table! table) + :never nil)) (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 @@ -339,6 +340,25 @@ :num-rows (count (rest rows)) :generated-columns 0})))) +(defn- create-from-csv-and-sync! + "This is separated from `create-csv-upload!` for testing" + [{:keys [db file schema table-name]}] + (let [driver (driver.u/database->driver db) + schema (some->> schema (ddl.i/format-name driver)) + table-name (some->> table-name (ddl.i/format-name driver)) + schema+table-name (table-identifier {:schema schema :name table-name}) + stats (create-from-csv! driver (:id db) schema+table-name file) + ;; Sync immediately to create the Table and its Fields; the scan is settings-dependent and can be async + table (sync-tables/create-or-reactivate-table! db {:name table-name :schema (not-empty schema)}) + _set_is_upload (t2/update! :model/Table (:id table) {:is_upload true}) + _sync (scan-and-sync-table! db table) + ;; Set the display_name of the auto-generated primary key column to the same as its name, so that if users + ;; download results from the table as a CSV and reupload, we'll recognize it as the same column + auto-pk-field (table-id->auto-pk-column (:id table)) + _ (t2/update! :model/Field (:id auto-pk-field) {:display_name (:name auto-pk-field)})] + {:table table + :stats stats})) + (mu/defn create-csv-upload! "Main entry point for CSV uploading. @@ -376,22 +396,14 @@ (collection/check-write-perms-for-collection collection-id) (try (let [timer (start-timer) - driver (driver.u/database->driver database) filename-prefix (or (second (re-matches #"(.*)\.(csv|tsv)$" filename)) filename) + driver (driver.u/database->driver database) table-name (->> (str table-prefix filename-prefix) (unique-table-name driver) (u/lower-case-en)) - schema+table-name (table-identifier {:schema schema-name :name table-name}) - stats (create-from-csv! driver (:id database) schema+table-name file) - ;; Sync immediately to create the Table and its Fields; the scan is settings-dependent and can be async - table (sync-tables/create-or-reactivate-table! database {:name table-name :schema (not-empty schema-name)}) - _set_is_upload (t2/update! :model/Table (:id table) {:is_upload true}) - _sync (scan-and-sync-table! database table) - ;; Set the display_name of the auto-generated primary key column to the same as its name, so that if users - ;; download results from the table as a CSV and reupload, we'll recognize it as the same column - auto-pk-field (table-id->auto-pk-column (:id table)) - _ (t2/update! :model/Field (:id auto-pk-field) {:display_name (:name auto-pk-field)}) + {:keys [stats + table]} (create-from-csv-and-sync! {:db database :file file :schema schema-name :table-name table-name}) card (card/create-card! {:collection_id collection-id :type :model diff --git a/src/metabase/upload/types.cljc b/src/metabase/upload/types.cljc index 94a1f721309f0fb6dd7791d499dc18c3fae8ac17..12bb2ea1abd5224aea77e7b9f2786b30ca6516fb 100644 --- a/src/metabase/upload/types.cljc +++ b/src/metabase/upload/types.cljc @@ -80,6 +80,8 @@ {::*boolean-int* ::boolean ::*float-or-int* ::float}) +;; TODO: the set of allowed promotions should be driver-specific, because not all drivers support coercions between all +;; types e.g. redshift does not allow coercions except between text types (def ^:private allowed-promotions "A mapping of which types a column can be implicitly relaxed to, based on the content of appended values. If we require a relaxation which is not allow-listed here, we will reject the corresponding file." diff --git a/test/metabase/upload_test.clj b/test/metabase/upload_test.clj index c25237c9e62718b1f966af5155b6b2cd562adde1..90a7daf1a52ac961de4157eee6af8922ff2924fb 100644 --- a/test/metabase/upload_test.clj +++ b/test/metabase/upload_test.clj @@ -97,18 +97,15 @@ [& body] `(do-with-mysql-local-infile-off (fn [] ~@body))) -(defn- format-schema [schema-name] - (when (driver/database-supports? driver/*driver* :schemas nil) - (ddl.i/format-name driver/*driver* schema-name))) - (defn sync-upload-test-table! "Creates a table in the app db and syncs it synchronously, setting is_upload=true. Returns the table instance. The result is identical to if the table was synced with [[metabase.sync/sync-database!]], but faster because it skips syncing every table in the test database." [& {:keys [database table-name schema-name]}] - (let [table (sync-tables/create-or-reactivate-table! database {:name table-name :schema (not-empty schema-name)})] + (let [schema-name (or schema-name (sql.tx/session-schema driver/*driver*)) + table (sync-tables/create-or-reactivate-table! database {:name table-name :schema (not-empty schema-name)})] (t2/update! :model/Table (:id table) {:is_upload true}) - (binding [upload/*sync-synchronously?* true] + (binding [upload/*auxiliary-sync-steps* :synchronous] (#'upload/scan-and-sync-table! database table)) (t2/select-one :model/Table (:id table)))) @@ -276,45 +273,63 @@ :topic topic {:order-by [[:id :desc]]})) +(defn create-from-csv-and-sync-with-defaults! + "Creates a table from a CSV file and syncs using [[upload/create-from-csv-and-sync!]]. Returns the synced Table." + [& {:keys [table-name file auxiliary-sync-steps] + :or {table-name (mt/random-name) + file (csv-file-with + ["id, name" + "1, Luke Skywalker" + "2, Darth Vader"] + "example csv file") + ;; usually we don't care about analyze or field values for tests, so skip by default for speed + auxiliary-sync-steps :never}}] + (let [schema (sql.tx/session-schema driver/*driver*) + db (t2/select-one :model/Database (mt/id))] + (binding [upload/*auxiliary-sync-steps* auxiliary-sync-steps] + (:table (#'upload/create-from-csv-and-sync! {:db db + :file file + :schema schema + :table-name table-name}))))) + (defn upload-example-csv! "Upload a small CSV file to the given collection ID. `grant-permission?` controls whether the current user is granted data permissions to the database." - [& {:keys [table-prefix collection-id grant-permission? uploads-enabled user-id db-id sync-synchronously? csv-file-prefix] - :or {collection-id nil ;; root collection - grant-permission? true - uploads-enabled true - user-id (mt/user->id :rasta) - db-id (mt/id) - sync-synchronously? true - ;; Make the file-name unique so the table names don't collide on cloud databases like redshift, where we use - ;; the same schema between test runs - csv-file-prefix (str "example csv file " (random-uuid))} + [& {:keys [table-prefix collection-id grant-permission? uploads-enabled user-id db-id auxiliary-sync-steps csv-file-prefix file] + :or {collection-id nil ;; root collection + grant-permission? true + uploads-enabled true + user-id (mt/user->id :rasta) + db-id (mt/id) + ;; usually we don't care about analyze or field values for tests, so skip by default for speed + auxiliary-sync-steps :never + csv-file-prefix "example csv file"} :as args}] (mt/with-temporary-setting-values [uploads-enabled uploads-enabled] (mt/with-current-user user-id - (let [db (t2/select-one :model/Database db-id) - schema-name (if (contains? args :schema-name) - (format-schema (:schema-name args)) - (sql.tx/session-schema driver/*driver*)) - file (csv-file-with - ["id, name" - "1, Luke Skywalker" - "2, Darth Vader"] - csv-file-prefix) - group-id (u/the-id (perms-group/all-users)) - grant? (and db - (not (mi/can-read? db)) - grant-permission?)] + (let [file (or file (csv-file-with + ["id, name" + "1, Luke Skywalker" + "2, Darth Vader"] + csv-file-prefix)) + db (t2/select-one :model/Database db-id) + schema-name (if (contains? args :schema-name) + (ddl.i/format-name driver/*driver* (:schema-name args)) + (sql.tx/session-schema driver/*driver*)) + group-id (u/the-id (perms-group/all-users)) + grant? (and db + (not (mi/can-read? db)) + grant-permission?)] (mt/with-restored-data-perms-for-group! group-id (when grant? (data-perms/set-database-permission! group-id db-id :perms/data-access :unrestricted)) - (u/prog1 (binding [upload/*sync-synchronously?* sync-synchronously?] - (upload/create-csv-upload! {:collection-id collection-id - :filename csv-file-prefix - :file file - :db-id db-id - :schema-name schema-name - :table-prefix table-prefix})))))))) + (binding [upload/*auxiliary-sync-steps* auxiliary-sync-steps] + (upload/create-csv-upload! {:collection-id collection-id + :filename csv-file-prefix + :file file + :db-id db-id + :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" @@ -329,9 +344,10 @@ (defn do-with-upload-table! [table thunk] (try (thunk table) (finally - (driver/drop-table! driver/*driver* - (:db_id table) - (#'upload/table-identifier table))))) + (when (not= driver/*driver* :redshift) ; redshift tests flake when tables are dropped + (driver/drop-table! driver/*driver* + (:db_id table) + (#'upload/table-identifier table)))))) (defn- table->card [table] (t2/select-one :model/Card :table_id (:id table))) @@ -402,20 +418,12 @@ (mt/test-drivers (mt/normal-drivers-with-feature :uploads) (with-mysql-local-infile-on-and-off (with-upload-table! - [table (let [table-name (mt/random-name)] - (@#'upload/create-from-csv! - driver/*driver* - (mt/id) - table-name - (csv-file-with lines)) - (sync-upload-test-table! :database (mt/db) :table-name table-name))] + [table (create-from-csv-and-sync-with-defaults! + :file (csv-file-with lines) + :auxiliary-sync-steps :synchronous)] (testing "Table and Fields exist after sync" (is (=? {:name #"(?i)_mb_row_id" - :semantic_type (if (= driver/*driver* :redshift) - ;; TODO: there is a bug in the redshift driver where the semantic_type is not set - ;; to type/PK even if the column is a PK - :type/Category - :type/PK) + :semantic_type :type/PK :base_type :type/BigInteger} (t2/select-one Field :database_position 0 :table_id (:id table)))) (is (=? {:name #"(?i)id" @@ -449,17 +457,12 @@ (mt/test-drivers (mt/normal-drivers-with-feature :uploads) (with-mysql-local-infile-on-and-off (with-upload-table! - [table (let [table-name (mt/random-name)] - (@#'upload/create-from-csv! - driver/*driver* - (mt/id) - table-name - (csv-file-with ["datetime" - "2022-01-01" - "2022-01-01 00:00" - "2022-01-01T00:00:00" - "2022-01-01T00:00"])) - (sync-upload-test-table! :database (mt/db) :table-name table-name))] + [table (create-from-csv-and-sync-with-defaults! + :file (csv-file-with ["datetime" + "2022-01-01" + "2022-01-01 00:00" + "2022-01-01T00:00:00" + "2022-01-01T00:00"]))] (testing "Fields exists after sync" (testing "Check the datetime column the correct base_type" (is (=? {:name #"(?i)datetime" @@ -474,8 +477,7 @@ (with-mysql-local-infile-on-and-off (mt/with-dynamic-redefs [driver/db-default-timezone (constantly "Z") upload/current-database (constantly (mt/db))] - (let [table-name (mt/random-name) - datetime-pairs [["2022-01-01T12:00:00-07" "2022-01-01T19:00:00Z"] + (let [datetime-pairs [["2022-01-01T12:00:00-07" "2022-01-01T19:00:00Z"] ["2022-01-01T12:00:00-07:00" "2022-01-01T19:00:00Z"] ["2022-01-01T12:00:00-07:30" "2022-01-01T19:30:00Z"] ["2022-01-01T12:00:00Z" "2022-01-01T12:00:00Z"] @@ -485,12 +487,8 @@ ["2022-01-01T12:00:00+07:30" "2022-01-01T04:30:00Z"]]] (testing "Fields exists after sync" (with-upload-table! - [table (do (@#'upload/create-from-csv! - driver/*driver* - (mt/id) - table-name - (csv-file-with (into ["offset_datetime"] (map first datetime-pairs)))) - (sync-upload-test-table! :database (mt/db) :table-name table-name))] + [table (create-from-csv-and-sync-with-defaults! + :file (csv-file-with (into ["offset_datetime"] (map first datetime-pairs))))] (testing "Check the offset datetime column the correct base_type" (is (=? {:name #"(?i)offset_datetime" :base_type :type/DateTimeWithLocalTZ} @@ -504,31 +502,26 @@ (mt/test-drivers (mt/normal-drivers-with-feature :uploads) (with-mysql-local-infile-on-and-off (with-upload-table! - [table (let [table-name (mt/random-name)] - (@#'upload/create-from-csv! - driver/*driver* - (mt/id) - table-name - (csv-file-with ["id,bool" - "1,true" - "2,false" - "3,TRUE" - "4,FALSE" - "5,t " - "6, f" - "7,\tT" - "8,F\t" - "9,y" - "10,n" - "11,Y" - "12,N" - "13,yes" - "14,no" - "15,YES" - "16,NO" - "17,1" - "18,0"])) - (sync-upload-test-table! :database (mt/db) :table-name table-name))] + [table (create-from-csv-and-sync-with-defaults! + :file (csv-file-with ["id,bool" + "1,true" + "2,false" + "3,TRUE" + "4,FALSE" + "5,t " + "6, f" + "7,\tT" + "8,F\t" + "9,y" + "10,n" + "11,Y" + "12,N" + "13,yes" + "14,no" + "15,YES" + "16,NO" + "17,1" + "18,0"]))] (testing "Table and Fields exist after sync" (testing "Check the boolean column has a boolean base_type" (is (=? {:name #"(?i)bool" @@ -542,23 +535,20 @@ (deftest create-from-csv-length-test (testing "Upload a CSV file with large names and numbers" (mt/test-drivers (mt/normal-drivers-with-feature :uploads) - (let [length-limit (driver/table-name-length-limit driver/*driver*) - ;; Ensure the name is unique as table names can collide when using redshift - long-name (->> "abc" str cycle (take (inc length-limit)) shuffle (apply str)) - short-name (subs long-name 0 (- length-limit (count "_yyyyMMddHHmmss"))) - table-name (u/upper-case-en (@#'upload/unique-table-name driver/*driver* long-name))] - (is (pos? length-limit) "driver/table-name-length-limit has been set") - (with-mysql-local-infile-on-and-off + (with-mysql-local-infile-on-and-off + (let [length-limit (driver/table-name-length-limit driver/*driver*) + ;; Ensure the name is unique as table names can collide when using redshift + long-name (->> "abc" str cycle (take (inc length-limit)) shuffle (apply str)) + short-name (subs long-name 0 (- length-limit (count "_yyyyMMddHHmmss"))) + table-name (u/upper-case-en (@#'upload/unique-table-name driver/*driver* long-name))] + (is (pos? length-limit) "driver/table-name-length-limit has been set") (with-upload-table! - [table (do (@#'upload/create-from-csv! - driver/*driver* - (mt/id) - table-name - (csv-file-with ["number,bool" - "1,true" - "2,false" - (format "%d,true" Long/MAX_VALUE)])) - (sync-upload-test-table! :database (mt/db) :table-name table-name))] + [table (create-from-csv-and-sync-with-defaults! + :table-name table-name + :file (csv-file-with ["number,bool" + "1,true" + "2,false" + (format "%d,true" Long/MAX_VALUE)]))] (let [table-re (re-pattern (str "(?i)" short-name "_\\d{14}"))] (testing "It truncates it to the right number of characters, allowing for the timestamp" (is (re-matches table-re (:name table)))) @@ -572,15 +562,10 @@ (testing "Upload a CSV file with a blank column name" (mt/test-drivers (mt/normal-drivers-with-feature :uploads) (with-upload-table! - [table (let [table-name (mt/random-name)] - (@#'upload/create-from-csv! - driver/*driver* - (mt/id) - table-name - (csv-file-with [",ship name," - "1,Serenity,Malcolm Reynolds" - "2,Millennium Falcon, Han Solo"])) - (sync-upload-test-table! :database (mt/db) :table-name table-name))] + [table (create-from-csv-and-sync-with-defaults! + :file (csv-file-with [",ship name," + "1,Serenity,Malcolm Reynolds" + "2,Millennium Falcon, Han Solo"]))] (testing "Check the data was uploaded into the table correctly" (is (= [@#'upload/auto-pk-column-name "unnamed_column" "ship_name" "unnamed_column_2"] (column-names-for-table table)))))))) @@ -590,15 +575,10 @@ (mt/test-drivers (mt/normal-drivers-with-feature :uploads) (with-mysql-local-infile-on-and-off (with-upload-table! - [table (let [table-name (mt/random-name)] - (@#'upload/create-from-csv! - driver/*driver* - (mt/id) - table-name - (csv-file-with ["unknown,unknown,unknown,unknown_2" - "1,Serenity,Malcolm Reynolds,Pistol" - "2,Millennium Falcon, Han Solo,Blaster"])) - (sync-upload-test-table! :database (mt/db) :table-name table-name))] + [table (create-from-csv-and-sync-with-defaults! + :file (csv-file-with ["unknown,unknown,unknown,unknown_2" + "1,Serenity,Malcolm Reynolds,Pistol" + "2,Millennium Falcon, Han Solo,Blaster"]))] (testing "Table and Fields exist after sync" (testing "Check the data was uploaded into the table correctly" (is (= [@#'upload/auto-pk-column-name "unknown" "unknown_2" "unknown_3" "unknown_2_2"] @@ -609,17 +589,12 @@ (mt/test-drivers (mt/normal-drivers-with-feature :uploads) (with-mysql-local-infile-on-and-off (with-upload-table! - [table (let [table-name (mt/random-name)] - (@#'upload/create-from-csv! - driver/*driver* - (mt/id) - table-name - (csv-file-with ["vchar,bool,bool-or-int,int" - " true,true, 1, 1" - " 1, 1, 0, 0" - " 2, 0, 0, 0" - " no, no, 1, 2"])) - (sync-upload-test-table! :database (mt/db) :table-name table-name))] + [table (create-from-csv-and-sync-with-defaults! + :file (csv-file-with ["vchar,bool,bool-or-int,int" + " true,true, 1, 1" + " 1, 1, 0, 0" + " 2, 0, 0, 0" + " no, no, 1, 2"]))] (testing "Check the data was uploaded into the table correctly" (is (= [[1 " true" true true 1] [2 " 1" true false 0] @@ -632,17 +607,14 @@ (mt/test-drivers (mt/normal-drivers-with-feature :uploads) (with-mysql-local-infile-on-and-off (with-upload-table! - [table (let [table-name (mt/random-name)] - (@#'upload/create-from-csv! - driver/*driver* - (mt/id) - table-name - (csv-file-with ["id,ship,name,weapon" - "1,Serenity,Malcolm Reynolds,Pistol" - "2,Millennium Falcon,Han Solo,Blaster" - ;; A huge ID to make extra sure we're using bigints - "9000000000,Razor Crest,Din Djarin,Spear"])) - (sync-upload-test-table! :database (mt/db) :table-name table-name))] + [table (binding [upload/*auxiliary-sync-steps* :synchronous] + (create-from-csv-and-sync-with-defaults! + :file (csv-file-with ["id,ship,name,weapon" + "1,Serenity,Malcolm Reynolds,Pistol" + "2,Millennium Falcon,Han Solo,Blaster" + ;; A huge ID to make extra sure we're using bigints + "9000000000,Razor Crest,Din Djarin,Spear"]) + :auxiliary-sync-steps :synchronous))] (testing "Check the data was uploaded into the table correctly" (is (= [@#'upload/auto-pk-column-name "id" "ship" "name" "weapon"] (column-names-for-table table))) @@ -652,44 +624,16 @@ :database_is_auto_increment false} (t2/select-one Field :database_position 1 :table_id (:id table)))))))))) -(deftest create-from-csv-existing-string-id-column-test - (testing "Upload a CSV file with an existing string ID column" - (mt/test-drivers (mt/normal-drivers-with-feature :uploads) - (with-mysql-local-infile-on-and-off - (with-upload-table! - [table (let [table-name (mt/random-name)] - (@#'upload/create-from-csv! - driver/*driver* - (mt/id) - table-name - (csv-file-with ["id,ship,name,weapon" - "a,Serenity,Malcolm Reynolds,Pistol" - "b,Millennium Falcon,Han Solo,Blaster"])) - (sync-upload-test-table! :database (mt/db) :table-name table-name))] - (testing "Check the data was uploaded into the table correctly" - (is (= [@#'upload/auto-pk-column-name "id" "ship" "name" "weapon"] - (column-names-for-table table))) - (is (=? {:name #"(?i)id" - :semantic_type :type/PK - :base_type :type/Text - :database_is_auto_increment false} - (t2/select-one Field :database_position 1 :table_id (:id table)))))))))) - (deftest create-from-csv-reserved-db-words-test (testing "Upload a CSV file with column names that are reserved by the DB, ignoring them" (testing "A single column whose name normalizes to _mb_row_id" (mt/test-drivers (mt/normal-drivers-with-feature :uploads) (with-mysql-local-infile-on-and-off (with-upload-table! - [table (let [table-name (mt/random-name)] - (@#'upload/create-from-csv! - driver/*driver* - (mt/id) - table-name - (csv-file-with ["_mb_ROW-id,ship,captain" - "100,Serenity,Malcolm Reynolds" - "3,Millennium Falcon, Han Solo"])) - (sync-upload-test-table! :database (mt/db) :table-name table-name))] + [table (create-from-csv-and-sync-with-defaults! + :file (csv-file-with ["_mb_ROW-id,ship,captain" + "100,Serenity,Malcolm Reynolds" + "3,Millennium Falcon, Han Solo"]))] (testing "Check the data was uploaded into the table correctly" (is (= ["_mb_row_id", "ship", "captain"] (column-names-for-table table))) @@ -700,15 +644,10 @@ (mt/test-drivers (mt/normal-drivers-with-feature :uploads) (with-mysql-local-infile-on-and-off (with-upload-table! - [table (let [table-name (mt/random-name)] - (@#'upload/create-from-csv! - driver/*driver* - (mt/id) - table-name - (csv-file-with ["_mb row id,ship,captain,_mb row id" - "100,Serenity,Malcolm Reynolds,200" - "3,Millennium Falcon, Han Solo,4"])) - (sync-upload-test-table! :database (mt/db) :table-name table-name))] + [table (create-from-csv-and-sync-with-defaults! + :file (csv-file-with ["_mb row id,ship,captain,_mb row id" + "100,Serenity,Malcolm Reynolds,200" + "3,Millennium Falcon, Han Solo,4"]))] (testing "Check the data was uploaded into the table correctly" (is (= ["_mb_row_id", "ship", "captain"] (column-names-for-table table))) @@ -719,15 +658,10 @@ (mt/test-drivers (mt/normal-drivers-with-feature :uploads) (with-mysql-local-infile-on-and-off (with-upload-table! - [table (let [table-name (mt/random-name)] - (@#'upload/create-from-csv! - driver/*driver* - (mt/id) - table-name - (csv-file-with ["_mb row id,ship,captain,_MB_ROW_ID" - "100,Serenity,Malcolm Reynolds,200" - "3,Millennium Falcon, Han Solo,4"])) - (sync-upload-test-table! :database (mt/db) :table-name table-name))] + [table (create-from-csv-and-sync-with-defaults! + :file (csv-file-with ["_mb row id,ship,captain,_MB_ROW_ID" + "100,Serenity,Malcolm Reynolds,200" + "3,Millennium Falcon, Han Solo,4"]))] (testing "Check the data was uploaded into the table correctly" (is (= ["_mb_row_id", "ship", "captain"] (column-names-for-table table))) @@ -740,15 +674,10 @@ (with-mysql-local-infile-on-and-off (testing "Can upload a CSV with missing values" (with-upload-table! - [table (let [table-name (mt/random-name)] - (@#'upload/create-from-csv! - driver/*driver* - (mt/id) - table-name - (csv-file-with ["column_that_has_one_value,column_that_doesnt_have_a_value" - "2" - " ,\n"])) - (sync-upload-test-table! :database (mt/db) :table-name table-name))] + [table (create-from-csv-and-sync-with-defaults! + :file (csv-file-with ["column_that_has_one_value,column_that_doesnt_have_a_value" + "2" + " ,\n"]))] (testing "Check the data was uploaded into the table correctly" (is (= [@#'upload/auto-pk-column-name "column_that_has_one_value", "column_that_doesnt_have_a_value"] (column-names-for-table table))) @@ -761,15 +690,10 @@ (mt/test-drivers (mt/normal-drivers-with-feature :uploads) (with-mysql-local-infile-on-and-off (with-upload-table! - [table (let [table-name (mt/random-name)] - (@#'upload/create-from-csv! - driver/*driver* - (mt/id) - table-name - (csv-file-with ["ship,captain" - "Serenity,Malcolm\tReynolds" - "Millennium\tFalcon,Han\tSolo"])) - (sync-upload-test-table! :database (mt/db) :table-name table-name))] + [table (create-from-csv-and-sync-with-defaults! + :file (csv-file-with ["ship,captain" + "Serenity,Malcolm\tReynolds" + "Millennium\tFalcon,Han\tSolo"]))] (testing "Check the data was uploaded into the table correctly" (is (= [@#'upload/auto-pk-column-name "ship", "captain"] (column-names-for-table table))) @@ -782,15 +706,10 @@ (mt/test-drivers (mt/normal-drivers-with-feature :uploads) (with-mysql-local-infile-on-and-off (with-upload-table! - [table (let [table-name (mt/random-name)] - (@#'upload/create-from-csv! - driver/*driver* - (mt/id) - table-name - (csv-file-with ["ship,captain" - "Serenity,\"Malcolm\rReynolds\"" - "\"Millennium\rFalcon\",\"Han\rSolo\""])) - (sync-upload-test-table! :database (mt/db) :table-name table-name))] + [table (create-from-csv-and-sync-with-defaults! + :file (csv-file-with ["ship,captain" + "Serenity,\"Malcolm\rReynolds\"" + "\"Millennium\rFalcon\",\"Han\rSolo\""]))] (testing "Check the data was uploaded into the table correctly" (is (= [@#'upload/auto-pk-column-name, "ship", "captain"] (column-names-for-table table))) @@ -803,17 +722,12 @@ (mt/test-drivers (mt/normal-drivers-with-feature :uploads) (with-mysql-local-infile-on-and-off (with-upload-table! - [table (let [table-name (mt/random-name)] - (@#'upload/create-from-csv! - driver/*driver* - (mt/id) - table-name - (csv-file-with ["ship,captain" - "Serenity,Malcolm Reynolds" - "Millennium Falcon, Han Solo"] - "star-wars" - (partial bom/bom-writer "UTF-8"))) - (sync-upload-test-table! :database (mt/db) :table-name table-name))] + [table (create-from-csv-and-sync-with-defaults! + :file (csv-file-with ["ship,captain" + "Serenity,Malcolm Reynolds" + "Millennium Falcon, Han Solo"] + "star-wars" + (partial bom/bom-writer "UTF-8")))] (testing "Check the data was uploaded into the table correctly" (is (= [@#'upload/auto-pk-column-name, "ship", "captain"] (column-names-for-table table))))))))) @@ -823,16 +737,11 @@ (mt/test-drivers (mt/normal-drivers-with-feature :uploads) (with-mysql-local-infile-on-and-off (with-upload-table! - [table (let [table-name (mt/random-name)] - (@#'upload/create-from-csv! - driver/*driver* - (mt/id) - table-name - (csv-file-with ["id integer); --,ship,captain" - "1,Serenity,--Malcolm Reynolds" - "2,;Millennium Falcon,Han Solo\""] - "\"; -- Very rude filename")) - (sync-upload-test-table! :database (mt/db) :table-name table-name))] + [table (create-from-csv-and-sync-with-defaults! + :file (csv-file-with ["id integer); --,ship,captain" + "1,Serenity,--Malcolm Reynolds" + "2,;Millennium Falcon,Han Solo\""] + "\"; -- Very rude filename"))] (testing "Check the data was uploaded into the table correctly" (is (= [@#'upload/auto-pk-column-name "id_integer_____" "ship" "captain"] (column-names-for-table table))) @@ -844,16 +753,11 @@ (testing "Upload a CSV file with Postgres's 'end of input' marker" (mt/test-drivers [:postgres] (with-upload-table! - [table (let [table-name (mt/random-name)] - (@#'upload/create-from-csv! - driver/*driver* - (mt/id) - table-name - (csv-file-with ["name" - "Malcolm" - "\\." - "Han"])) - (sync-upload-test-table! :database (mt/db) :table-name table-name))] + [table (create-from-csv-and-sync-with-defaults! + :file (csv-file-with ["name" + "Malcolm" + "\\." + "Han"]))] (testing "Check the data was uploaded into the table correctly" (is (= [[1 "Malcolm"] [2 "\\."] [3 "Han"]] (rows-for-table table)))))))) @@ -877,9 +781,9 @@ :value)))))))) (defn- update-csv-synchronously! - "Wraps [[upload/upload-csv!]] setting [[upload/*sync-synchronously?*]] to `true` for test purposes." + "Wraps [[upload/upload-csv!]] setting [[upload/*auxiliary-sync-steps*]] to `synchronous` for test purposes." [options] - (binding [upload/*sync-synchronously?* true] + (binding [upload/*auxiliary-sync-steps* :synchronous] (upload/update-csv! options))) (defn- update-csv! @@ -893,7 +797,7 @@ db-id (u/the-id db) original-sync-values (select-keys db [:is_on_demand :is_full_sync]) in-future? (atom false) - schema-name (or (sql.tx/session-schema driver/*driver*) "public") + schema-name (sql.tx/session-schema driver/*driver*) _ (t2/update! :model/Database db-id {:is_on_demand false :is_full_sync false})] (try @@ -902,11 +806,8 @@ (swap! in-future? (constantly true)) (thunk))] (testing "Happy path with schema, and without table-prefix" - ;; make sure the schema exists - (let [sql (format "CREATE SCHEMA IF NOT EXISTS %s;" (sql.tx/qualify-and-quote driver/*driver* schema-name))] - (jdbc/execute! (sql-jdbc.conn/db->pooled-connection-spec db) sql)) (with-upload-table! - [new-table (card->table (upload-example-csv! :schema-name schema-name :sync-synchronously? false))] + [new-table (card->table (upload-example-csv! :schema-name schema-name :auxiliary-sync-steps :asynchronous))] (is (=? {:display :table :database_id db-id :dataset_query {:database db-id @@ -943,9 +844,9 @@ (testing "Happy path with table prefix, and without schema" (if (driver/database-supports? driver/*driver* :schemas (mt/db)) (is (thrown-with-msg? - java.lang.Exception - #"^A schema has not been set." - (upload-example-csv! :table-prefix "uploaded_magic_" :schema-name nil))) + java.lang.Exception + #"^A schema has not been set." + (upload-example-csv! :table-prefix "uploaded_magic_" :schema-name nil))) (with-upload-table! [table (card->table (upload-example-csv! :table-prefix "uploaded_magic_"))] (is (=? {:name #"(?i)example csv file(.*)"} (table->card table))) @@ -1000,7 +901,7 @@ :model "Table" :model_id pos? :details {:db-id pos? - :schema-name (format-schema "public") + :schema-name (sql.tx/session-schema driver/*driver*) :table-name string? :model-id pos? :stats {:num-rows 2 @@ -1088,6 +989,8 @@ rows [["Obi-Wan Kenobi"]]}}] (let [driver driver/*driver* db-id (mt/id) + table-name (ddl.i/format-name driver table-name) + schema-name (ddl.i/format-name driver schema-name) schema+table-name (#'upload/table-identifier {:schema schema-name :name table-name}) insert-col-names (remove #{upload/auto-pk-column-keyword} (keys col->upload-type)) col-definitions (#'upload/column-definitions driver col->upload-type)] @@ -1115,8 +1018,7 @@ file (csv-file-with ["name" "Luke Skywalker" - "Darth Vader"] - (mt/random-name)) + "Darth Vader"]) is-upload true}}] (mt/with-temporary-setting-values [uploads-enabled uploads-enabled] (mt/with-current-user user-id @@ -1193,7 +1095,7 @@ (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 [] (mt/random-name))))))) + (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*))) @@ -1214,7 +1116,7 @@ :id int-type :name vchar-type) :rows [[10 "Obi-Wan Kenobi"]]})] - (let [file (csv-file-with csv-rows (mt/random-name))] + (let [file (csv-file-with csv-rows)] (is (some? (update-csv! action {:file file, :table-id (:id table)}))) (testing "Check the data was uploaded into the table correctly" (is (=? (updated-contents action @@ -1267,7 +1169,7 @@ :name vchar-type) :rows [[1, "some_text"]]})] - (let [file (csv-file-with csv-rows (mt/random-name))] + (let [file (csv-file-with csv-rows)] (when error-message (is (= {:message error-message :data {:status-code 422}} @@ -1306,7 +1208,7 @@ :rows [[1000000,1.0,"some_text",false,#t "2020-01-01",#t "2020-01-01T00:00:00",#t "2020-01-01T00:00:00"]]})] (let [csv-rows ["biginteger,float,text,boolean,date,datetime,offset_datetime" "2000000,2.0,some_text,true,2020-02-02,2020-02-02T02:02:02,2020-02-02T02:02:02+02:00"] - file (csv-file-with csv-rows (mt/random-name))] + file (csv-file-with csv-rows)] (is (some? (update-csv! action {:file file, :table-id (:id table)}))) (testing "Check the data was uploaded into the table correctly" (is (=? (updated-contents action @@ -1324,7 +1226,7 @@ (let [csv-rows ["name"]] (with-upload-table! [table (create-upload-table!)] - (let [file (csv-file-with csv-rows (mt/random-name))] + (let [file (csv-file-with csv-rows)] (is (= {:row-count 0} (update-csv! action {:file file, :table-id (:id table)}))) (testing "Check the data was not uploaded into the table" @@ -1342,7 +1244,7 @@ :name vchar-type) :rows [["Obi-Wan Kenobi"]]})] (let [csv-rows ["_MB-row ID,name" "1000,Luke Skywalker"] - file (csv-file-with csv-rows (mt/random-name))] + file (csv-file-with csv-rows)] (is (= {:row-count 1} (update-csv! action {:file file, :table-id (:id table)}))) ;; Only create auto-pk columns for drivers that supported uploads before auto-pk columns @@ -1372,9 +1274,13 @@ (testing "Check a _mb_row_id column wasn't created" (is (= ["name"] (column-names-for-table table)))) - (testing "Check the data was uploaded into the table" + (case action + ::upload/append (is (= [["Obi-Wan Kenobi"] ["Luke Skywalker"]] + (rows-for-table table))) + ::upload/replace + (is (= [["Luke Skywalker"]] (rows-for-table table)))))) (io/delete-file file)))))))) @@ -1388,7 +1294,7 @@ :bool_column bool-type) :rows [[true]]})] (let [csv-rows ["bool_column" "not a bool"] - file (csv-file-with csv-rows (mt/random-name)) + file (csv-file-with csv-rows) get-auto-pk (fn [] (t2/select-one :model/Field :table_id (:id table) :name upload/auto-pk-column-name))] (is (nil? (get-auto-pk))) @@ -1473,7 +1379,7 @@ (mt/with-premium-features #{:audit-app} (with-upload-table! [table (create-upload-table!)] (let [csv-rows ["name" "Luke Skywalker"] - file (csv-file-with csv-rows (mt/random-name))] + file (csv-file-with csv-rows)] (update-csv! action {:file file, :table-id (:id table)}) (is (=? {:topic :upload-append @@ -1481,7 +1387,7 @@ :model "Table" :model_id (:id table) :details {:db-id pos? - :schema-name (format-schema "public") + :schema-name (sql.tx/session-schema driver/*driver*) :table-name string? :stats {:num-rows 1 :num-columns 1 @@ -1513,7 +1419,7 @@ (testing "with duplicate normalized _mb_row_id columns in the CSV file" (with-upload-table! [table (create-upload-table!)] (let [csv-rows ["_mb_row_id,name,-MB-ROW-ID" "1000,Luke Skywalker,1001"] - file (csv-file-with csv-rows (mt/random-name))] + file (csv-file-with csv-rows)] (is (= {:row-count 1} (update-csv! action {:file file, :table-id (:id table)}))) (testing "Check the data was uploaded into the table, but the _mb_row_id was ignored" @@ -1552,7 +1458,7 @@ :rows [["Obi-Wan Kenobi" "No one really knows me"]])] (let [csv-rows ["shame,name" "Nothing - you can't prove it,Puke Nightstalker"] - file (csv-file-with csv-rows (mt/random-name))] + file (csv-file-with csv-rows)] (testing "The new row is inserted with the values correctly reordered" (is (= {:row-count 1} (update-csv! action {:file file, :table-id (:id table)}))) @@ -1571,7 +1477,7 @@ (with-upload-table! [table (create-upload-table!)] ;; Reorder as well for good measure (let [csv-rows ["game,name" "Witticisms,Fluke Skytalker"] - file (csv-file-with csv-rows (mt/random-name))] + file (csv-file-with csv-rows)] (testing "The new row is inserted with the values correctly reordered" (is (= {:row-count 1} (update-csv! action {:file file, :table-id (:id table)}))) (is (=? (updated-contents action @@ -1627,7 +1533,7 @@ :rows [[valid "Obi-Wan Kenobi"]]})] (let [;; The CSV contains 50 valid rows and 1 invalid row csv-rows `["test_column,name" ~@(repeat 50 (str valid ",Darth Vadar")) ~(str invalid ",Luke Skywalker")] - file (csv-file-with csv-rows (mt/random-name))] + file (csv-file-with csv-rows)] (testing "\nShould return an appropriate error message" (is (= {:message msg :data {:status-code 422}} @@ -1664,7 +1570,7 @@ :test_column vchar-type) :rows [["valid"]]})] (let [csv-rows `["test_column" ~@(repeat 50 "valid too") ~(apply str (repeat 256 "x"))] - file (csv-file-with csv-rows (mt/random-name))] + file (csv-file-with csv-rows)] (testing "\nShould return an appropriate error message" (is (=? {;; the error message is different for different drivers, but postgres and mysql have "too long" in the message :message #"[\s\S]*too long[\s\S]*" @@ -1696,7 +1602,7 @@ :test_column upload-type) :rows []})] (let [csv-rows ["test_column" uncoerced] - file (csv-file-with csv-rows (mt/random-name))] + file (csv-file-with csv-rows)] (testing "\nAppend should succeed" (is (= {:row-count 1} (update-csv! action {:file file, :table-id (:id table)})))) @@ -1715,8 +1621,13 @@ ;; inserted rows are rolled back (binding [driver/*insert-chunk-rows* 1] (doseq [{:keys [upload-type uncoerced coerced fail-msg] :as args} - [{:upload-type int-type, :uncoerced "2.0", :coerced 2} ;; value is coerced to int - {:upload-type int-type, :uncoerced "2.1", :coerced 2.1} ;; column is promoted to float + [(merge + {:upload-type int-type, :uncoerced "2.1"} + (if (= driver/*driver* :redshift) + ;; TODO: redshift doesn't allow promotion of ints to floats + {:fail-msg "There's a value with the wrong type \\('double precision'\\) in the 'test_column' column"} + {:coerced 2.1})) ; column is promoted to float + {:upload-type int-type, :uncoerced "2.0", :coerced 2} ; value is coerced to int {:upload-type float-type, :uncoerced "2", :coerced 2.0} {:upload-type bool-type, :uncoerced "0", :coerced false} {:upload-type bool-type, :uncoerced "1.0", :fail-msg "'1.0' is not a recognizable boolean"} @@ -1728,7 +1639,7 @@ :test_column upload-type) :rows []})] (let [csv-rows ["test_column" uncoerced] - file (csv-file-with csv-rows (mt/random-name)) + file (csv-file-with csv-rows) update! (fn [] (update-csv! action {:file file, :table-id (:id table)}))] (if (contains? args :coerced) @@ -1747,8 +1658,8 @@ (update!))))) (io/delete-file file))))))))))) -(deftest update-coercing-multiple-columns-test - (mt/test-drivers (mt/normal-drivers-with-feature :uploads) +(deftest update-promotion-multiple-columns-test + (mt/test-drivers (disj (mt/normal-drivers-with-feature :uploads) :redshift) ; redshift doesn't support promotion (doseq [action (actions-to-test driver/*driver*)] (testing (action-testing-str action) (with-mysql-local-infile-on-and-off @@ -1780,15 +1691,10 @@ (mt/test-drivers (mt/normal-drivers-with-feature :uploads) (with-mysql-local-infile-on-and-off (with-upload-table! - [table (let [table-name (mt/random-name)] - (@#'upload/create-from-csv! - driver/*driver* - (mt/id) - table-name - (csv-file-with ["float-1,float-2" - "1, 1.0" - "1.0, 1"])) - (sync-upload-test-table! :database (mt/db) :table-name table-name))] + [table (create-from-csv-and-sync-with-defaults! + :file (csv-file-with ["float-1,float-2" + "1, 1.0" + "1.0, 1"]))] (testing "Check the data was uploaded into the table correctly" (is (= [[1 1.0 1.0] [2 1.0 1.0]] @@ -1799,15 +1705,10 @@ (mt/test-drivers (mt/normal-drivers-with-feature :uploads) (with-mysql-local-infile-on-and-off (with-upload-table! - [table (let [table-name (mt/random-name)] - (@#'upload/create-from-csv! - driver/*driver* - (mt/id) - table-name - (csv-file-with ["float-1,float-2" - "1, 1.1" - "1.1, 1"])) - (sync-upload-test-table! :database (mt/db) :table-name table-name))] + [table (create-from-csv-and-sync-with-defaults! + :file (csv-file-with ["float-1,float-2" + "1, 1.1" + "1.1, 1"]))] (testing "Check the data was uploaded into the table correctly" (is (= [[1 1.0 1.1] [2 1.1 1.0]] @@ -1828,7 +1729,7 @@ (let [csv-rows ["number-1, number-2" "1.0, 1" "1 , 1.0"] - file (csv-file-with csv-rows (mt/random-name))] + file (csv-file-with csv-rows)] (is (some? (update-csv! action {:file file, :table-id (:id table)}))) (is (=? (updated-contents action [[1 1 1]]