From 4d31c458a5a87c72e5cf2ea27c58638987f91c20 Mon Sep 17 00:00:00 2001
From: Cam Saul <1455846+camsaul@users.noreply.github.com>
Date: Thu, 28 Apr 2022 12:40:09 -0700
Subject: [PATCH] `mbql.u/add-temporal-unit` should ignore invalid units
(#22156)
* mbql.u/add-temporal-unit should ignore invalid units rather than erroring
* Test fixes :wrench:
---
shared/src/metabase/mbql/normalize.cljc | 4 +-
shared/src/metabase/mbql/schema.cljc | 27 +++++++-----
shared/src/metabase/mbql/util.cljc | 13 +++++-
shared/test/metabase/mbql/normalize_test.cljc | 34 +++++++++------
shared/test/metabase/mbql/schema_test.cljc | 43 ++++++++++---------
shared/test/metabase/mbql/util_test.cljc | 10 +++++
6 files changed, 83 insertions(+), 48 deletions(-)
diff --git a/shared/src/metabase/mbql/normalize.cljc b/shared/src/metabase/mbql/normalize.cljc
index ffeebe4ed61..154d1bf6bca 100644
--- a/shared/src/metabase/mbql/normalize.cljc
+++ b/shared/src/metabase/mbql/normalize.cljc
@@ -365,7 +365,7 @@
:else
x)
(catch #?(:clj Throwable :cljs js/Error) e
- (throw (ex-info (i18n/tru "Error normalizing form.")
+ (throw (ex-info (i18n/tru "Error normalizing form: {0}" (ex-message e))
{:form x, :path path, :special-fn special-fn}
e))))))
@@ -589,7 +589,7 @@
(canonicalize-mbql-clause x)
(catch #?(:clj Throwable :cljs js/Error) e
(log/error (i18n/tru "Invalid clause:") x)
- (throw (ex-info (i18n/tru "Invalid MBQL clause")
+ (throw (ex-info (i18n/tru "Invalid MBQL clause: {0}" (ex-message e))
{:clause x}
e))))))
mbql-query))
diff --git a/shared/src/metabase/mbql/schema.cljc b/shared/src/metabase/mbql/schema.cljc
index ee6a166c78c..6ea9c07a8df 100644
--- a/shared/src/metabase/mbql/schema.cljc
+++ b/shared/src/metabase/mbql/schema.cljc
@@ -293,21 +293,28 @@
validate-bin-width
validate-num-bins))
+(defn valid-temporal-unit-for-base-type?
+ "Whether `temporal-unit` (e.g. `:day`) is valid for the given `base-type` (e.g. `:type/Date`). If either is `nil` this
+ will return truthy. Accepts either map of `field-options` or `base-type` and `temporal-unit` passed separately."
+ ([{:keys [base-type temporal-unit] :as _field-options}]
+ (valid-temporal-unit-for-base-type? base-type temporal-unit))
+
+ ([base-type temporal-unit]
+ (if-let [units (when (core/and temporal-unit base-type)
+ (condp #(isa? %2 %1) base-type
+ :type/Date date-bucketing-units
+ :type/Time time-bucketing-units
+ :type/DateTime datetime-bucketing-units
+ nil))]
+ (contains? units temporal-unit)
+ true)))
+
(defn- validate-temporal-unit [schema]
;; TODO - consider breaking this out into separate constraints for the three different types so we can generate more
;; specific error messages
(s/constrained
schema
- (fn [{:keys [base-type temporal-unit]}]
- (if-not temporal-unit
- true
- (if-let [units (condp #(isa? %2 %1) base-type
- :type/Date date-bucketing-units
- :type/Time time-bucketing-units
- :type/DateTime datetime-bucketing-units
- nil)]
- (contains? units temporal-unit)
- true)))
+ valid-temporal-unit-for-base-type?
"Invalid :temporal-unit for the specified :base-type."))
(defn- no-binning-options-at-top-level [schema]
diff --git a/shared/src/metabase/mbql/util.cljc b/shared/src/metabase/mbql/util.cljc
index ac10df9ae60..94b68d0bc49 100644
--- a/shared/src/metabase/mbql/util.cljc
+++ b/shared/src/metabase/mbql/util.cljc
@@ -4,10 +4,12 @@
#?@
(:clj
[(:require [clojure.string :as str]
+ [clojure.tools.logging :as log]
[metabase.mbql.schema :as mbql.s]
[metabase.mbql.schema.helpers :as schema.helpers]
[metabase.mbql.util.match :as mbql.match]
[metabase.shared.util.i18n :as i18n]
+ metabase.util.i18n
[potemkin :as p]
[schema.core :as s])]
:cljs
@@ -661,10 +663,17 @@
(defn with-temporal-unit
"Set the `:temporal-unit` of a `:field` clause to `unit`."
- [clause unit]
+ [[_ _ {:keys [base-type]} :as clause] unit]
;; it doesn't make sense to call this on an `:expression` or `:aggregation`.
(assert (is-clause? :field clause))
- (assoc-field-options clause :temporal-unit unit))
+ (if (or (not base-type)
+ (mbql.s/valid-temporal-unit-for-base-type? base-type unit))
+ (assoc-field-options clause :temporal-unit unit)
+ (do
+ #?(:clj
+ (log/warn (metabase.util.i18n/trs "{0} is not a valid temporal unit for {1}; not adding to clause {2}"
+ unit base-type (pr-str clause))))
+ clause)))
(defn remove-namespaced-options
"Update a `:field`, `:expression` reference, or `:aggregation` reference clause by removing all namespaced keys in the
diff --git a/shared/test/metabase/mbql/normalize_test.cljc b/shared/test/metabase/mbql/normalize_test.cljc
index e171b73b9c8..667179b2a51 100644
--- a/shared/test/metabase/mbql/normalize_test.cljc
+++ b/shared/test/metabase/mbql/normalize_test.cljc
@@ -419,7 +419,7 @@
"Make sure token normalization works correctly on source queries"
{{:database 4
:type :query
- :query {"source_query" {:native "SELECT * FROM PRODUCTS WHERE CATEGORY = {{category}} LIMIT 10",
+ :query {"source_query" {:native "SELECT * FROM PRODUCTS WHERE CATEGORY = {{category}} LIMIT 10"
"template_tags" {:category {:name "category"
:display-name "Category"
:type "text"
@@ -427,7 +427,7 @@
:default "Widget"}}}}}
{:database 4
:type :query
- :query {:source-query {:native "SELECT * FROM PRODUCTS WHERE CATEGORY = {{category}} LIMIT 10",
+ :query {:source-query {:native "SELECT * FROM PRODUCTS WHERE CATEGORY = {{category}} LIMIT 10"
:template-tags {"category" {:name "category"
:display-name "Category"
:type :text
@@ -437,7 +437,7 @@
{:database 4
:type :query
:query {"source_query" {"source_table" 1, "aggregation" "rows"}}}
- {:database 4,
+ {:database 4
:type :query
:query {:source-query {:source-table 1, :aggregation :rows}}}}))
@@ -558,7 +558,7 @@
"expressions should handle datetime arithemtics"
{{:query {:expressions {:prev_month ["+" ["field-id" 13] ["interval" -1 "month"]]}}}
- {:query {:expressions {"prev_month" [:+ [:field-id 13] [:interval -1 :month]]}}},
+ {:query {:expressions {"prev_month" [:+ [:field-id 13] [:interval -1 :month]]}}}
{:query {:expressions {:prev_month ["-" ["field-id" 13] ["interval" 1 "month"] ["interval" 1 "day"]]}}}
{:query {:expressions {"prev_month" [:- [:field-id 13] [:interval 1 :month] [:interval 1 :day]]}}}}
@@ -854,7 +854,7 @@
:query {:filter [:and
[:segment "gaid:-11"]
[:time-interval [:field-id 6851] -365 :day {}]]}}
- {:database 1,
+ {:database 1
:type :query
:query {:filter
[:and
@@ -943,7 +943,7 @@
:default "Widget"}}}}}
{:database 4
:type :query
- :query {:source-query {:native "SELECT * FROM PRODUCTS WHERE CATEGORY = {{category}} LIMIT 10",
+ :query {:source-query {:native "SELECT * FROM PRODUCTS WHERE CATEGORY = {{category}} LIMIT 10"
:template-tags {"category" {:name "category"
:display-name "Category"
:type :text
@@ -1122,7 +1122,7 @@
(t/testing "make sure source queries get normalized properly!"
(t/is (= {:database 4
:type :query
- :query {:source-query {:native "SELECT * FROM PRODUCTS WHERE CATEGORY = {{category}} LIMIT 10",
+ :query {:source-query {:native "SELECT * FROM PRODUCTS WHERE CATEGORY = {{category}} LIMIT 10"
:template-tags {"category" {:name "category"
:display-name "Category"
:type :text
@@ -1131,7 +1131,7 @@
(mbql.normalize/normalize
{:database 4
:type :query
- :query {"source_query" {:native "SELECT * FROM PRODUCTS WHERE CATEGORY = {{category}} LIMIT 10",
+ :query {"source_query" {:native "SELECT * FROM PRODUCTS WHERE CATEGORY = {{category}} LIMIT 10"
"template_tags" {:category {:name "category"
:display-name "Category"
:type "text"
@@ -1402,13 +1402,21 @@
(t/is (= {:query bad-query}
(ex-data e))))
(t/testing "\nParent exception(s) should be even more specific"
- (let [cause #?(:clj (some-> ^Throwable e .getCause)
- :cljs (ex-cause e))]
+ (let [cause (ex-cause e)]
(t/is (some? cause))
- (t/is (= "Error normalizing form."
- #?(:clj (.getMessage cause)
- :cljs (ex-message cause))))
+ (t/is (re-find #"Error normalizing form:" (ex-message cause)))
(t/is (= {:form bad-query
:path []
:special-fn nil}
(ex-data cause)))))))))
+
+(t/deftest ^:parallel remove-unsuitable-temporal-units-test
+ (t/testing "Ignore unsuitable temporal units (such as bucketing a Date by minute) rather than erroring (#16485)"
+ ;; this query is with legacy MBQL syntax. It's just copied directly from the original issue
+ (let [query {:query {:filter ["<"
+ ["datetime-field" ["field-literal" "date_seen" "type/Date"] "minute"]
+ "2021-05-01T12:30:00"]}}]
+ (t/is (= {:query {:filter [:<
+ [:field "date_seen" {:base-type :type/Date}]
+ "2021-05-01T12:30:00"]}}
+ (mbql.normalize/normalize query))))))
diff --git a/shared/test/metabase/mbql/schema_test.cljc b/shared/test/metabase/mbql/schema_test.cljc
index ad467ad96de..481aa526cd4 100644
--- a/shared/test/metabase/mbql/schema_test.cljc
+++ b/shared/test/metabase/mbql/schema_test.cljc
@@ -5,27 +5,28 @@
(t/deftest ^:parallel field-clause-test
(t/testing "Make sure our schema validates `:field` clauses correctly"
- (t/are [clause expected] (= expected
- (not (s/check mbql.s/field clause)))
- [:field 1 nil] true
- [:field 1 {}] true
- [:field 1 {:x true}] true
- [:field 1 2] false
- [:field "wow" nil] false
- [:field "wow" {}] false
- [:field "wow" 1] false
- [:field "wow" {:base-type :type/Integer}] true
- [:field "wow" {:base-type 100}] false
- [:field "wow" {:base-type :type/Integer, :temporal-unit :month}] true
- [:field "wow" {:base-type :type/Date, :temporal-unit :month}] true
- [:field "wow" {:base-type :type/DateTimeWithTZ, :temporal-unit :month}] true
- [:field "wow" {:base-type :type/Time, :temporal-unit :month}] false
- [:field 1 {:binning {:strategy :num-bins}}] false
- [:field 1 {:binning {:strategy :num-bins, :num-bins 1}}] true
- [:field 1 {:binning {:strategy :num-bins, :num-bins 1.5}}] false
- [:field 1 {:binning {:strategy :num-bins, :num-bins -1}}] false
- [:field 1 {:binning {:strategy :default}}] true
- [:field 1 {:binning {:strategy :fake}}] false)))
+ (doseq [[clause expected] {[:field 1 nil] true
+ [:field 1 {}] true
+ [:field 1 {:x true}] true
+ [:field 1 2] false
+ [:field "wow" nil] false
+ [:field "wow" {}] false
+ [:field "wow" 1] false
+ [:field "wow" {:base-type :type/Integer}] true
+ [:field "wow" {:base-type 100}] false
+ [:field "wow" {:base-type :type/Integer, :temporal-unit :month}] true
+ [:field "wow" {:base-type :type/Date, :temporal-unit :month}] true
+ [:field "wow" {:base-type :type/DateTimeWithTZ, :temporal-unit :month}] true
+ [:field "wow" {:base-type :type/Time, :temporal-unit :month}] false
+ [:field 1 {:binning {:strategy :num-bins}}] false
+ [:field 1 {:binning {:strategy :num-bins, :num-bins 1}}] true
+ [:field 1 {:binning {:strategy :num-bins, :num-bins 1.5}}] false
+ [:field 1 {:binning {:strategy :num-bins, :num-bins -1}}] false
+ [:field 1 {:binning {:strategy :default}}] true
+ [:field 1 {:binning {:strategy :fake}}] false}]
+ (t/testing (pr-str clause)
+ (t/is (= expected
+ (not (s/check mbql.s/field clause))))))))
(t/deftest ^:parallel validate-template-tag-names-test
(t/testing "template tags with mismatched keys/`:names` in definition should be disallowed\n"
diff --git a/shared/test/metabase/mbql/util_test.cljc b/shared/test/metabase/mbql/util_test.cljc
index 2de494358d0..255c3ce7028 100644
--- a/shared/test/metabase/mbql/util_test.cljc
+++ b/shared/test/metabase/mbql/util_test.cljc
@@ -792,3 +792,13 @@
[:aggregation 0] [:aggregation 0]
[:aggregation 0 {::namespaced true}] [:aggregation 0]
[:aggregation 0 {::namespaced true, :a 1}] [:aggregation 0 {:a 1}]))
+
+(t/deftest with-temporal-unit-test
+ (t/is (= [:field 1 {:temporal-unit :day}]
+ (mbql.u/with-temporal-unit [:field 1 nil] :day)))
+ (t/is (= [:field "t" {:base-type :type/Date, :temporal-unit :day}]
+ (mbql.u/with-temporal-unit [:field "t" {:base-type :type/Date}] :day)))
+ (t/testing "Ignore invalid temporal units if `:base-type` is specified (#16485)"
+ ;; `:minute` doesn't make sense for a DATE
+ (t/is (= [:field "t" {:base-type :type/Date}]
+ (mbql.u/with-temporal-unit [:field "t" {:base-type :type/Date}] :minute)))))
--
GitLab