From b75577b5292889032f2bb3fc77593efb8f4da882 Mon Sep 17 00:00:00 2001
From: lbrdnk <lbrdnk@users.noreply.github.com>
Date: Mon, 26 Aug 2024 14:51:45 +0200
Subject: [PATCH] Fix `DateTimeRange` substitution for open ranges (#47175)
* Fix DateTimeRange substitution for open ranges
* Add E2E repro for #47172
* cljfmt
* Add test for datetime column
* Update range boundary for open range
* Update test names
* Apply suggestions from code review
Co-authored-by: Kamil Mielnik <kamil@kamilmielnik.com>
* Fix e2e
---------
Co-authored-by: Nemanja <31325167+nemanjaglumac@users.noreply.github.com>
Co-authored-by: Kamil Mielnik <kamil@kamilmielnik.com>
---
...dashboard-filters-reproductions.cy.spec.js | 32 +++-
.../driver/common/parameters/dates.clj | 10 +-
.../driver/sql/parameters/substitution.clj | 19 ++-
.../driver/sql/parameters/substitute_test.clj | 142 +++++++++++++++++-
4 files changed, 193 insertions(+), 10 deletions(-)
diff --git a/e2e/test/scenarios/filters-reproductions/dashboard-filters-reproductions.cy.spec.js b/e2e/test/scenarios/filters-reproductions/dashboard-filters-reproductions.cy.spec.js
index b9053b7948d..916a7a6ed23 100644
--- a/e2e/test/scenarios/filters-reproductions/dashboard-filters-reproductions.cy.spec.js
+++ b/e2e/test/scenarios/filters-reproductions/dashboard-filters-reproductions.cy.spec.js
@@ -34,6 +34,7 @@ import {
setModelMetadata,
sidebar,
tableHeaderClick,
+ tableInteractive,
undoToast,
updateDashboardCards,
visitDashboard,
@@ -240,7 +241,7 @@ describe("issue 8030 + 32444", () => {
});
});
-describe("issue 12720", () => {
+describe("issue 12720, issue 47172", () => {
function clickThrough(title) {
visitDashboard(ORDERS_DASHBOARD_ID);
cy.findAllByTestId("dashcard-container").contains(title).click();
@@ -291,7 +292,7 @@ describe("issue 12720", () => {
{
card_id: SQL_ID,
row: 0,
- col: 6, // making sure it doesn't overlap the existing card
+ col: 8, // making sure it doesn't overlap the existing card
size_x: 7,
size_y: 5,
parameter_mappings: [
@@ -330,6 +331,33 @@ describe("issue 12720", () => {
clickThrough("12720_SQL");
clickThrough("Orders");
});
+
+ it("should apply the specific (before|after) filter on a native question with field filter (metabase#47172)", () => {
+ visitDashboard(ORDERS_DASHBOARD_ID);
+
+ getDashboardCard(1).within(() => {
+ cy.findByTestId("TableFooter").should("exist");
+ cy.findByText("There was a problem displaying this chart.").should(
+ "not.exist",
+ );
+
+ cy.log("Drill down to the question");
+ cy.intercept("POST", "/api/card/*/query").as("cardQuery");
+ cy.findByText(questionDetails.name).click();
+ });
+
+ cy.location("search").should("eq", `?filter=${dashboardFilter.default}`);
+ cy.wait("@cardQuery");
+ tableInteractive().should("be.visible").and("contain", "97.44");
+ cy.findByTestId("question-row-count").should(
+ "not.have.text",
+ "Showing 0 rows",
+ );
+ cy.findByTestId("question-row-count").should(
+ "have.text",
+ "Showing 1,980 rows",
+ );
+ });
});
describe("issue 12985 > dashboard filter dropdown/search", () => {
diff --git a/src/metabase/driver/common/parameters/dates.clj b/src/metabase/driver/common/parameters/dates.clj
index 9a2f0087189..01e2fe90fd6 100644
--- a/src/metabase/driver/common/parameters/dates.clj
+++ b/src/metabase/driver/common/parameters/dates.clj
@@ -506,6 +506,13 @@
{:start date-str
:end date-str}))
+(defn- maybe-adjust-open-range
+ [{:keys [start end] :as range} unit-fn]
+ (assert (some some? [start end]))
+ (cond (and start end) range
+ start (update range :start t/+ (unit-fn 1))
+ end (update range :end t/- (unit-fn 1))))
+
(mu/defn date-str->datetime-range :- DateStringRange
"Generate range from `date-range-str`.
@@ -525,7 +532,8 @@
(catch Throwable _
(fallback-raw-range raw-date-str)))]
(-> (update-vals range-raw date-str->qp-aware-offset-dt)
- (update :end exclusive-datetime-range-end (date-str->unit-fn (:end range-raw)))
+ (m/update-existing :end exclusive-datetime-range-end (date-str->unit-fn (:end range-raw)))
+ (maybe-adjust-open-range (date-str->unit-fn ((some-fn :start :end) range-raw)))
format-date-range)))
(mu/defn date-string->filter :- mbql.s/Filter
diff --git a/src/metabase/driver/sql/parameters/substitution.clj b/src/metabase/driver/sql/parameters/substitution.clj
index 4f35c1bfffb..5b4185fe015 100644
--- a/src/metabase/driver/sql/parameters/substitution.clj
+++ b/src/metabase/driver/sql/parameters/substitution.clj
@@ -215,12 +215,19 @@
(defmethod ->replacement-snippet-info [:sql DateTimeRange]
[driver {:keys [start end]} & [field-identifier]]
- (let [[start end] (map (fn [s]
- (->prepared-substitution driver (maybe-parse-temporal-literal s)))
- [start end])]
- {:replacement-snippet (format "%s >= %s AND %s < %s"
- field-identifier (:sql-string start) field-identifier (:sql-string end))
- :prepared-statement-args (concat (:param-values start) (:param-values end))}))
+ (let [[start end] (map (fn [s]
+ (when s
+ (->prepared-substitution driver (maybe-parse-temporal-literal s))))
+ [start end])
+ start-expr-native (when start
+ (format "%s >= %s" field-identifier (:sql-string start)))
+ end-expr-native (when end
+ (format "%s < %s" field-identifier (:sql-string end)))]
+ {:replacement-snippet (str/join " AND " (remove nil? [start-expr-native end-expr-native]))
+ :prepared-statement-args (into []
+ (comp (keep :param-values)
+ cat)
+ [start end])}))
;;; ------------------------------------- Field Filter replacement snippet info --------------------------------------
diff --git a/test/metabase/driver/sql/parameters/substitute_test.clj b/test/metabase/driver/sql/parameters/substitute_test.clj
index 0744de87d81..65a3e70b962 100644
--- a/test/metabase/driver/sql/parameters/substitute_test.clj
+++ b/test/metabase/driver/sql/parameters/substitute_test.clj
@@ -552,7 +552,7 @@
expand*
(dissoc :template-tags)))))
-(deftest expand-field-filters-test
+(deftest expand-field-filters-for-date-field-test
(mt/with-temporary-setting-values [start-of-week :sunday]
(testing "dimension (date/single)"
(is (= {:query "SELECT * FROM checkins WHERE \"PUBLIC\".\"CHECKINS\".\"DATE\" = ?;"
@@ -645,6 +645,146 @@
"SELECT * FROM ORDERS WHERE TOTAL > 100 [[AND {{created}} #]] AND CREATED_AT < now()"
nil))))))
+(defn- expand-with-field-filter-param-on-datetime-field
+ ([field-filter-param]
+ (expand-with-field-filter-param-on-datetime-field "SELECT * FROM orders WHERE {{date}};" field-filter-param))
+
+ ([sql field-filter-param]
+ ;; TIMEZONE FIXME
+ (mt/with-clock (t/mock-clock #t "2016-06-07T12:00-00:00" (t/zone-id "UTC"))
+ (-> {:native {:query
+ sql
+ :template-tags {"date" {:name "date"
+ :display-name "Created At"
+ :type :dimension
+ :widget-type :date/all-options
+ :dimension [:field (meta/id :orders :created-at) nil]}}}
+ :parameters (when field-filter-param
+ [(merge {:target [:dimension [:template-tag "date"]]}
+ field-filter-param)])}
+ expand*
+ (dissoc :template-tags)))))
+
+;;;; The following test is [[expand-field-filters-for-date-field-test]] adjusted to datetime field.
+(deftest expand-field-filters-for-datetime-field-test
+ (mt/with-temporary-setting-values [start-of-week :sunday]
+ (testing "dimension (date/all-options) (`on` filter with with time)"
+ (is (= {:query "SELECT * FROM orders WHERE \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" >= ? AND \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" < ?;",
+ :params [#t "2024-08-20T10:20Z[UTC]"
+ #t "2024-08-20T10:21Z[UTC]"]}
+ (expand-with-field-filter-param-on-datetime-field {:type :date/all-options, :value "2024-08-20T10:20:00"}))))
+ (testing "dimension (date/all-options) (`before` filter with with time)"
+ (is (= {:query "SELECT * FROM orders WHERE \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" < ?;",
+ :params [#t "2024-08-20T10:20Z[UTC]"]}
+ (expand-with-field-filter-param-on-datetime-field {:type :date/all-options, :value "~2024-08-20T10:20:00"}))))
+ (testing "dimension (date/all-options) (`after` filter with with time)"
+ (is (= {:query "SELECT * FROM orders WHERE \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" >= ?;",
+ :params [#t "2024-08-20T10:21Z[UTC]"]}
+ (expand-with-field-filter-param-on-datetime-field {:type :date/all-options, :value "2024-08-20T10:20:00~"}))))
+ (testing "dimension (date/single)"
+ (is (= {:query
+ "SELECT * FROM orders WHERE \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" >= ? AND \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" < ?;",
+ :params [#t "2016-07-01T00:00Z[UTC]"
+ #t "2016-07-02T00:00Z[UTC]"]}
+ (expand-with-field-filter-param-on-datetime-field {:type :date/single, :value "2016-07-01"}))))
+ (testing "dimension (date/range)"
+ (is (= {:query
+ "SELECT * FROM orders WHERE \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" >= ? AND \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" < ?;",
+ :params [#t "2016-07-01T00:00Z[UTC]"
+ #t "2016-08-02T00:00Z[UTC]"]}
+ (expand-with-field-filter-param-on-datetime-field {:type :date/range, :value "2016-07-01~2016-08-01"}))))
+ (testing "dimension (date/month-year)"
+ (is (= {:query
+ "SELECT * FROM orders WHERE \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" >= ? AND \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" < ?;",
+ :params [#t "2016-07-01T00:00Z[UTC]"
+ #t "2016-08-01T00:00Z[UTC]"]}
+ (expand-with-field-filter-param-on-datetime-field {:type :date/month-year, :value "2016-07"}))))
+ (testing "dimension (date/quarter-year)"
+ (is (= {:query
+ "SELECT * FROM orders WHERE \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" >= ? AND \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" < ?;",
+ :params [#t "2016-01-01T00:00Z[UTC]"
+ #t "2016-04-01T00:00Z[UTC]"]}
+ (expand-with-field-filter-param-on-datetime-field {:type :date/quarter-year, :value "Q1-2016"}))))
+ (testing "dimension (date/all-options, before)"
+ (is (= {:query "SELECT * FROM orders WHERE \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" < ?;", :params [#t "2016-07-01T00:00Z[UTC]"]}
+ (expand-with-field-filter-param-on-datetime-field {:type :date/all-options, :value "~2016-07-01"}))))
+ (testing "dimension (date/all-options, after)"
+ (is (= {:query "SELECT * FROM orders WHERE \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" >= ?;", :params [#t "2016-07-02T00:00Z[UTC]"]}
+ (expand-with-field-filter-param-on-datetime-field {:type :date/all-options, :value "2016-07-01~"}))))
+ (testing "relative date — 'yesterday'"
+ (is (= {:query
+ "SELECT * FROM orders WHERE \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" >= ? AND \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" < ?;",
+ :params [#t "2016-06-06T00:00Z[UTC]"
+ #t "2016-06-07T00:00Z[UTC]"]}
+ (expand-with-field-filter-param-on-datetime-field {:type :date/range, :value "yesterday"}))))
+ (testing "relative date — 'past7days'"
+ (is (= {:query
+ "SELECT * FROM orders WHERE \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" >= ? AND \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" < ?;",
+ :params [#t "2016-05-31T00:00Z[UTC]"
+ #t "2016-06-07T00:00Z[UTC]"]}
+ (expand-with-field-filter-param-on-datetime-field {:type :date/range, :value "past7days"}))))
+ (testing "relative date — 'past30days'"
+ (is (= {:query
+ "SELECT * FROM orders WHERE \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" >= ? AND \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" < ?;",
+ :params [#t "2016-05-08T00:00Z[UTC]"
+ #t "2016-06-07T00:00Z[UTC]"]}
+ (expand-with-field-filter-param-on-datetime-field {:type :date/range, :value "past30days"}))))
+ (testing "relative date — 'thisweek'"
+ (is (= {:query
+ "SELECT * FROM orders WHERE \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" >= ? AND \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" < ?;",
+ :params [#t "2016-06-05T00:00Z[UTC]"
+ #t "2016-06-12T00:00Z[UTC]"]}
+ (expand-with-field-filter-param-on-datetime-field {:type :date/range, :value "thisweek"}))))
+ (testing "relative date — 'thismonth'"
+ (is (= {:query
+ "SELECT * FROM orders WHERE \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" >= ? AND \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" < ?;",
+ :params [#t "2016-06-01T00:00Z[UTC]"
+ #t "2016-07-01T00:00Z[UTC]"]}
+ (expand-with-field-filter-param-on-datetime-field {:type :date/range, :value "thismonth"}))))
+ (testing "relative date — 'thisyear'"
+ (is (= {:query
+ "SELECT * FROM orders WHERE \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" >= ? AND \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" < ?;",
+ :params [#t "2016-01-01T00:00Z[UTC]"
+ #t "2017-01-01T00:00Z[UTC]"]}
+ (expand-with-field-filter-param-on-datetime-field {:type :date/range, :value "thisyear"}))))
+ (testing "relative date — 'lastweek'"
+ (is (= {:query
+ "SELECT * FROM orders WHERE \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" >= ? AND \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" < ?;",
+ :params [#t "2016-05-29T00:00Z[UTC]"
+ #t "2016-06-05T00:00Z[UTC]"]}
+ (expand-with-field-filter-param-on-datetime-field {:type :date/range, :value "lastweek"}))))
+ (testing "relative date — 'lastmonth'"
+ (is (= {:query
+ "SELECT * FROM orders WHERE \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" >= ? AND \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" < ?;",
+ :params [#t "2016-05-01T00:00Z[UTC]"
+ #t "2016-06-01T00:00Z[UTC]"]}
+ (expand-with-field-filter-param-on-datetime-field {:type :date/range, :value "lastmonth"}))))
+ (testing "relative date — 'lastyear'"
+ (is (= {:query
+ "SELECT * FROM orders WHERE \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" >= ? AND \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" < ?;",
+ :params [#t "2015-01-01T00:00Z[UTC]"
+ #t "2016-01-01T00:00Z[UTC]"]}
+ (expand-with-field-filter-param-on-datetime-field {:type :date/range, :value "lastyear"}))))
+ (testing "dimension with no value — just replace with an always true clause (e.g. 'WHERE 1 = 1')"
+ (is (= {:query "SELECT * FROM orders WHERE 1 = 1;"
+ :params []}
+ (expand-with-field-filter-param-on-datetime-field nil))))
+ (testing "dimension — number — should get parsed to Number"
+ (is (= {:query "SELECT * FROM orders WHERE \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" = 100;"
+ :params []}
+ (expand-with-field-filter-param-on-datetime-field {:type :number, :value "100"}))))
+ (testing "dimension — text"
+ (is (= {:query "SELECT * FROM orders WHERE \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" = ?;"
+ :params ["100"]}
+ (expand-with-field-filter-param-on-datetime-field {:type :text, :value "100"}))))
+ (testing (str "*OPTIONAL* Field Filter params should not get replaced with 1 = 1 if the param is not present "
+ "(#5541, #9489). *Optional params should be emitted entirely.")
+ (is (= {:query "SELECT * FROM ORDERS WHERE TOTAL > 100 AND CREATED_AT < now()"
+ :params []}
+ (expand-with-field-filter-param-on-datetime-field
+ "SELECT * FROM ORDERS WHERE TOTAL > 100 [[AND {{created}} #]] AND CREATED_AT < now()"
+ nil))))))
+
(deftest ^:parallel expand-exclude-field-filter-test
(mt/with-driver :h2
(testing "exclude date parts"
--
GitLab