Skip to content

Commit

Permalink
backport "[QP] When querying a bare model with parameters, wrap an ex…
Browse files Browse the repository at this point in the history
…tra stage (#39963)" (#40047)

Fixes #39940.

Co-authored-by: Braden Shepherdson <braden@metabase.com>
  • Loading branch information
metabase-bot[bot] and bshepherdson committed Mar 13, 2024
1 parent 01d8e7d commit 0a1e15d
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 4 deletions.
12 changes: 9 additions & 3 deletions src/metabase/query_processor/middleware/parameters.clj
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,17 @@

(defn- move-top-level-params-to-inner-query
"Move any top-level parameters to the same level (i.e., 'inner query') as the query they affect."
[{:keys [parameters], query-type :type, :as outer-query}]
[{:keys [info parameters], query-type :type, :as outer-query}]
{:pre [(#{:query :native} query-type)]}
(cond-> (set/rename-keys outer-query {:parameters :user-parameters})
(seq parameters)
(assoc-in [query-type :parameters] parameters)))
;; TODO: Native models should be within scope of dashboard filters, by applying the filter on an outer stage.
;; That doesn't work, so the logic below requires MBQL queries only to fix the regression.
;; Native models don't actual get filtered even when linked to dashboard filters, but that's not a regression.
;; This can be fixed properly once this middleware is powered by MLv2. See #40011.
(and (seq parameters)
(:metadata/model-metadata info)
(= query-type :query)) (update query-type (fn [inner-query] {:source-query inner-query}))
(seq parameters) (assoc-in [query-type :parameters] parameters)))

(defn- expand-parameters
"Expand parameters in the `outer-query`, and if the query is using a native source query, expand params in that as
Expand Down
23 changes: 22 additions & 1 deletion test/metabase/query_processor/middleware/parameters_test.clj
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,28 @@
:native {:query "WOW", :parameters ["My Param"]}
:user-parameters ["My Param"]}
(#'parameters/move-top-level-params-to-inner-query
{:type :native, :native {:query "WOW"}, :parameters ["My Param"]}))))
{:type :native, :native {:query "WOW"}, :parameters ["My Param"]})))
(testing "when top-level query is a model"
(testing "and there are parameters, wrap it up as a :source-query"
(is (= {:type :query
:query {:source-query {:source-table 5}
:parameters ["My Param"]}
:info {:metadata/model-metadata []}
:user-parameters ["My Param"]}
(#'parameters/move-top-level-params-to-inner-query
{:type :query
:query {:source-table 5}
:parameters ["My Param"]
:info {:metadata/model-metadata []}}))))
(testing "without parameters, leave the model at the top level"
(is (= {:type :query
:query {:source-table 5
:parameters ["My Param"]}
:user-parameters ["My Param"]}
(#'parameters/move-top-level-params-to-inner-query
{:type :query
:query {:source-table 5}
:parameters ["My Param"]}))))))

(defn- substitute-params [query]
(letfn [(thunk []
Expand Down

0 comments on commit 0a1e15d

Please sign in to comment.