Skip to content
Snippets Groups Projects
Unverified Commit 4d31c458 authored by Cam Saul's avatar Cam Saul Committed by GitHub
Browse files

`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:
parent b07bd109
Branches jest-role-hidden
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
shared/src/metabase/mbql/normalize.cljc
+ 2
− 2
View file @ 4d31c458
......@@ -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))
......
shared/src/metabase/mbql/schema.cljc
+ 17
− 10
View file @ 4d31c458
......@@ -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]
......
shared/src/metabase/mbql/util.cljc
+ 11
− 2
View file @ 4d31c458
......@@ -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
......
shared/test/metabase/mbql/normalize_test.cljc
+ 21
− 13
View file @ 4d31c458
......@@ -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))))))
shared/test/metabase/mbql/schema_test.cljc
+ 22
− 21
View file @ 4d31c458
......@@ -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"
......
shared/test/metabase/mbql/util_test.cljc
+ 10
− 0
View file @ 4d31c458
......@@ -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)))))
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment