Implement `:bulk/create` action for SQL JDBC. Tons of bug fixes (#23878)

* Actions multimethod refactor.

* Fix function called with wrong number of args

* Test fixes

* Bulk create action (second pass)

* Appease clj-kondo

* Fix bulk insert happy path

* Implement/fix everything

* Linter and test fixes 



* Fix bad metadata (thanks Eastwood!)

* Add some comments about why we're doing what we're doing

* Serdes v2 for Native Query Snippets (#23961)

Also standardizes date/time output to `ZonedDateTime`, rather than whatever the JDBC happens to return.

Co-authored-by: Braden Shepherdson <Braden.Shepherdson@gmail.com>

.clj-kondo/config.edn

@@ -345,45 +345,49 @@
                                         saml20-clj.core                                                 saml
                                         toucan.db                                                       db
                                         toucan.models                                                   models}}}
- :lint-as {metabase.api.common/let-404 clojure.core/let
-           metabase.db.data-migrations/defmigration clojure.core/def
-           metabase.query-processor.error-type/deferror clojure.core/def
-           metabase.models.setting/defsetting clj-kondo.lint-as/def-catch-all
-           metabase.mbql.schema.macros/defclause clj-kondo.lint-as/def-catch-all
-           metabase.public-settings.premium-features/defenterprise clj-kondo.lint-as/def-catch-all
-           metabase.public-settings.premium-features/defenterprise-schema clj-kondo.lint-as/def-catch-all
-           metabase.public-settings.premium-features/define-premium-feature clojure.core/def
-           metabase.sync.util/sum-for clojure.core/for
-           metabase.sync.util/with-emoji-progress-bar clojure.core/let
-           metabase.driver.sql-jdbc.execute.diagnostic/capturing-diagnostic-info clojure.core/fn
-           metabase.util.files/with-open-path-to-resource clojure.core/let
-           metabase.util.ssh/with-ssh-tunnel clojure.core/let
-           metabase.db.liquibase/with-liquibase clojure.core/let
-           metabase.models.setting.multi-setting/define-multi-setting clojure.core/def
-           metabase.integrations.ldap/with-ldap-connection clojure.core/fn
-           metabase.test/are+ clojure.test/are
-           metabase.test/defdataset clojure.core/def
-           metabase.test/with-temp-file clojure.core/let
-           metabase.test/with-open-channels clojure.core/let
-           metabase.test/with-user-in-groups clojure.core/let
-           metabase.test.data.interface/defdataset clojure.core/def
-           metabase.test.data.interface/defdataset-edn clojure.core/def
-           metabase-enterprise.serialization.test-util/with-random-dump-dir clojure.core/let
-           metabase.driver.mongo.util/with-mongo-connection clojure.core/let
-           metabase.driver.mongo.query-processor/mongo-let clojure.core/let
-           toucan.db/with-call-counting clojure.core/fn
 
-           potemkin.types/defprotocol+ clojure.core/defprotocol
-           potemkin/defprotocol+ clojure.core/defprotocol
-           potemkin.types/defrecord+ clojure.core/defrecord
-           potemkin/defrecord+ clojure.core/defrecord
-           potemkin.types/deftype+ clojure.core/deftype
-           potemkin/deftype+ clojure.core/deftype
-           clojurewerkz.quartzite.jobs/defjob clojure.core/defn
-           honeysql.util/defalias clojure.core/def
-           honeysql.helpers/defhelper clj-kondo.lint-as/def-catch-all
-           clojure.core.logic/defne clj-kondo.lint-as/def-catch-all
-           monger.operators/defoperator clojure.core/def}
+ :lint-as
+ {metabase-enterprise.serialization.test-util/with-random-dump-dir      clojure.core/let
+  metabase.api.common/let-404                                           clojure.core/let
+  metabase.db.data-migrations/defmigration                              clojure.core/def
+  metabase.db.liquibase/with-liquibase                                  clojure.core/let
+  metabase.driver.mongo.query-processor/mongo-let                       clojure.core/let
+  metabase.driver.mongo.util/with-mongo-connection                      clojure.core/let
+  metabase.driver.sql-jdbc.actions/with-jdbc-transaction                clojure.core/let
+  metabase.driver.sql-jdbc.execute.diagnostic/capturing-diagnostic-info clojure.core/fn
+  metabase.integrations.ldap/with-ldap-connection                       clojure.core/fn
+  metabase.mbql.schema.macros/defclause                                 clj-kondo.lint-as/def-catch-all
+  metabase.models.setting.multi-setting/define-multi-setting            clojure.core/def
+  metabase.models.setting/defsetting                                    clj-kondo.lint-as/def-catch-all
+  metabase.public-settings.premium-features/defenterprise               clj-kondo.lint-as/def-catch-all
+  metabase.public-settings.premium-features/defenterprise-schema        clj-kondo.lint-as/def-catch-all
+  metabase.public-settings.premium-features/define-premium-feature      clojure.core/def
+  metabase.query-processor.error-type/deferror                          clojure.core/def
+  metabase.sync.util/sum-for                                            clojure.core/for
+  metabase.sync.util/with-emoji-progress-bar                            clojure.core/let
+  metabase.test.data.interface/defdataset                               clojure.core/def
+  metabase.test.data.interface/defdataset-edn                           clojure.core/def
+  metabase.test/are+                                                    clojure.test/are
+  metabase.test/defdataset                                              clojure.core/def
+  metabase.test/with-open-channels                                      clojure.core/let
+  metabase.test/with-temp-file                                          clojure.core/let
+  metabase.test/with-user-in-groups                                     clojure.core/let
+  metabase.util.files/with-open-path-to-resource                        clojure.core/let
+  metabase.util.ssh/with-ssh-tunnel                                     clojure.core/let
+
+  clojure.core.logic/defne           clj-kondo.lint-as/def-catch-all
+  clojurewerkz.quartzite.jobs/defjob clojure.core/defn
+  honeysql.helpers/defhelper         clj-kondo.lint-as/def-catch-all
+  honeysql.util/defalias             clojure.core/def
+  monger.operators/defoperator       clojure.core/def
+  potemkin.types/defprotocol+        clojure.core/defprotocol
+  potemkin.types/defrecord+          clojure.core/defrecord
+  potemkin.types/deftype+            clojure.core/deftype
+  potemkin/defprotocol+              clojure.core/defprotocol
+  potemkin/defrecord+                clojure.core/defrecord
+  potemkin/deftype+                  clojure.core/deftype
+  toucan.db/with-call-counting       clojure.core/fn}
+
  :hooks
  {:analyze-call
   {metabase.api.common/defendpoint                            hooks.metabase.api.common/defendpoint

enterprise/backend/src/metabase_enterprise/serialization/v2/models.clj

@@ -10,5 +10,6 @@
    "Dimension"
    "Field"
    "Metric"
+   "NativeQuerySnippet"
    "Setting"
    "Table"])

enterprise/backend/test/metabase_enterprise/serialization/v2/e2e/yaml_test.clj

@@ -28,50 +28,56 @@
 (defn- strip-labels [path]
   (mapv #(dissoc % :label) path))
 
-(defn- random-key [prefix n]
-  (keyword (str prefix (rand-int n))))
+(defn- random-key
+  ([prefix n] (random-key prefix n 0))
+  ([prefix n floor] (keyword (str prefix (+ floor (rand-int n))))))
 
 (deftest e2e-storage-ingestion-test
   (ts/with-random-dump-dir [dump-dir "serdesv2-"]
     (ts/with-empty-h2-app-db
       ;; TODO Generating some nested collections would make these tests more robust.
-      (test-gen/insert! {:collection     [[100 {:refs {:personal_owner_id ::rs/omit}}]]
-                         :database       [[10]]
-                         :table          (into [] (for [db [:db0 :db1 :db2 :db3 :db4 :db5 :db6 :db7 :db8 :db9]]
-                                                    [10 {:refs {:db_id db}}]))
-                         :field          (into [] (for [n     (range 100)
-                                                        :let [table (keyword (str "t" n))]]
-                                                    [10 {:refs {:table_id table}}]))
-                         :core-user      [[10]]
-                         :card           [[100 {:refs (let [db (rand-int 10)
-                                                            t  (rand-int 10)]
-                                                        {:database_id   (keyword (str "db" db))
-                                                         :table_id      (keyword (str "t" (+ t (* 10 db))))
-                                                         :collection_id (random-key "coll" 100)
-                                                         :creator_id    (random-key "u" 10)})}]]
-                         :dashboard      [[100 {:refs {:collection_id   (random-key "coll" 100)
-                                                       :creator_id      (random-key "u" 10)}}]]
-                         :dashboard-card [[300 {:refs {:card_id      (random-key "c" 100)
-                                                       :dashboard_id (random-key "d" 100)}}]]
-                         :dimension      [;; 20 with both IDs set
-                                          [20 {:refs {:field_id                (random-key "field" 1000)
-                                                      :human_readable_field_id (random-key "field" 1000)}}]
-                                          ;; 20 with just :field_id
-                                          [20 {:refs {:field_id                (random-key "field" 1000)
-                                                      :human_readable_field_id ::rs/omit}}]]
-                         :metric         [[30 {:refs {:table_id   (random-key "t" 100)
-                                                      :creator_id (random-key "u" 10)}}]]})
+      (test-gen/insert!
+        {:collection             [[100 {:refs {:personal_owner_id ::rs/omit}}]
+                                  [10  {:refs     {:personal_owner_id ::rs/omit}
+                                        :spec-gen {:namespace :snippets}}]]
+         :database               [[10]]
+         :table                  (into [] (for [db [:db0 :db1 :db2 :db3 :db4 :db5 :db6 :db7 :db8 :db9]]
+                                            [10 {:refs {:db_id db}}]))
+         :field                  (into [] (for [n     (range 100)
+                                                :let [table (keyword (str "t" n))]]
+                                            [10 {:refs {:table_id table}}]))
+         :core-user              [[10]]
+         :card                   [[100 {:refs (let [db (rand-int 10)
+                                                    t  (rand-int 10)]
+                                                {:database_id   (keyword (str "db" db))
+                                                 :table_id      (keyword (str "t" (+ t (* 10 db))))
+                                                 :collection_id (random-key "coll" 100)
+                                                 :creator_id    (random-key "u" 10)})}]]
+         :dashboard              [[100 {:refs {:collection_id   (random-key "coll" 100)
+                                               :creator_id      (random-key "u" 10)}}]]
+         :dashboard-card         [[300 {:refs {:card_id      (random-key "c" 100)
+                                               :dashboard_id (random-key "d" 100)}}]]
+         :dimension              [;; 20 with both IDs set
+                                  [20 {:refs {:field_id                (random-key "field" 1000)
+                                              :human_readable_field_id (random-key "field" 1000)}}]
+                                  ;; 20 with just :field_id
+                                  [20 {:refs {:field_id                (random-key "field" 1000)
+                                              :human_readable_field_id ::rs/omit}}]]
+         :metric                 [[30 {:refs {:table_id   (random-key "t" 100)
+                                              :creator_id (random-key "u" 10)}}]]
+         :native-query-snippet   [[10 {:refs {:creator_id    (random-key "u" 10)
+                                              :collection_id (random-key "coll" 10 100)}}]]})
       (let [extraction (into [] (extract/extract-metabase {}))
             entities   (reduce (fn [m entity]
                                  (update m (-> entity :serdes/meta last :model)
                                          (fnil conj []) entity))
                                {} extraction)]
-        (is (= 100 (-> entities (get "Collection") count)))
+        (is (= 110 (-> entities (get "Collection") count)))
 
         (testing "storage"
           (storage.yaml/store! (seq extraction) dump-dir)
           (testing "for Collections"
-            (is (= 100 (count (dir->file-set (io/file dump-dir "Collection")))))
+            (is (= 110 (count (dir->file-set (io/file dump-dir "Collection")))))
             (doseq [{:keys [entity_id slug] :as coll} (get entities "Collection")
                     :let [filename (str entity_id "+" (#'u.yaml/truncate-label slug) ".yaml")]]
               (is (= (dissoc coll :serdes/meta)
@@ -168,13 +174,23 @@
           (testing "for metrics"
             (is (= 30 (count (dir->file-set (io/file dump-dir "Metric")))))
             (doseq [{:keys [entity_id name] :as metric} (get entities "Metric")
-                    :let [filename (str entity_id "+" name ".yaml")]]
+                    :let [filename (str entity_id "+" (#'u.yaml/truncate-label name) ".yaml")]]
               (is (= (-> metric
                          (dissoc :serdes/meta)
                          (update :created_at u.date/format)
                          (update :updated_at u.date/format))
                      (yaml/from-file (io/file dump-dir "Metric" filename))))))
 
+          (testing "for native query snippets"
+            (is (= 10 (count (dir->file-set (io/file dump-dir "NativeQuerySnippet")))))
+            (doseq [{:keys [entity_id name] :as snippet} (get entities "NativeQuerySnippet")
+                    :let [filename (str entity_id "+" (#'u.yaml/truncate-label name) ".yaml")]]
+              (is (= (-> snippet
+                         (dissoc :serdes/meta)
+                         (update :created_at u.date/format)
+                         (update :updated_at u.date/format))
+                     (yaml/from-file (io/file dump-dir "NativeQuerySnippet" filename))))))
+
           (testing "for settings"
             (is (= (into {} (for [{:keys [key value]} (get entities "Setting")]
                               [key value]))

enterprise/backend/test/metabase_enterprise/serialization/v2/extract_test.clj

@@ -2,9 +2,10 @@
   (:require [clojure.test :refer :all]
             [metabase-enterprise.serialization.test-util :as ts]
             [metabase-enterprise.serialization.v2.extract :as extract]
-            [metabase.models :refer [Card Collection Dashboard DashboardCard Database Dimension Field Metric Table
-                                     User]]
-            [metabase.models.serialization.base :as serdes.base]))
+            [metabase.models :refer [Card Collection Dashboard DashboardCard Database Dimension Field Metric
+                                     NativeQuerySnippet Table User]]
+            [metabase.models.serialization.base :as serdes.base])
+  (:import java.time.ZonedDateTime))
 
 (defn- select-one [model-name where]
   (first (into [] (serdes.base/raw-reducible-query model-name {:where where}))))
@@ -130,6 +131,9 @@
                   :dataset_query "{\"json\": \"string values\"}"} ; Undecoded, still a string.
                  (select-keys ser [:serdes/meta :table_id :creator_id :collection_id :dataset_query])))
           (is (not (contains? ser :id)))
+          (is (instance? ZonedDateTime (:created_at ser)))
+          (is (or (nil? (:updated_at ser))
+                  (instance? ZonedDateTime (:updated_at ser))))
 
           (testing "cards depend on their Table and Collection"
             (is (= #{[{:model "Database"   :id "My Database"}
@@ -266,3 +270,50 @@
             (is (= #{[{:model "Database"   :id "My Database"}
                       {:model "Table"      :id "Schemaless Table"}]}
                    (set (serdes.base/serdes-dependencies ser))))))))))
+
+(deftest native-query-snippets-test
+  (ts/with-empty-h2-app-db
+    (ts/with-temp-dpc [User               [{ann-id       :id}        {:first_name "Ann"
+                                                                      :last_name  "Wilson"
+                                                                      :email      "ann@heart.band"}]
+                       Collection         [{coll-id     :id
+                                            coll-eid    :entity_id}  {:name              "Shared Collection"
+                                                                      :personal_owner_id nil
+                                                                      :namespace         :snippets}]
+                       NativeQuerySnippet [{s1-id       :id
+                                            s1-eid      :entity_id}  {:name          "Snippet 1"
+                                                                      :collection_id coll-id
+                                                                      :creator_id    ann-id}]
+                       NativeQuerySnippet [{s2-id       :id
+                                            s2-eid      :entity_id}  {:name          "Snippet 2"
+                                                                      :collection_id nil
+                                                                      :creator_id    ann-id}]]
+      (testing "native query snippets"
+        (testing "can belong to :snippets collections"
+          (let [ser (serdes.base/extract-one "NativeQuerySnippet" {} (select-one "NativeQuerySnippet" [:= :id s1-id]))]
+            (is (= {:serdes/meta             [{:model "NativeQuerySnippet" :id s1-eid :label "Snippet 1"}]
+                    :collection_id           coll-eid
+                    :creator_id              "ann@heart.band"}
+                   (select-keys ser [:serdes/meta :collection_id :creator_id])))
+            (is (not (contains? ser :id)))
+            (is (instance? ZonedDateTime (:created_at ser)))
+            (is (or (nil? (:updated_at ser))
+                    (instance? ZonedDateTime (:updated_at ser))))
+
+            (testing "and depend on the Collection"
+              (is (= #{[{:model "Collection" :id coll-eid}]}
+                     (set (serdes.base/serdes-dependencies ser)))))))
+
+        (testing "or can be outside collections"
+          (let [ser (serdes.base/extract-one "NativeQuerySnippet" {} (select-one "NativeQuerySnippet" [:= :id s2-id]))]
+            (is (= {:serdes/meta             [{:model "NativeQuerySnippet" :id s2-eid :label "Snippet 2"}]
+                    :collection_id           nil
+                    :creator_id              "ann@heart.band"}
+                   (select-keys ser [:serdes/meta :collection_id :creator_id])))
+            (is (not (contains? ser :id)))
+            (is (instance? ZonedDateTime (:created_at ser)))
+            (is (or (nil? (:updated_at ser))
+                    (instance? ZonedDateTime (:updated_at ser))))
+
+            (testing "and has no deps"
+              (is (empty? (serdes.base/serdes-dependencies ser))))))))))

src/metabase/actions.clj

@@ -6,6 +6,7 @@
    [metabase.driver :as driver]
    [metabase.mbql.normalize :as mbql.normalize]
    [metabase.mbql.schema :as mbql.s]
+   [metabase.mbql.util :as mbql.u]
    [metabase.models.database :refer [Database]]
    [metabase.models.setting :as setting]
    [metabase.util :as u]
@@ -37,7 +38,7 @@
 
 (defmulti normalize-action-arg-map
   "Normalize the `arg-map` passed to [[perform-action!]] for a specific `action`."
-  {:arglists '([action arg-map])}
+  {:arglists '([action arg-map]), :added "0.44.0"}
   (fn [action _arg-map]
     (keyword action)))
 
@@ -49,7 +50,7 @@
   "Return the appropriate spec to use to validate the arg map passed to [[perform-action!*]].
 
     (action-arg-map-spec :row/create) => :actions.args.crud/row.create"
-  {:arglists '([action])}
+  {:arglists '([action]), :added "0.44.0"}
   keyword)
 
 (defmethod action-arg-map-spec :default
@@ -73,7 +74,7 @@
 
   DON'T CALL THIS METHOD DIRECTLY TO PERFORM ACTIONS -- use [[perform-action!]] instead which does normalization,
   validation, and binds Database-local values."
-  {:arglists '([driver action database arg-map])}
+  {:arglists '([driver action database arg-map]), :added "0.44.0"}
   (fn [driver action _database _arg-map]
     [(driver/dispatch-on-initialized-driver driver)
      (keyword action)])
@@ -114,7 +115,7 @@
         spec    (action-arg-map-spec action)
         arg-map (normalize-action-arg-map action arg-map)]
     (when (s/invalid? (s/conform spec arg-map))
-      (throw (ex-info (format "Invalid Action arg map: %s" (s/explain-str spec arg-map))
+      (throw (ex-info (format "Invalid Action arg map for %s: %s" action (s/explain-str spec arg-map))
                       (s/explain-data spec arg-map))))
     ;; Check that Actions are enabled globally.
     (when-not (experimental-enable-actions)
@@ -122,7 +123,7 @@
                       {:status-code 400})))
     ;; Check that Actions are enabled for this specific Database.
     (let [{database-id :database}                         arg-map
-          {db-settings :settings, driver :engine, :as db} (Database database-id)]
+          {db-settings :settings, driver :engine, :as db} (api/check-404 (Database database-id))]
       ;; make sure the Driver supports Actions.
       (when-not (driver/database-supports? driver :actions db)
         (throw (ex-info (i18n/tru "{0} Database {1} does not support actions."
@@ -145,39 +146,57 @@
 
 ;;;; Action definitions.
 
-;;; Common base spec for all Actions. All Actions at least require
+;;; Common base spec for *all* Actions. All Actions at least require
 ;;;
-;;;    {:database <id>, :query {:source-table <id>}}
+;;;    {:database <id>}
+;;;
+;;; Anything else required depends on the action type.
 
 (s/def :actions.args/id
   (s/and integer? pos?))
 
-(s/def :actions.args.crud.common/database
+(s/def :actions.args.common/database
   :actions.args/id)
 
-(s/def :actions.args.crud.common.query/source-table
+(s/def :actions.args/common
+  (s/keys :req-un [:actions.args.common/database]))
+
+;;; Common base spec for all CRUD row Actions. All CRUD row Actions at least require
+;;;
+;;;    {:database <id>, :query {:source-table <id>}}
+
+(s/def :actions.args.crud.row.common.query/source-table
   :actions.args/id)
 
-(s/def :actions.args.crud.common/query
-  (s/keys :req-un [:actions.args.crud.common.query/source-table]))
+(s/def :actions.args.crud.row.common/query
+  (s/keys :req-un [:actions.args.crud.row.common.query/source-table]))
 
-(s/def :actions.args.crud/common
-  (s/keys :req-un [:actions.args.crud.common/database
-                   :actions.args.crud.common/query]))
+(s/def :actions.args.crud.row/common
+  (s/merge
+   :actions.args/common
+   (s/keys :req-un [:actions.args.crud.row.common/query])))
 
 ;;; the various `:row/*` Actions all treat their args map as an MBQL query.
 
-(defn- normalize-as-mbql-query [query]
-  (let [query (assoc (mbql.normalize/normalize query)
-                     :type :query)]
-    (try
-      (schema/validate mbql.s/Query query)
-      (catch Exception e
-        (throw (ex-info
-                (ex-message e)
-                {:exception-data (ex-data e)
-                 :status-code    400}))))
-    query))
+(defn- normalize-as-mbql-query
+  "Normalize `query` as an MBQL query. Optional arg `:exclude` is a set of *normalized* keys to exclude from recursive
+  normalization, e.g. `:create-row` for the `:row/create` Action (we don't want to normalize the row input since
+  preserving case and `snake_keys` in the request body is important)."
+  ([query]
+   (let [query (mbql.normalize/normalize (assoc query :type :query))]
+     (try
+       (schema/validate mbql.s/Query query)
+       (catch Exception e
+         (throw (ex-info
+                 (ex-message e)
+                 {:exception-data (ex-data e)
+                  :status-code    400}))))
+     query))
+
+  ([query & {:keys [exclude]}]
+   (let [query (update-keys query mbql.u/normalize-token)]
+     (merge (select-keys query exclude)
+            (normalize-as-mbql-query (apply dissoc query exclude))))))
 
 ;;;; `:row/create`
 
@@ -189,14 +208,14 @@
 
 (defmethod normalize-action-arg-map :row/create
   [_action query]
-  (normalize-as-mbql-query query))
+  (normalize-as-mbql-query query :exclude #{:create-row}))
 
 (s/def :actions.args.crud.row.create/create-row
   (s/map-of keyword? any?))
 
 (s/def :actions.args.crud/row.create
   (s/merge
-   :actions.args.crud/common
+   :actions.args.crud.row/common
    (s/keys :req-un [:actions.args.crud.row.create/create-row])))
 
 (defmethod action-arg-map-spec :row/create
@@ -213,14 +232,14 @@
 
 (defmethod normalize-action-arg-map :row/update
   [_action query]
-  (normalize-as-mbql-query query))
+  (normalize-as-mbql-query query :exclude #{:update-row}))
 
 (s/def :actions.args.crud.row.update.query/filter
   vector?) ; MBQL filter clause
 
 (s/def :actions.args.crud.row.update/query
   (s/merge
-   :actions.args.crud.common/query
+   :actions.args.crud.row.common/query
    (s/keys :req-un [:actions.args.crud.row.update.query/filter])))
 
 (s/def :actions.args.crud.row.update/update-row
@@ -228,7 +247,7 @@
 
 (s/def :actions.args.crud/row.update
   (s/merge
-   :actions.args.crud/common
+   :actions.args.crud.row/common
    (s/keys :req-un [:actions.args.crud.row.update/update-row
                     :actions.args.crud.row.update/query])))
 
@@ -252,14 +271,47 @@
 
 (s/def :actions.args.crud.row.delete/query
   (s/merge
-   :actions.args.crud.common/query
+   :actions.args.crud.row.common/query
    (s/keys :req-un [:actions.args.crud.row.delete.query/filter])))
 
 (s/def :actions.args.crud/row.delete
   (s/merge
-   :actions.args.crud/common
+   :actions.args.crud.row/common
    (s/keys :req-un [:actions.args.crud.row.delete/query])))
 
 (defmethod action-arg-map-spec :row/delete
   [_action]
   :actions.args.crud/row.delete)
+
+;;;; Bulk actions
+
+;;;; `:bulk/create`
+
+;;; For `bulk/create` the request body is to `POST /api/action/:action-namespace/:action-name/:table-id` is just a
+;;; vector of rows but the API endpoint itself calls [[perform-action!]] with
+;;;
+;;;    {:database <database-id>, :table-id <table-id>, :arg <request-body>}
+;;;
+;;; and we transform this to
+;;;
+;;;     {:database <database-id>, :table-id <table-id>, :rows <request-body>}
+
+(defmethod normalize-action-arg-map :bulk/create
+  [_action {:keys [database table-id], rows :arg, :as _arg-map}]
+  {:database database, :table-id table-id, :rows rows})
+
+(s/def :actions.args.crud.bulk.create/table-id
+  :actions.args/id)
+
+(s/def :actions.args.crud.bulk.create/rows
+  (s/cat :rows (s/+ (s/map-of keyword? any?))))
+
+(s/def :actions.args.crud.bulk/create
+  (s/merge
+   :actions.args/common
+   (s/keys :req-un [:actions.args.crud.bulk.create/table-id
+                    :actions.args.crud.bulk.create/rows])))
+
+(defmethod action-arg-map-spec :bulk/create
+  [_action]
+  :actions.args.crud.bulk/create)

src/metabase/api/action.clj

@@ -8,6 +8,7 @@
             [metabase.models.action :as action]
             [metabase.models.database :refer [Database]]
             [metabase.models.setting :as setting]
+            [metabase.models.table :as table]
             [metabase.util :as u]
             [metabase.util.i18n :as i18n :refer [trs]]
             [schema.core :as s]
@@ -23,7 +24,9 @@
   "Generic API endpoint for executing any sort of Action with source Table ID specified as part of the route."
   [action-namespace action-name table-id :as {:keys [body]}]
   (let [action (keyword action-namespace action-name)]
-    (actions/perform-action! action {:table-id table-id, :arg body})))
+    (actions/perform-action! action {:database (api/check-404 (table/table-id->database-id table-id))
+                                     :table-id table-id
+                                     :arg      body})))
 
 (defn check-actions-enabled
   "Check whether Actions are enabled and allowed for the [[metabase.models.database]] with `database-id`, or return a
@@ -64,6 +67,8 @@
   (db/delete! HTTPAction :action_id action-id)
   api/generic-204-no-content)
 
+;; TODO -- 99% sure these are busted. See https://github.com/metabase/metabase/issues/23935
+
 (api/defendpoint POST "/"
   "Create a new HTTP action."
   [action database]

src/metabase/driver/h2.clj

@@ -7,6 +7,7 @@
             [metabase.db.spec :as mdb.spec]
             [metabase.driver :as driver]
             [metabase.driver.common :as driver.common]
+            [metabase.driver.h2.actions :as h2.actions]
             [metabase.driver.sql-jdbc.connection :as sql-jdbc.conn]
             [metabase.driver.sql-jdbc.execute :as sql-jdbc.execute]
             [metabase.driver.sql-jdbc.sync :as sql-jdbc.sync]
@@ -21,6 +22,9 @@
   (:import [java.sql Clob ResultSet ResultSetMetaData]
            java.time.OffsetTime))
 
+;; method impls live in this namespace
+(comment h2.actions/keep-me)
+
 (driver/register! :h2, :parent :sql-jdbc)
 
 ;;; +----------------------------------------------------------------------------------------------------------------+

src/metabase/driver/h2/actions.clj

+(ns metabase.driver.h2.actions
+  "Method impls for [[metabase.driver.sql-jdbc.actions]] for `:h2`."
+  (:require [metabase.driver.sql-jdbc.actions :as sql-jdbc.actions]))
+
+(defmethod sql-jdbc.actions/base-type->sql-type-map :h2
+  [_driver]
+  {:type/BigInteger     "BIGINT"
+   :type/Boolean        "BOOL"
+   :type/Date           "DATE"
+   :type/DateTime       "DATETIME"
+   :type/DateTimeWithTZ "TIMESTAMP WITH TIME ZONE"
+   :type/Decimal        "DECIMAL"
+   :type/Float          "FLOAT"
+   :type/Integer        "INTEGER"
+   :type/Text           "VARCHAR"
+   :type/Time           "TIME"})
+
+;; H2 doesn't need to do anything special with nested transactions; the original transaction can proceed even if some
+;; specific statement errored.
+(defmethod sql-jdbc.actions/do-nested-transaction :h2
+  [_driver _conn thunk]
+  (thunk))

src/metabase/driver/postgres.clj

@@ -14,6 +14,7 @@
             [metabase.driver.common :as driver.common]
             [metabase.driver.ddl.interface :as ddl.i]
             [metabase.driver.ddl.postgres :as ddl.postgres]
+            [metabase.driver.postgres.actions :as postgres.actions]
             [metabase.driver.sql-jdbc.common :as sql-jdbc.common]
             [metabase.driver.sql-jdbc.connection :as sql-jdbc.conn]
             [metabase.driver.sql-jdbc.execute :as sql-jdbc.execute]
@@ -37,7 +38,9 @@
            metabase.util.honeysql_extensions.Identifier))
 
 (comment
-  ddl.postgres/keep-me)
+  ;; method impls live in these namespaces.
+  ddl.postgres/keep-me
+  postgres.actions/keep-me)
 
 (driver/register! :postgres, :parent :sql-jdbc)
 

src/metabase/driver/postgres/actions.clj

+(ns metabase.driver.postgres.actions
+  "Method impls for [[metabase.driver.sql-jdbc.actions]] for `:postgres`."
+  (:require
+   [clojure.java.jdbc :as jdbc]
+   [metabase.driver.sql-jdbc.actions :as sql-jdbc.actions]
+   [metabase.util.i18n :refer [tru]]))
+
+(defn- constraint->columns [conn constraint-name]
+  (->> ["select column_name from information_schema.constraint_column_usage where constraint_name = ?" constraint-name]
+       (jdbc/query conn {:identifers identity, :transaction? false})
+       (map :column_name)))
+
+(defn- violates-not-null-constraint [_conn error-message]
+  (let [[match? value column]
+        (re-find #"ERROR:\s+(\w+) value in column \"([^\"]+)\" violates not-null constraint" error-message)]
+    (when match?
+      [{:message (tru "{0} violates not-null constraint" value)
+        :column column}])))
+
+(defn- violates-unique-constraint [conn error-message]
+  (let [[match? constraint _value]
+        (re-find #"ERROR:\s+duplicate key value violates unique constraint \"([^\"]+)\"" error-message)]
+    (when match?
+      (let [columns (constraint->columns conn constraint)]
+        (mapv
+         (fn [column]
+           {:message (tru "violates unique constraint {0}" constraint)
+            :constraint constraint
+            :column column})
+         columns)))))
+
+(defn- update-or-delete-with-fk-constraint [conn error-message]
+  (let [[match? table constraint ref-table _columns _value]
+        (re-find #"ERROR:\s+update or delete on table \"([^\"]+)\" violates foreign key constraint \"([^\"]+)\" on table \"([^\"]+)\"" error-message)]
+    (when match?
+      (let [columns (constraint->columns conn constraint)]
+        (mapv
+         (fn [column]
+           {:message (tru "violates foreign key constraint {0}" constraint)
+            :table table
+            :ref-table ref-table
+            :constraint constraint
+            :column column})
+         columns)))))
+
+(defmethod sql-jdbc.actions/parse-sql-error :postgres
+  [_driver conn message]
+  (some #(% conn message)
+        [violates-not-null-constraint
+         violates-unique-constraint
+         update-or-delete-with-fk-constraint]))
+
+(defmethod sql-jdbc.actions/base-type->sql-type-map :postgres
+  [_driver]
+  {:type/BigInteger          "BIGINT"
+   :type/Boolean             "BOOL"
+   :type/Date                "DATE"
+   :type/DateTime            "TIMESTAMP"
+   :type/DateTimeWithTZ      "TIMESTAMP WITH TIME ZONE"
+   :type/DateTimeWithLocalTZ "TIMESTAMP WITH TIME ZONE"
+   :type/Decimal             "DECIMAL"
+   :type/Float               "FLOAT"
+   :type/Integer             "INTEGER"
+   :type/IPAddress           "INET"
+   :type/Text                "TEXT"
+   :type/Time                "TIME"
+   :type/TimeWithTZ          "TIME WITH TIME ZONE"
+   :type/UUID                "UUID"})
+
+;; For Postgres creating a Savepoint and rolling it back on error seems to be enough to let the parent transaction
+;; proceed if some particular statement encounters an error.
+(defmethod sql-jdbc.actions/do-nested-transaction :postgres
+  [_driver ^java.sql.Connection conn thunk]
+  (let [savepoint (.setSavepoint conn)]
+    (try
+      (thunk)
+      (catch Throwable e
+        (.rollback conn savepoint)
+        (throw e))
+      (finally
+        (.releaseSavepoint conn savepoint)))))

src/metabase/driver/sql_jdbc/connection.clj

@@ -20,7 +20,11 @@
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
 (defmulti connection-details->spec
-  "Given a Database `details-map`, return a JDBC connection spec."
+  "Given a Database `details-map`, return a JDBC connection spec.
+
+  DO NOT USE THIS METHOD DIRECTLY UNLESS YOU KNOW WHAT YOU ARE DOING! THIS RETURNS AN UNPOOLED CONNECTION SPEC! IF YOU
+  WANT A CONNECTION SPEC FOR RUNNING QUERIES USE [[db->pooled-connection-spec]] INSTEAD WHICH WILL RETURN A *POOLED*
+  CONNECTION SPEC."
   {:arglists '([driver details-map])}
   driver/dispatch-on-initialized-driver
   :hierarchy #'driver/hierarchy)

src/metabase/models/native_query_snippet.clj

@@ -2,7 +2,9 @@
   (:require [metabase.models.collection :as collection]
             [metabase.models.interface :as mi]
             [metabase.models.native-query-snippet.permissions :as snippet.perms]
+            [metabase.models.serialization.base :as serdes.base]
             [metabase.models.serialization.hash :as serdes.hash]
+            [metabase.models.serialization.util :as serdes.util]
             [metabase.util :as u]
             [metabase.util.i18n :refer [deferred-tru tru]]
             [metabase.util.schema :as su]
@@ -63,3 +65,40 @@
             (complement #(boolean (re-find #"^\s+" %)))
             (complement #(boolean (re-find #"}" %)))))
    (deferred-tru "snippet names cannot include '}' or start with spaces")))
+
+;;; ------------------------------------------------- Serialization --------------------------------------------------
+
+(defmethod serdes.base/extract-query "NativeQuerySnippet" [_ {:keys [user]}]
+  ;; TODO This join over the subset of collections this user can see is shared by a few things - factor it out?
+  (serdes.base/raw-reducible-query
+    "NativeQuerySnippet"
+    {:select     [:snippet.*]
+     :from       [[:native_query_snippet :snippet]]
+     :left-join  [[:collection :coll] [:= :coll.id :snippet.collection_id]]
+     :where      (if user
+                   ;; :snippet.collection_id is nullable, but this is a left join, so it works out neatly:
+                   ;; if this snippet has no collection, :coll.personal_owner_id is effectively NULL.
+                   [:or [:= :coll.personal_owner_id user] [:is :coll.personal_owner_id nil]]
+                   [:is :coll.personal_owner_id nil])}))
+
+(defmethod serdes.base/serdes-generate-path "NativeQuerySnippet" [_ snippet]
+  [(assoc (serdes.base/infer-self-path "NativeQuerySnippet" snippet)
+          :label (:name snippet))])
+
+(defmethod serdes.base/extract-one "NativeQuerySnippet"
+  [_ _ snippet]
+  (-> (serdes.base/extract-one-basics "NativeQuerySnippet" snippet)
+      (update :creator_id serdes.util/export-fk-keyed 'User :email)
+      (update :collection_id #(when % (serdes.util/export-fk % 'Collection)))))
+
+(defmethod serdes.base/load-xform "NativeQuerySnippet" [snippet]
+  (-> snippet
+      serdes.base/load-xform-basics
+      (update :creator_id serdes.util/import-fk-keyed 'User :email)
+      (update :collection_id #(when % (serdes.util/import-fk % 'Collection)))))
+
+(defmethod serdes.base/serdes-dependencies "NativeQuerySnippet"
+  [{:keys [collection_id]}]
+  (if collection_id
+    [[{:model "Collection" :id collection_id}]]
+    []))

src/metabase/models/serialization/base.clj

@@ -12,7 +12,8 @@
   (:require [clojure.tools.logging :as log]
             [metabase.models.serialization.hash :as serdes.hash]
             [toucan.db :as db]
-            [toucan.models :as models]))
+            [toucan.models :as models])
+  (:import [java.time Instant LocalDateTime OffsetDateTime ZoneId]))
 
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                              :serdes/meta                                                      |
@@ -216,19 +217,32 @@
 (defmethod extract-query :default [model-name _]
   (raw-reducible-query model-name))
 
+(defn- ->zoned-date-time
+  "Given an Instant, LocalDateTime, or ZonedDateTime, convert it to a ZonedDateTime at UTC.
+  LocalDateTime is treated as already being in UTC."
+  [t]
+  (cond
+    (instance? LocalDateTime  t) (.atZone            ^LocalDateTime  t (ZoneId/of "UTC"))
+    (instance? OffsetDateTime t) (.atZoneSameInstant ^OffsetDateTime t (ZoneId/of "UTC"))
+    (instance? Instant        t) (.atZone            ^Instant        t (ZoneId/of "UTC"))
+    :else                        t))
+
 (defn extract-one-basics
   "A helper for writing [[extract-one]] implementations. It takes care of the basics:
   - Convert to a vanilla Clojure map.
   - Add `:serdes/meta` by calling [[serdes-generate-path]].
   - Drop the primary key.
+  - Making :created_at and :updated_at into UTC-based LocalDateTimes.
 
   Returns the Clojure map."
   [model-name entity]
   (let [model (db/resolve-model (symbol model-name))
         pk    (models/primary-key model)]
-    (-> entity
-        (assoc :serdes/meta (serdes-generate-path model-name entity))
-        (dissoc pk))))
+    (cond-> entity
+      true (assoc :serdes/meta (serdes-generate-path model-name entity))
+      true (dissoc pk)
+      (:created_at entity) (update :created_at ->zoned-date-time)
+      (:updated_at entity) (update :updated_at ->zoned-date-time))))
 
 (defmethod extract-one :default [model-name _opts entity]
   (extract-one-basics model-name entity))

test/metabase/actions/test_util.clj

@@ -35,7 +35,7 @@
         (reset! db (mt/db))
         (thunk))
       (finally
-        (let [{driver :engine, db-id :id} @db]
+        (when-let [{driver :engine, db-id :id} @db]
           (tx/destroy-db! driver (tx/get-dataset-definition
                                   (data.impl/resolve-dataset-definition 'metabase.actions.test-util 'actions-test-data)))
           (db/delete! Database :id db-id))))))

test/metabase/actions_test.clj

+(ns metabase.actions-test
+  (:require
+   [clojure.test :refer :all]
+   [metabase.actions :as actions]))
+
+(deftest normalize-as-mbql-query-test
+  (testing "Make sure normalize-as-mbql-query can exclude certain keys from normalization"
+    (is (= {:database    1
+            :type        :query
+            :updated-row {:my_snake_case_column 1000
+                          "CamelCaseColumn"     {:ABC 200}}
+            :query       {:source-table 2}}
+           (#'actions/normalize-as-mbql-query
+            {"database"   1
+             :updated_row {:my_snake_case_column 1000
+                           "CamelCaseColumn"     {:ABC 200}}
+             :query       {"source_table" 2}}
+            :exclude #{:updated-row})))))

test/metabase/api/action_test.clj

@@ -53,57 +53,75 @@
               (is (schema= ExpectedGetCardActionAPIResponse
                            action)))))))))
 
-(defn- mock-requests []
-  [{:action       "action/row/create"
-    :request-body (assoc (mt/mbql-query categories) :create-row {:name "created_row"})
-    :expect-fn    (fn [result]
-                    ;; check that we return the entire row:
-                    (is (= "created_row" (get-in result [:created-row :name])))
-                    (is (= (set [:name :id])
-                           (set (keys (:created-row result))))))}
-   {:action       "action/row/update"
-    :request-body (assoc (mt/mbql-query categories {:filter [:= $id 1]})
-                         :update_row {:name "updated_row"})
-    :expected     {:rows-updated [1]}}
-   {:action       "action/row/delete"
-    :request-body (mt/mbql-query categories {:filter [:= $id 1]})
-    :expected     {:rows-deleted [1]}}
-   {:action       "action/row/update"
-    :request-body (assoc (mt/mbql-query categories {:filter [:= $id 10]})
-                         :update_row {:name "new-category-name"})
-    :expected     {:rows-updated [1]}}])
+(defn- format-field-name
+  "Format `field-name` appropriately for the current driver (e.g. uppercase it if we're testing against H2)."
+  [field-name]
+  (keyword (mt/format-name (name field-name))))
+
+(defn- categories-row-count []
+  (first (mt/first-row (mt/run-mbql-query categories {:aggregation [[:count]]}))))
+
+(deftest create-test
+  (testing "POST /api/action/row/create"
+    (mt/test-drivers (mt/normal-drivers-with-feature :actions)
+      (actions.test-util/with-actions-test-data-and-actions-enabled
+        (let [response (mt/user-http-request :crowberto :post 200
+                                             "action/row/create"
+                                             (assoc (mt/mbql-query categories) :create-row {(format-field-name :name) "created_row"}))]
+          (is (schema= {:created-row {(format-field-name :id)   (s/eq 76)
+                                      (format-field-name :name) (s/eq "created_row")}}
+                       response)
+              "Create should return the entire row")
+          (let [created-id (get-in response [:created-row (format-field-name :id)])]
+            (is (= "created_row" (-> (mt/rows (mt/run-mbql-query categories {:filter [:= $id created-id]})) last last))
+                "The record at created-id should now have its name set to \"created_row\"")))))))
 
-(deftest happy-path-test
-  (testing "Make sure it's possible to use known actions end-to-end if preconditions are satisfied"
-    (actions.test-util/with-actions-test-data-and-actions-enabled
-      (doseq [{:keys [action request-body expected expect-fn]} (mock-requests)]
-        (testing action
-          (let [result (mt/user-http-request :crowberto :post 200 action request-body)]
-            (when expected (is (= expected result)))
-            (when expect-fn (expect-fn result))))))))
-
-(deftest create-update-delete-test
-  (testing "Make sure actions are acting on rows."
-    (actions.test-util/with-actions-test-data-and-actions-enabled
-      (let [[create update delete] (mock-requests)
-            {created-id :id :as created-row}
-            (:created-row (mt/user-http-request :crowberto :post 200 (:action create) (:request-body create)))]
-        (is (= [:id :name] (keys created-row))
-            "Create should return the entire row")
-        (is (= "created_row" (:name created-row))
-            "Create should return the correct value for name")
-        (is (= "created_row" (-> (mt/rows (mt/run-mbql-query categories {:filter [:= $id created-id]})) last last))
-            "The record at created-id should now have its name set to \"created_row\"")
-        (is (= (:expected update) (mt/user-http-request :crowberto :post 200 (:action update)
-                                                        (assoc (mt/mbql-query categories {:filter [:= $id created-id]})
-                                                               :update_row {:name "updated_row"})))
+(deftest create-invalid-data-test
+  (testing "POST /api/action/row/create -- invalid data"
+    (mt/test-drivers (mt/normal-drivers-with-feature :actions)
+      (actions.test-util/with-actions-test-data-and-actions-enabled
+        (is (= 75
+               (categories-row-count)))
+        (is (schema= {:message  (s/constrained
+                                 s/Str
+                                 (case driver/*driver*
+                                   :h2       #(str/starts-with? % "Data conversion error converting \"created_row\"")
+                                   :postgres #(str/starts-with? % "ERROR: invalid input syntax for type integer: \"created_row\"")))
+                      s/Keyword s/Any}
+                     ;; bad data -- ID is a string instead of an Integer.
+                     (mt/user-http-request :crowberto :post 400
+                                           "action/row/create"
+                                           (assoc (mt/mbql-query categories) :create-row {(format-field-name :id) "created_row"}))))
+        (testing "no row should have been inserted"
+          (is (= 75
+                 (categories-row-count))))))))
+
+(deftest update-test
+  (testing "POST /api/action/row/update"
+    (mt/test-drivers (mt/normal-drivers-with-feature :actions)
+      (actions.test-util/with-actions-test-data-and-actions-enabled
+        (is (= {:rows-updated [1]}
+               (mt/user-http-request :crowberto :post 200
+                                     "action/row/update"
+                                     (assoc (mt/mbql-query categories {:filter [:= $id 50]})
+                                            :update_row {(format-field-name :name) "updated_row"})))
             "Update should return the right shape")
-        (is (= "updated_row" (-> (mt/rows (mt/run-mbql-query categories {:filter [:= $id created-id]})) last last))
-            "The row should actually be updated")
-        (is (= (:expected delete)
-               (mt/user-http-request :crowberto :post 200 (:action delete) (mt/mbql-query categories {:filter [:= $id created-id]})))
+        (is (= "updated_row"
+               (-> (mt/rows (mt/run-mbql-query categories {:filter [:= $id 50]})) last last))
+            "The row should actually be updated")))))
+
+(deftest delete-test
+  (testing "POST /api/action/row/delete"
+    (mt/test-drivers (mt/normal-drivers-with-feature :actions)
+      (actions.test-util/with-actions-test-data-and-actions-enabled
+        (is (= {:rows-deleted [1]}
+               (mt/user-http-request :crowberto :post 200
+                                     "action/row/delete"
+                                     (mt/mbql-query categories {:filter [:= $id 50]})))
             "Delete should return the right shape")
-        (is (= [] (mt/rows (mt/run-mbql-query categories {:filter [:= $id created-id]})))
+        (is (= 74
+               (categories-row-count)))
+        (is (= [] (mt/rows (mt/run-mbql-query categories {:filter [:= $id 50]})))
             "Selecting for deleted rows should return an empty result")))))
 
 ;; TODO: update test for this when we get something other than categories
@@ -114,6 +132,29 @@
           (let [request-body (mt/mbql-query categories {:filter [:= $id 22]})]
             (mt/user-http-request :crowberto :post 400 "action/row/delete" request-body))))))
 
+(defn- mock-requests
+  "Mock requests for testing validation for various actions. Don't use these for happy path tests! It's way too hard to
+  wrap your head around them. Use them for validating preconditions and stuff like that."
+  []
+  [{:action       "action/row/create"
+    :request-body (assoc (mt/mbql-query categories) :create-row {(format-field-name :name) "created_row"})
+    :expect-fn    (fn [result]
+                    ;; check that we return the entire row
+                    (is (schema= {:created-row {(format-field-name :id)   su/IntGreaterThanZero
+                                                (format-field-name :name) su/NonBlankString}}
+                                 result)))}
+   {:action       "action/row/update"
+    :request-body (assoc (mt/mbql-query categories {:filter [:= $id 1]})
+                         :update_row {(format-field-name :name) "updated_row"})
+    :expected     {:rows-updated [1]}}
+   {:action       "action/row/delete"
+    :request-body (mt/mbql-query categories {:filter [:= $id 1]})
+    :expected     {:rows-deleted [1]}}
+   {:action       "action/row/update"
+    :request-body (assoc (mt/mbql-query categories {:filter [:= $id 10]})
+                         :update_row {(format-field-name :name) "new-category-name"})
+    :expected     {:rows-updated [1]}}])
+
 (deftest feature-flags-test
   (testing "Disable endpoints unless both global and Database feature flags are enabled"
     (doseq [{:keys [action request-body]} (mock-requests)
@@ -168,28 +209,34 @@
                    (:message (mt/user-http-request :crowberto :post 400 action (dissoc request-body :query))))))))))
 
 (deftest row-update-action-gives-400-when-matching-more-than-one
-  (actions.test-util/with-actions-enabled
-    (let [query-that-returns-more-than-one (assoc (mt/mbql-query users {:filter [:>= $id 1]}) :update_row {:name "new-name"})
-          result-count (count (mt/rows (qp/process-query query-that-returns-more-than-one)))]
-      (is (< 1 result-count))
-      (doseq [{:keys [action]} (filter #(= "action/row/update" (:action %)) (mock-requests))
-              :when (not= action "action/row/create")] ;; the query in create is not used to select values to act upopn.
-        (is (re= #"Sorry, this would update [\d|,]+ rows, but you can only act on 1"
-                 (:message (mt/user-http-request :crowberto :post 400 action query-that-returns-more-than-one))))
-        (is (= result-count (count (mt/rows (qp/process-query query-that-returns-more-than-one))))
-            "The result-count after a rollback must remain the same!")))))
+  (mt/test-drivers (mt/normal-drivers-with-feature :actions)
+    (actions.test-util/with-actions-enabled
+      (let [query-that-returns-more-than-one (assoc (mt/mbql-query users {:filter [:>= $id 1]})
+                                                    :update_row {(format-field-name :name) "new-name"})
+            result-count                     (count (mt/rows (qp/process-query query-that-returns-more-than-one)))]
+        (is (< 1 result-count))
+        (doseq [{:keys [action]} (filter #(= "action/row/update" (:action %)) (mock-requests))
+                :when            (not= action "action/row/create")] ;; the query in create is not used to select values to act upopn.
+          (is (schema= {:message #"Sorry, this would update [\d|,]+ rows, but you can only act on 1"
+                        s/Keyword s/Any}
+                       (mt/user-http-request :crowberto :post 400 action query-that-returns-more-than-one)))
+          (is (= result-count (count (mt/rows (qp/process-query query-that-returns-more-than-one))))
+              "The result-count after a rollback must remain the same!"))))))
 
 (deftest row-delete-action-gives-400-when-matching-more-than-one
-  (actions.test-util/with-actions-enabled
-    (let [query-that-returns-more-than-one (assoc (mt/mbql-query checkins {:filter [:>= $id 1]}) :update_row {:name "new-name"})
-          result-count (count (mt/rows (qp/process-query query-that-returns-more-than-one)))]
-      (is (< 1 result-count))
-      (doseq [{:keys [action]} (filter #(= "action/row/delete" (:action %)) (mock-requests))
-              :when (not= action "action/row/create")] ;; the query in create is not used to select values to act upopn.
-        (is (re= #"Sorry, this would delete [\d|,]+ rows, but you can only act on 1"
-                 (:message (mt/user-http-request :crowberto :post 400 action query-that-returns-more-than-one))))
-        (is (= result-count (count (mt/rows (qp/process-query query-that-returns-more-than-one))))
-            "The result-count after a rollback must remain the same!")))))
+  (mt/test-drivers (mt/normal-drivers-with-feature :actions)
+    (actions.test-util/with-actions-enabled
+      (let [query-that-returns-more-than-one (assoc (mt/mbql-query checkins {:filter [:>= $id 1]})
+                                                    :update_row {(format-field-name :name) "new-name"})
+            result-count                     (count (mt/rows (qp/process-query query-that-returns-more-than-one)))]
+        (is (< 1 result-count))
+        (doseq [{:keys [action]} (filter #(= "action/row/delete" (:action %)) (mock-requests))
+                :when            (not= action "action/row/create")] ;; the query in create is not used to select values to act upopn.
+          (is (schema= {:message  #"Sorry, this would delete [\d|,]+ rows, but you can only act on 1"
+                        s/Keyword s/Any}
+                       (mt/user-http-request :crowberto :post 400 action query-that-returns-more-than-one)))
+          (is (= result-count (count (mt/rows (qp/process-query query-that-returns-more-than-one))))
+              "The result-count after a rollback must remain the same!"))))))
 
 (deftest unknown-row-action-gives-404
   (actions.test-util/with-actions-enabled
@@ -205,3 +252,56 @@
           (is (= "Failed to fetch Table 2,147,483,647: Table does not exist, or belongs to a different Database."
                  (:message (mt/user-http-request :crowberto :post 404 action
                                                  (assoc-in request-body [:query :source-table] Integer/MAX_VALUE))))))))))
+
+(deftest bulk-create-happy-path-test
+  (testing "POST /api/action/bulk/create/:table-id"
+    (mt/test-drivers (mt/normal-drivers-with-feature :actions)
+      (actions.test-util/with-actions-test-data-and-actions-enabled
+        (is (= 75
+               (categories-row-count)))
+        (is (= {:created-rows [{(format-field-name :id) 76, (format-field-name :name) "NEW_A"}
+                               {(format-field-name :id) 77, (format-field-name :name) "NEW_B"}]}
+               (mt/user-http-request :crowberto :post 200
+                                     (format "action/bulk/create/%d" (mt/id :categories))
+                                     [{(format-field-name :name) "NEW_A"}
+                                      {(format-field-name :name) "NEW_B"}])))
+        (is (= [[76 "NEW_A"]
+                [77 "NEW_B"]]
+               (mt/rows (mt/run-mbql-query categories {:filter   [:starts-with $name "NEW"]
+                                                       :order-by [[:asc $id]]}))))
+        (is (= 77
+               (categories-row-count)))))))
+
+(deftest bulk-create-failure-test
+  (testing "POST /api/action/bulk/create/:table-id"
+    (mt/test-drivers (mt/normal-drivers-with-feature :actions)
+      (actions.test-util/with-actions-test-data-and-actions-enabled
+        (testing "error in some of the rows in request body"
+          (is (= 75
+                 (categories-row-count)))
+          (testing "Should report indices of bad rows"
+            (is (schema= {:errors [(s/one {:index (s/eq 1)
+                                           :error (s/constrained
+                                                   s/Str
+                                                   (case driver/*driver*
+                                                     :h2       #(str/starts-with? % "NULL not allowed for column \"NAME\"")
+                                                     :postgres #(str/starts-with? % "ERROR: null value in column \"name\"")))}
+                                          "first error")
+                                   (s/one {:index (s/eq 3)
+                                           :error (s/constrained
+                                                   s/Str
+                                                   (case driver/*driver*
+                                                     :h2       #(str/starts-with? % "Data conversion error converting \"STRING\"")
+                                                     :postgres #(str/starts-with? % "ERROR: invalid input syntax for type integer: \"STRING\"")))}
+                                          "second error")]}
+                         (mt/user-http-request :crowberto :post 400
+                                               (format "action/bulk/create/%d" (mt/id :categories))
+                                               [{(format-field-name :name) "NEW_A"}
+                                                ;; invalid because name has to be non-nil
+                                                {(format-field-name :name) nil}
+                                                {(format-field-name :name) "NEW_B"}
+                                                ;; invalid because ID is supposed to be an integer
+                                                {(format-field-name :id) "STRING"}]))))
+          (testing "Should not have committed any of the valid rows"
+            (is (= 75
+                   (categories-row-count)))))))))

test/metabase/api/database_test.clj

@@ -299,7 +299,7 @@
                                                              :visibility_type   "normal"
                                                              :has_field_values  "list"
                                                              :database_position 1
-                                                             :database_required false})]
+                                                             :database_required true})]
                                      :segments     []
                                      :metrics      []
                                      :id           (mt/id :categories)

test/metabase/api/table_test.clj

@@ -435,19 +435,19 @@
              :display_name "Categories"
              :fields       [(merge
                              (field-details (Field (mt/id :categories :id)))
-                             {:table_id         (mt/id :categories)
+                             {:table_id          (mt/id :categories)
                               :semantic_type     "type/PK"
-                              :name             "ID"
-                              :display_name     "ID"
-                              :database_type    "BIGINT"
-                              :base_type        "type/BigInteger"
-                              :effective_type   "type/BigInteger"
-                              :has_field_values "none"
+                              :name              "ID"
+                              :display_name      "ID"
+                              :database_type     "BIGINT"
+                              :base_type         "type/BigInteger"
+                              :effective_type    "type/BigInteger"
+                              :has_field_values  "none"
                               :database_required false})
                             (merge
                              (field-details (Field (mt/id :categories :name)))
                              {:table_id                 (mt/id :categories)
-                              :semantic_type             "type/Name"
+                              :semantic_type            "type/Name"
                               :name                     "NAME"
                               :display_name             "Name"
                               :database_type            "VARCHAR"
@@ -458,7 +458,7 @@
                               :has_field_values         "list"
                               :database_position        1
                               :position                 1
-                              :database_required        false})]
+                              :database_required        true})]
              :id           (mt/id :categories)})
            (mt/user-http-request :rasta :get 200 (format "table/%d/query_metadata" (mt/id :categories)))))))
 

test/metabase/test/data.clj

@@ -180,9 +180,9 @@
   `(run-mbql-query* (mbql-query ~table-name ~(or query {}))))
 
 (defn format-name
-  "Format a SQL schema, table, or field identifier in the correct way for the current database by calling the driver's
-  implementation of `format-name`. (Most databases use the default implementation of `identity`; H2 uses
-  `clojure.string/upper-case`.) This function DOES NOT quote the identifier."
+  "Format a SQL schema, table, or field identifier in the correct way for the current database by calling the current
+  driver's implementation of [[ddl.i/format-name]]. (Most databases use the default implementation of `identity`; H2
+  uses [[clojure.string/upper-case]].) This function DOES NOT quote the identifier."
   [a-name]
   (assert ((some-fn keyword? string? symbol?) a-name)
     (str "Cannot format `nil` name -- did you use a `$field` without specifying its Table? (Change the form to"