From ced090aa86d5c855255998cfbbf027d618afd978 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cam=20Sa=C3=BCl?= <cammsaul@gmail.com>
Date: Wed, 31 May 2017 13:17:04 -0700
Subject: [PATCH] Cleanup backported from nested-queries branch :shower:
---
src/metabase/api/database.clj | 1 +
src/metabase/api/public.clj | 2 +-
src/metabase/api/table.clj | 2 +
src/metabase/driver.clj | 11 ++
src/metabase/driver/generic_sql.clj | 1 +
.../driver/generic_sql/query_processor.clj | 4 +-
.../googleanalytics/query_processor.clj | 6 +-
src/metabase/driver/mongo/util.clj | 23 ---
src/metabase/middleware.clj | 11 +-
src/metabase/query_processor/annotate.clj | 25 +--
src/metabase/query_processor/expand.clj | 44 ++---
src/metabase/query_processor/interface.clj | 163 ++++++++++--------
.../middleware/permissions.clj | 6 +-
src/metabase/query_processor/permissions.clj | 6 +-
src/metabase/query_processor/resolve.clj | 12 +-
src/metabase/query_processor/util.clj | 69 +++++++-
src/metabase/sync_database/analyze.clj | 2 +-
src/metabase/util/honeysql_extensions.clj | 1 +
test/metabase/http_client.clj | 1 -
test/metabase/models/card_test.clj | 36 ++--
test/metabase/permissions_collection_test.clj | 3 -
test/metabase/permissions_test.clj | 2 +-
test/metabase/query_processor/util_test.clj | 50 ++++++
test/metabase/test/data.clj | 7 +-
test/metabase/test/data/users.clj | 2 -
test_resources/log4j.properties | 2 +-
26 files changed, 313 insertions(+), 179 deletions(-)
diff --git a/src/metabase/api/database.clj b/src/metabase/api/database.clj
index 49ea7918e35..e6e3808ee17 100644
--- a/src/metabase/api/database.clj
+++ b/src/metabase/api/database.clj
@@ -246,6 +246,7 @@
(sample-data/add-sample-dataset!)
(Database :is_sample true))
+
;;; ------------------------------------------------------------ PUT /api/database/:id ------------------------------------------------------------
(api/defendpoint PUT "/:id"
diff --git a/src/metabase/api/public.clj b/src/metabase/api/public.clj
index ba3690a093f..996fbd43e7a 100644
--- a/src/metabase/api/public.clj
+++ b/src/metabase/api/public.clj
@@ -88,11 +88,11 @@
(card-with-uuid uuid))
-
(defn run-query-for-card-with-id
"Run the query belonging to Card with CARD-ID with PARAMETERS and other query options (e.g. `:constraints`)."
[card-id parameters & options]
(u/prog1 (-> (let [parameters (if (string? parameters) (json/parse-string parameters keyword) parameters)]
+ ;; run this query with full superuser perms
(binding [api/*current-user-permissions-set* (atom #{"/"})
qp/*allow-queries-with-no-executor-id* true]
(apply card-api/run-query-for-card card-id, :parameters parameters, :context :public-question, options)))
diff --git a/src/metabase/api/table.clj b/src/metabase/api/table.clj
index 91909a55583..83354618e2a 100644
--- a/src/metabase/api/table.clj
+++ b/src/metabase/api/table.clj
@@ -2,6 +2,7 @@
"/api/table endpoints."
(:require [clojure.tools.logging :as log]
[compojure.core :refer [GET PUT]]
+ [medley.core :as m]
[metabase
[sync-database :as sync-database]
[util :as u]]
@@ -87,6 +88,7 @@
{include_sensitive_fields (s/maybe su/BooleanString)}
(-> (api/read-check Table id)
(hydrate :db [:fields :target] :field_values :segments :metrics)
+ (m/dissoc-in [:db :details])
(update-in [:fields] (if (Boolean/parseBoolean include_sensitive_fields)
;; If someone passes include_sensitive_fields return hydrated :fields as-is
identity
diff --git a/src/metabase/driver.clj b/src/metabase/driver.clj
index 80a4ff3bc61..3eb090d1542 100644
--- a/src/metabase/driver.clj
+++ b/src/metabase/driver.clj
@@ -323,6 +323,17 @@
(log/warn (format "Don't know how to map class '%s' to a Field base_type, falling back to :type/*." klass))
:type/*))
+(defn values->base-type
+ "Given a sequence of VALUES, return the most common base type."
+ [values]
+ (->> values
+ (filter (complement nil?)) ; filter out `nil` values
+ (take 1000) ; take up to 1000 values
+ (group-by (comp class->base-type class)) ; now group by their base-type
+ (sort-by (comp (partial * -1) count second)) ; sort the map into pairs of [base-type count] with highest count as first pair
+ ffirst)) ; take the base-type from the first pair
+
+
;; ## Driver Lookup
(defn engine->driver
diff --git a/src/metabase/driver/generic_sql.clj b/src/metabase/driver/generic_sql.clj
index 4d97cf54354..a2157fa9076 100644
--- a/src/metabase/driver/generic_sql.clj
+++ b/src/metabase/driver/generic_sql.clj
@@ -220,6 +220,7 @@
(defn honeysql-form->sql+args
"Convert HONEYSQL-FORM to a vector of SQL string and params, like you'd pass to JDBC."
+ {:style/indent 1}
[driver honeysql-form]
{:pre [(map? honeysql-form)]}
(let [[sql & args] (try (binding [hformat/*subquery?* false]
diff --git a/src/metabase/driver/generic_sql/query_processor.clj b/src/metabase/driver/generic_sql/query_processor.clj
index 73e8c0623f9..e8c254dd685 100644
--- a/src/metabase/driver/generic_sql/query_processor.clj
+++ b/src/metabase/driver/generic_sql/query_processor.clj
@@ -243,7 +243,7 @@
(h/limit items)
(h/offset (* items (dec page)))))
-(defn- apply-source-table [_ honeysql-form {{table-name :name, schema :schema} :source-table}]
+(defn- apply-source-table [honeysql-form {{table-name :name, schema :schema} :source-table}]
{:pre [table-name]}
(h/from honeysql-form (hx/qualify-and-escape-dots schema table-name)))
@@ -252,7 +252,7 @@
;; will get swapped around and we'll be left with old version of the function that nobody implements
;; 2) This is a vector rather than a map because the order the clauses get handled is important for some drivers.
;; For example, Oracle needs to wrap the entire query in order to apply its version of limit (`WHERE ROWNUM`).
- [:source-table apply-source-table
+ [:source-table (u/drop-first-arg apply-source-table)
:aggregation #'sql/apply-aggregation
:breakout #'sql/apply-breakout
:fields #'sql/apply-fields
diff --git a/src/metabase/driver/googleanalytics/query_processor.clj b/src/metabase/driver/googleanalytics/query_processor.clj
index 1424d4d7ae1..cc0990c380f 100644
--- a/src/metabase/driver/googleanalytics/query_processor.clj
+++ b/src/metabase/driver/googleanalytics/query_processor.clj
@@ -3,7 +3,7 @@
(:require [clojure.string :as s]
[clojure.tools.reader.edn :as edn]
[medley.core :as m]
- [metabase.query-processor.expand :as ql]
+ [metabase.query-processor.util :as qputil]
[metabase.util :as u])
(:import [com.google.api.services.analytics.model GaData GaData$ColumnHeaders]
[metabase.query_processor.interface AgFieldRef DateTimeField DateTimeValue Field RelativeDateTimeValue Value]))
@@ -251,7 +251,7 @@
[{query :query}]
(let [[aggregation-type metric-name] (first-aggregation query)]
(when (and aggregation-type
- (= :metric (ql/normalize-token aggregation-type))
+ (= :metric (qputil/normalize-token aggregation-type))
(string? metric-name))
metric-name)))
@@ -266,7 +266,7 @@
(defn- filter-type ^clojure.lang.Keyword [filter-clause]
(when (and (sequential? filter-clause)
(u/string-or-keyword? (first filter-clause)))
- (ql/normalize-token (first filter-clause))))
+ (qputil/normalize-token (first filter-clause))))
(defn- compound-filter? [filter-clause]
(contains? #{:and :or :not} (filter-type filter-clause)))
diff --git a/src/metabase/driver/mongo/util.clj b/src/metabase/driver/mongo/util.clj
index ce830438fdc..c7a153a1a7c 100644
--- a/src/metabase/driver/mongo/util.clj
+++ b/src/metabase/driver/mongo/util.clj
@@ -137,26 +137,3 @@
(if *mongo-connection*
(f# *mongo-connection*)
(-with-mongo-connection f# ~database))))
-
-;; TODO - this isn't neccesarily Mongo-specific; consider moving
-(defn values->base-type
- "Given a sequence of values, return `Field.base_type` in the most ghetto way possible.
- This just gets counts the types of *every* value and returns the `base_type` for class whose count was highest."
- [values-seq]
- {:pre [(sequential? values-seq)]}
- (or (->> values-seq
- ;; TODO - why not do a query to return non-nil values of this column instead
- (filter identity)
- ;; it's probably fine just to consider the first 1,000 *non-nil* values when trying to type a column instead
- ;; of iterating over the whole collection. (VALUES-SEQ should be up to 10,000 values, but we don't know how many are
- ;; nil)
- (take 1000)
- (group-by type)
- ;; create tuples like [Integer count].
- (map (fn [[klass valus]]
- [klass (count valus)]))
- (sort-by second)
- last ; last result will be tuple with highest count
- first ; keep just the type
- driver/class->base-type) ; convert to Field base_type
- :type/*))
diff --git a/src/metabase/middleware.clj b/src/metabase/middleware.clj
index 5e2467e44bf..9ca52e30797 100644
--- a/src/metabase/middleware.clj
+++ b/src/metabase/middleware.clj
@@ -100,17 +100,12 @@
"Return User ID and superuser status for Session with SESSION-ID if it is valid and not expired."
[session-id]
(when (and session-id (init-status/complete?))
- (when-let [session (or (session-with-id session-id)
- (println "no matching session with ID") ; DEBUG
- )]
- (if (session-expired? session)
- (printf "session-is-expired! %d min / %d min\n" (session-age-minutes session) (config/config-int :max-session-age)) ; DEBUG
+ (when-let [session (session-with-id session-id)]
+ (when-not (session-expired? session)
{:metabase-user-id (:user_id session)
:is-superuser? (:is_superuser session)}))))
(defn- add-current-user-info [{:keys [metabase-session-id], :as request}]
- (when-not (init-status/complete?)
- (println "Metabase is not initialized yet!")) ; DEBUG
(merge request (current-user-info-for-session metabase-session-id)))
(defn wrap-current-user-id
@@ -347,7 +342,7 @@
(try (binding [*automatically-catch-api-exceptions* false]
(handler request))
(catch Throwable e
- (log/error (.getMessage e))
+ (log/warn (.getMessage e))
{:status 400, :body "An error occurred."}))))
(defn message-only-exceptions
diff --git a/src/metabase/query_processor/annotate.clj b/src/metabase/query_processor/annotate.clj
index 8200086f5f8..734e4bfc8dd 100644
--- a/src/metabase/query_processor/annotate.clj
+++ b/src/metabase/query_processor/annotate.clj
@@ -134,16 +134,19 @@
(expression-aggregate-field-info ag)
(aggregate-field-info ag))))))
-
(defn- generic-info-for-missing-key
- "Return a set of bare-bones metadata for a Field named K when all else fails."
- [k]
- {:base-type :type/*
+ "Return a set of bare-bones metadata for a Field named K when all else fails.
+ Scan the INITIAL-VALUES of K in an attempt to determine the `base-type`."
+ [k & [initial-values]]
+ {:base-type (if (seq initial-values)
+ (driver/values->base-type initial-values)
+ :type/*)
:preview-display true
:special-type nil
:field-name k
:field-display-name k})
+
(defn- info-for-duplicate-field
"The Clojure JDBC driver automatically appends suffixes like `count_2` to duplicate columns if multiple columns come back with the same name;
since at this time we can't resolve those normally (#1786) fall back to using the metadata for the first column (e.g., `count`).
@@ -159,14 +162,14 @@
(defn- info-for-missing-key
"Metadata for a field named K, which we weren't able to resolve normally.
If possible, we work around This defaults to generic information "
- [fields k]
+ [fields k initial-values]
(or (info-for-duplicate-field fields k)
- (generic-info-for-missing-key k)))
+ (generic-info-for-missing-key k initial-values)))
(defn- add-unknown-fields-if-needed
"When create info maps for any fields we didn't expect to come back from the query.
Ideally, this should never happen, but on the off chance it does we still want to return it in the results."
- [actual-keys fields]
+ [actual-keys initial-rows fields]
{:pre [(set? actual-keys) (every? keyword? actual-keys)]}
(let [expected-keys (u/prog1 (set (map :field-name fields))
(assert (every? keyword? <>)))
@@ -175,7 +178,7 @@
(log/warn (u/format-color 'yellow "There are fields we weren't expecting in the results: %s\nExpected: %s\nActual: %s"
missing-keys expected-keys actual-keys)))
(concat fields (for [k missing-keys]
- (info-for-missing-key fields k)))))
+ (info-for-missing-key fields k (map k initial-rows))))))
(defn- convert-field-to-expected-format
"Rename keys, provide default values, etc. for FIELD so it is in the format expected by the frontend."
@@ -238,14 +241,14 @@
(defn- resolve-sort-and-format-columns
"Collect the Fields referenced in QUERY, sort them according to the rules at the top
of this page, format them as expected by the frontend, and return the results."
- [query result-keys]
+ [query result-keys initial-rows]
{:pre [(set? result-keys)]}
(when (seq result-keys)
(->> (collect-fields (dissoc query :expressions))
(map qualify-field-name)
(add-aggregate-fields-if-needed query)
(map (u/rpartial update :field-name keyword))
- (add-unknown-fields-if-needed result-keys)
+ (add-unknown-fields-if-needed result-keys initial-rows)
(sort/sort-fields query)
(map convert-field-to-expected-format)
(filter (comp (partial contains? result-keys) :name))
@@ -261,7 +264,7 @@
[query {:keys [columns rows], :as results}]
(let [row-maps (for [row rows]
(zipmap columns row))
- cols (resolve-sort-and-format-columns (:query query) (set columns))
+ cols (resolve-sort-and-format-columns (:query query) (set columns) (take 10 row-maps))
columns (mapv :name cols)]
(assoc results
:cols (vec (for [col cols]
diff --git a/src/metabase/query_processor/expand.clj b/src/metabase/query_processor/expand.clj
index a6a1d3c7bc8..97d22d5bc9a 100644
--- a/src/metabase/query_processor/expand.clj
+++ b/src/metabase/query_processor/expand.clj
@@ -2,25 +2,16 @@
"Converts a Query Dict as received by the API into an *expanded* one that contains extra information that will be needed to
construct the appropriate native Query, and perform various post-processing steps such as Field ordering."
(:refer-clojure :exclude [< <= > >= = != and or not filter count distinct sum min max + - / *])
- (:require [clojure
- [core :as core]
- [string :as str]]
+ (:require [clojure.core :as core]
[clojure.tools.logging :as log]
- [metabase.query-processor.interface :as i]
+ [metabase.query-processor
+ [interface :as i]
+ [util :as qputil]]
[metabase.util :as u]
[metabase.util.schema :as su]
[schema.core :as s])
- (:import [metabase.query_processor.interface AgFieldRef BetweenFilter ComparisonFilter CompoundFilter Expression ExpressionRef FieldPlaceholder RelativeDatetime StringFilter ValuePlaceholder]))
-
-;;; # ------------------------------------------------------------ Token dispatch ------------------------------------------------------------
-
-(s/defn ^:always-validate normalize-token :- s/Keyword
- "Convert a string or keyword in various cases (`lisp-case`, `snake_case`, or `SCREAMING_SNAKE_CASE`) to a lisp-cased keyword."
- [token :- su/KeywordOrString]
- (-> (name token)
- str/lower-case
- (str/replace #"_" "-")
- keyword))
+ (:import [metabase.query_processor.interface AgFieldRef BetweenFilter ComparisonFilter CompoundFilter Expression ExpressionRef
+ FieldPlaceholder RelativeDatetime StringFilter Value ValuePlaceholder]))
;;; # ------------------------------------------------------------ Clause Handlers ------------------------------------------------------------
@@ -38,7 +29,7 @@
[id :- su/IntGreaterThanZero]
(i/map->FieldPlaceholder {:field-id id}))
-(s/defn ^:private ^:always-validate field :- i/AnyFieldOrExpression
+(s/defn ^:private ^:always-validate field :- i/AnyField
"Generic reference to a `Field`. F can be an integer Field ID, or various other forms like `fk->` or `aggregation`."
[f]
(if (integer? f)
@@ -58,7 +49,7 @@
([f _ unit] (log/warn (u/format-color 'yellow (str "The syntax for datetime-field has changed in MBQL '98. [:datetime-field <field> :as <unit>] is deprecated. "
"Prefer [:datetime-field <field> <unit>] instead.")))
(datetime-field f unit))
- ([f unit] (assoc (field f) :datetime-unit (normalize-token unit))))
+ ([f unit] (assoc (field f) :datetime-unit (qputil/normalize-token unit))))
(s/defn ^:ql ^:always-validate fk-> :- FieldPlaceholder
"Reference to a `Field` that belongs to another `Table`. DEST-FIELD-ID is the ID of this Field, and FK-FIELD-ID is the ID of the foreign key field
@@ -72,11 +63,12 @@
(i/map->FieldPlaceholder {:fk-field-id fk-field-id, :field-id dest-field-id}))
-(s/defn ^:private ^:always-validate value :- ValuePlaceholder
+(s/defn ^:private ^:always-validate value :- (s/cond-pre Value ValuePlaceholder)
"Literal value. F is the `Field` it relates to, and V is `nil`, or a boolean, string, numerical, or datetime value."
[f v]
(cond
(instance? ValuePlaceholder v) v
+ (instance? Value v) v
:else (i/map->ValuePlaceholder {:field-placeholder (field f), :value v})))
(s/defn ^:private ^:always-validate field-or-value
@@ -94,11 +86,11 @@
(relative-datetime :current)
(relative-datetime -31 :day)"
- ([n] (s/validate (s/eq :current) (normalize-token n))
+ ([n] (s/validate (s/eq :current) (qputil/normalize-token n))
(relative-datetime 0 nil))
([n :- s/Int, unit] (i/map->RelativeDatetime {:amount n, :unit (if (nil? unit)
:day ; give :unit a default value so we can simplify the schema a bit and require a :unit
- (normalize-token unit))})))
+ (qputil/normalize-token unit))})))
(s/defn ^:ql ^:always-validate expression :- ExpressionRef
{:added "0.17.0"}
@@ -170,8 +162,8 @@
;; make sure the ag map is still typed correctly
(u/prog1 (cond
(:operator ag) (i/map->Expression ag)
- (:field ag) (i/map->AggregationWithField (update ag :aggregation-type normalize-token))
- :else (i/map->AggregationWithoutField (update ag :aggregation-type normalize-token)))
+ (:field ag) (i/map->AggregationWithField (update ag :aggregation-type qputil/normalize-token))
+ :else (i/map->AggregationWithoutField (update ag :aggregation-type qputil/normalize-token)))
(s/validate i/Aggregation <>)))))))
;; also handle varargs for convenience
@@ -288,7 +280,7 @@
(filter {} (time-interval (field-id 100) :current :day)) "
[f n unit]
(if-not (integer? n)
- (case (normalize-token n)
+ (case (qputil/normalize-token n)
:current (recur f 0 unit)
:last (recur f -1 unit)
:next (recur f 1 unit))
@@ -353,7 +345,7 @@
(map? subclause) subclause ; already parsed by `asc` or `desc`
(vector? subclause) (let [[f direction] subclause]
(log/warn (u/format-color 'yellow "The syntax for order-by has changed in MBQL '98. [<field> :ascending/:descending] is deprecated. Prefer [:asc/:desc <field>] instead."))
- (order-by-subclause (normalize-token direction) f))))
+ (order-by-subclause (qputil/normalize-token direction) f))))
(defn ^:ql order-by
"Specify how ordering should be done for this query.
@@ -432,7 +424,7 @@
(fn-for-token :starts-with) -> #'starts-with"
[token]
- (let [token (normalize-token token)]
+ (let [token (qputil/normalize-token token)]
(core/or (token->ql-fn token)
(throw (Exception. (str "Illegal clause (no matching fn found): " token))))))
@@ -506,4 +498,4 @@
(is-clause? :field-id [\"FIELD-ID\" 2000]) ; -> true"
[clause-keyword clause]
(core/and (sequential? clause)
- (core/= (normalize-token (first clause)) clause-keyword)))
+ (core/= (qputil/normalize-token (first clause)) clause-keyword)))
diff --git a/src/metabase/query_processor/interface.clj b/src/metabase/query_processor/interface.clj
index 05397e945ad..c2aa5bee4fc 100644
--- a/src/metabase/query_processor/interface.clj
+++ b/src/metabase/query_processor/interface.clj
@@ -33,13 +33,21 @@
Not neccesarily bound when using various functions like `fk->` in the REPL."
nil)
+(defn driver-supports?
+ "Does the currently bound `*driver*` support FEATURE?
+ (This returns `nil` if `*driver*` is unbound. `*driver*` is always bound when running queries the normal way,
+ but may not be when calling this function directly from the REPL.)"
+ [feature]
+ (when *driver*
+ ((resolve 'metabase.driver/driver-supports?) *driver* feature)))
+
;; `assert-driver-supports` doesn't run check when `*driver*` is unbound (e.g., when used in the REPL)
;; Allows flexibility when composing queries for tests or interactive development
(defn assert-driver-supports
"When `*driver*` is bound, assert that is supports keyword FEATURE."
[feature]
(when *driver*
- (when-not (contains? ((resolve 'metabase.driver/features) *driver*) feature)
+ (when-not (driver-supports? feature)
(throw (Exception. (str (name feature) " is not supported by this driver."))))))
;; Expansion Happens in a Few Stages:
@@ -70,9 +78,11 @@
"Return a vector of name components of the form `[table-name parent-names... field-name]`"))
-;;; # ------------------------------------------------------------ "RESOLVED" TYPES: FIELD + VALUE ------------------------------------------------------------
+;;; +----------------------------------------------------------------------------------------------------------------------------------------------------------------+
+;;; | FIELDS |
+;;; +----------------------------------------------------------------------------------------------------------------------------------------------------------------+
-;; Field is the expansion of a Field ID in the standard QL
+;; Field is the "expanded" form of a Field ID (field reference) in MBQL
(s/defrecord Field [field-id :- su/IntGreaterThanZero
field-name :- su/NonBlankString
field-display-name :- su/NonBlankString
@@ -98,6 +108,7 @@
[table-name])
field-name)))
+;;; DateTimeField
(def ^:const datetime-field-units
"Valid units for a `DateTimeField`."
@@ -122,7 +133,7 @@
(contains? relative-datetime-value-units (keyword unit)))
-;; wrapper around Field
+;; DateTimeField is just a simple wrapper around Field
(s/defrecord DateTimeField [field :- Field
unit :- DatetimeFieldUnit]
clojure.lang.Named
@@ -136,78 +147,53 @@
[nil expression-name]))
-;; Value is the expansion of a value within a QL clause
-;; Information about the associated Field is included for convenience
-(s/defrecord Value [value :- (s/maybe (s/cond-pre s/Bool s/Num su/NonBlankString))
- field :- (s/named (s/cond-pre Field ExpressionRef) ; TODO - Value doesn't need the whole field, just the relevant type info / units
- "field or expression reference")])
-
-;; e.g. an absolute point in time (literal)
-(s/defrecord DateTimeValue [value :- Timestamp
- field :- DateTimeField])
-
-(s/defrecord RelativeDateTimeValue [amount :- s/Int
- unit :- DatetimeValueUnit
- field :- DateTimeField])
-
-(defprotocol ^:private IDateTimeValue
- (unit [this]
- "Get the `unit` associated with a `DateTimeValue` or `RelativeDateTimeValue`.")
-
- (add-date-time-units [this n]
- "Return a new `DateTimeValue` or `RelativeDateTimeValue` with N `units` added to it."))
-
-(extend-protocol IDateTimeValue
- DateTimeValue
- (unit [this] (:unit (:field this)))
- (add-date-time-units [this n] (assoc this :value (u/relative-date (unit this) n (:value this))))
-
- RelativeDateTimeValue
- (unit [this] (:unit this))
- (add-date-time-units [this n] (update this :amount (partial + n))))
-
-
-;;; # ------------------------------------------------------------ PLACEHOLDER TYPES: FIELDPLACEHOLDER + VALUEPLACEHOLDER ------------------------------------------------------------
+;;; Placeholder Types
;; Replace Field IDs with these during first pass
(s/defrecord FieldPlaceholder [field-id :- su/IntGreaterThanZero
fk-field-id :- (s/maybe (s/constrained su/IntGreaterThanZero
- (fn [_] (or (assert-driver-supports :foreign-keys) true))
- "foreign-keys is not supported by this driver."))
+ (fn [_] (or (assert-driver-supports :foreign-keys) true)) ; assert-driver-supports will throw Exception if driver is bound
+ "foreign-keys is not supported by this driver.")) ; and driver does not support foreign keys
datetime-unit :- (s/maybe (apply s/enum datetime-field-units))])
(s/defrecord AgFieldRef [index :- s/Int])
-
;; TODO - add a method to get matching expression from the query?
-(def FieldPlaceholderOrAgRef
- "Schema for either a `FieldPlaceholder` or `AgFieldRef`."
- (s/named (s/cond-pre FieldPlaceholder AgFieldRef) "Valid field (not a field ID or aggregate field reference)"))
+
+
(def FieldPlaceholderOrExpressionRef
"Schema for either a `FieldPlaceholder` or `ExpressionRef`."
(s/named (s/cond-pre FieldPlaceholder ExpressionRef)
"Valid field or expression reference."))
-
(s/defrecord RelativeDatetime [amount :- s/Int
unit :- DatetimeValueUnit])
-
-(declare RValue Aggregation)
+(declare Aggregation AnyField AnyValueLiteral)
(def ^:private ExpressionOperator (s/named (s/enum :+ :- :* :/) "Valid expression operator"))
(s/defrecord Expression [operator :- ExpressionOperator
- args :- [(s/cond-pre (s/recursive #'RValue)
+ args :- [(s/cond-pre (s/recursive #'AnyValueLiteral)
+ (s/recursive #'AnyField)
(s/recursive #'Aggregation))]
custom-name :- (s/maybe su/NonBlankString)])
-(def AnyFieldOrExpression
- "Schema for a `FieldPlaceholder`, `AgRef`, or `Expression`."
- (s/named (s/cond-pre ExpressionRef Expression FieldPlaceholderOrAgRef)
- "Valid field, ag field reference, expression, or expression reference."))
+(def AnyField
+ "Schema for a anything that is considered a valid 'field'."
+ (s/named (s/cond-pre Field
+ FieldPlaceholder
+ AgFieldRef
+ Expression
+ ExpressionRef)
+ "AnyField: field, ag field reference, expression, expression reference, or field literal."))
+
+
+;;; +----------------------------------------------------------------------------------------------------------------------------------------------------------------+
+;;; | VALUES |
+;;; +----------------------------------------------------------------------------------------------------------------------------------------------------------------+
(def LiteralDatetimeString
"Schema for an MBQL datetime string literal, in ISO-8601 format."
@@ -225,14 +211,48 @@
"Schema for something that is orderable value in MBQL (either a number or datetime)."
(s/named (s/cond-pre s/Num Datetime) "Valid orderable value (must be number or datetime)"))
-(def AnyValue
- "Schema for anything that is a considered a valid value in MBQL - `nil`, a `Boolean`, `Number`, `String`, or relative datetime form."
+(def AnyValueLiteral
+ "Schema for anything that is a considered a valid value literal in MBQL - `nil`, a `Boolean`, `Number`, `String`, or relative datetime form."
(s/named (s/maybe (s/cond-pre s/Bool su/NonBlankString OrderableValue)) "Valid value (must be nil, boolean, number, string, or a relative-datetime form)"))
+
+;; Value is the expansion of a value within a QL clause
+;; Information about the associated Field is included for convenience
+;; TODO - Value doesn't need the whole field, just the relevant type info / units
+(s/defrecord Value [value :- AnyValueLiteral
+ field :- (s/recursive #'AnyField)])
+
+;; e.g. an absolute point in time (literal)
+(s/defrecord DateTimeValue [value :- Timestamp
+ field :- DateTimeField])
+
+(s/defrecord RelativeDateTimeValue [amount :- s/Int
+ unit :- DatetimeValueUnit
+ field :- DateTimeField])
+
+(defprotocol ^:private IDateTimeValue
+ (unit [this]
+ "Get the `unit` associated with a `DateTimeValue` or `RelativeDateTimeValue`.")
+
+ (add-date-time-units [this n]
+ "Return a new `DateTimeValue` or `RelativeDateTimeValue` with N `units` added to it."))
+
+(extend-protocol IDateTimeValue
+ DateTimeValue
+ (unit [this] (:unit (:field this)))
+ (add-date-time-units [this n] (assoc this :value (u/relative-date (unit this) n (:value this))))
+
+ RelativeDateTimeValue
+ (unit [this] (:unit this))
+ (add-date-time-units [this n] (update this :amount (partial + n))))
+
+
+;;; Placeholder Types
+
;; Replace values with these during first pass over Query.
;; Include associated Field ID so appropriate the info can be found during Field resolution
(s/defrecord ValuePlaceholder [field-placeholder :- FieldPlaceholderOrExpressionRef
- value :- AnyValue])
+ value :- AnyValueLiteral])
(def OrderableValuePlaceholder
"`ValuePlaceholder` schema with the additional constraint that the value be orderable (a number or datetime)."
@@ -242,21 +262,16 @@
"`ValuePlaceholder` schema with the additional constraint that the value be a string/"
(s/constrained ValuePlaceholder (comp string? :value) ":value must be a string"))
-(def FieldOrAnyValue
- "Schema that accepts either a `FieldPlaceholder` or `ValuePlaceholder`."
- (s/named (s/cond-pre FieldPlaceholder ValuePlaceholder) "Field or value"))
-
-;; (def FieldOrOrderableValue (s/named (s/cond-pre FieldPlaceholder OrderableValuePlaceholder) "Field or orderable value (number or datetime)"))
-;; (def FieldOrStringValue (s/named (s/cond-pre FieldPlaceholder StringValuePlaceholder) "Field or string literal"))
+(def AnyFieldOrValue
+ "Schema that accepts anything normally considered a field (including expressions and literals) *or* a value or value placehoder."
+ (s/named (s/cond-pre AnyField Value ValuePlaceholder) "Field or value"))
-(def RValue
- "Schema for anything that can be an [RValue](https://github.com/metabase/metabase/wiki/Query-Language-'98#rvalues) -
- a `Field`, `Value`, or `Expression`."
- (s/named (s/cond-pre AnyValue FieldPlaceholderOrExpressionRef Expression)
- "RValue"))
+;;; +----------------------------------------------------------------------------------------------------------------------------------------------------------------+
+;;; | CLAUSES |
+;;; +----------------------------------------------------------------------------------------------------------------------------------------------------------------+
-;;; # ------------------------------------------------------------ CLAUSE SCHEMAS ------------------------------------------------------------
+;;; aggregation
(s/defrecord AggregationWithoutField [aggregation-type :- (s/named (s/enum :count :cumulative-count)
"Valid aggregation type")
@@ -281,9 +296,11 @@
"standard-deviation-aggregations is not supported by this driver."))
+;;; filter
+
(s/defrecord EqualityFilter [filter-type :- (s/enum := :!=)
field :- FieldPlaceholderOrExpressionRef
- value :- FieldOrAnyValue])
+ value :- AnyFieldOrValue])
(s/defrecord ComparisonFilter [filter-type :- (s/enum :< :<= :> :>=)
field :- FieldPlaceholderOrExpressionRef
@@ -316,28 +333,38 @@
(s/named (s/cond-pre SimpleFilterClause NotFilter CompoundFilter)
"Valid filter clause"))
+
+;;; order-by
+
(def OrderByDirection
"Schema for the direction in an `OrderBy` subclause."
(s/named (s/enum :ascending :descending) "Valid order-by direction"))
(def OrderBy
"Schema for top-level `order-by` clause in an MBQL query."
- (s/named {:field AnyFieldOrExpression
+ (s/named {:field AnyField
:direction OrderByDirection}
"Valid order-by subclause"))
+;;; page
+
(def Page
"Schema for the top-level `page` clause in a MBQL query."
(s/named {:page su/IntGreaterThanZero
:items su/IntGreaterThanZero}
"Valid page clause"))
+
+;;; +----------------------------------------------------------------------------------------------------------------------------------------------------------------+
+;;; | QUERY |
+;;; +----------------------------------------------------------------------------------------------------------------------------------------------------------------+
+
(def Query
"Schema for an MBQL query."
{(s/optional-key :aggregation) [Aggregation]
(s/optional-key :breakout) [FieldPlaceholderOrExpressionRef]
- (s/optional-key :fields) [AnyFieldOrExpression]
+ (s/optional-key :fields) [AnyField]
(s/optional-key :filter) Filter
(s/optional-key :limit) su/IntGreaterThanZero
(s/optional-key :order-by) [OrderBy]
diff --git a/src/metabase/query_processor/middleware/permissions.clj b/src/metabase/query_processor/middleware/permissions.clj
index e4c32e1b8c4..61cccf4a5d0 100644
--- a/src/metabase/query_processor/middleware/permissions.clj
+++ b/src/metabase/query_processor/middleware/permissions.clj
@@ -5,12 +5,14 @@
[metabase.util :as u]))
(defn- check-query-permissions* [query]
- ;; TODO - should we do anything if there is no *current-user-id* (for something like a pulse?)
(u/prog1 query
(when *current-user-id*
(perms/check-query-permissions *current-user-id* query))))
(defn check-query-permissions
- "Middleware that check that the current user has permissions to run the current query."
+ "Middleware that check that the current user has permissions to run the current query.
+ This only applies if `*current-user-id*` is bound. In other cases, like when running
+ public Cards or sending pulses, permissions need to be checked separately before allowing
+ the relevant objects to be create (e.g., when saving a new Pulse or 'publishing' a Card)."
[qp]
(comp qp check-query-permissions*))
diff --git a/src/metabase/query_processor/permissions.clj b/src/metabase/query_processor/permissions.clj
index 5dd16462d78..bbf2a888d43 100644
--- a/src/metabase/query_processor/permissions.clj
+++ b/src/metabase/query_processor/permissions.clj
@@ -62,6 +62,7 @@
(defn- ^:deprecated table-id [source-or-join-table]
+ {:post [(integer? %)]}
(or (:id source-or-join-table)
(:table-id source-or-join-table)))
@@ -75,6 +76,7 @@
(or (user-can-run-query-referencing-table? user-id (table-id table))
(throw-permissions-exception "You do not have permissions to run queries referencing table '%s'." (table-identifier table))))
+;; TODO - why is this the only function here that takes `user-id`?
(defn- throw-if-cannot-run-query
"Throw an exception if USER-ID doesn't have permissions to run QUERY."
[user-id {:keys [source-table join-tables]}]
@@ -118,8 +120,8 @@
(defn check-query-permissions
"Check that User with USER-ID has permissions to run QUERY, or throw an exception."
- [user-id {query-type :type, database :database, query :query, {card-id :card-id} :info}]
- {:pre [(integer? user-id)]}
+ [user-id {query-type :type, database :database, query :query, {card-id :card-id} :info, :as outer-query}]
+ {:pre [(integer? user-id) (map? outer-query)]}
(let [native? (= (keyword query-type) :native)
collection-id (db/select-one-field :collection_id 'Card :id card-id)]
(cond
diff --git a/src/metabase/query_processor/resolve.clj b/src/metabase/query_processor/resolve.clj
index 1d1747d15df..33fcc695de7 100644
--- a/src/metabase/query_processor/resolve.clj
+++ b/src/metabase/query_processor/resolve.clj
@@ -194,7 +194,7 @@
;; Otherwise fetch + resolve the Fields in question
(let [fields (->> (u/key-by :id (db/select [field/Field :name :display_name :base_type :special_type :visibility_type :table_id :parent_id :description :id]
:visibility_type [:not= "sensitive"]
- :id [:in field-ids]))
+ :id [:in field-ids]))
(m/map-vals rename-mb-field-keys)
(m/map-vals #(assoc % :parent (when-let [parent-id (:parent-id %)]
(i/map->FieldPlaceholder {:field-id parent-id})))))]
@@ -207,7 +207,11 @@
;; Recurse in case any new (nested) unresolved fields were found.
(recur (dec max-iterations))))))))
-(defn- fk-field-ids->info [source-table-id fk-field-ids]
+(defn- fk-field-ids->info
+ "Given a SOURCE-TABLE-ID and collection of FK-FIELD-IDS, return a sequence of maps containing IDs and identifiers for those FK fields and their target tables and fields.
+ FK-FIELD-IDS are IDs of fields that belong to the source table. For example, SOURCE-TABLE-ID might be 'checkins' and FK-FIELD-IDS might have the IDs for 'checkins.user_id'
+ and the like."
+ [source-table-id fk-field-ids]
(when (seq fk-field-ids)
(db/query {:select [[:source-fk.name :source-field-name]
[:source-fk.id :source-field-id]
@@ -219,8 +223,8 @@
:from [[field/Field :source-fk]]
:left-join [[field/Field :target-pk] [:= :source-fk.fk_target_field_id :target-pk.id]
[Table :target-table] [:= :target-pk.table_id :target-table.id]]
- :where [:and [:in :source-fk.id (set fk-field-ids)]
- [:= :source-fk.table_id source-table-id]
+ :where [:and [:in :source-fk.id (set fk-field-ids)]
+ [:= :source-fk.table_id source-table-id]
(mdb/isa :source-fk.special_type :type/FK)]})))
(defn- fk-field-ids->joined-tables
diff --git a/src/metabase/query_processor/util.clj b/src/metabase/query_processor/util.clj
index 47f80946e2a..304527c8b29 100644
--- a/src/metabase/query_processor/util.clj
+++ b/src/metabase/query_processor/util.clj
@@ -3,7 +3,11 @@
(:require [buddy.core
[codecs :as codecs]
[hash :as hash]]
- [cheshire.core :as json]))
+ [cheshire.core :as json]
+ [clojure.string :as str]
+ [metabase.util :as u]
+ [metabase.util.schema :as su]
+ [schema.core :as s]))
(defn mbql-query?
"Is the given query an MBQL query?"
@@ -33,6 +37,69 @@
(format ":: userID: %s queryType: %s queryHash: %s" executed-by query-type (codecs/bytes->hex query-hash)))))
+;;; ------------------------------------------------------------ Normalization ------------------------------------------------------------
+
+;; The following functions make it easier to deal with MBQL queries, which are case-insensitive, string/keyword insensitive, and underscore/hyphen insensitive.
+;; These should be preferred instead of assuming the frontend will always pass in clauses the same way, since different variation are all legal under MBQL '98.
+
+;; TODO - In the future it might make sense to simply walk the entire query and normalize the whole thing when it comes in. I've tried implementing middleware
+;; to do that but it ended up breaking a few things that wrongly assume different clauses will always use a certain case (e.g. SQL `:template_tags`). Fixing
+;; all of that is out-of-scope for the nested queries PR but should possibly be revisited in the future.
+
+(s/defn ^:always-validate normalize-token :- s/Keyword
+ "Convert a string or keyword in various cases (`lisp-case`, `snake_case`, or `SCREAMING_SNAKE_CASE`) to a lisp-cased keyword."
+ [token :- su/KeywordOrString]
+ (-> (name token)
+ str/lower-case
+ (str/replace #"_" "-")
+ keyword))
+
+(defn get-normalized
+ "Get the value for normalized key K in map M, regardless of how the key was specified in M,
+ whether string or keyword, lisp-case, snake_case, or SCREAMING_SNAKE_CASE.
+
+ (get-normalized {\"NUM_TOUCANS\" 2} :num-toucans) ; -> 2"
+ ([m k]
+ {:pre [(or (u/maybe? map? m)
+ (println "Not a map:" m))]}
+ (let [k (normalize-token k)]
+ (some (fn [[map-k v]]
+ (when (= k (normalize-token map-k))
+ v))
+ m)))
+ ([m k not-found]
+ (or (get-normalized m k)
+ not-found)))
+
+(defn get-in-normalized
+ "Like `get-normalized`, but accepts a sequence of keys KS, like `get-in`.
+
+ (get-in-normalized {\"NUM_BIRDS\" {\"TOUCANS\" 2}} [:num-birds :toucans]) ; -> 2"
+ ([m ks]
+ {:pre [(u/maybe? sequential? ks)]}
+ (loop [m m, [k & more] ks]
+ (if-not k
+ m
+ (recur (get-normalized m k) more))))
+ ([m ks not-found]
+ (or (get-in-normalized m ks)
+ not-found)))
+
+(defn dissoc-normalized
+ "Remove all matching keys from map M regardless of case, string/keyword, or hypens/underscores.
+
+ (dissoc-normalized {\"NUM_TOUCANS\" 3} :num-toucans) ; -> {}"
+ [m k]
+ {:pre [(or (u/maybe? map? m)
+ (println "Not a map:" m))]}
+ (let [k (normalize-token k)]
+ (loop [m m, [map-k & more, :as ks] (keys m)]
+ (cond
+ (not (seq ks)) m
+ (= k (normalize-token map-k)) (recur (dissoc m map-k) more)
+ :else (recur m more)))))
+
+
;;; ------------------------------------------------------------ Hashing ------------------------------------------------------------
(defn- select-keys-for-hashing
diff --git a/src/metabase/sync_database/analyze.clj b/src/metabase/sync_database/analyze.clj
index 87985f8b622..8881ba24d65 100644
--- a/src/metabase/sync_database/analyze.clj
+++ b/src/metabase/sync_database/analyze.clj
@@ -44,7 +44,7 @@
(try
(queries/table-row-count table)
(catch Throwable e
- (log/error (u/format-color 'red "Unable to determine row count for '%s': %s\n%s" (:name table) (.getMessage e) (u/pprint-to-str (u/filtered-stacktrace e)))))))
+ (log/warn (u/format-color 'red "Unable to determine row count for '%s': %s\n%s" (:name table) (.getMessage e) (u/pprint-to-str (u/filtered-stacktrace e)))))))
(defn test-for-cardinality?
"Should FIELD should be tested for cardinality?"
diff --git a/src/metabase/util/honeysql_extensions.clj b/src/metabase/util/honeysql_extensions.clj
index 70bfe6de28e..2b08b2a006c 100644
--- a/src/metabase/util/honeysql_extensions.clj
+++ b/src/metabase/util/honeysql_extensions.clj
@@ -84,6 +84,7 @@
(rest sql-string-or-vector))))))
+;; Single-quoted string literal
(defrecord Literal [literal]
ToSql
(to-sql [_]
diff --git a/test/metabase/http_client.clj b/test/metabase/http_client.clj
index 8324c12a618..b16790230d6 100644
--- a/test/metabase/http_client.clj
+++ b/test/metabase/http_client.clj
@@ -67,7 +67,6 @@
or throw an Exception if that fails."
[{:keys [email password], :as credentials}]
{:pre [(string? email) (string? password)]}
- (println "Authenticating" email) ; DEBUG
(try
(:id (client :post 200 "session" credentials))
(catch Throwable e
diff --git a/test/metabase/models/card_test.clj b/test/metabase/models/card_test.clj
index 3f6cb57eddc..0d7fc226e14 100644
--- a/test/metabase/models/card_test.clj
+++ b/test/metabase/models/card_test.clj
@@ -9,8 +9,8 @@
[permissions :as perms]]
[metabase.query-processor.expand :as ql]
[metabase.test
- [data :refer [id]]
- [util :as tu :refer [random-name]]]
+ [data :as data]
+ [util :as tu]]
[metabase.test.data.users :refer :all]
[metabase.util :as u]
[toucan.db :as db]
@@ -26,9 +26,9 @@
(let [get-dashboard-count (fn [] (dashboard-count (Card card-id)))]
[(get-dashboard-count)
- (do (db/insert! DashboardCard :card_id card-id, :dashboard_id (:id (create-dash! (random-name))), :parameter_mappings [])
+ (do (db/insert! DashboardCard :card_id card-id, :dashboard_id (:id (create-dash! (tu/random-name))), :parameter_mappings [])
(get-dashboard-count))
- (do (db/insert! DashboardCard :card_id card-id, :dashboard_id (:id (create-dash! (random-name))), :parameter_mappings [])
+ (do (db/insert! DashboardCard :card_id card-id, :dashboard_id (:id (create-dash! (tu/random-name))), :parameter_mappings [])
(get-dashboard-count))])))
@@ -60,25 +60,25 @@
(expect
false
- (tt/with-temp Card [card {:dataset_query {:database (id), :type "native"}}]
+ (tt/with-temp Card [card {:dataset_query {:database (data/id), :type "native"}}]
(binding [*current-user-permissions-set* (delay #{})]
(mi/can-read? card))))
(expect
- (tt/with-temp Card [card {:dataset_query {:database (id), :type "native"}}]
- (binding [*current-user-permissions-set* (delay #{(perms/native-read-path (id))})]
+ (tt/with-temp Card [card {:dataset_query {:database (data/id), :type "native"}}]
+ (binding [*current-user-permissions-set* (delay #{(perms/native-read-path (data/id))})]
(mi/can-read? card))))
;; in order to *write* a native card user should need native readwrite access
(expect
false
- (tt/with-temp Card [card {:dataset_query {:database (id), :type "native"}}]
- (binding [*current-user-permissions-set* (delay #{(perms/native-read-path (id))})]
+ (tt/with-temp Card [card {:dataset_query {:database (data/id), :type "native"}}]
+ (binding [*current-user-permissions-set* (delay #{(perms/native-read-path (data/id))})]
(mi/can-write? card))))
(expect
- (tt/with-temp Card [card {:dataset_query {:database (id), :type "native"}}]
- (binding [*current-user-permissions-set* (delay #{(perms/native-readwrite-path (id))})]
+ (tt/with-temp Card [card {:dataset_query {:database (data/id), :type "native"}}]
+ (binding [*current-user-permissions-set* (delay #{(perms/native-readwrite-path (data/id))})]
(mi/can-write? card))))
@@ -100,24 +100,24 @@
(defn- mbql [query]
- {:database (id)
+ {:database (data/id)
:type :query
:query query})
;; MBQL w/o JOIN
(expect
- #{(perms/object-path (id) "PUBLIC" (id :venues))}
+ #{(perms/object-path (data/id) "PUBLIC" (data/id :venues))}
(query-perms-set (mbql (ql/query
- (ql/source-table (id :venues))))
+ (ql/source-table (data/id :venues))))
:read))
;; MBQL w/ JOIN
(expect
- #{(perms/object-path (id) "PUBLIC" (id :checkins))
- (perms/object-path (id) "PUBLIC" (id :venues))}
+ #{(perms/object-path (data/id) "PUBLIC" (data/id :checkins))
+ (perms/object-path (data/id) "PUBLIC" (data/id :venues))}
(query-perms-set (mbql (ql/query
- (ql/source-table (id :checkins))
- (ql/order-by (ql/asc (ql/fk-> (id :checkins :venue_id) (id :venues :name))))))
+ (ql/source-table (data/id :checkins))
+ (ql/order-by (ql/asc (ql/fk-> (data/id :checkins :venue_id) (data/id :venues :name))))))
:read))
;; invalid/legacy card should return perms for something that doesn't exist so no one gets to see it
diff --git a/test/metabase/permissions_collection_test.clj b/test/metabase/permissions_collection_test.clj
index fec9b5777ec..77c78293e23 100644
--- a/test/metabase/permissions_collection_test.clj
+++ b/test/metabase/permissions_collection_test.clj
@@ -18,9 +18,6 @@
;; but not Rasta (all-users)
(defn- api-call-was-successful? {:style/indent 0} [response]
- (when (and (string? response)
- (not= response "You don't have permissions to do that."))
- (println "RESPONSE:" response)) ; DEBUG
(and (not= response "You don't have permissions to do that.")
(not= response "Unauthenticated")))
diff --git a/test/metabase/permissions_test.clj b/test/metabase/permissions_test.clj
index 43262069da6..5171091191b 100644
--- a/test/metabase/permissions_test.clj
+++ b/test/metabase/permissions_test.clj
@@ -129,7 +129,7 @@
:table_id (u/get-id table)
:dataset_query {:database (u/get-id db)
:type "native"
- :query (format "SELECT count(*) FROM \"%s\";" (:name table))}}))
+ :native {:query (format "SELECT count(*) FROM \"%s\";" (:name table))}}}))
(def ^:dynamic *card:db1-count-of-venues*)
diff --git a/test/metabase/query_processor/util_test.clj b/test/metabase/query_processor/util_test.clj
index 357583558a2..eea0cdd83e2 100644
--- a/test/metabase/query_processor/util_test.clj
+++ b/test/metabase/query_processor/util_test.clj
@@ -105,3 +105,53 @@
(qputil/query-hash {:database 2
:type "native"
:native {:query "SELECT pg_sleep(15), 2 AS two"}})))
+
+
+;;; ------------------------------------------------------------ Tests for get-normalized and get-in-normalized ------------------------------------------------------------
+
+(expect 2 (qputil/get-normalized {"num_toucans" 2} :num-toucans))
+(expect 2 (qputil/get-normalized {"NUM_TOUCANS" 2} :num-toucans))
+(expect 2 (qputil/get-normalized {"num-toucans" 2} :num-toucans))
+(expect 2 (qputil/get-normalized {:num_toucans 2} :num-toucans))
+(expect 2 (qputil/get-normalized {:NUM_TOUCANS 2} :num-toucans))
+(expect 2 (qputil/get-normalized {:num-toucans 2} :num-toucans))
+
+(expect
+ nil
+ (qputil/get-normalized nil :num-toucans))
+
+(expect 2 (qputil/get-in-normalized {"BIRDS" {"NUM_TOUCANS" 2}} [:birds :num-toucans]))
+(expect 2 (qputil/get-in-normalized {"birds" {"num_toucans" 2}} [:birds :num-toucans]))
+(expect 2 (qputil/get-in-normalized {"birds" {"num-toucans" 2}} [:birds :num-toucans]))
+(expect 2 (qputil/get-in-normalized {:BIRDS {:NUM_TOUCANS 2}} [:birds :num-toucans]))
+(expect 2 (qputil/get-in-normalized {:birds {:num_toucans 2}} [:birds :num-toucans]))
+(expect 2 (qputil/get-in-normalized {:birds {:num-toucans 2}} [:birds :num-toucans]))
+
+(expect
+ 2
+ (qputil/get-in-normalized {:num-toucans 2} [:num-toucans]))
+
+(expect
+ nil
+ (qputil/get-in-normalized nil [:birds :num-toucans]))
+
+(expect
+ 10
+ (qputil/get-in-normalized
+ {"dataset_query" {"query" {"source_table" 10}}}
+ [:dataset-query :query :source-table]))
+
+(expect {} (qputil/dissoc-normalized {"NUM_TOUCANS" 3} :num-toucans))
+(expect {} (qputil/dissoc-normalized {"num_toucans" 3} :num-toucans))
+(expect {} (qputil/dissoc-normalized {"num-toucans" 3} :num-toucans))
+(expect {} (qputil/dissoc-normalized {:NUM_TOUCANS 3} :num-toucans))
+(expect {} (qputil/dissoc-normalized {:num_toucans 3} :num-toucans))
+(expect {} (qputil/dissoc-normalized {:num-toucans 3} :num-toucans))
+
+(expect
+ {}
+ (qputil/dissoc-normalized {:num-toucans 3, "NUM_TOUCANS" 3, "num_toucans" 3} :num-toucans))
+
+(expect
+ nil
+ (qputil/dissoc-normalized nil :num-toucans))
diff --git a/test/metabase/test/data.clj b/test/metabase/test/data.clj
index 83606ed5b39..867e0ea0440 100644
--- a/test/metabase/test/data.clj
+++ b/test/metabase/test/data.clj
@@ -120,7 +120,12 @@
`(run-query* (query ~table ~@forms)))
-(defn format-name [nm]
+(defn format-name
+ "Format a SQL schema, table, or field identifier in the correct way for the current database by calling the
+ driver's implementation of `format-name`.
+ (Most databases use the default implementation of `identity`; H2 uses `clojure.string/upper-case`.)
+ This function DOES NOT quote the identifier."
+ [nm]
(i/format-name *driver* (name nm)))
(defn- get-table-id-or-explode [db-id table-name]
diff --git a/test/metabase/test/data/users.clj b/test/metabase/test/data/users.clj
index 773f80d5f96..a1415e76d12 100644
--- a/test/metabase/test/data/users.clj
+++ b/test/metabase/test/data/users.clj
@@ -70,7 +70,6 @@
{:pre [(string? email) (string? first) (string? last) (string? password) (m/boolean? superuser) (m/boolean? active)]}
(wait-for-initiailization)
(or (User :email email)
- (println "Creating test user:" email) ; DEBUG
(db/insert! User
:email email
:first_name first
@@ -139,7 +138,6 @@
(when-not (= status-code 401)
(throw e))
;; If we got a 401 unauthenticated clear the tokens cache + recur
- (printf "Got 401 (Unauthenticated) for %s. Clearing cached auth tokens and retrying request.\n" username) ; DEBUG
(reset! tokens {})
(apply client-fn username args)))))
diff --git a/test_resources/log4j.properties b/test_resources/log4j.properties
index 993488ed722..161945c47bd 100644
--- a/test_resources/log4j.properties
+++ b/test_resources/log4j.properties
@@ -16,8 +16,8 @@ log4j.appender.file.layout.ConversionPattern=%d [%t] %-5p%c - %m%n
# customizations to logging by package
log4j.logger.com.mchange=ERROR
+log4j.logger.org.eclipse.jetty.server.HttpChannel=ERROR
log4j.logger.metabase=ERROR
-log4j.logger.metabase.middleware=INFO
log4j.logger.metabase.test-setup=INFO
log4j.logger.metabase.test.data.datasets=INFO
log4j.logger.metabase.util.encryption=INFO
--
GitLab