MLv2 normalization overhaul (#40142)

* Wow

* Test fix 

* Fixes

* Actions should use strings for column names (fix :update-row and :create-row normalization)

* MLv2 schema should check against keys for the other query type

* Ok, have I fixed things?

* More fixes 

* Fix indentation

* Another round of test fixes. 

* Hopefully the last few test fixes 

* We need to test normalization for queries that have keyword keys as well.

* Fix Cljs i18n namespaces

* Sort namespaces

* Only test against H2

* Test fixes 

* Register MBQL clause schemas and test fixes 

* Test fixes and PR feedback

* Test fix

.clj-kondo/config.edn

@@ -526,6 +526,9 @@
    metabase.db.schema-migrations-test.impl/test-migrations                                                                   hooks.metabase.db.schema-migrations-test.impl/test-migrations
    metabase.dashboard-subscription-test/with-link-card-fixture-for-dashboard                                                 hooks.common/let-second
    metabase.driver.bigquery-cloud-sdk-test/calculate-bird-scarcity                                                           hooks.metabase.query-processor-test.expressions-test/calculate-bird-scarcity
+   metabase.lib.schema.mbql-clause/define-mbql-clause                                                                        hooks.metabase.lib.schema.mbql-clause/define-mbql-clause
+   metabase.lib.schema.mbql-clause/define-catn-mbql-clause                                                                   hooks.metabase.lib.schema.mbql-clause/define-mbql-clause
+   metabase.lib.schema.mbql-clause/define-tuple-mbql-clause                                                                  hooks.metabase.lib.schema.mbql-clause/define-mbql-clause
    metabase.lib.test-util.macros/$ids                                                                                        hooks.metabase.test.data/$ids
    metabase.lib.test-util.macros/mbql-query                                                                                  hooks.metabase.test.data/mbql-query
    metabase.lib.test-util.macros/with-testing-against-standard-queries                                                       hooks.metabase.lib.test-util.macros/with-testing-against-standard-queries

.clj-kondo/hooks/clojure/test.clj

@@ -28,7 +28,6 @@
      metabase.test.util.log/with-log-level
      metabase.test.util.log/with-log-messages-for-level
      metabase.test.util.misc/with-single-admin-user
-     metabase.test.util.timezone/with-system-timezone-id
      metabase.test.util/with-all-users-permission
      metabase.test.util/with-column-remappings
      metabase.test.util/with-discarded-collections-perms-changes
@@ -62,7 +61,6 @@
      metabase.test/with-non-admin-groups-no-root-collection-perms
      metabase.test/with-persistence-enabled
      metabase.test/with-single-admin-user
-     metabase.test/with-system-timezone-id
      metabase.test/with-temp-env-var-value
      metabase.test/with-temp-vals-in-db
      metabase.test/with-temporary-raw-setting-values

.clj-kondo/hooks/metabase/lib/schema/mbql_clause.clj

+(ns hooks.metabase.lib.schema.mbql-clause
+  (:require
+   [clj-kondo.hooks-api :as api]))
+
+(defn define-mbql-clause
+  "Register the schema keyword so LSP can jump to it."
+  [x]
+  (letfn [(update-def-keyword [k]
+            (-> (api/keyword-node (keyword "mbql.clause" (name (api/sexpr k))))
+                (with-meta (meta k))
+                (api/reg-keyword! 'metabase.lib.schema.mbql-clause/define-mbql-clause)))
+          (update-children [[_def k :as children]]
+            (if (api/keyword-node? k)
+              (update (vec children) 1 update-def-keyword)
+              children))
+          (update-node [node]
+            (if (api/list-node? node)
+              (-> (api/list-node (update-children (:children node)))
+                  (with-meta (meta node)))
+              node))]
+    (update x :node update-node)))

enterprise/backend/src/metabase_enterprise/advanced_config/api/logs.clj

@@ -40,7 +40,7 @@
              (deferred-tru "Must be a string like 2020-04 or 2222-11."))}
   (let [[year month] (mc/coerce [:tuple
                                  [:int {:title "year" :min 0 :max 9999}]
-                                 [:int {:title "month" :min 0 :max 12}]]
+                                 [:int {:title "month" :min 0 :max 12}]] ; month 0 ???
                                 (str/split yyyy-mm #"\-")
                                 (mtx/string-transformer))]
     (api/check-superuser)

enterprise/backend/test/metabase_enterprise/serialization/test_util.clj

@@ -202,8 +202,8 @@
                                                  :name root-card-name
                                                  :dataset_query {:type :query
                                                                  :database db-id
-                                                                 :query {:source-table table-id}
-                                                                 :expressions {"Price Known" [:> [:field numeric-field-id nil] 0]}}}
+                                                                 :query {:source-table table-id
+                                                                         :expressions  {"Price Known" [:> [:field numeric-field-id nil] 0]}}}}
                   Card       {card-id-nested :id} {:table_id table-id
                                                    :name "My Nested Card"
                                                    :collection_id collection-id

modules/drivers/bigquery-cloud-sdk/test/metabase/driver/bigquery_cloud_sdk/query_processor_test.clj

@@ -202,7 +202,7 @@
            (native-timestamp-query (mt/id) "2018-08-31 00:00:00" "UTC"))
         "A UTC date is returned, we should read/return it as UTC")
 
-    (test.tz/with-system-timezone-id "America/Chicago"
+    (test.tz/with-system-timezone-id! "America/Chicago"
       (t2.with-temp/with-temp [Database db {:engine  :bigquery-cloud-sdk
                                             :details (assoc (:details (mt/db))
                                                             :use-jvm-timezone true)}]
@@ -212,7 +212,7 @@
                  "is already in the JVM's timezone. The test puts the JVM's timezone into America/Chicago an ensures that "
                  "the correct date is compared"))))
 
-    (test.tz/with-system-timezone-id "Asia/Jakarta"
+    (test.tz/with-system-timezone-id! "Asia/Jakarta"
       (t2.with-temp/with-temp [Database db {:engine  :bigquery-cloud-sdk
                                             :details (assoc (:details (mt/db))
                                                             :use-jvm-timezone true)}]
@@ -314,13 +314,13 @@
       [:field "x" {:base-type :type/DateTime, :temporal-unit :day-of-week}] nil
       (meta/field-metadata :checkins :date)                                 :date)))
 
-(deftest ^:parallel reconcile-temporal-types-test
+(deftest reconcile-temporal-types-test
   (doseq [test-case (bigquery.qp.reconciliation-tu/test-cases)]
-    (testing (str \newline (u/pprint-to-str (list `bigquery.qp.reconciliation-tu/test-temporal-type-reconciliation test-case)))
-      (bigquery.qp.reconciliation-tu/test-temporal-type-reconciliation test-case))))
+    (testing (str \newline (u/pprint-to-str (list `bigquery.qp.reconciliation-tu/test-temporal-type-reconciliation! test-case)))
+      (bigquery.qp.reconciliation-tu/test-temporal-type-reconciliation! test-case))))
 
-(deftest ^:parallel reconcile-temporal-types-date-extraction-filters-test
-  (mt/with-report-timezone-id nil
+(deftest reconcile-temporal-types-date-extraction-filters-test
+  (mt/with-report-timezone-id! nil
     (qp.store/with-metadata-provider bigquery.qp.reconciliation-tu/mock-temporal-fields-metadata-provider
       (binding [*print-meta* true]
         (testing "\ndate extraction filters"
@@ -338,10 +338,10 @@
                      (sql.qp/->honeysql :bigquery-cloud-sdk [:= [:field (:id field) {:temporal-unit     :day-of-week
                                                                                      ::add/source-table "ABC"}] 1]))))))))))
 
-(deftest ^:parallel reconcile-unix-timestamps-test
+(deftest reconcile-unix-timestamps-test
   (testing "temporal type reconciliation should work for UNIX timestamps (#15376)"
     (mt/test-driver :bigquery-cloud-sdk
-      (mt/with-report-timezone-id nil
+      (mt/with-report-timezone-id! nil
         (mt/dataset test-data
           (qp.store/with-metadata-provider (lib.tu/merged-mock-metadata-provider
                                             (lib.metadata.jvm/application-database-metadata-provider (mt/id))
@@ -363,10 +363,10 @@
               (is (= :completed
                      (:status (qp/process-query query)))))))))))
 
-(deftest ^:parallel temporal-type-conversion-test
+(deftest temporal-type-conversion-test
   (mt/with-driver :bigquery-cloud-sdk
     (qp.store/with-metadata-provider (mt/id)
-      (mt/with-report-timezone-id "US/Pacific"
+      (mt/with-report-timezone-id! "US/Pacific"
         (let [temporal-string "2022-01-01"
               convert         (fn [from-t to-t]
                                 (->> (#'bigquery.qp/->temporal-type to-t (#'bigquery.qp/->temporal-type from-t temporal-string))
@@ -397,9 +397,9 @@
                 (is (= [(str (u/upper-case-en (name to-t)) "(TIMESTAMP(?, 'US/Pacific'), 'US/Pacific')") temporal-string]
                        (convert :timestamp to-t)))))))))))
 
-(deftest ^:parallel reconcile-relative-datetimes-test-1
+(deftest reconcile-relative-datetimes-test-1
   (mt/with-driver :bigquery-cloud-sdk
-    (mt/with-report-timezone-id nil
+    (mt/with-report-timezone-id! nil
       (qp.store/with-metadata-provider (mt/id)
         (testing "relative-datetime clauses on their own"
           (doseq [[t unit expected-sql]
@@ -419,12 +419,12 @@
                          (sql.qp/format-honeysql :bigquery-cloud-sdk
                                                  {:where hsql}))))))))))))
 
-(deftest ^:parallel reconcile-relative-datetimes-test-2
+(deftest reconcile-relative-datetimes-test-2
   (mt/with-driver :bigquery-cloud-sdk
     (qp.store/with-metadata-provider (mt/id)
       (testing "relative-datetime clauses on their own when a reporting timezone is set"
         (doseq [timezone ["UTC" "US/Pacific"]]
-          (mt/with-report-timezone-id timezone
+          (mt/with-report-timezone-id! timezone
             (doseq [[t [unit expected-sql]]
                     {:time      [:hour ["TIME_TRUNC("
                                         "  TIME_ADD(CURRENT_TIME('{{timezone}}'), INTERVAL -1 hour),"
@@ -619,13 +619,13 @@
                   (is (= [[0]]
                          (mt/rows (qp/process-query query)))))))))))))
 
-(deftest ^:parallel current-datetime-honeysql-form-test
+(deftest current-datetime-honeysql-form-test
   (mt/test-driver :bigquery-cloud-sdk
     (qp.store/with-metadata-provider (mt/id)
       (testing (str "The object returned by `current-datetime-honeysql-form` should be a magic object that can take on "
                     "whatever temporal type we want.")
         (doseq [report-timezone [nil "UTC"]]
-          (mt/with-report-timezone-id report-timezone
+          (mt/with-report-timezone-id! report-timezone
             (let [form (sql.qp/current-datetime-honeysql-form :bigquery-cloud-sdk)]
               (is (= nil
                      (#'bigquery.qp/temporal-type form))
@@ -645,12 +645,12 @@
                          (sql/format-expr (#'bigquery.qp/->temporal-type temporal-type form)))
                       "Should convert to the correct SQL"))))))))))
 
-(deftest ^:parallel current-datetime-honeysql-form-test-2
+(deftest current-datetime-honeysql-form-test-2
   (mt/test-driver :bigquery-cloud-sdk
     (qp.store/with-metadata-provider (mt/id)
       (testing (str "The object returned by `current-datetime-honeysql-form` should use the reporting timezone when set.")
         (doseq [timezone ["UTC" "US/Pacific"]]
-          (mt/with-report-timezone-id timezone
+          (mt/with-report-timezone-id! timezone
             (let [form (sql.qp/current-datetime-honeysql-form :bigquery-cloud-sdk)]
               (is (= ["CURRENT_TIMESTAMP()"]
                      (sql/format-expr form))
@@ -667,13 +667,13 @@
                          (sql/format-expr (#'bigquery.qp/->temporal-type temporal-type form)))
                       "Should specify the correct timezone in the SQL for non-timestamp functions"))))))))))
 
-(deftest ^:parallel add-interval-honeysql-form-test
+(deftest add-interval-honeysql-form-test
   ;; this doesn't test conversion to/from time because there's no unit we can use that works for all for. So we'll
   ;; just test the 3 that support `:day` and that should be proof the logic is working. (The code that actually uses
   ;; this is tested e2e by [[filter-by-relative-date-ranges-test]] anyway.)
   (mt/test-driver :bigquery-cloud-sdk
     (qp.store/with-metadata-provider (mt/id)
-      (mt/with-report-timezone-id nil
+      (mt/with-report-timezone-id! nil
         (doseq [initial-type [:date :datetime :timestamp]
                 :let         [form (sql.qp/add-interval-honeysql-form
                                     :bigquery-cloud-sdk
@@ -697,7 +697,7 @@
                        (sql/format-expr (#'bigquery.qp/->temporal-type new-type form)))
                     "Should convert to the correct SQL")))))))))
 
-(deftest ^:parallel filter-by-relative-date-ranges-test
+(deftest filter-by-relative-date-ranges-test
   (mt/with-driver :bigquery-cloud-sdk
     (testing "Make sure the SQL we generate for filters against relative-datetimes is typed correctly"
       (doseq [[field-type [unit expected-sql]]
@@ -719,7 +719,7 @@
                                                             :base-type      field-type
                                                             :effective-type field-type
                                                             :database-type  (name (bigquery.tx/base-type->bigquery-type field-type))})]})
-          (mt/with-report-timezone-id nil
+          (mt/with-report-timezone-id! nil
             (testing (format "%s field" field-type)
               (is (= [expected-sql]
                      (sql/format {:where (sql.qp/->honeysql
@@ -730,11 +730,11 @@
                                            [:relative-datetime -1 unit]])}
                                  {:dialect ::h2x/unquoted-dialect}))))))))))
 
-(deftest ^:parallel filter-by-relative-date-ranges-test-2
+(deftest filter-by-relative-date-ranges-test-2
   (mt/with-driver :bigquery-cloud-sdk
     (testing "Make sure the SQL we generate for filters against relative-datetimes uses the reporting timezone when set"
       (doseq [timezone ["UTC" "US/Pacific"]]
-        (mt/with-report-timezone-id timezone
+        (mt/with-report-timezone-id! timezone
           (letfn [(mock-metadata-provider [field-type]
                     (lib.tu/mock-metadata-provider
                      meta/metadata-provider
@@ -813,11 +813,11 @@
        :col      col-key
        :expected expected})))
 
-(defn- can-we-filter-against-relative-datetime? [field unit report-timezone]
+(defn- can-we-filter-against-relative-datetime?! [field unit report-timezone]
   (try
     (mt/test-driver :bigquery-cloud-sdk
       (mt/dataset attempted-murders
-        (mt/with-report-timezone-id report-timezone
+        (mt/with-report-timezone-id! report-timezone
           (mt/run-mbql-query attempts
             {:aggregation [[:count]]
              :filter      [:time-interval (mt/id :attempts field) :last unit]}))))
@@ -825,25 +825,25 @@
     (catch Throwable _
       false)))
 
-(defn- run-filter-test-table-tests-for-field [field report-timezone]
+(defn- run-filter-test-table-tests-for-field! [field report-timezone]
   (testing (str "Make sure filtering against relative date ranges works correctly regardless of underlying column "
                 "type (#11725)")
     (doseq [test-case       (test-table-seq filter-test-table)
             :when           (= (:row test-case) field)
             :let            [unit (:col test-case)]]
       (if (:expected test-case)
-        (is (can-we-filter-against-relative-datetime? field unit report-timezone))
-        (is (not (can-we-filter-against-relative-datetime? field unit report-timezone)))))))
+        (is (can-we-filter-against-relative-datetime?! field unit report-timezone))
+        (is (not (can-we-filter-against-relative-datetime?! field unit report-timezone)))))))
 
-(deftest ^:parallel filter-by-relative-date-ranges-e2e-time-test      (run-filter-test-table-tests-for-field :time nil))
-(deftest ^:parallel filter-by-relative-date-ranges-e2e-datetime-test  (run-filter-test-table-tests-for-field :datetime nil))
-(deftest ^:parallel filter-by-relative-date-ranges-e2e-date-test      (run-filter-test-table-tests-for-field :date nil))
-(deftest ^:parallel filter-by-relative-date-ranges-e2e-timestamp-test (run-filter-test-table-tests-for-field :datetime_tz nil))
+(deftest filter-by-relative-date-ranges-e2e-time-test      (run-filter-test-table-tests-for-field! :time nil))
+(deftest filter-by-relative-date-ranges-e2e-datetime-test  (run-filter-test-table-tests-for-field! :datetime nil))
+(deftest filter-by-relative-date-ranges-e2e-date-test      (run-filter-test-table-tests-for-field! :date nil))
+(deftest filter-by-relative-date-ranges-e2e-timestamp-test (run-filter-test-table-tests-for-field! :datetime_tz nil))
 
-(deftest ^:parallel filter-by-relative-date-ranges-e2e-time-report-timezone-test      (run-filter-test-table-tests-for-field :time "UTC"))
-(deftest ^:parallel filter-by-relative-date-ranges-e2e-datetime-report-timezone-test  (run-filter-test-table-tests-for-field :datetime "UTC"))
-(deftest ^:parallel filter-by-relative-date-ranges-e2e-date-report-timezone-test      (run-filter-test-table-tests-for-field :date "UTC"))
-(deftest ^:parallel filter-by-relative-date-ranges-e2e-timestamp-report-timezone-test (run-filter-test-table-tests-for-field :datetime_tz "UTC"))
+(deftest filter-by-relative-date-ranges-e2e-time-report-timezone-test      (run-filter-test-table-tests-for-field! :time "UTC"))
+(deftest filter-by-relative-date-ranges-e2e-datetime-report-timezone-test  (run-filter-test-table-tests-for-field! :datetime "UTC"))
+(deftest filter-by-relative-date-ranges-e2e-date-report-timezone-test      (run-filter-test-table-tests-for-field! :date "UTC"))
+(deftest filter-by-relative-date-ranges-e2e-timestamp-report-timezone-test (run-filter-test-table-tests-for-field! :datetime_tz "UTC"))
 
 ;; This is a table of different BigQuery column types -> temporal units we should be able to bucket them by for
 ;; breakout purposes.
@@ -854,11 +854,11 @@
    [:date        true     false   false true  true  true   true     true  false           false        true         true          true         true          true           true]
    [:datetime_tz true     true    true  true  true  true   true     true  true            true         true         true          true         true          true           true]])
 
-(defn- can-breakout? [field unit report-timezone]
+(defn- can-breakout?! [field unit report-timezone]
   (try
     (mt/test-driver :bigquery-cloud-sdk
       (mt/dataset attempted-murders
-        (mt/with-report-timezone-id report-timezone
+        (mt/with-report-timezone-id! report-timezone
           (mt/run-mbql-query attempts
             {:aggregation [[:count]]
              :breakout    [[:field (mt/id :attempts field) {:temporal-unit unit}]]}))))
@@ -866,25 +866,25 @@
     (catch Throwable _
       false)))
 
-(defn- run-breakout-test-table-tests-for-field
+(defn- run-breakout-test-table-tests-for-field!
   [field report-timezone]
   (testing "Make sure datetime breakouts like :minute-of-hour work correctly for different temporal types"
     (doseq [test-case (test-table-seq breakout-test-table)
             :when     (= (:row test-case) field)
             :let      [unit (:col test-case)]]
       (if (:expected test-case)
-        (is (can-breakout? field unit report-timezone))
-        (is (not (can-breakout? field unit report-timezone)))))))
+        (is (can-breakout?! field unit report-timezone))
+        (is (not (can-breakout?! field unit report-timezone)))))))
 
-(deftest ^:parallel breakout-by-bucketed-datetimes-e2e-time-test      (run-breakout-test-table-tests-for-field :time nil))
-(deftest ^:parallel breakout-by-bucketed-datetimes-e2e-datetime-test  (run-breakout-test-table-tests-for-field :datetime nil))
-(deftest ^:parallel breakout-by-bucketed-datetimes-e2e-date-test      (run-breakout-test-table-tests-for-field :date nil))
-(deftest ^:parallel breakout-by-bucketed-datetimes-e2e-timestamp-test (run-breakout-test-table-tests-for-field :datetime_tz nil))
+(deftest breakout-by-bucketed-datetimes-e2e-time-test      (run-breakout-test-table-tests-for-field! :time nil))
+(deftest breakout-by-bucketed-datetimes-e2e-datetime-test  (run-breakout-test-table-tests-for-field! :datetime nil))
+(deftest breakout-by-bucketed-datetimes-e2e-date-test      (run-breakout-test-table-tests-for-field! :date nil))
+(deftest breakout-by-bucketed-datetimes-e2e-timestamp-test (run-breakout-test-table-tests-for-field! :datetime_tz nil))
 
-(deftest ^:parallel breakout-by-bucketed-datetimes-e2e-time-report-timezone-test      (run-breakout-test-table-tests-for-field :time "UTC"))
-(deftest ^:parallel breakout-by-bucketed-datetimes-e2e-datetime-report-timezone-test  (run-breakout-test-table-tests-for-field :datetime "UTC"))
-(deftest ^:parallel breakout-by-bucketed-datetimes-e2e-date-report-timezone-test      (run-breakout-test-table-tests-for-field :date "UTC"))
-(deftest ^:parallel breakout-by-bucketed-datetimes-e2e-timestamp-report-timezone-test (run-breakout-test-table-tests-for-field :datetime_tz "UTC"))
+(deftest breakout-by-bucketed-datetimes-e2e-time-report-timezone-test      (run-breakout-test-table-tests-for-field! :time "UTC"))
+(deftest breakout-by-bucketed-datetimes-e2e-datetime-report-timezone-test  (run-breakout-test-table-tests-for-field! :datetime "UTC"))
+(deftest breakout-by-bucketed-datetimes-e2e-date-report-timezone-test      (run-breakout-test-table-tests-for-field! :date "UTC"))
+(deftest breakout-by-bucketed-datetimes-e2e-timestamp-report-timezone-test (run-breakout-test-table-tests-for-field! :datetime_tz "UTC"))
 
 (deftest ^:parallel string-escape-test
   (mt/test-driver :bigquery-cloud-sdk

modules/drivers/bigquery-cloud-sdk/test/metabase/driver/bigquery_cloud_sdk/query_processor_test/reconciliation_test_util.clj

@@ -181,11 +181,11 @@
                             (repeat (dec num-args) filter-value))]
     (sql.qp/->honeysql :bigquery-cloud-sdk filter-clause)))
 
-(defn test-temporal-type-reconciliation
+(defn test-temporal-type-reconciliation!
   [test-case]
   (mt/test-driver :bigquery-cloud-sdk
     (qp.store/with-metadata-provider mock-temporal-fields-metadata-provider
-      (mt/with-report-timezone-id nil
+      (mt/with-report-timezone-id! nil
         (binding [*print-meta* true]
           (when-let [test-case (expand-test-case test-case)]
             (is (= (temporal-type-reconciliation-expected-value test-case)

modules/drivers/druid/test/metabase/driver/druid/query_processor_test.clj

@@ -260,7 +260,7 @@
           (mt/with-temporary-setting-values [report-timezone "America/Los_Angeles"]
             (is (= expected
                    (table-rows-sample))))
-          (mt/with-system-timezone-id "America/Chicago"
+          (mt/with-system-timezone-id! "America/Chicago"
             (is (= expected
                    (table-rows-sample)))))))))
 
@@ -531,17 +531,17 @@
                         ["4"  155.0]]
               :columns ["venue_price" "Sum-41"]}
              (mt/rows+column-names
-               (druid-query
-                 {:aggregation [[:aggregation-options [:- [:sum $venue_price] 41] {:name "Sum-41"}]]
-                  :breakout    [$venue_price]})))))))
+              (druid-query
+                {:aggregation [[:aggregation-options [:- [:sum $venue_price] 41] {:name "Sum-41"}]]
+                 :breakout    [$venue_price]})))))))
 
 (deftest distinct-count-of-two-dimensions-test
   (mt/test-driver :druid
-    (is (= {:rows    [[98]]
+    (is (= {:rows    [[979]]
             :columns ["count"]}
            (mt/rows+column-names
-             (druid-query
-               {:aggregation [[:distinct [:+ $checkins.venue_category_name $checkins.venue_name]]]}))))))
+            (druid-query
+              {:aggregation [[:distinct [:+ $id $checkins.venue_price]]]}))))))
 
 (deftest metrics-inside-aggregation-clauses-test
   (mt/test-driver :druid

modules/drivers/googleanalytics/test/metabase/driver/googleanalytics/query_processor_test.clj

@@ -20,7 +20,7 @@
            (#'ga.qp/built-in-segment {:filter [:and [:segment 100] [:segment "ga::B"]]})))))
 
 (deftest filter-test
-  (mt/with-report-timezone-id nil
+  (mt/with-report-timezone-id! nil
     (testing "\nabsolute datetimes"
       (doseq [[filter-type expected] {:=  {:start-date "2019-11-18", :end-date "2019-11-18"}
                                       :<  {:end-date "2019-11-17"}
@@ -74,7 +74,7 @@
                         (#'ga.qp/parse-filter:interval [:= [:field 'field {:temporal-unit :week}] [:relative-datetime 0 :week]])))))
           (testing (str "\nthis week at Saturday 6PM local time (Saturday 11PM UTC) should be the same as this week "
                         "Saturday 8PM local time (Sunday 1 AM UTC)")
-            (mt/with-report-timezone-id "US/Eastern"
+            (mt/with-report-timezone-id! "US/Eastern"
               (doseq [system-timezone ["US/Eastern" "UTC"]]
                 (testing "\nGoogle Analytics should prefer report timezone (if set) to system timezone"
                   (testing (format "\nSystem timezone = %s" system-timezone)

modules/drivers/googleanalytics/test/metabase/driver/googleanalytics_test.clj

@@ -107,7 +107,7 @@
   (mt/with-database-timezone-id nil
     (testing "\nsystem timezone should not affect the queries that get generated"
       (doseq [system-timezone-id ["UTC" "US/Pacific"]]
-        (mt/with-system-timezone-id system-timezone-id
+        (mt/with-system-timezone-id! system-timezone-id
           (mt/with-clock (t/mock-clock (t/instant (t/zoned-date-time
                                                    (t/local-date "2019-11-18")
                                                    (t/local-time 0)
@@ -238,7 +238,7 @@
 (deftest almost-e2e-test-1
   ;; system timezone ID shouldn't affect generated query
   (doseq [system-timezone-id ["UTC" "US/Pacific"]]
-    (mt/with-system-timezone-id system-timezone-id
+    (mt/with-system-timezone-id! system-timezone-id
       (mt/with-clock (t/mock-clock (t/instant (t/zoned-date-time
                                                (t/local-date "2019-11-18")
                                                (t/local-time 0)
@@ -267,7 +267,7 @@
 
 (deftest almost-e2e-test-2
   (doseq [system-timezone-id ["UTC" "US/Pacific"]]
-    (mt/with-system-timezone-id system-timezone-id
+    (mt/with-system-timezone-id! system-timezone-id
       (mt/with-clock (t/mock-clock (t/instant (t/zoned-date-time
                                                (t/local-date "2019-11-18")
                                                (t/local-time 0)
@@ -282,7 +282,7 @@
 (deftest almost-e2e-test-3
   (testing "system timezone ID shouldn't affect generated query"
     (doseq [system-timezone-id ["UTC" "US/Pacific"]]
-      (mt/with-system-timezone-id system-timezone-id
+      (mt/with-system-timezone-id! system-timezone-id
         (mt/with-clock (t/mock-clock (t/instant (t/zoned-date-time
                                                  (t/local-date "2019-11-18")
                                                  (t/local-time 0)
@@ -339,7 +339,7 @@
 (deftest almost-e2e-time-interval-test
   (testing "Make sure filtering by the previous 4 months actually filters against the right months (#10701)"
     (doseq [system-timezone-id ["UTC" "US/Pacific"]]
-      (mt/with-system-timezone-id system-timezone-id
+      (mt/with-system-timezone-id! system-timezone-id
         (mt/with-clock (t/mock-clock (t/instant (t/zoned-date-time
                                                  (t/local-date "2019-11-18")
                                                  (t/local-time 0)

modules/drivers/mongo/test/metabase/driver/mongo/query_processor_test.clj

@@ -188,7 +188,7 @@
     (testing "Result timezone is respected when grouping by hour (#11149)"
       (mt/dataset attempted-murders
         (testing "Querying in UTC works"
-          (mt/with-system-timezone-id "UTC"
+          (mt/with-system-timezone-id! "UTC"
             (is (= [["2019-11-20T20:00:00Z" 1]
                     ["2019-11-19T00:00:00Z" 1]
                     ["2019-11-18T20:00:00Z" 1]
@@ -199,7 +199,7 @@
                                :order-by [[:desc [:field %datetime {:temporal-unit :hour}]]]
                                :limit 4}))))))
         (testing "Querying in Kathmandu works"
-          (mt/with-system-timezone-id "Asia/Kathmandu"
+          (mt/with-system-timezone-id! "Asia/Kathmandu"
             (is (= [["2019-11-21T01:00:00+05:45" 1]
                     ["2019-11-19T06:00:00+05:45" 1]
                     ["2019-11-19T02:00:00+05:45" 1]

modules/drivers/sqlserver/test/metabase/driver/sqlserver_test.clj

@@ -423,8 +423,8 @@
                               :filter [:= [:field %attempts.datetime {:base-type :type/DateTime}]]
                               :order-by [[:asc $id]]
                               :limit limit})]
-          (doseq [with-tz-setter [#'qp.test-util/do-with-report-timezone-id
-                                  #'test.tz/do-with-system-timezone-id
+          (doseq [with-tz-setter [#'qp.test-util/do-with-report-timezone-id!
+                                  #'test.tz/do-with-system-timezone-id!
                                   #'qp.test-util/do-with-database-timezone-id
                                   #'qp.test-util/do-with-results-timezone-id]
                   timezone ["UTC" "Pacific/Auckland"]]

src/metabase/actions.clj

@@ -2,21 +2,18 @@
   "Code related to the new writeback Actions."
   (:require
    [clojure.spec.alpha :as s]
-   [malli.core :as mc]
-   [malli.error :as me]
    [metabase.api.common :as api]
    [metabase.driver :as driver]
    [metabase.lib.metadata :as lib.metadata]
+   [metabase.lib.schema.actions :as lib.schema.actions]
    [metabase.mbql.normalize :as mbql.normalize]
-   [metabase.mbql.schema :as mbql.s]
-   [metabase.mbql.util :as mbql.u]
    [metabase.models :refer [Database]]
    [metabase.models.setting :as setting]
-   [metabase.query-processor.error-type :as qp.error-type]
    [metabase.query-processor.middleware.permissions :as qp.perms]
    [metabase.query-processor.store :as qp.store]
    [metabase.util :as u]
    [metabase.util.i18n :as i18n]
+   [metabase.util.malli :as mu]
    [toucan2.core :as t2]))
 
 (setting/defsetting database-enable-actions
@@ -147,12 +144,15 @@
   [action-or-id]
   (check-actions-enabled-for-database! (api/check-404 (database-for-action action-or-id))))
 
-(defn perform-action!
+(mu/defn perform-action!
   "Perform an `action`. Invoke this function for performing actions, e.g. in API endpoints;
   implement [[perform-action!*]] to add support for a new driver/action combo. The shape of `arg-map` depends on the
   `action` being performed. [[action-arg-map-spec]] returns the specific spec used to validate `arg-map` for a given
   `action`."
-  [action arg-map]
+  [action
+   arg-map :- [:map
+               [:create-row {:optional true} [:maybe ::lib.schema.actions/row]]
+               [:update-row {:optional true} [:maybe ::lib.schema.actions/row]]]]
   (let [action  (keyword action)
         spec    (action-arg-map-spec action)
         arg-map (normalize-action-arg-map action arg-map)] ; is arg-map always just a regular query?
@@ -199,25 +199,6 @@
    :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
-  "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))]
-     (when-let [error (me/humanize (mc/explain mbql.s/Query query))]
-       (throw (ex-info
-               (i18n/tru "Invalid query: {0}" (pr-str error))
-               {:status-code 400, :type qp.error-type/invalid-query})))
-     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`
 
 ;;; row/create requires at least
@@ -228,10 +209,10 @@
 
 (defmethod normalize-action-arg-map :row/create
   [_action query]
-  (normalize-as-mbql-query query :exclude #{:create-row}))
+  (mbql.normalize/normalize-or-throw query))
 
 (s/def :actions.args.crud.row.create/create-row
-  (s/map-of keyword? any?))
+  (s/map-of string? any?))
 
 (s/def :actions.args.crud/row.create
   (s/merge
@@ -252,7 +233,7 @@
 
 (defmethod normalize-action-arg-map :row/update
   [_action query]
-  (normalize-as-mbql-query query :exclude #{:update-row}))
+  (mbql.normalize/normalize-or-throw query))
 
 (s/def :actions.args.crud.row.update.query/filter
   vector?) ; MBQL filter clause
@@ -263,7 +244,7 @@
    (s/keys :req-un [:actions.args.crud.row.update.query/filter])))
 
 (s/def :actions.args.crud.row.update/update-row
-  (s/map-of keyword? any?))
+  (s/map-of string? any?))
 
 (s/def :actions.args.crud/row.update
   (s/merge
@@ -284,7 +265,7 @@
 
 (defmethod normalize-action-arg-map :row/delete
   [_action query]
-  (normalize-as-mbql-query query))
+  (mbql.normalize/normalize-or-throw query))
 
 (s/def :actions.args.crud.row.delete.query/filter
   vector?) ; MBQL filter clause

src/metabase/actions/execution.clj

@@ -6,6 +6,9 @@
    [metabase.actions.http-action :as http-action]
    [metabase.analytics.snowplow :as snowplow]
    [metabase.api.common :as api]
+   [metabase.lib.schema.actions :as lib.schema.actions]
+   [metabase.lib.schema.id :as lib.schema.id]
+   [metabase.mbql.schema :as mbql.s]
    [metabase.models :refer [Card DashboardCard Database Table]]
    [metabase.models.action :as action]
    [metabase.models.persisted-info :as persisted-info]
@@ -18,6 +21,7 @@
    [metabase.util :as u]
    [metabase.util.i18n :refer [tru]]
    [metabase.util.log :as log]
+   [metabase.util.malli :as mu]
    [toucan2.core :as t2]))
 
 (defn- execute-query-action!
@@ -96,7 +100,13 @@
                 :parameters             request-parameters
                 :destination-parameters destination-param-ids})))
 
-(defn- build-implicit-query
+(mu/defn ^:private build-implicit-query :- [:map
+                                            [:query          ::mbql.s/Query]
+                                            [:row-parameters ::lib.schema.actions/row]
+                                            ;; TODO -- the schema for these should probably be
+                                            ;; `:metabase.lib.schema.parameter/parameter` instead of `:any`, but I'm not
+                                            ;; 100% sure about that.
+                                            [:prefetch-parameters {:optional true} [:tuple :any]]]
   [{:keys [model_id parameters] :as _action} implicit-action request-parameters]
   (let [{database-id :db_id
          table-id :id :as table} (implicit-action-table model_id)
@@ -107,38 +117,37 @@
                                       (into {})
                                       (m/filter-keys (set (map :id parameters))))
         _                        (api/check (action/unique-field-slugs? table-fields)
-                                   400
-                                   (tru "Cannot execute implicit action on a table with ambiguous column names."))
+                                            400
+                                            (tru "Cannot execute implicit action on a table with ambiguous column names."))
         _                        (api/check (= (count pk-fields) 1)
-                                   400
-                                   (tru "Must execute implicit action on a table with a single primary key."))
+                                            400
+                                            (tru "Must execute implicit action on a table with a single primary key."))
         _                        (check-no-extra-parameters request-parameters (keys slug->field-name))
         pk-field                 (first pk-fields)
         ;; Ignore params with nil values; the client doesn't reliably omit blank, optional parameters from the
         ;; request. See discussion at #29049
-        simple-parameters        (->> (update-keys request-parameters (comp keyword slug->field-name))
+        simple-parameters        (->> (update-keys request-parameters slug->field-name)
                                       (filter (fn [[_k v]] (some? v)))
                                       (into {}))
-        pk-field-name            (keyword (:name pk-field))
+        pk-field-name            (:name pk-field)
         row-parameters           (cond-> simple-parameters
                                    (not= implicit-action :row/create) (dissoc pk-field-name))
-        requires_pk              (contains? #{:row/delete :row/update} implicit-action)]
-    (api/check (or (not requires_pk)
+        requires-pk?             (contains? #{:row/delete :row/update} implicit-action)]
+    (api/check (or (not requires-pk?)
                    (some? (get simple-parameters pk-field-name)))
                400
                (tru "Missing primary key parameter: {0}"
                     (pr-str (u/slugify (:name pk-field)))))
-    (cond->
-      {:query {:database database-id,
-               :type :query,
-               :query {:source-table table-id}}
-       :row-parameters row-parameters}
+    (cond-> {:query {:database database-id,
+                     :type :query,
+                     :query {:source-table table-id}}
+             :row-parameters row-parameters}
 
-      requires_pk
+      requires-pk?
       (assoc-in [:query :query :filter]
                 [:= [:field (:id pk-field) nil] (get simple-parameters pk-field-name)])
 
-      requires_pk
+      requires-pk?
       (assoc :prefetch-parameters [{:target [:dimension [:field (:id pk-field) nil]]
                                     :type "id"
                                     :value [(get simple-parameters pk-field-name)]}]))))
@@ -159,7 +168,7 @@
     (binding [qp.perms/*card-id* (:model_id action)]
       (actions/perform-action! implicit-action arg-map))))
 
-(defn execute-action!
+(mu/defn execute-action!
   "Execute the given action with the given parameters of shape `{<parameter-id> <value>}."
   [action request-parameters]
   (let [;; if a value is supplied for a hidden parameter, it should raise an error
@@ -186,10 +195,12 @@
       (execute-custom-action action request-parameters)
       (throw (ex-info (tru "Unknown action type {0}." (name (:type action))) action)))))
 
-(defn execute-dashcard!
+(mu/defn execute-dashcard!
   "Execute the given action in the dashboard/dashcard context with the given parameters
    of shape `{<parameter-id> <value>}."
-  [dashboard-id dashcard-id request-parameters]
+  [dashboard-id       :- ::lib.schema.id/dashboard
+   dashcard-id        :- ::lib.schema.id/dashcard
+   request-parameters :- [:maybe [:map-of :string :any]]]
   (let [dashcard (api/check-404 (t2/select-one DashboardCard
                                                :id dashcard-id
                                                :dashboard_id dashboard-id))

src/metabase/api/common.clj

@@ -123,7 +123,7 @@
 
     (check test1 code1 message1
            test2 code2 message2)"
-  {:style/indent 1, :arglists '([condition [code message] & more] [condition code message & more])}
+  {:style/indent [:form], :arglists '([condition [code message] & more] [condition code message & more])}
   [condition & args]
   (let [[code message & more] (if (sequential? (first args))
                                 (concat (first args) (rest args))

src/metabase/api/dashboard.clj

@@ -13,6 +13,7 @@
    [metabase.automagic-dashboards.populate :as populate]
    [metabase.email.messages :as messages]
    [metabase.events :as events]
+   [metabase.lib.schema.parameter :as lib.schema.parameter]
    [metabase.mbql.normalize :as mbql.normalize]
    [metabase.mbql.schema :as mbql.s]
    [metabase.mbql.util :as mbql.u]
@@ -918,7 +919,7 @@
     (get-in card [:dataset_query :native :template-tags (u/qualified-name tag)])))
 
 (defn- param-type->op [type]
-  (if (get-in mbql.s/parameter-types [type :operator])
+  (if (get-in lib.schema.parameter/types [type :operator])
     (keyword (name type))
     :=))
 
@@ -1133,10 +1134,10 @@
   [dashboard-id dashcard-id :as {{:keys [parameters], :as _body} :body}]
   {dashboard-id ms/PositiveInt
    dashcard-id  ms/PositiveInt
-   parameters  [:maybe [:map-of :keyword :any]]}
+   parameters  [:maybe [:map-of :string :any]]}
   (api/read-check :model/Dashboard dashboard-id)
   ;; Undo middleware string->keyword coercion
-  (actions.execution/execute-dashcard! dashboard-id dashcard-id (update-keys parameters name)))
+  (actions.execution/execute-dashcard! dashboard-id dashcard-id parameters))
 
 ;;; ---------------------------------- Running the query associated with a Dashcard ----------------------------------
 

src/metabase/api/dataset.clj

@@ -10,8 +10,8 @@
    [metabase.driver.util :as driver.u]
    [metabase.events :as events]
    [metabase.lib.schema.id :as lib.schema.id]
+   [metabase.lib.schema.info :as lib.schema.info]
    [metabase.mbql.normalize :as mbql.normalize]
-   [metabase.mbql.schema :as mbql.s]
    [metabase.models.card :refer [Card]]
    [metabase.models.database :as database :refer [Database]]
    [metabase.models.params.custom-values :as custom-values]
@@ -98,7 +98,7 @@
   "Schema for valid export formats for downloading query results."
   (into [:enum] export-formats))
 
-(mu/defn export-format->context :- mbql.s/Context
+(mu/defn export-format->context :- ::lib.schema.info/context
   "Return the `:context` that should be used when saving a QueryExecution triggered by a request to download results
   in `export-format`.
 

src/metabase/driver.clj

@@ -115,7 +115,7 @@
 (add-watch
  #'hierarchy
  nil
- (fn [_ _ _ _]
+ (fn [_key _ref _old-state _new-state]
    (when (not= hierarchy driver.impl/hierarchy)
      ;; this is a dev-facing error so no need to i18n it.
      (throw (Exception. (str "Don't alter #'metabase.driver/hierarchy directly, since it is imported from "

src/metabase/driver/common/parameters/dates.clj

@@ -4,6 +4,7 @@
    [clojure.string :as str]
    [java-time.api :as t]
    [medley.core :as m]
+   [metabase.lib.schema.parameter :as lib.schema.parameter]
    [metabase.mbql.schema :as mbql.s]
    [metabase.mbql.util :as mbql.u]
    [metabase.models.params :as params]
@@ -26,7 +27,7 @@
 (mu/defn date-type?
   "Is param type `:date` or some subtype like `:date/month-year`?"
   [param-type :- :keyword]
-  (= (get-in mbql.s/parameter-types [param-type :type]) :date))
+  (= (get-in lib.schema.parameter/types [param-type :type]) :date))
 
 (defn not-single-date-type?
   "Does date `param-type` represent a range of dates, rather than a single absolute date? (The value may be relative,

src/metabase/driver/common/parameters/operators.clj

@@ -8,6 +8,7 @@
                {:source-field 5}]]
      :value [3 5]}"
   (:require
+   [metabase.lib.schema.parameter :as lib.schema.parameter]
    [metabase.mbql.schema :as mbql.s]
    [metabase.models.params :as params]
    [metabase.query-processor.error-type :as qp.error-type]
@@ -16,7 +17,7 @@
 
 (mu/defn ^:private operator-arity :- [:maybe [:enum :unary :binary :variadic]]
   [param-type]
-  (get-in mbql.s/parameter-types [param-type :operator]))
+  (get-in lib.schema.parameter/types [param-type :operator]))
 
 (defn operator?
   "Returns whether param-type is an \"operator\" type."