Fix public questions with required parameters (#10794)

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;

frontend/src/metabase/meta/types/Dataset.js

@@ -41,4 +41,5 @@ export type DatasetData = {
 export type Dataset = {
   data: DatasetData,
   json_query: DatasetQuery,
+  error?: string,
 };

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 =>

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

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`.

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`).

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

src/metabase/query_processor/error_type.clj

+(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)

src/metabase/query_processor/middleware/catch_exceptions.clj

 (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)})))
 
 

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

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]))))
 
 

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}

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 []