From 37873844dcdc73142e3b4ef2efffd5c9c177e543 Mon Sep 17 00:00:00 2001
From: Tom Robinson <tlrobinson@gmail.com>
Date: Wed, 2 Oct 2019 13:36:48 -0700
Subject: [PATCH] Fix public questions with required parameters (#10794)
---
.../components/LoadingAndErrorWrapper.jsx | 5 +-
frontend/src/metabase/meta/types/Dataset.js | 1 +
.../public/containers/PublicQuestion.jsx | 12 ++-
src/metabase/api/dataset.clj | 9 +-
src/metabase/api/embed.clj | 13 ++-
src/metabase/api/public.clj | 30 +++++--
src/metabase/api/routes.clj | 8 +-
src/metabase/query_processor/error_type.clj | 87 +++++++++++++++++++
.../middleware/catch_exceptions.clj | 6 +-
.../middleware/parameters/native/values.clj | 14 +--
.../middleware/process_userland_query.clj | 2 +-
test/metabase/api/embed_test.clj | 3 +-
test/metabase/api/public_test.clj | 35 ++++++++
13 files changed, 190 insertions(+), 35 deletions(-)
create mode 100644 src/metabase/query_processor/error_type.clj
diff --git a/frontend/src/metabase/components/LoadingAndErrorWrapper.jsx b/frontend/src/metabase/components/LoadingAndErrorWrapper.jsx
index af08de46963..a7c9f5a7771 100644
--- a/frontend/src/metabase/components/LoadingAndErrorWrapper.jsx
+++ b/frontend/src/metabase/components/LoadingAndErrorWrapper.jsx
@@ -42,9 +42,10 @@ export default class LoadingAndErrorWrapper extends Component {
// NOTE Atte Keinänen 5/10/17 Dashboard API endpoint returns the error as JSON with `message` field
(error.data && (error.data.message ? error.data.message : error.data)) ||
error.statusText ||
- error.message;
+ error.message ||
+ error;
- if (!errorMessage || typeof errorMessage === "object") {
+ if (!errorMessage || typeof errorMessage !== "string") {
errorMessage = t`An error occurred`;
}
return errorMessage;
diff --git a/frontend/src/metabase/meta/types/Dataset.js b/frontend/src/metabase/meta/types/Dataset.js
index faea3fa5644..02ce487463d 100644
--- a/frontend/src/metabase/meta/types/Dataset.js
+++ b/frontend/src/metabase/meta/types/Dataset.js
@@ -41,4 +41,5 @@ export type DatasetData = {
export type Dataset = {
data: DatasetData,
json_query: DatasetQuery,
+ error?: string,
};
diff --git a/frontend/src/metabase/public/containers/PublicQuestion.jsx b/frontend/src/metabase/public/containers/PublicQuestion.jsx
index a248584747b..02907e450e5 100644
--- a/frontend/src/metabase/public/containers/PublicQuestion.jsx
+++ b/frontend/src/metabase/public/containers/PublicQuestion.jsx
@@ -184,18 +184,26 @@ export default class PublicQuestion extends Component {
/>
);
+ const parameters = card && getParametersWithExtras(card);
+
return (
<EmbedFrame
name={card && card.name}
description={card && card.description}
- parameters={card && getParametersWithExtras(card)}
+ parameters={parameters}
actionButtons={actionButtons}
parameterValues={parameterValues}
setParameterValue={this.setParameterValue}
>
- <LoadingAndErrorWrapper loading={!result} noWrapper>
+ <LoadingAndErrorWrapper
+ className="flex-full"
+ loading={!result}
+ error={typeof result === "string" ? result : null}
+ noWrapper
+ >
{() => (
<Visualization
+ error={result && result.error}
rawSeries={[{ card: card, data: result && result.data }]}
className="full flex-full z1"
onUpdateVisualizationSettings={settings =>
diff --git a/src/metabase/api/dataset.clj b/src/metabase/api/dataset.clj
index 34a3f15b321..a89457ba743 100644
--- a/src/metabase/api/dataset.clj
+++ b/src/metabase/api/dataset.clj
@@ -15,6 +15,7 @@
[metabase.query-processor :as qp]
[metabase.query-processor
[async :as qp.async]
+ [error-type :as qp.error-type]
[util :as qputil]]
[metabase.query-processor.middleware.constraints :as constraints]
[metabase.util
@@ -102,7 +103,7 @@
(defn- as-format-response
"Return a response containing the `results` of a query in the specified format."
{:style/indent 1, :arglists '([export-format results])}
- [export-format {{:keys [rows cols]} :data, :keys [status], :as response}]
+ [export-format {{:keys [rows cols]} :data, :keys [status error], error-type :error_type, :as response}]
(api/let-404 [export-conf (ex/export-formats export-format)]
(if (= status :completed)
;; successful query, send file
@@ -114,8 +115,10 @@
"Content-Disposition" (format "attachment; filename=\"query_result_%s.%s\""
(du/date->iso-8601) (:ext export-conf))}}
;; failed query, send error message
- {:status 500
- :body (:error response)})))
+ {:status (if (qp.error-type/server-error? error-type)
+ 500
+ 400)
+ :body error})))
(s/defn as-format-async
"Write the results of an async query to API `respond` or `raise` functions in `export-format`. `in-chan` should be a
diff --git a/src/metabase/api/embed.clj b/src/metabase/api/embed.clj
index a789d98a8ae..622973dd926 100644
--- a/src/metabase/api/embed.clj
+++ b/src/metabase/api/embed.clj
@@ -67,8 +67,8 @@
(defn- validate-param-sets
"Validate that sets of params passed as part of the JWT token and by the user (as query params, i.e. as part of the
- URL) are valid for the OBJECT-EMBEDDING-PARAMS. TOKEN-PARAMS and USER-PARAMS should be sets of all valid param keys
- specified in the JWT or by the user, respectively."
+ URL) are valid for the `object-embedding-params`. `token-params` and `user-params` should be sets of all valid param
+ keys specified in the JWT or by the user, respectively."
[object-embedding-params token-params user-params]
;; TODO - maybe make this log/debug once embedding is wrapped up
(log/debug "Validating params for embedded object:\n"
@@ -108,7 +108,7 @@
param))
(s/defn ^:private remove-locked-and-disabled-params
- "Remove the `:parameters` for DASHBOARD-OR-CARD that listed as `disabled` or `locked` in the `embedding-params`
+ "Remove the `:parameters` for `dashboard-or-card` that listed as `disabled` or `locked` in the `embedding-params`
whitelist, or not present in the whitelist. This is done so the frontend doesn't display widgets for params the user
can't set."
[dashboard-or-card, embedding-params :- su/EmbeddingParams]
@@ -144,7 +144,7 @@
:default (:default tag)}))
(defn- add-implicit-card-parameters
- "Add template tag parameter information to CARD's `:parameters`."
+ "Add template tag parameter information to `card`'s `:parameters`."
[card]
(update card :parameters concat (template-tag-parameters card)))
@@ -197,8 +197,8 @@
;;; ---------------------------- Card Fns used by both /api/embed and /api/preview_embed -----------------------------
(defn card-for-unsigned-token
- "Return the info needed for embedding about Card specified in TOKEN.
- Additional CONSTRAINTS can be passed to the `public-card` function that fetches the Card."
+ "Return the info needed for embedding about Card specified in `token`. Additional `constraints` can be passed to the
+ `public-card` function that fetches the Card."
{:style/indent 1}
[unsigned-token & {:keys [embedding-params constraints]}]
(let [card-id (eu/get-in-unsigned-token-or-throw unsigned-token [:resource :question])
@@ -332,7 +332,6 @@
(dashboard-for-unsigned-token unsigned, :constraints {:enable_embedding true})))
-
(defn- card-for-signed-token-async
"Fetch the results of running a Card belonging to a Dashboard using a JSON Web Token signed with the
`embedding-secret-key`.
diff --git a/src/metabase/api/public.clj b/src/metabase/api/public.clj
index 4b70d2b8d42..d350220a1f3 100644
--- a/src/metabase/api/public.clj
+++ b/src/metabase/api/public.clj
@@ -25,6 +25,7 @@
[dimension :refer [Dimension]]
[field :refer [Field]]
[params :as params]]
+ [metabase.query-processor.error-type :as qp.error-type]
[metabase.query-processor.middleware.constraints :as constraints]
[metabase.util
[embed :as embed]
@@ -42,14 +43,14 @@
;;; -------------------------------------------------- Public Cards --------------------------------------------------
(defn- remove-card-non-public-columns
- "Remove everyting from public CARD that shouldn't be visible to the general public."
+ "Remove everyting from public `card` that shouldn't be visible to the general public."
[card]
(card/map->CardInstance
(u/select-nested-keys card [:id :name :description :display :visualization_settings
[:dataset_query :type [:native :template-tags]]])))
(defn public-card
- "Return a public Card matching key-value CONDITIONS, removing all columns that should not be visible to the general
+ "Return a public Card matching key-value `conditions`, removing all columns that should not be visible to the general
public. Throws a 404 if the Card doesn't exist."
[& conditions]
(-> (api/check-404 (apply db/select-one [Card :id :dataset_query :description :display :name :visualization_settings]
@@ -66,13 +67,24 @@
(api/check-public-sharing-enabled)
(card-with-uuid uuid))
-(defn- transform-results [results]
- (if (= (:status results) :failed)
- ;; if the query failed instead of returning anything about the query just return a generic error message
- (ex-info "An error occurred while running the query." {:status-code 400})
- (u/select-nested-keys
- results
- [[:data :cols :rows :rows_truncated :insights] [:json_query :parameters] :error :status])))
+(defmulti ^:private transform-results
+ {:arglists '([results])}
+ :status)
+
+(defmethod transform-results :default
+ [results]
+ (u/select-nested-keys
+ results
+ [[:data :cols :rows :rows_truncated :insights] [:json_query :parameters] :status]))
+
+(defmethod transform-results :failed
+ [{:keys [error], error-type :error_type, :as results}]
+ ;; if the query failed instead, unless the error type is specified and is EXPLICITLY allowed to be shown for embeds,
+ ;; instead of returning anything about the query just return a generic error message
+ (merge
+ (select-keys results [:status :error :error_type])
+ (when-not (qp.error-type/show-in-embeds? error-type)
+ {:error (tru "An error occurred while running the query.")})))
(defn run-query-for-card-with-id-async
"Run the query belonging to Card with `card-id` with `parameters` and other query options (e.g. `:constraints`).
diff --git a/src/metabase/api/routes.clj b/src/metabase/api/routes.clj
index 2a2be750a4d..b28754f522b 100644
--- a/src/metabase/api/routes.clj
+++ b/src/metabase/api/routes.clj
@@ -41,21 +41,21 @@
[metabase.util.i18n :refer [deferred-tru]]))
(def ^:private +generic-exceptions
- "Wrap ROUTES so any Exception thrown is just returned as a generic 400, to prevent details from leaking in public
+ "Wrap `routes` so any Exception thrown is just returned as a generic 400, to prevent details from leaking in public
endpoints."
middleware.exceptions/genericize-exceptions)
(def ^:private +message-only-exceptions
- "Wrap ROUTES so any Exception thrown is just returned as a 400 with only the message from the original
+ "Wrap `routes` so any Exception thrown is just returned as a 400 with only the message from the original
Exception (i.e., remove the original stacktrace), to prevent details from leaking in public endpoints."
middleware.exceptions/message-only-exceptions)
(def ^:private +apikey
- "Wrap ROUTES so they may only be accessed with proper apikey credentials."
+ "Wrap `routes` so they may only be accessed with a correct API key header."
middleware.auth/enforce-api-key)
(def ^:private +auth
- "Wrap ROUTES so they may only be accessed with proper authentiaction credentials."
+ "Wrap `routes` so they may only be accessed with proper authentication credentials."
middleware.auth/enforce-authentication)
(defroutes ^{:doc "Ring routes for API endpoints."} routes
diff --git a/src/metabase/query_processor/error_type.clj b/src/metabase/query_processor/error_type.clj
new file mode 100644
index 00000000000..532721325b6
--- /dev/null
+++ b/src/metabase/query_processor/error_type.clj
@@ -0,0 +1,87 @@
+(ns metabase.query-processor.error-type
+ "A hierarchy of all QP error types. Ideally all QP exceptions should be `ex-data` maps with an `:type` key whose value
+ is one of the types here. If you see an Exception in QP code that doesn't return an `:type`, add it!")
+
+(def ^:private hierarchy
+ (-> (make-hierarchy)
+ ;; errors deriving from `:client-error` are the equivalent of HTTP 4xx client status codes
+ (derive :client-error :error)
+ (derive :invalid-query :client-error)
+ ;; errors deriving from `:unexpected-server-error` are the equivalent of HTTP 5xx status codes
+ (derive :unexpected-server-error :error)
+ (derive :unexpected-qp-error :unexpected-server-error)
+ (derive :unexpected-db-error :unexpected-server-error)))
+
+(defn known-error-types
+ "Set of all known QP error types."
+ []
+ (descendants hierarchy :error))
+
+(defn known-error-type?
+ "Is `error-type` a known QP error type (i.e., one defined with `deferror` above)?"
+ [error-type]
+ (isa? hierarchy error-type :error))
+
+(defn show-in-embeds?
+ "Should errors of this type be shown to users of Metabase in embedded Cards or Dashboards? Normally, we return a
+ generic 'Query Failed' error message for embedded queries, so as not to leak information. Some errors (like missing
+ parameter errors), however, should be shown even in these situations."
+ [error-type]
+ (isa? hierarchy error-type :show-in-embeds?))
+
+(defmacro ^:private deferror
+ {:style/indent 1}
+ [error-name docstring & {:keys [parent show-in-embeds?]}]
+ {:pre [(some? parent)]}
+ `(do
+ (def ~error-name ~docstring ~(keyword error-name))
+ (alter-var-root #'hierarchy derive ~(keyword error-name) ~(keyword parent))
+ ~(when show-in-embeds?
+ `(alter-var-root #'hierarchy derive ~(keyword error-name) :show-in-embeds?))))
+
+;;;; ### Client Errors
+
+(deferror client
+ "Generic ancestor type for all errors with the query map itself. Equivalent of a HTTP 4xx status code."
+ :parent :error)
+
+(defn client-error?
+ "Is `error-type` a client error type, the equivalent of an HTTP 4xx status code?"
+ [error-type]
+ (isa? hierarchy error-type :client))
+
+(deferror missing-required-permissions
+ "The current user does not have required permissions to run the current query."
+ :parent client)
+
+(deferror invalid-query
+ "Generic ancestor type for errors with the query map itself."
+ :parent client)
+
+(deferror missing-required-parameter
+ "The query is parameterized, and a required parameter was not supplied."
+ :parent invalid-query
+ :show-in-embeds? true)
+
+;;;; ### Server-Side Errors
+
+(deferror server
+ "Generic ancestor type for all unexpected server-side errors. Equivalent of a HTTP 5xx status code."
+ :parent :error)
+
+(defn server-error?
+ "Is `error-type` a server error type, the equivalent of an HTTP 5xx status code?"
+ [error-type]
+ (isa? hierarchy error-type :server))
+
+;;;; #### QP Errors
+
+(deferror qp
+ "Generic ancestor type for all unexpected errors (e.g., uncaught Exceptions) in QP code."
+ :parent server)
+
+;;;; #### Data Warehouse (DB) Errors
+
+(deferror db
+ "Generic ancestor type for all unexpected errors returned or thrown by a data warehouse when running a query."
+ :parent server)
diff --git a/src/metabase/query_processor/middleware/catch_exceptions.clj b/src/metabase/query_processor/middleware/catch_exceptions.clj
index fbd1ee48156..35a6cd3c64e 100644
--- a/src/metabase/query_processor/middleware/catch_exceptions.clj
+++ b/src/metabase/query_processor/middleware/catch_exceptions.clj
@@ -1,6 +1,8 @@
(ns metabase.query-processor.middleware.catch-exceptions
"Middleware for catching exceptions thrown by the query processor and returning them in a friendlier format."
- (:require [metabase.query-processor.interface :as qp.i]
+ (:require [metabase.query-processor
+ [error-type :as qp.error-type]
+ [interface :as qp.i]]
[metabase.util :as u]
schema.utils)
(:import clojure.lang.ExceptionInfo
@@ -85,6 +87,8 @@
(when-let [error-msg (and (= error-type :schema.core/error)
(explain-schema-validation-error error))]
{:error error-msg})
+ (when (qp.error-type/known-error-type? error-type)
+ {:error_type error-type})
{:ex-data (dissoc data :schema)})))
diff --git a/src/metabase/query_processor/middleware/parameters/native/values.clj b/src/metabase/query_processor/middleware/parameters/native/values.clj
index 1c12f18b448..4ab025e5c38 100644
--- a/src/metabase/query_processor/middleware/parameters/native/values.clj
+++ b/src/metabase/query_processor/middleware/parameters/native/values.clj
@@ -10,6 +10,7 @@
:value \"2015-01-01~2016-09-01\"}}}"
(:require [clojure.string :as str]
[metabase.models.field :refer [Field]]
+ [metabase.query-processor.error-type :as qp.error-type]
[metabase.query-processor.middleware.parameters.native.interface :as i]
[metabase.util
[i18n :as ui18n :refer [tru]]
@@ -18,8 +19,7 @@
[toucan.db :as db])
(:import java.text.NumberFormat
java.util.UUID
- [metabase.query_processor.middleware.parameters.native.interface CommaSeparatedNumbers FieldFilter
- MultipleValues]))
+ [metabase.query_processor.middleware.parameters.native.interface CommaSeparatedNumbers FieldFilter MultipleValues]))
(def ^:private ParamType
(s/enum :number
@@ -68,13 +68,17 @@
;;; FieldFilter Params (Field Filters) (e.g. WHERE {{x}})
+(defn- missing-required-param-exception [param-display-name]
+ (ex-info (tru "You''ll need to pick a value for ''{0}'' before this query can run." param-display-name)
+ {:type qp.error-type/missing-required-parameter}))
+
(s/defn ^:private default-value-for-field-filter
"Return the default value for a FieldFilter (`:type` = `:dimension`) param defined by the map `tag`, if one is set."
[tag :- TagParam]
(when (and (:required tag) (not (:default tag)))
- (throw (Exception. (tru "''{0}'' is a required param." (:display-name tag)))))
+ (throw (missing-required-param-exception (:display-name tag))))
(when-let [default (:default tag)]
- {:type (:widget-type tag :dimension) ; widget-type is the actual type of the default value if set
+ {: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}))
@@ -123,7 +127,7 @@
[{:keys [default display-name required]} :- TagParam]
(or default
(when required
- (throw (Exception. (tru "''{0}'' is a required param." display-name))))))
+ (throw (missing-required-param-exception display-name)))))
;;; Parsing Values
diff --git a/src/metabase/query_processor/middleware/process_userland_query.clj b/src/metabase/query_processor/middleware/process_userland_query.clj
index 89047b931ee..2906d760c89 100644
--- a/src/metabase/query_processor/middleware/process_userland_query.clj
+++ b/src/metabase/query_processor/middleware/process_userland_query.clj
@@ -68,7 +68,7 @@
:cols []}}
;; include stacktrace and preprocessed/native stages of the query if available in the response which should
;; make debugging queries a bit easier
- (-> (select-keys result [:stacktrace :preprocessed :native])
+ (-> (select-keys result [:stacktrace :preprocessed :native :error_type])
(m/dissoc-in [:preprocessed :info]))))
diff --git a/test/metabase/api/embed_test.clj b/test/metabase/api/embed_test.clj
index 1b70fd33782..dfab4c3e6fe 100644
--- a/test/metabase/api/embed_test.clj
+++ b/test/metabase/api/embed_test.clj
@@ -428,7 +428,8 @@
;; but if the card has an invalid query we should just get a generic "query failed" exception (rather than leaking
;; query info)
(expect
- "An error occurred while running the query."
+ {:status "failed"
+ :error "An error occurred while running the query."}
(tu.log/suppress-output
(with-embedding-enabled-and-new-secret-key
(with-temp-dashcard [dashcard {:dash {:enable_embedding true}
diff --git a/test/metabase/api/public_test.clj b/test/metabase/api/public_test.clj
index b6b65a73d3e..365aa64061b 100644
--- a/test/metabase/api/public_test.clj
+++ b/test/metabase/api/public_test.clj
@@ -191,6 +191,41 @@
:parameters (json/encode [{:name "Venue ID", :slug "venue_id", :type "id", :value 2}]))
[:json_query :parameters]))))
+;; Cards with required params
+(defn- do-with-required-param-card [f]
+ (tu/with-temporary-setting-values [enable-public-sharing true]
+ (with-temp-public-card [{uuid :public_uuid}
+ {:dataset_query
+ {:database (data/id)
+ :type :native
+ :native {:query "SELECT count(*) FROM venues v WHERE price = {{price}}"
+ :template-tags {"price" {:name "price"
+ :display-name "Price"
+ :type :number
+ :required true}}}}}]
+ (f uuid))))
+
+;; should be able to run a Card with a required param
+(expect
+ [[22]]
+ (do-with-required-param-card
+ (fn [uuid]
+ (qp.test/rows
+ (http/client :get 200 (str "public/card/" uuid "/query")
+ :parameters (json/encode [{:type "category"
+ :target [:variable [:template-tag "price"]]
+ :value 1}]))))))
+
+;; if you're missing a required param, the error message should get passed thru, rather than the normal generic 'Query
+;; Failed' message that we show for most embedding errors
+(expect
+ {:status "failed"
+ :error "You'll need to pick a value for 'Price' before this query can run."
+ :error_type "missing-required-parameter"}
+ (do-with-required-param-card
+ (fn [uuid]
+ (http/client :get 200 (str "public/card/" uuid "/query")))))
+
;; make sure CSV (etc.) downloads take editable params into account (#6407)
(defn- card-with-date-field-filter []
--
GitLab