From b48f04afd03f02b93d58a5a980686cc71b19797c Mon Sep 17 00:00:00 2001
From: Cam Saul <cammsaul@gmail.com>
Date: Mon, 13 Nov 2017 15:06:35 -0800
Subject: [PATCH] Fix handling default values for Dimension (Field Filter)
params
---
.../middleware/parameters/dates.clj | 6 +-
.../middleware/parameters/sql.clj | 49 +++++++--
.../middleware/parameters/sql_test.clj | 104 +++++++++++++++++-
3 files changed, 146 insertions(+), 13 deletions(-)
diff --git a/src/metabase/query_processor/middleware/parameters/dates.clj b/src/metabase/query_processor/middleware/parameters/dates.clj
index 812e3961b8b..98bc18c3c08 100644
--- a/src/metabase/query_processor/middleware/parameters/dates.clj
+++ b/src/metabase/query_processor/middleware/parameters/dates.clj
@@ -199,10 +199,10 @@
"Takes a string description of a date range such as 'lastmonth' or '2016-07-15~2016-08-6' and
return a MAP with `:start` and `:end` as iso8601 string formatted dates, respecting the given timezone."
[date-string report-timezone]
- (let [tz (t/time-zone-for-id report-timezone)
+ (let [tz (t/time-zone-for-id report-timezone)
formatter-local-tz (tf/formatter "yyyy-MM-dd" tz)
- formatter-no-tz (tf/formatter "yyyy-MM-dd")
- today (.withTimeAtStartOfDay (t/to-time-zone (t/now) tz))]
+ formatter-no-tz (tf/formatter "yyyy-MM-dd")
+ today (.withTimeAtStartOfDay (t/to-time-zone (t/now) tz))]
;; Relative dates respect the given time zone because a notion like "last 7 days" might mean a different range of days
;; depending on the user timezone
(or (->> (execute-decoders relative-date-string-decoders :range today date-string)
diff --git a/src/metabase/query_processor/middleware/parameters/sql.clj b/src/metabase/query_processor/middleware/parameters/sql.clj
index cab108d6167..f1046f6716d 100644
--- a/src/metabase/query_processor/middleware/parameters/sql.clj
+++ b/src/metabase/query_processor/middleware/parameters/sql.clj
@@ -67,6 +67,9 @@
;; TAGS in this case are simple params like {{x}} that get replaced with a single value ("ABC" or 1) as opposed to a
;; "FieldFilter" clause like Dimensions
+;;
+;; Since 'Dimension' (Field Filters) are considered their own `:type`, to *actually* store the type of a Dimension
+;; look at the key `:widget_type`. This applies to things like the default value for a Dimension as well.
(def ^:private TagParam
"Schema for values passed in as part of the `:template_tags` list."
{(s/optional-key :id) su/NonBlankString ; this is used internally by the frontend
@@ -74,7 +77,7 @@
:display_name su/NonBlankString
:type (s/enum "number" "dimension" "text" "date")
(s/optional-key :dimension) [s/Any]
- (s/optional-key :widget_type) su/NonBlankString
+ (s/optional-key :widget_type) su/NonBlankString ; type of the [default] value if `:type` itself is `dimension`
(s/optional-key :required) s/Bool
(s/optional-key :default) s/Any})
@@ -109,7 +112,7 @@
;;; +----------------------------------------------------------------------------------------------------------------+
;; These functions build a map of information about the types and values of the params used in a query.
-;; (These functions don't pare the query itself, but instead look atthe values of `:template_tags` and `:parameters`
+;; (These functions don't parse the query itself, but instead look at the values of `:template_tags` and `:parameters`
;; passed along with the query.)
;;
;; (query->params-map some-query)
@@ -119,7 +122,10 @@
;; :value "\2015-01-01~2016-09-01"\}}}
(s/defn ^:private ^:always-validate param-with-target
- "Return the param in PARAMS with a matching TARGET."
+ "Return the param in PARAMS with a matching TARGET. TARGET is something like:
+
+ [:dimension [:template-tag <param-name>]] ; for Dimensions (Field Filters)
+ [:variable [:template-tag <param-name>]] ; for other types of params"
[params :- (s/maybe [DimensionValue]), target]
(when-let [matching-params (seq (for [param params
:when (= (:target param) target)]
@@ -129,26 +135,53 @@
first
vec) matching-params)))
-(s/defn ^:private ^:always-validate param-value-for-tag [tag :- TagParam, params :- (s/maybe [DimensionValue])]
- (when (not= (:type tag) "dimension")
- (:value (param-with-target params ["variable" ["template-tag" (:name tag)]]))))
+
+;;; Dimension Params (Field Filters) (e.g. WHERE {{x}})
+
+(s/defn ^:private ^:always-validate default-value-for-dimension :- (s/maybe DimensionValue)
+ "Return the default value for a Dimension (Field Filter) param defined by the map TAG, if one is set."
+ [tag :- TagParam]
+ (when-let [default (:default tag)]
+ {:type (:widget_type tag "dimension") ; widget_type is the actual type of the default value if set
+ :target ["dimension" ["template-tag" (:name tag)]]
+ :value default}))
(s/defn ^:private ^:always-validate dimension->field-id :- su/IntGreaterThanZero
[dimension]
(:field-id (ql/expand-ql-sexpr dimension)))
(s/defn ^:private ^:always-validate dimension-value-for-tag :- (s/maybe Dimension)
+ "Return the \"Dimension\" value of a param, if applicable. \"Dimension\" here means what is called a \"Field
+ Filter\" in the Native Query Editor."
[tag :- TagParam, params :- (s/maybe [DimensionValue])]
(when-let [dimension (:dimension tag)]
(map->Dimension {:field (or (db/select-one [Field :name :parent_id :table_id], :id (dimension->field-id dimension))
(throw (Exception. (str "Can't find field with ID: " (dimension->field-id dimension)))))
- :param (param-with-target params ["dimension" ["template-tag" (:name tag)]])})))
+ :param (or
+ ;; look in the sequence of params we were passed to see if there's anything that matches
+ (param-with-target params ["dimension" ["template-tag" (:name tag)]])
+ ;; if not, check and see if we have a default param
+ (default-value-for-dimension tag))})))
+
-(s/defn ^:private ^:always-validate default-value-for-tag [{:keys [default display_name required]} :- TagParam]
+;;; Non-Dimension Params (e.g. WHERE x = {{x}})
+
+(s/defn ^:private ^:always-validate param-value-for-tag [tag :- TagParam, params :- (s/maybe [DimensionValue])]
+ (when (not= (:type tag) "dimension")
+ (:value (param-with-target params ["variable" ["template-tag" (:name tag)]]))))
+
+(s/defn ^:private ^:always-validate default-value-for-tag
+ "Return the `:default` value for a param if no explicit values were passsed. This only applies to non-Dimension
+ (non-Field Filter) params. Default values for Dimension (Field Filter) params are handled above in
+ `default-value-for-dimension`."
+ [{:keys [default display_name required]} :- TagParam]
(or default
(when required
(throw (Exception. (format "'%s' is a required param." display_name))))))
+
+;;; Parsing Values
+
(s/defn ^:private ^:always-validate parse-number :- s/Num
"Parse a string like `1` or `2.0` into a valid number. Done mostly to keep people from passing in
things that aren't numbers, like SQL identifiers."
diff --git a/test/metabase/query_processor/middleware/parameters/sql_test.clj b/test/metabase/query_processor/middleware/parameters/sql_test.clj
index 48271d23708..cdeddffbd1f 100644
--- a/test/metabase/query_processor/middleware/parameters/sql_test.clj
+++ b/test/metabase/query_processor/middleware/parameters/sql_test.clj
@@ -5,7 +5,7 @@
[driver :as driver]
[query-processor :as qp]
[query-processor-test :refer [engines-that-support first-row format-rows-by]]]
- [metabase.query-processor.middleware.parameters.sql :refer :all]
+ [metabase.query-processor.middleware.parameters.sql :refer :all, :as sql]
[metabase.test
[data :as data]
[util :as tu]]
@@ -427,7 +427,8 @@
(generic-sql/quote-name datasets/*driver* identifier))
(defn- checkins-identifier []
- ;; HACK ! I don't have all day to write protocol methods to make this work the "right" way so for BigQuery and Presto we will just hackily return the correct identifier here
+ ;; HACK ! I don't have all day to write protocol methods to make this work the "right" way so for BigQuery and
+ ;; Presto we will just hackily return the correct identifier here
(case datasets/*engine*
:bigquery "[test_data.checkins]"
:presto "\"default\".\"checkins\""
@@ -546,3 +547,102 @@
:native {:query "SELECT * FROM ORDERS WHERE true [[ AND ID = {{id}} OR USER_ID = {{id}} ]]"
:template_tags {:id {:name "id", :display_name "ID", :type "text"}}}
:parameters [{:type "category", :target ["variable" ["template-tag" "id"]], :value "2"}]})))
+
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | RELATIVE DATES & DEFAULTS IN "DIMENSION" PARAMS (#6059) |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+;; Make sure relative date forms like `past5days` work correctly with Field Filters
+(expect
+ {:query (str "SELECT count(*) AS \"count\", \"DATE\" "
+ "FROM CHECKINS "
+ "WHERE CAST(\"PUBLIC\".\"CHECKINS\".\"DATE\" AS date) BETWEEN ? AND ? "
+ "GROUP BY \"DATE\"")
+ :template_tags {:checkin_date {:name "checkin_date"
+ :display_name "Checkin Date"
+ :type "dimension"
+ :dimension ["field-id" (data/id :checkins :date)]}}
+ :params [#inst "2017-10-31T00:00:00.000000000-00:00"
+ #inst "2017-11-04T00:00:00.000000000-00:00"]}
+ (with-redefs [t/now (fn [] (t/date-time 2017 11 05 12 0 0))]
+ (:native (expand {:driver (driver/engine->driver :h2)
+ :native {:query (str "SELECT count(*) AS \"count\", \"DATE\" "
+ "FROM CHECKINS "
+ "WHERE {{checkin_date}} "
+ "GROUP BY \"DATE\"")
+ :template_tags {:checkin_date {:name "checkin_date"
+ :display_name "Checkin Date"
+ :type "dimension"
+ :dimension ["field-id" (data/id :checkins :date)]}}}
+ :parameters [{:type "date/range"
+ :target ["dimension" ["template-tag" "checkin_date"]]
+ :value "past5days"}]}))))
+
+;; Make sure defaults values get picked up for field filter clauses
+(expect
+ {:field {:name "DATE", :parent_id nil, :table_id (data/id :checkins)}
+ :param {:type "date/all-options"
+ :target ["dimension" ["template-tag" "checkin_date"]]
+ :value "past5days"}}
+ (#'sql/dimension-value-for-tag {:name "checkin_date"
+ :display_name "Checkin Date"
+ :type "dimension"
+ :dimension [:field-id (data/id :checkins :date)]
+ :default "past5days"
+ :widget_type "date/all-options"}
+ nil))
+
+;; Make sure we can specify the type of a default value for a "Dimension" (Field Filter) by setting the
+;; `:widget_type` key. Check that it works correctly with relative dates...
+(expect
+ {:query (str "SELECT count(*) AS \"count\", \"DATE\" "
+ "FROM CHECKINS "
+ "WHERE CAST(\"PUBLIC\".\"CHECKINS\".\"DATE\" AS date) BETWEEN ? AND ? "
+ "GROUP BY \"DATE\"")
+ :template_tags {:checkin_date
+ {:name "checkin_date"
+ :display_name "Checkin Date"
+ :type "dimension"
+ :dimension ["field-id" (data/id :checkins :date)]
+ :default "past5days"
+ :widget_type "date/all-options"}}
+ :params [#inst "2017-10-31T00:00:00.000000000-00:00"
+ #inst "2017-11-04T00:00:00.000000000-00:00"]}
+ (with-redefs [t/now (fn [] (t/date-time 2017 11 05 12 0 0))]
+ (:native (expand {:driver (driver/engine->driver :h2)
+ :native {:query (str "SELECT count(*) AS \"count\", \"DATE\" "
+ "FROM CHECKINS "
+ "WHERE {{checkin_date}} "
+ "GROUP BY \"DATE\"")
+ :template_tags {:checkin_date {:name "checkin_date"
+ :display_name "Checkin Date"
+ :type "dimension"
+ :dimension ["field-id" (data/id :checkins :date)]
+ :default "past5days"
+ :widget_type "date/all-options"}}}}))))
+
+;; Check that it works with absolute dates as well
+(expect
+ {:query (str "SELECT count(*) AS \"count\", \"DATE\" "
+ "FROM CHECKINS "
+ "WHERE CAST(\"PUBLIC\".\"CHECKINS\".\"DATE\" AS date) = ? "
+ "GROUP BY \"DATE\"")
+ :template_tags {:checkin_date {:name "checkin_date"
+ :display_name "Checkin Date"
+ :type "dimension"
+ :dimension ["field-id" (data/id :checkins :date)]
+ :default "2017-11-14"
+ :widget_type "date/all-options"}}
+ :params [#inst "2017-11-14T00:00:00.000000000-00:00"]}
+ (:native (expand {:driver (driver/engine->driver :h2)
+ :native {:query (str "SELECT count(*) AS \"count\", \"DATE\" "
+ "FROM CHECKINS "
+ "WHERE {{checkin_date}} "
+ "GROUP BY \"DATE\"")
+ :template_tags {:checkin_date {:name "checkin_date"
+ :display_name "Checkin Date"
+ :type "dimension"
+ :dimension ["field-id" (data/id :checkins :date)]
+ :default "2017-11-14"
+ :widget_type "date/all-options"}}}})))
--
GitLab