diff --git a/frontend/src/metabase/query_builder/components/QueryHeader.jsx b/frontend/src/metabase/query_builder/components/QueryHeader.jsx
index 8aa7ee0d02dd8cc4795a69a6ad4fe3618a53ee9e..ae177b31d2e25881ffbe31ac6ee704c42226b87f 100644
--- a/frontend/src/metabase/query_builder/components/QueryHeader.jsx
+++ b/frontend/src/metabase/query_builder/components/QueryHeader.jsx
@@ -63,8 +63,8 @@ export default class QueryHeader extends Component {
componentWillUnmount() {
clearTimeout(this.timeout);
- if (this.requesetPromise) {
- this.requesetPromise.cancel();
+ if (this.requestPromise) {
+ this.requestPromise.cancel();
}
}
@@ -90,8 +90,8 @@ export default class QueryHeader extends Component {
}
// TODO: reduxify
- this.requesetPromise = cancelable(CardApi.create(card));
- return this.requesetPromise.then(newCard => {
+ this.requestPromise = cancelable(CardApi.create(card));
+ return this.requestPromise.then(newCard => {
this.props.notifyCardCreatedFn(newCard);
this.setState({
@@ -115,8 +115,8 @@ export default class QueryHeader extends Component {
}
// TODO: reduxify
- this.requesetPromise = cancelable(CardApi.update(card));
- return this.requesetPromise.then(updatedCard => {
+ this.requestPromise = cancelable(CardApi.update(card));
+ return this.requestPromise.then(updatedCard => {
if (this.props.fromUrl) {
this.onGoBack();
return;
diff --git a/frontend/src/metabase/query_builder/components/dataref/TablePane.jsx b/frontend/src/metabase/query_builder/components/dataref/TablePane.jsx
index 206a720240a4325a4fa0aafe00b1bd9ec2860d57..e1d0104a659f21941b58590f0e2ae63fdfff238b 100644
--- a/frontend/src/metabase/query_builder/components/dataref/TablePane.jsx
+++ b/frontend/src/metabase/query_builder/components/dataref/TablePane.jsx
@@ -115,7 +115,7 @@ export default class TablePane extends Component {
<h1>{table.display_name}</h1>
{description}
{queryButton}
- { table.metrics.length > 0 &&
+ { table.metrics && (table.metrics.length > 0) &&
<ExpandableItemList
name="Metrics"
type="metrics"
@@ -123,7 +123,7 @@ export default class TablePane extends Component {
items={table.metrics}
/>
}
- { table.segments.length > 0 &&
+ { table.segments && (table.segments.length > 0) &&
<ExpandableItemList
name="Segments"
type="segments"
diff --git a/src/metabase/api/card.clj b/src/metabase/api/card.clj
index 12541c5dd35fff4213d8750dabf0008e5f72cc60..53df71031d685b3f36db501ab2a73c826276eed1 100644
--- a/src/metabase/api/card.clj
+++ b/src/metabase/api/card.clj
@@ -26,8 +26,10 @@
[query :as query]
[table :refer [Table]]
[view-log :refer [ViewLog]]]
+ [metabase.query-processor
+ [interface :as qpi]
+ [util :as qputil]]
[metabase.query-processor.middleware.cache :as cache]
- [metabase.query-processor.util :as qputil]
[metabase.util.schema :as su]
[ring.util.codec :as codec]
[schema.core :as s]
@@ -163,7 +165,7 @@
cards)))
-;;; ------------------------------------------------------------ /api/card & /api/card/:id endpoints ------------------------------------------------------------
+;;; ------------------------------------------------------------ Fetching a Card or Cards ------------------------------------------------------------
(def ^:private CardFilterOption
"Schema for a valid card filter option."
@@ -193,6 +195,17 @@
(filterv mi/can-read?)))) ; filterv because we want make sure all the filtering is done while current user perms set is still bound
+(api/defendpoint GET "/:id"
+ "Get `Card` with ID."
+ [id]
+ (-> (api/read-check Card id)
+ (hydrate :creator :dashboard_count :labels :can_write :collection)
+ (assoc :actor_id api/*current-user-id*)
+ (->> (events/publish-event! :card-read))
+ (dissoc :actor_id)))
+
+
+;;; ------------------------------------------------------------ Saving Cards ------------------------------------------------------------
(api/defendpoint POST "/"
"Create a new `Card`."
[:as {{:keys [dataset_query description display name visualization_settings collection_id]} :body}]
@@ -218,27 +231,63 @@
(events/publish-event! :card-create)))
-(api/defendpoint GET "/:id"
- "Get `Card` with ID."
- [id]
- (-> (api/read-check Card id)
- (hydrate :creator :dashboard_count :labels :can_write :collection)
- (assoc :actor_id api/*current-user-id*)
- (->> (events/publish-event! :card-read))
- (dissoc :actor_id)))
+;;; ------------------------------------------------------------ Updating Cards ------------------------------------------------------------
(defn- check-permissions-for-collection
"Check that we have permissions to add or remove cards from `Collection` with COLLECTION-ID."
[collection-id]
(api/check-403 (perms/set-has-full-permissions? @api/*current-user-permissions-set* (perms/collection-readwrite-path collection-id))))
+(defn- check-allowed-to-change-collection
+ "If we're changing the `collection_id` of the Card, make sure we have write permissions for the new group."
+ [card collection-id]
+ (when (and collection-id
+ (not= collection-id (:collection_id card)))
+ (check-permissions-for-collection collection-id)))
+
(defn check-data-permissions-for-query
"Check that we have *data* permissions to run the QUERY in question."
[query]
{:pre [(map? query)]}
(api/check-403 (perms/set-has-full-permissions-for-set? @api/*current-user-permissions-set* (card/query-perms-set query :read))))
-;; TODO - This endpoint desperately needs to be broken out into smaller, bite-sized chunks
+(defn- check-allowed-to-modify-query
+ "If the query is being modified, check that we have data permissions to run the query."
+ [card query]
+ (when (and query
+ (not= query (:dataset_query card)))
+ (check-data-permissions-for-query query)))
+
+(defn- check-allowed-to-unarchive
+ "When unarchiving a Card, make sure we have data permissions for the Card query before doing so."
+ [card archived?]
+ (when (and (false? archived?)
+ (:archived card))
+ (check-data-permissions-for-query (:dataset_query card))))
+
+(defn- check-allowed-to-change-embedding
+ "You must be a superuser to change the value of `enable_embedding` or `embedding_params`. Embedding must be enabled."
+ [card enable-embedding? embedding-params]
+ (when (or (and (not (nil? enable-embedding?))
+ (not= enable-embedding? (:enable_embedding card)))
+ (and embedding-params
+ (not= embedding-params (:embedding_params card))))
+ (api/check-embedding-enabled)
+ (api/check-superuser)))
+
+(defn- publish-card-update!
+ "Publish an event appropriate for the update(s) done to this CARD (`:card-update`, or archiving/unarchiving events)."
+ [card archived?]
+ (let [event (cond
+ ;; card was archived
+ (and archived?
+ (not (:archived card))) :card-archive
+ ;; card was unarchived
+ (and (false? archived?)
+ (:archived card)) :card-unarchive
+ :else :card-update)]
+ (events/publish-event! event (assoc card :actor_id api/*current-user-id*))))
+
(api/defendpoint PUT "/:id"
"Update a `Card`."
[id :as {{:keys [dataset_query description display name visualization_settings archived collection_id enable_embedding embedding_params], :as body} :body}]
@@ -252,44 +301,26 @@
embedding_params (s/maybe su/EmbeddingParams)
collection_id (s/maybe su/IntGreaterThanZero)}
(let [card (api/write-check Card id)]
- ;; if we're changing the `collection_id` of the Card, make sure we have write permissions for the new group
- (when (and (not (nil? collection_id)) (not= (:collection_id card) collection_id))
- (check-permissions-for-collection collection_id))
- ;; if the query is being modified, check that we have data permissions to run the query
- (when (and dataset_query
- (not= dataset_query (:dataset_query card)))
- (check-data-permissions-for-query dataset_query))
- ;; the same applies to unarchiving a Card: make sure we have data permissions for the Card query before doing so
- (when (and (false? archived)
- (:archived card))
- (check-data-permissions-for-query (:dataset_query card)))
- ;; you must be a superuser to change the value of `enable_embedding` or `embedding_params`. Embedding must be enabled
- (when (or (and (not (nil? enable_embedding))
- (not= enable_embedding (:enable_embedding card)))
- (and embedding_params
- (not= embedding_params (:embedding_params card))))
- (api/check-embedding-enabled)
- (api/check-superuser))
+ ;; Do various permissions checks
+ (check-allowed-to-change-collection card collection_id)
+ (check-allowed-to-modify-query card dataset_query)
+ (check-allowed-to-unarchive card archived)
+ (check-allowed-to-change-embedding card enable_embedding embedding_params)
;; ok, now save the Card
(db/update! Card id
;; `collection_id` and `description` can be `nil` (in order to unset them). Other values should only be modified if they're passed in as non-nil
(u/select-keys-when body
:present #{:collection_id :description}
:non-nil #{:dataset_query :display :name :visualization_settings :archived :enable_embedding :embedding_params}))
- (let [event (cond
- ;; card was archived
- (and archived
- (not (:archived card))) :card-archive
- ;; card was unarchived
- (and (false? archived)
- (:archived card)) :card-unarchive
- :else :card-update)
- card (assoc (Card id) :actor_id api/*current-user-id*)]
- (events/publish-event! event card)
+ ;; Fetch the updated Card from the DB
+ (let [card (Card id)]
+ (publish-card-update! card archived)
;; include same information returned by GET /api/card/:id since frontend replaces the Card it currently has with returned one -- See #4142
(hydrate card :creator :dashboard_count :labels :can_write :collection))))
+;;; ------------------------------------------------------------ Deleting Cards ------------------------------------------------------------
+
;; TODO - Pretty sure this endpoint is not actually used any more, since Cards are supposed to get archived (via PUT /api/card/:id) instead of deleted.
;; Should we remove this?
(api/defendpoint DELETE "/:id"
@@ -407,7 +438,7 @@
:card-id card-id
:dashboard-id dashboard-id}]
(api/check-not-archived card)
- (qp/dataset-query query options)))
+ (qp/process-query-and-save-execution! query options)))
(api/defendpoint POST "/:card-id/query"
"Run the query associated with a Card."
@@ -428,6 +459,7 @@
:constraints nil
:context (dataset-api/export-format->context export-format)))))
+
;;; ------------------------------------------------------------ Sharing is Caring ------------------------------------------------------------
(api/defendpoint POST "/:card-id/public_link"
diff --git a/src/metabase/api/dataset.clj b/src/metabase/api/dataset.clj
index 7666d83e7fd55382770e080c7339684a33fe9961..913c177238aca7600ac3581214c5b264b6910bff 100644
--- a/src/metabase/api/dataset.clj
+++ b/src/metabase/api/dataset.clj
@@ -19,6 +19,8 @@
[schema.core :as s])
(:import [java.io ByteArrayInputStream ByteArrayOutputStream]))
+;;; ------------------------------------------------------------ Constants ------------------------------------------------------------
+
(def ^:private ^:const max-results-bare-rows
"Maximum number of rows to return specifically on :rows type queries via the API."
2000)
@@ -32,24 +34,20 @@
{:max-results max-results
:max-results-bare-rows max-results-bare-rows})
+
+;;; ------------------------------------------------------------ Running a Query Normally ------------------------------------------------------------
+
(api/defendpoint POST "/"
"Execute a query and retrieve the results in the usual format."
- [:as {{:keys [database] :as body} :body}]
+ [:as {{:keys [database], :as query} :body}]
+ {database s/Int}
(api/read-check Database database)
;; add sensible constraints for results limits on our query
- (let [query (assoc body :constraints default-query-constraints)]
- (qp/dataset-query query {:executed-by api/*current-user-id*, :context :ad-hoc})))
+ (let [query (assoc query :constraints default-query-constraints)]
+ (qp/process-query-and-save-execution! query {:executed-by api/*current-user-id*, :context :ad-hoc})))
-;; TODO - this is no longer used. Should we remove it?
-(api/defendpoint POST "/duration"
- "Get historical query execution duration."
- [:as {{:keys [database], :as query} :body}]
- (api/read-check Database database)
- ;; try calculating the average for the query as it was given to us, otherwise with the default constraints if there's no data there.
- ;; if we still can't find relevant info, just default to 0
- {:average (or (query/average-execution-time-ms (qputil/query-hash query))
- (query/average-execution-time-ms (qputil/query-hash (assoc query :constraints default-query-constraints)))
- 0)})
+
+;;; ------------------------------------------------------------ Downloading Query Results in Other Formats ------------------------------------------------------------
(defn- export-to-csv [columns rows]
(with-out-str
@@ -123,8 +121,22 @@
(let [query (json/parse-string query keyword)]
(api/read-check Database (:database query))
(as-format export-format
- (qp/dataset-query (dissoc query :constraints)
+ (qp/process-query-and-save-execution! (dissoc query :constraints)
{:executed-by api/*current-user-id*, :context (export-format->context export-format)}))))
+
+;;; ------------------------------------------------------------ Other Endpoints ------------------------------------------------------------
+
+;; TODO - this is no longer used. Should we remove it?
+(api/defendpoint POST "/duration"
+ "Get historical query execution duration."
+ [:as {{:keys [database], :as query} :body}]
+ (api/read-check Database database)
+ ;; try calculating the average for the query as it was given to us, otherwise with the default constraints if there's no data there.
+ ;; if we still can't find relevant info, just default to 0
+ {:average (or (query/average-execution-time-ms (qputil/query-hash query))
+ (query/average-execution-time-ms (qputil/query-hash (assoc query :constraints default-query-constraints)))
+ 0)})
+
(api/define-routes
(middleware/streaming-json-response (route-fn-name 'POST "/")))
diff --git a/src/metabase/api/pulse.clj b/src/metabase/api/pulse.clj
index 71b05be793010d1acfe379a507ecc3b004c8f2cb..51cf434532d7889fc5cfcc6f99b41a5874cffb92 100644
--- a/src/metabase/api/pulse.clj
+++ b/src/metabase/api/pulse.clj
@@ -104,7 +104,7 @@
"Get HTML rendering of a `Card` with ID."
[id]
(let [card (api/read-check Card id)
- result (qp/dataset-query (:dataset_query card) {:executed-by api/*current-user-id*, :context :pulse, :card-id id})]
+ result (qp/process-query-and-save-execution! (:dataset_query card) {:executed-by api/*current-user-id*, :context :pulse, :card-id id})]
{:status 200, :body (html [:html [:body {:style "margin: 0;"} (binding [render/*include-title* true
render/*include-buttons* true]
(render/render-pulse-card card result))]])}))
@@ -113,7 +113,7 @@
"Get JSON object containing HTML rendering of a `Card` with ID and other information."
[id]
(let [card (api/read-check Card id)
- result (qp/dataset-query (:dataset_query card) {:executed-by api/*current-user-id*, :context :pulse, :card-id id})
+ result (qp/process-query-and-save-execution! (:dataset_query card) {:executed-by api/*current-user-id*, :context :pulse, :card-id id})
data (:data result)
card-type (render/detect-pulse-card-type card data)
card-html (html (binding [render/*include-title* true]
@@ -127,7 +127,7 @@
"Get PNG rendering of a `Card` with ID."
[id]
(let [card (api/read-check Card id)
- result (qp/dataset-query (:dataset_query card) {:executed-by api/*current-user-id*, :context :pulse, :card-id id})
+ result (qp/process-query-and-save-execution! (:dataset_query card) {:executed-by api/*current-user-id*, :context :pulse, :card-id id})
ba (binding [render/*include-title* true]
(render/render-pulse-card-to-png card result))]
{:status 200, :headers {"Content-Type" "image/png"}, :body (ByteArrayInputStream. ba)}))
diff --git a/src/metabase/api/tiles.clj b/src/metabase/api/tiles.clj
index 4ebc3f16da261d37be303b278b3f254689ffdeea..56a5d60b18193b9cb5c3f1783a17f7c8cb264b11 100644
--- a/src/metabase/api/tiles.clj
+++ b/src/metabase/api/tiles.clj
@@ -129,7 +129,7 @@
lon-col-idx (Integer/parseInt lon-col-idx)
query (json/parse-string query keyword)
updated-query (update query :query (u/rpartial query-with-inside-filter lat-field-id lon-field-id x y zoom))
- result (qp/dataset-query updated-query {:executed-by api/*current-user-id*, :context :map-tiles})
+ result (qp/process-query-and-save-execution! updated-query {:executed-by api/*current-user-id*, :context :map-tiles})
points (for [row (-> result :data :rows)]
[(nth row lat-col-idx) (nth row lon-col-idx)])]
;; manual ring response here. we simply create an inputstream from the byte[] of our image
diff --git a/src/metabase/pulse.clj b/src/metabase/pulse.clj
index d0a8919184826d4e364beed9707647685cf5ee41..f4168a9998800eeb04e75d1d962fa0ffb5d07bc2 100644
--- a/src/metabase/pulse.clj
+++ b/src/metabase/pulse.clj
@@ -23,8 +23,9 @@
(let [{:keys [creator_id dataset_query]} card]
(try
{:card card
- :result (qp/dataset-query dataset_query (merge {:executed-by creator_id, :context :pulse, :card-id card-id}
- options))}
+ :result (qp/process-query-and-save-execution! dataset_query
+ (merge {:executed-by creator_id, :context :pulse, :card-id card-id}
+ options))}
(catch Throwable t
(log/warn (format "Error running card query (%n)" card-id) t))))))
diff --git a/src/metabase/query_processor.clj b/src/metabase/query_processor.clj
index 2e079b455c015ba3a1b4508f01f18113bd6b8176..9f486d28aa308146171f94da7bfff64f7fdd823f 100644
--- a/src/metabase/query_processor.clj
+++ b/src/metabase/query_processor.clj
@@ -110,8 +110,6 @@
[query]
((qp-pipeline execute-query) query))
-
-
(def ^{:arglists '([query])} expand
"Expand a QUERY the same way it would normally be done as part of query processing.
This is useful for things that need to look at an expanded query, such as permissions checking for Cards."
@@ -126,6 +124,13 @@
;;; | DATASET-QUERY PUBLIC API |
;;; +----------------------------------------------------------------------------------------------------+
+;; The only difference between `process-query` and `process-query-and-save-execution!` (below) is that the
+;; latter records a `QueryExecution` (inserts a new row) recording some stats about this Query run including
+;; execution time and type of query ran
+;;
+;; `process-query-and-save-execution!` is the function used by various things like API endpoints and pulses;
+;; `process-query` is more of an internal function
+
(defn- save-query-execution!
"Save a `QueryExecution` and update the average execution time for the corresponding `Query`."
[query-execution]
@@ -216,7 +221,11 @@
(save-and-return-failed-query! query-execution (.getMessage e))))))
(def ^:private DatasetQueryOptions
- "Schema for the options map for the `dataset-query` function."
+ "Schema for the options map for the `dataset-query` function.
+ This becomes available to QP middleware as the `:info` dictionary in the top level of a query.
+ When the query is finished running, most of these values are saved in the new `QueryExecution` row.
+ In some cases, these values are used by the middleware; for example, the permissions-checking middleware
+ will check Collection permissions if applicable if `card-id` is non-nil."
(s/constrained {:context query-execution/Context
(s/optional-key :executed-by) (s/maybe su/IntGreaterThanZero)
(s/optional-key :card-id) (s/maybe su/IntGreaterThanZero)
@@ -227,7 +236,7 @@
*allow-queries-with-no-executor-id*))
"executed-by cannot be nil unless *allow-queries-with-no-executor-id* is true"))
-(s/defn ^:always-validate dataset-query
+(s/defn ^:always-validate process-query-and-save-execution!
"Process and run a json based dataset query and return results.
Takes 2 arguments:
diff --git a/src/metabase/query_processor/annotate.clj b/src/metabase/query_processor/annotate.clj
index 88442881722b9ae41c89850f06e95d0610a8e61c..c51c24072db3e8c1d805b68dc14f1fcc0fe067a5 100644
--- a/src/metabase/query_processor/annotate.clj
+++ b/src/metabase/query_processor/annotate.clj
@@ -10,7 +10,9 @@
[metabase
[driver :as driver]
[util :as u]]
- [metabase.models.field :refer [Field]]
+ [metabase.models
+ [field :refer [Field]]
+ [humanization :as humanization]]
[metabase.query-processor
[interface :as i]
[sort :as sort]]
@@ -145,7 +147,7 @@
:preview-display true
:special-type nil
:field-name k
- :field-display-name k})
+ :field-display-name (humanization/name->human-readable-name (name k))})
(defn- info-for-duplicate-field
diff --git a/src/metabase/util/schema.clj b/src/metabase/util/schema.clj
index 0edf6185e910fe1ca4f069ff8f3fb2f922d13eef..01600347032c10226b6e2fd1ae632d03c171d3d0 100644
--- a/src/metabase/util/schema.clj
+++ b/src/metabase/util/schema.clj
@@ -79,10 +79,17 @@
(s/named (s/cond-pre s/Keyword s/Str) "Keyword or string"))
(def FieldType
- "Schema for a valid Field type (does it derive from `:type/*`?"
+ "Schema for a valid Field type (does it derive from `:type/*`)?"
(with-api-error-message (s/pred (u/rpartial isa? :type/*) "Valid field type")
"value must be a valid field type."))
+(def FieldTypeKeywordOrString
+ "Like `FieldType` (e.g. a valid derivative of `:type/*`) but allows either a keyword or a string.
+ This is useful especially for validating API input or objects coming out of the DB as it is unlikely
+ those values will be encoded as keywords at that point."
+ (with-api-error-message (s/pred #(isa? (keyword %) :type/*) "Valid field type (keyword or string)")
+ "value must be a valid field type (keyword or string)."))
+
(def Map
"Schema for a valid map."
(with-api-error-message (s/pred map? "Valid map")
diff --git a/test/metabase/api/card_test.clj b/test/metabase/api/card_test.clj
index a902540a5dac7515be02d5ea236d7503cab49dd8..0c4f47d260a5fae320702ba3a9214d4baa27bf99 100644
--- a/test/metabase/api/card_test.clj
+++ b/test/metabase/api/card_test.clj
@@ -20,7 +20,7 @@
[table :refer [Table]]
[view-log :refer [ViewLog]]]
[metabase.test
- [data :refer :all]
+ [data :as data :refer :all]
[util :as tu :refer [match-$ random-name]]]
[metabase.test.data.users :refer :all]
[toucan.db :as db]
@@ -28,8 +28,6 @@
(:import java.io.ByteArrayInputStream
java.util.UUID))
-;;; CARD LIFECYCLE
-
;;; Helpers
(def ^:const card-defaults
@@ -44,7 +42,38 @@
:query_type "query"
:cache_ttl nil})
-;; ## GET /api/card
+(defn- do-with-self-cleaning-random-card-name
+ "Generate a random card name (or use CARD-NAME), pass it to F, then delete any Cards with that name afterwords."
+ [f & [card-name]]
+ (let [card-name (or card-name (random-name))]
+ (try (f card-name)
+ (finally (db/delete! Card :name card-name)))))
+
+(defmacro ^:private with-self-cleaning-random-card-name
+ "Generate a random card name (or optionally use CARD-NAME) and bind it to CARD-NAME-BINDING.
+ Execute BODY and then delete and Cards with that name afterwards."
+ {:style/indent 1, :arglists '([[card-name-binding] & body] [[card-name-binding card-name] & body])}
+ [[card-name-binding card-name] & body]
+ `(do-with-self-cleaning-random-card-name (fn [~card-name-binding]
+ ~@body)
+ ~@(when card-name [card-name])))
+
+(defn- mbql-count-query [database-id table-id]
+ {:database database-id
+ :type "query"
+ :query {:source-table table-id, :aggregation {:aggregation-type "count"}}})
+
+(defn- card-with-name-and-query [card-name query]
+ {:name card-name
+ :display "scalar"
+ :dataset_query query
+ :visualization_settings {:global {:title nil}}})
+
+
+;;; +------------------------------------------------------------------------------------------------------------------------+
+;;; | FETCHING CARDS & FILTERING |
+;;; +------------------------------------------------------------------------------------------------------------------------+
+
;; Filter cards by database
(expect
[true
@@ -169,15 +198,18 @@
:visualization_settings {:global {:title nil}}
:database_id database-id ; these should be inferred automatically
:table_id table-id})
- ;; make sure we clean up after ourselves as well and delete the Card we create
- (dissoc (u/prog1 ((user->client :rasta) :post 200 "card" {:name card-name
- :display "scalar"
- :dataset_query (mbql-count-query database-id table-id)
- :visualization_settings {:global {:title nil}}})
- (db/delete! Card :id (u/get-id <>)))
- :created_at :updated_at :id)))
-
-;; ## GET /api/card/:id
+ (with-self-cleaning-random-card-name [_ card-name]
+ (dissoc ((user->client :rasta) :post 200 "card" {:name card-name
+ :display "scalar"
+ :dataset_query (mbql-count-query database-id table-id)
+ :visualization_settings {:global {:title nil}}})
+ :created_at :updated_at :id))))
+
+
+;;; +------------------------------------------------------------------------------------------------------------------------+
+;;; | FETCHING A SPECIFIC CARD |
+;;; +------------------------------------------------------------------------------------------------------------------------+
+
;; Test that we can fetch a card
(tt/expect-with-temp [Database [{database-id :id}]
Table [{table-id :id} {:db_id database-id}]
@@ -221,7 +253,10 @@
;; now a non-admin user shouldn't be able to fetch this card
((user->client :rasta) :get 403 (str "card/" (u/get-id card)))))
-;; ## PUT /api/card/:id
+
+;;; +------------------------------------------------------------------------------------------------------------------------+
+;;; | UPDATING A CARD |
+;;; +------------------------------------------------------------------------------------------------------------------------+
;; updating a card that doesn't exist should give a 404
(expect "Not found."
@@ -297,10 +332,14 @@
(Card id)))
;; deleting a card that doesn't exist should return a 404 (#1957)
-(expect "Not found."
+(expect
+ "Not found."
((user->client :crowberto) :delete 404 "card/12345"))
-;; # CARD FAVORITE STUFF
+
+;;; +------------------------------------------------------------------------------------------------------------------------+
+;;; | FAVORITING |
+;;; +------------------------------------------------------------------------------------------------------------------------+
;; Helper Functions
(defn- fave? [card]
@@ -343,6 +382,10 @@
(fave? card))]))
+;;; +------------------------------------------------------------------------------------------------------------------------+
+;;; | LABELS |
+;;; +------------------------------------------------------------------------------------------------------------------------+
+
;;; POST /api/card/:id/labels
;; Check that we can update card labels
(tt/expect-with-temp [Card [{card-id :id}]
@@ -362,6 +405,10 @@
(update-labels [])])) ; (3)
+;;; +------------------------------------------------------------------------------------------------------------------------+
+;;; | CSV/JSON/XLSX DOWNLOADS |
+;;; +------------------------------------------------------------------------------------------------------------------------+
+
;;; POST /api/:card-id/query/csv
(defn- do-with-temp-native-card {:style/indent 0} [f]
@@ -461,6 +508,7 @@
(spreadsheet/select-sheet "Query result")
(spreadsheet/select-columns {:A :col})))))
+
;;; +------------------------------------------------------------------------------------------------------------------------+
;;; | COLLECTIONS |
;;; +------------------------------------------------------------------------------------------------------------------------+
@@ -468,26 +516,19 @@
;; Make sure we can create a card and specify its `collection_id` at the same time
(tt/expect-with-temp [Collection [collection]]
(u/get-id collection)
- (do
+ (with-self-cleaning-random-card-name [card-name]
(perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
- (let [{card-id :id} ((user->client :rasta) :post 200 "card" {:name "My Cool Card"
- :display "scalar"
- :dataset_query (mbql-count-query (id) (id :venues))
- :visualization_settings {:global {:title nil}}
- :collection_id (u/get-id collection)})]
- ;; make sure we clean up after ourselves and delete the newly created Card
- (u/prog1 (db/select-one-field :collection_id Card :id card-id)
- (db/delete! Card :id card-id)))))
+ (let [{card-id :id} ((user->client :rasta) :post 200 "card" (assoc (card-with-name-and-query card-name (mbql-count-query (data/id) (data/id :venues)))
+ :collection_id (u/get-id collection)))]
+ (db/select-one-field :collection_id Card :id card-id))))
;; Make sure we card creation fails if we try to set a `collection_id` we don't have permissions for
(expect
"You don't have permissions to do that."
- (tt/with-temp Collection [collection]
- ((user->client :rasta) :post 403 "card" {:name "My Cool Card"
- :display "scalar"
- :dataset_query (mbql-count-query (id) (id :venues))
- :visualization_settings {:global {:title nil}}
- :collection_id (u/get-id collection)})))
+ (with-self-cleaning-random-card-name [card-name]
+ (tt/with-temp Collection [collection]
+ ((user->client :rasta) :post 403 "card" (assoc (card-with-name-and-query card-name (mbql-count-query (data/id) (data/id :venues)))
+ :collection_id (u/get-id collection))))))
;; Make sure we can change the `collection_id` of a Card if it's not in any collection
(tt/expect-with-temp [Card [card]
diff --git a/test/metabase/driver/generic_sql/native_test.clj b/test/metabase/driver/generic_sql/native_test.clj
index f30255b8377f3780264eb1bfe3629ef17502103b..1a3621a513f3802974aea70d76d723f0c4dcb340 100644
--- a/test/metabase/driver/generic_sql/native_test.clj
+++ b/test/metabase/driver/generic_sql/native_test.clj
@@ -1,33 +1,35 @@
(ns metabase.driver.generic-sql.native-test
(:require [expectations :refer :all]
+ [medley.core :as m]
[metabase.models.database :refer [Database]]
[metabase.query-processor :as qp]
[metabase.test.data :refer :all]
[toucan.db :as db]))
;; Just check that a basic query works
-(expect {:status :completed
- :row_count 2
- :data {:rows [[100]
- [99]]
- :columns ["ID"]
- :cols [{:name "ID", :base_type :type/Integer}]
- :native_form {:query "SELECT ID FROM VENUES ORDER BY ID DESC LIMIT 2;"}}}
+(expect
+ {:status :completed
+ :row_count 2
+ :data {:rows [[100]
+ [99]]
+ :columns ["ID"]
+ :cols [{:name "ID", :base_type :type/Integer}]
+ :native_form {:query "SELECT ID FROM VENUES ORDER BY ID DESC LIMIT 2;"}}}
(qp/process-query {:native {:query "SELECT ID FROM VENUES ORDER BY ID DESC LIMIT 2;"}
:type :native
:database (id)}))
;; Check that column ordering is maintained
(expect
- {:status :completed
- :row_count 2
- :data {:rows [[100 "Mohawk Bend" 46]
- [99 "Golden Road Brewing" 10]]
- :columns ["ID" "NAME" "CATEGORY_ID"]
- :cols [{:name "ID", :base_type :type/Integer}
- {:name "NAME", :base_type :type/Text}
- {:name "CATEGORY_ID", :base_type :type/Integer}]
- :native_form {:query "SELECT ID, NAME, CATEGORY_ID FROM VENUES ORDER BY ID DESC LIMIT 2;"}}}
+ {:status :completed
+ :row_count 2
+ :data {:rows [[100 "Mohawk Bend" 46]
+ [99 "Golden Road Brewing" 10]]
+ :columns ["ID" "NAME" "CATEGORY_ID"]
+ :cols [{:name "ID", :base_type :type/Integer}
+ {:name "NAME", :base_type :type/Text}
+ {:name "CATEGORY_ID", :base_type :type/Integer}]
+ :native_form {:query "SELECT ID, NAME, CATEGORY_ID FROM VENUES ORDER BY ID DESC LIMIT 2;"}}}
(qp/process-query {:native {:query "SELECT ID, NAME, CATEGORY_ID FROM VENUES ORDER BY ID DESC LIMIT 2;"}
:type :native
:database (id)}))
diff --git a/test/metabase/query_processor_test.clj b/test/metabase/query_processor_test.clj
index dec254d8d3b617779831b2896b3678aaa0f2ec6a..253833859bea56d8861a10f544471d7a92d653ab 100644
--- a/test/metabase/query_processor_test.clj
+++ b/test/metabase/query_processor_test.clj
@@ -285,7 +285,9 @@
(printf "(%s %s) failed: %s" f v (.getMessage e))
(throw e)))))))))))
-(def formatted-venues-rows (partial format-rows-by [int str int (partial u/round-to-decimals 4) (partial u/round-to-decimals 4) int]))
+(def ^{:arglists '([results])} formatted-venues-rows
+ "Helper function to format the rows in RESULTS when running a 'raw data' query against the Venues test table."
+ (partial format-rows-by [int str int (partial u/round-to-decimals 4) (partial u/round-to-decimals 4) int]))
(defn rows
diff --git a/test/metabase/query_processor_test/aggregation_test.clj b/test/metabase/query_processor_test/aggregation_test.clj
index eb49a041dab4a1b6fd4e946c569261b42ff79569..12f90194ecc2df927e87ad3bc66dcffca1a70f31 100644
--- a/test/metabase/query_processor_test/aggregation_test.clj
+++ b/test/metabase/query_processor_test/aggregation_test.clj
@@ -153,7 +153,7 @@
(datasets/expect-with-engines (disj non-timeseries-engines :mongo :bigquery :presto)
[(aggregate-col :count)
(assoc (aggregate-col :count)
- :display_name "count_2"
+ :display_name "Count 2"
:name "count_2"
:preview_display true)]
(-> (data/run-query venues
diff --git a/test/metabase/test/data.clj b/test/metabase/test/data.clj
index 867e0ea04402d8e9ccab401ccd09c2d99275ea7a..449598a2dcf88664ce7e00dc53e2fa1b576f6e54 100644
--- a/test/metabase/test/data.clj
+++ b/test/metabase/test/data.clj
@@ -211,6 +211,12 @@
;; add extra metadata for fields
(add-extra-metadata! database-definition <>)))
+(defn- reload-test-extensions [engine]
+ (println "Reloading test extensions for driver:" engine)
+ (let [extension-ns (symbol (str "metabase.test.data." (name engine)))]
+ (println (format "(require '%s 'metabase.test.data.datasets :reload)" extension-ns))
+ (require extension-ns 'metabase.test.data.datasets :reload)))
+
(defn get-or-create-database!
"Create DBMS database associated with DATABASE-DEFINITION, create corresponding Metabase `Databases`/`Tables`/`Fields`, and sync the `Database`.
DRIVER should be an object that implements `IDriverTestExtensions`; it defaults to the value returned by the method `driver` for the
@@ -218,9 +224,18 @@
([database-definition]
(get-or-create-database! *driver* database-definition))
([driver database-definition]
- (let [engine (i/engine driver)]
- (or (i/metabase-instance database-definition engine)
- (create-database! database-definition engine driver)))))
+ (let [engine (i/engine driver)
+ get-or-create! (fn []
+ (or (i/metabase-instance database-definition engine)
+ (create-database! database-definition engine driver)))]
+ (try
+ (get-or-create!)
+ ;; occasionally we'll see an error like
+ ;; java.lang.IllegalArgumentException: No implementation of method: :database->connection-details of protocol: IDriverTestExtensions found for class: metabase.driver.h2.H2Driver
+ ;; to fix this we just need to reload a couple namespaces and then try again
+ (catch IllegalArgumentException _
+ (reload-test-extensions engine)
+ (get-or-create!))))))
(defn do-with-temp-db