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

Dashboard QP endpoints should ignore :default values and treat them as no... 

Dashboard QP endpoints should ignore :default values and treat them as no value and compile Field filters to `1 = 1` (#20503)

* Fix

* More logging

* Fix unbalanced parens

* Tweak logging

* Add another test

* Tweak test to use :string/=

* More logging

* Updated fix for #20493

* Enable test

* Try disabling the #19494 tests to see what happens.

* Rework #19494 test
parent 094534e0
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
Showing
with 84 additions and 62 deletions
frontend/test/metabase/scenarios/dashboard-filters-sql/dashboard-filters-sql-text-category.cy.spec.js
+ 1
− 1
View file @ f6509034
......@@ -55,7 +55,7 @@ Object.entries(DASHBOARD_SQL_TEXT_FILTERS).forEach(
});
});
it.skip(`should work for "${filter}" when set as the default filter and when that filter is removed (metabase#20493)`, () => {
it(`should work for "${filter}" when set as the default filter and when that filter is removed (metabase#20493)`, () => {
cy.findByText("Default value")
.next()
.click();
......
src/metabase/driver/common/parameters/operators.clj
+ 4
− 3
View file @ f6509034
......@@ -10,6 +10,7 @@
(:require [metabase.mbql.schema :as mbql.s]
[metabase.models.params :as params]
[metabase.query-processor.error-type :as qp.error-type]
[metabase.util.i18n :refer [tru]]
[schema.core :as s]))
(s/defn ^:private operator-arity :- (s/maybe (s/enum :unary :binary :variadic))
......@@ -39,14 +40,14 @@
(maybe-arity-error 2)
:variadic
(when-not (seq param-value)
(throw (ex-info (format "No values provided for operator: %s" param-type)
(when-not (sequential? param-value)
(throw (ex-info (tru "Invalid values provided for operator: {0}" param-type)
{:param-type param-type
:param-value param-value
:field-id (second field)
:type qp.error-type/invalid-parameter})))
(throw (ex-info (format "Unrecognized operation: %s" param-type)
(throw (ex-info (tru "Unrecognized operation: {0}" param-type)
{:param-type param-type
:param-value param-value
:field-id (second field)
......
src/metabase/driver/sql/parameters/substitute.clj
+ 4
− 0
View file @ f6509034
(ns metabase.driver.sql.parameters.substitute
(:require [clojure.string :as str]
[clojure.tools.logging :as log]
[metabase.driver :as driver]
[metabase.driver.common.parameters :as i]
[metabase.driver.sql.parameters.substitution :as substitution]
[metabase.query-processor.error-type :as error-type]
[metabase.util :as u]
[metabase.util.i18n :refer [tru]]))
(defn- substitute-field-filter [[sql args missing] in-optional? k {:keys [_field value], :as v}]
......@@ -77,6 +79,7 @@
{\"bird_type\" \"Steller's Jay\"})
;; -> [\"select * from foobars where bird_type = ?\" [\"Steller's Jay\"]]"
[parsed-query param->value]
(log/tracef "Substituting params\n%s\nin query:\n%s" (u/pprint-to-str param->value) (u/pprint-to-str parsed-query))
(let [[sql args missing] (try
(substitute* param->value parsed-query false)
(catch Throwable e
......@@ -85,6 +88,7 @@
:params param->value
:parsed-query parsed-query}
e))))]
(log/tracef "=>%s\n%s" sql (pr-str args))
(when (seq missing)
(throw (ex-info (tru "Cannot run the query: missing required parameters: {0}" (set missing))
{:type error-type/missing-required-parameter
......
src/metabase/query_processor/card.clj
+ 3
− 1
View file @ f6509034
......@@ -25,7 +25,7 @@
[toucan.db :as db]))
(defn- query-magic-ttl
"Compute a 'magic' cache TTL time (in seconds) for QUERY by multipling its historic average execution times by the
"Compute a 'magic' cache TTL time (in seconds) for `query` by multipling its historic average execution times by the
`query-caching-ttl-ratio`. If the TTL is less than a second, this returns `nil` (i.e., the cache should not be
utilized.)"
[query]
......@@ -206,4 +206,6 @@
(api/check-not-archived card)
(when (seq parameters)
(validate-card-parameters card-id (normalize/normalize-fragment [:parameters] parameters)))
(log/tracef "Running query for Card %d:\n%s" card-id
(u/pprint-to-str query))
(run query info)))
src/metabase/query_processor/dashboard.clj
+ 16
− 2
View file @ f6509034
......@@ -52,7 +52,9 @@
mapping))
(:mappings matching-param))
(log/tracef "Parameter has no mapping for Card %d; skipping" card-id))]
(log/tracef "Found matching mapping for Card %d:\n%s" card-id (u/pprint-to-str matching-mapping))
(log/tracef "Found matching mapping for Card %d, Dashcard %d:\n%s"
card-id dashcard-id
(u/pprint-to-str (update matching-mapping :dashcard #(select-keys % [:id :parameter_mappings]))))
;; if `request-param` specifies type, then validate that the type is allowed
(when (:type request-param)
(qp.card/check-allowed-parameter-value-type
......@@ -111,7 +113,16 @@
(log/tracef "Resolving Dashboard %d Card %d query request parameters" dashboard-id card-id)
(let [request-params (normalize/normalize-fragment [:parameters] request-params)
dashboard (api/check-404 (db/select-one Dashboard :id dashboard-id))
dashboard-param-id->param (dashboard/dashboard->resolved-params dashboard)
dashboard-param-id->param (into {}
;; remove the `:default` values from Dashboard params. We don't ACTUALLY want to
;; use these values ourselves -- the expectation is that the frontend will pass
;; them in as an actual `:value` if it wants to use them. If we leave them
;; around things get confused and it prevents us from actually doing the
;; expected `1 = 1` substitution for Field filters. See comments in #20503 for
;; more information.
(map (fn [[param-id param]]
[param-id (dissoc param :default)]))
(dashboard/dashboard->resolved-params dashboard))
request-param-id->param (into {} (map (juxt :id identity)) request-params)
merged-parameters (vals (merge (dashboard-param-defaults dashboard-param-id->param card-id)
request-param-id->param))]
......@@ -151,6 +162,9 @@
options
{:parameters resolved-params
:dashboard-id dashboard-id})]
(log/tracef "Running Query for Dashboard %d, Card %d, Dashcard %d with options\n%s"
dashboard-id card-id dashcard-id
(u/pprint-to-str options))
;; we've already validated our parameters, so we don't need the [[qp.card]] namespace to do it again
(binding [qp.card/*allow-arbitrary-mbql-parameters* true]
(m/mapply qp.card/run-query-for-card-async card-id export-format options))))
src/metabase/query_processor/middleware/parameters.clj
+ 2
− 4
View file @ f6509034
......@@ -7,7 +7,6 @@
[metabase.mbql.normalize :as normalize]
[metabase.mbql.schema :as mbql.s]
[metabase.mbql.util :as mbql.u]
[metabase.query-processor.interface :as i]
[metabase.query-processor.middleware.parameters.mbql :as params.mbql]
[metabase.query-processor.middleware.parameters.native :as params.native]
[metabase.util :as u]
......@@ -19,7 +18,7 @@
(s/defn ^:private move-join-condition-to-source-query :- mbql.s/Join
"Joins aren't allowed to have `:filter` clauses, generated by the `expand-mbql-params` function below. Move the filter
clause into the `:source-query`, converting `:source-table` to a source query if needed."
[{:keys [source-table source-query], filter-clause :filter, :as join}]
[{:keys [source-table], filter-clause :filter, :as join}]
(if-not filter-clause
join
(if source-table
......@@ -80,8 +79,7 @@
"If any parameters were supplied then substitute them into the query."
[query]
(u/prog1 (expand-parameters query)
(when (and (not i/*disable-qp-logging*)
(not= <> query))
(when (not= <> query)
(when-let [diff (second (data/diff query <>))]
(log/tracef "\n\nSubstituted params:\n%s\n" (u/pprint-to-str 'cyan diff))))))
......
test/metabase/driver/sql/parameters/substitute_test.clj
+ 2
− 3
View file @ f6509034
......@@ -167,8 +167,7 @@
(substitute query {"param" (i/map->FieldFilter
{:field (Field (mt/id :venues field))
:value {:type operator
:value value}})}))))))))
:value value}})})))))))))
;;; -------------------------------------------- Referenced Card Queries ---------------------------------------------
......@@ -176,7 +175,7 @@
(testing "Referenced card query substitution"
(let [query ["SELECT * FROM " (param "#123")]]
(is (= ["SELECT * FROM (SELECT 1 `x`)" []]
(substitute query {"#123" (i/map->ReferencedCardQuery {:card-id 123, :query "SELECT 1 `x`"})})))))))
(substitute query {"#123" (i/map->ReferencedCardQuery {:card-id 123, :query "SELECT 1 `x`"})}))))))
;;; --------------------------------------------- Native Query Snippets ----------------------------------------------
......
test/metabase/query_processor/dashboard_test.clj
+ 52
− 48
View file @ f6509034
......@@ -7,8 +7,7 @@
[metabase.query-processor :as qp]
[metabase.query-processor.card-test :as qp.card-test]
[metabase.query-processor.dashboard :as qp.dashboard]
[metabase.test :as mt]
[schema.core :as s]))
[metabase.test :as mt]))
;; there are more tests in [[metabase.api.dashboard-test]]
......@@ -47,43 +46,6 @@
(qp/process-query (assoc query :async? false) info))
options)))
(deftest merge-defaults-from-mappings-test
(testing "DashboardCard parameter mappings can specify default values, and we should respect those"
(mt/with-temp* [Card [{card-id :id} {:dataset_query {:database (mt/id)
:type :native
:native {:query "SELECT {{x}}"
:template-tags {"x" {:id "abc"
:name "x"
:display-name "X"
:type :number
:required true}}}}}]
;; `:name` doesn't matter for Dashboard parameter mappings.
Dashboard [{dashboard-id :id} {:parameters [{:name "A_DIFFERENT_X"
:slug "x_slug"
:id "__X__"
:type :category
:default 3}]}]
DashboardCard [{dashcard-id :id} {:card_id card-id
:dashboard_id dashboard-id
:parameter_mappings [{:parameter_id "__X__"
:card_id card-id
:target [:variable [:template-tag "x"]]}]}]]
(testing "param resolution code should include default values"
(is (schema= [(s/one
{:type (s/eq :category)
:id (s/eq "__X__")
:default (s/eq 3)
:target (s/eq [:variable [:template-tag "x"]])
s/Keyword s/Any}
"parameter")]
(#'qp.dashboard/resolve-params-for-query dashboard-id card-id dashcard-id nil))))
(testing "make sure it works end-to-end"
(is (schema= {:status (s/eq :completed)
:data {:rows (s/eq [[3]])
s/Keyword s/Any}
s/Keyword s/Any}
(run-query-for-dashcard dashboard-id card-id dashcard-id)))))))
(deftest default-value-precedence-test-field-filters
(testing "If both Dashboard and Card have default values for a Field filter parameter, Card defaults should take precedence\n"
(mt/dataset sample-dataset
......@@ -168,16 +130,14 @@
"filters for the DashCard we're running a query for (#19494)")
(mt/dataset sample-dataset
(mt/with-temp* [Card [{card-id :id} {:dataset_query (mt/mbql-query products {:aggregation [[:count]]})}]
Dashboard [{dashboard-id :id} {:parameters [{:name "Category (DashCard 1)"
:slug "category_1"
:id "CATEGORY_1"
:type :string/=
:default ["Doohickey"]}
Dashboard [{dashboard-id :id} {:parameters [{:name "Category (DashCard 1)"
:slug "category_1"
:id "CATEGORY_1"
:type :string/=}
{:name "Category (DashCard 2)"
:slug "category_2"
:id "CATEGORY_2"
:type :string/=
:default ["Gadget"]}]}]
:type :string/=}]}]
DashboardCard [{dashcard-1-id :id} {:card_id card-id
:dashboard_id dashboard-id
:parameter_mappings [{:parameter_id "CATEGORY_1"
......@@ -190,7 +150,51 @@
:target [:dimension (mt/$ids $products.category)]}]}]]
(testing "DashCard 1 (Category = Doohickey)"
(is (= [[42]]
(mt/rows (run-query-for-dashcard dashboard-id card-id dashcard-1-id)))))
(mt/rows (run-query-for-dashcard dashboard-id card-id dashcard-1-id
:parameters [{:id "CATEGORY_1"
:value ["Doohickey"]}]))))
(testing "DashCard 2 should ignore DashCard 1 params"
(is (= [[200]]
(mt/rows (run-query-for-dashcard dashboard-id card-id dashcard-2-id
:parameters [{:id "CATEGORY_1"
:value ["Doohickey"]}]))))))
(testing "DashCard 2 (Category = Gadget)"
(is (= [[53]]
(mt/rows (run-query-for-dashcard dashboard-id card-id dashcard-2-id)))))))))
(mt/rows (run-query-for-dashcard dashboard-id card-id dashcard-2-id
:parameters [{:id "CATEGORY_2"
:value ["Gadget"]}]))))
(testing "DashCard 1 should ignore DashCard 2 params"
(is (= [[200]]
(mt/rows (run-query-for-dashcard dashboard-id card-id dashcard-1-id
:parameters [{:id "CATEGORY_2"
:value ["Gadget"]}]))))))))))
(deftest field-filters-should-work-if-no-value-is-specified-test
(testing "Field Filters should work if no value is specified (#20493)"
(mt/dataset sample-dataset
(let [query (mt/native-query {:query "SELECT COUNT(*) FROM \"PRODUCTS\" WHERE {{cat}}"
:template-tags {"cat" {:id "__cat__"
:name "cat"
:display-name "Cat"
:type :dimension
:dimension [:field (mt/id :products :category) nil]
:widget-type :string/=
:default nil}}})]
(mt/with-native-query-testing-context query
(is (= [[200]]
(mt/rows (qp/process-query query)))))
(mt/with-temp* [Card [{card-id :id} {:dataset_query query}]
Dashboard [{dashboard-id :id} {:parameters [{:name "Text"
:slug "text"
:id "_text_"
:type :string/=
:sectionId "string"
:default ["Doohickey"]}]}]
DashboardCard [{dashcard-id :id} {:parameter_mappings [{:parameter_id "_text_"
:card_id card-id
:target [:dimension [:template-tag "cat"]]}]
:card_id card-id
:visualization_settings {}
:dashboard_id dashboard-id}]]
(is (= [[200]]
(mt/rows (run-query-for-dashcard dashboard-id card-id dashcard-id)))))))))
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