diff --git a/src/metabase/lib/schema/common.cljc b/src/metabase/lib/schema/common.cljc
index 5388a007ebd82109e545e1d7747a2f3ffbe27caa..92505670e0d26f4cf956c805bd88b6e7d9d7b034 100644
--- a/src/metabase/lib/schema/common.cljc
+++ b/src/metabase/lib/schema/common.cljc
@@ -10,7 +10,9 @@
(mr/def ::non-blank-string
[:and
[:string {:min 1}]
- [:fn (complement str/blank?)]])
+ [:fn
+ {:error/message "non-blank string"}
+ (complement str/blank?)]])
;;; Schema representing an integer than must also be greater than or equal to zero.
(mr/def ::int-greater-than-or-equal-to-zero
diff --git a/src/metabase/lib/schema/expression/temporal.cljc b/src/metabase/lib/schema/expression/temporal.cljc
index 76129a5539fc2f21f4a015bd3e25eded4bb94d94..92e358cd84a9b9947c452903c6e6f8add63ce730 100644
--- a/src/metabase/lib/schema/expression/temporal.cljc
+++ b/src/metabase/lib/schema/expression/temporal.cljc
@@ -7,7 +7,19 @@
[metabase.lib.schema.literal :as literal]
[metabase.lib.schema.mbql-clause :as mbql-clause]
[metabase.lib.schema.temporal-bucketing :as temporal-bucketing]
- [metabase.util.malli.registry :as mr]))
+ [metabase.util.malli.registry :as mr])
+ #?@
+ (:clj
+ [(:import
+ (java.time ZoneId))]
+ :cljs
+ [(:require
+ ["moment" :as moment]
+ ["moment-timezone" :as mtz])]))
+
+#?(:cljs
+ ;; so the moment-timezone stuff gets loaded
+ (comment mtz/keep-me))
(mbql-clause/define-tuple-mbql-clause :interval :- :type/Interval
:int
@@ -34,19 +46,35 @@
:get-day :get-day-of-week
:get-month :get-quarter :get-year}]
(mbql-clause/define-tuple-mbql-clause temporal-extract-op :- :type/Integer
- #_:datetime [:schema [:ref ::expression/temporal]]))
-
-(mbql-clause/define-tuple-mbql-clause :get-week :- :type/Integer
- #_:datetime [:schema [:ref ::expression/temporal]]
- ;; TODO should this be in the options map?
- #_:mode [:maybe [:enum :iso :us :instance]])
-
-(mbql-clause/define-tuple-mbql-clause :convert-timezone
- #_:datetime [:schema [:ref ::expression/temporal]]
- ;; TODO could be better specified - perhaps with a build time macro to inline the timezones?
- ;; NOT expressions?
- #_:target [:string]
- #_:source [:maybe [:string]])
+ #_:datetime [:schema [:ref ::expression/temporal]]))
+
+(mr/def ::get-week-mode
+ [:enum :iso :us :instance])
+
+(mbql-clause/define-catn-mbql-clause :get-week :- :type/Integer
+ [:datetime [:schema [:ref ::expression/temporal]]]
+ ;; TODO : the mode should probably go in the options map in modern MBQL rather than have it be a separate positional
+ ;; argument. But we can't refactor everything in one go, so that will have to be a future refactor.
+ [:mode [:? [:schema [:ref ::get-week-mode]]]])
+
+(mr/def ::timezone-id
+ [:and
+
+ ::common/non-blank-string
+ (into [:enum
+ {:error/message "valid timezone ID"
+ :error/fn (fn [{:keys [value]} _]
+ (str "invalid timezone ID: " (pr-str value)))}]
+ (sort
+ #?( ;; 600 timezones on java 17
+ :clj (ZoneId/getAvailableZoneIds)
+ ;; 596 timezones on moment-timezone 0.5.38
+ :cljs (.names (.-tz moment)))))])
+
+(mbql-clause/define-catn-mbql-clause :convert-timezone
+ [:datetime [:schema [:ref ::expression/temporal]]]
+ [:target [:schema [:ref ::timezone-id]]]
+ [:source [:? [:schema [:ref ::timezone-id]]]])
(lib.hierarchy/derive :convert-timezone :lib.type-of/type-is-type-of-first-arg)
diff --git a/test/metabase/lib/schema/expression/temporal_test.cljc b/test/metabase/lib/schema/expression/temporal_test.cljc
index 2b58ecc2728ca4134f489cf635aeec88b302087e..16072df9d1e93e2db4d2a4e4b302f771f7a13c33 100644
--- a/test/metabase/lib/schema/expression/temporal_test.cljc
+++ b/test/metabase/lib/schema/expression/temporal_test.cljc
@@ -4,7 +4,8 @@
[malli.core :as mc]
[malli.error :as me]
[metabase.lib.schema]
- [metabase.lib.schema.expression :as expression]))
+ [metabase.lib.schema.expression :as expression]
+ [metabase.lib.schema.expression.temporal :as temporal]))
(comment metabase.lib.schema/keep-me)
@@ -68,3 +69,64 @@
[:relative-datetime {:lib/uuid "00000000-0000-0000-0000-000000000000"} :current :day]
[:relative-datetime {:lib/uuid "00000000-0000-0000-0000-000000000000"} :current :minute]
[:relative-datetime {:lib/uuid "00000000-0000-0000-0000-000000000000"} :current]))
+
+(deftest ^:parallel timezone-id-test
+ (are [input error] (= error
+ (me/humanize (mc/explain ::temporal/timezone-id input)))
+ "US/Pacific" nil
+ "US/Specific" ["invalid timezone ID: \"US/Specific\""]
+ "" ["should be at least 1 characters" "non-blank string" "invalid timezone ID: \"\""]
+ " " ["non-blank string" "invalid timezone ID: \" \""]
+ nil ["should be a string" "non-blank string" "invalid timezone ID: nil"]))
+
+(deftest ^:parallel convert-timezone-test
+ (are [clause error] (= error
+ (me/humanize (mc/explain :mbql.clause/convert-timezone clause)))
+ ;; with both target and source timezone
+ [:convert-timezone
+ {:lib/uuid "00000000-0000-0000-0000-000000000000"}
+ [:field {:lib/uuid "00000000-0000-0000-0000-000000000000"} 1]
+ "Asia/Seoul"
+ "US/Pacific"]
+ nil
+
+ ;; with just the target timezone
+ [:convert-timezone
+ {:lib/uuid "00000000-0000-0000-0000-000000000000"}
+ [:field {:lib/uuid "00000000-0000-0000-0000-000000000000"} 1]
+ "Asia/Seoul"]
+ nil
+
+ ;; source cannot be nil
+ [:convert-timezone
+ {:lib/uuid "00000000-0000-0000-0000-000000000000"}
+ [:field {:lib/uuid "00000000-0000-0000-0000-000000000000"} 1]
+ "Asia/Seoul"
+ nil]
+ [nil nil nil nil ["should be a string" "non-blank string" "invalid timezone ID: nil" "Valid :convert-timezone clause"]]
+
+ ;; invalid timezone ID
+ [:convert-timezone
+ {:lib/uuid "00000000-0000-0000-0000-000000000000"}
+ [:field {:lib/uuid "00000000-0000-0000-0000-000000000000"} 1]
+ "US/Specific"]
+ [nil nil nil ["invalid timezone ID: \"US/Specific\""]]))
+
+(deftest ^:parallel get-week-test
+ (are [clause error] (= error
+ (me/humanize (mc/explain :mbql.clause/get-week clause)))
+ ;; without mode
+ [:get-week {:lib/uuid "00000000-0000-0000-0000-000000000000"} "2023-05-25"]
+ nil
+
+ ;; with mode
+ [:get-week {:lib/uuid "00000000-0000-0000-0000-000000000000"} "2023-05-25" :iso]
+ nil
+
+ ;; invalid mode
+ [:get-week {:lib/uuid "00000000-0000-0000-0000-000000000000"} "2023-05-25" :isolation]
+ [nil nil nil ["should be either :iso, :us or :instance" "Valid :get-week clause"]]
+
+ ;; mode is not allowed to be nil
+ [:get-week {:lib/uuid "00000000-0000-0000-0000-000000000000"} "2023-05-25" nil]
+ [nil nil nil ["should be either :iso, :us or :instance" "Valid :get-week clause"]]))
diff --git a/test/metabase/query_processor_test/test_mlv2.clj b/test/metabase/query_processor_test/test_mlv2.clj
index 47d37c59b7059c6fc2c735dab9337f41a3a57be9..c00bb64bc3471036cb0d54bf12be8d5fb53dcab3 100644
--- a/test/metabase/query_processor_test/test_mlv2.clj
+++ b/test/metabase/query_processor_test/test_mlv2.clj
@@ -29,16 +29,6 @@
place, [[metabase.models.query.permissions-test/invalid-queries-test]]."
false)
-(defn- skip-conversion-tests?
- "Whether to skip conversion tests against a `legacy-query`."
- [legacy-query]
- (or
- *skip-conversion-tests*
- ;; #29958: `:convert-timezone` with 2 args is broken
- (mbql.u/match-one legacy-query
- [:convert-timezone _expr _source-timezone]
- "#29958")))
-
(defn- skip-metadata-calculation-tests? [legacy-query]
(or
;; #29907: wrong column name for joined columns in `:breakout`
@@ -62,7 +52,7 @@
(defn- test-mlv2-metadata [original-query _qp-metadata]
{:pre [(map? original-query)]}
- (when-not (or (skip-conversion-tests? original-query)
+ (when-not (or *skip-conversion-tests*
(skip-metadata-calculation-tests? original-query))
(do-with-legacy-query-testing-context
original-query
@@ -82,7 +72,7 @@
(is (any? mlv2-metadata)))))))))))
(defn- test-mlv2-conversion [query]
- (when-not (skip-conversion-tests? query)
+ (when-not *skip-conversion-tests*
(do-with-legacy-query-testing-context
query
(^:once fn* []