Add identifier-type to hx/identifier

[ci drivers]

modules/drivers/bigquery/src/metabase/driver/bigquery.clj

@@ -8,7 +8,6 @@
              [string :as str]]
             [honeysql
              [core :as hsql]
-             [format :as hformat]
              [helpers :as h]]
             [metabase
              [config :as config]
@@ -22,9 +21,7 @@
             [metabase.mbql
              [schema :as mbql.s]
              [util :as mbql.u]]
-            [metabase.models
-             [field :refer [Field]]
-             [table :as table]]
+            [metabase.models.table :as table]
             [metabase.query-processor
              [store :as qp.store]
              [util :as qputil]]
@@ -41,9 +38,9 @@
            [com.google.api.services.bigquery Bigquery Bigquery$Builder BigqueryScopes]
            [com.google.api.services.bigquery.model QueryRequest QueryResponse Table TableCell TableFieldSchema TableList
             TableList$Tables TableReference TableRow TableSchema]
-           honeysql.format.ToSql
            java.sql.Time
-           [java.util Collections Date]))
+           [java.util Collections Date]
+           metabase.util.honeysql_extensions.Identifier))
 
 (driver/register! :bigquery, :parent #{:google :sql})
 
@@ -55,7 +52,10 @@
    (and (string? s)
         (re-matches #"^([a-zA-Z_][a-zA-Z_0-9]*){1,128}$" s))))
 
-(defn- dataset-name-for-current-query
+(def ^:private BigQueryIdentifierString
+  (s/pred valid-bigquery-identifier? "Valid BigQuery identifier"))
+
+(s/defn ^:private dataset-name-for-current-query :- BigQueryIdentifierString
   "Fetch the dataset name for the database associated with this query, needed because BigQuery requires you to qualify
   identifiers with it. This is primarily called automatically for the `to-sql` implementation of the
   `BigQueryIdentifier` record type; see its definition for more details.
@@ -63,8 +63,9 @@
   This looks for the value inside the SQL QP's `*query*` dynamic var; since this won't be bound for non-MBQL queries,
   you will want to avoid this function for SQL queries."
   []
-  {:pre [(map? sql.qp/*query*)], :post [(valid-bigquery-identifier? %)]}
-  (:dataset-id sql.qp/*query*))
+  (or (some-> sql.qp/*query* :dataset-id)
+      (when (qp.store/initialized?)
+        (some-> (qp.store/database) :details :dataset-id))))
 
 
 ;;; +----------------------------------------------------------------------------------------------------------------+
@@ -147,13 +148,15 @@
     "NUMERIC"   :type/Decimal
     :type/*))
 
-(defn- table-schema->metabase-field-info [^TableSchema schema]
+(s/defn ^:private table-schema->metabase-field-info
+  [schema :- TableSchema]
   (for [^TableFieldSchema field (.getFields schema)]
     {:name          (.getName field)
      :database-type (.getType field)
      :base-type     (bigquery-type->base-type (.getType field))}))
 
-(defmethod driver/describe-table :bigquery [_ database {table-name :name}]
+(defmethod driver/describe-table :bigquery
+  [_ database {table-name :name}]
   {:schema nil
    :name   table-name
    :fields (set (table-schema->metabase-field-info (.getSchema (get-table database table-name))))})
@@ -305,40 +308,39 @@
 ;;; |                                                Query Processor                                                 |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
-;; This record type used for BigQuery table and field identifiers, since BigQuery has some stupid rules about how to
-;; quote them (tables are like `dataset.table` and fields are like `dataset.table`.`field`)
-;; This implements HoneySql's ToSql protocol, so we can just output this directly in most of our QP code below
-;;
-;; TODO - this is totally unnecessary now, we can just override `->honeysql` for `Field` and `Table` instead. FIXME!
-(defrecord ^:private BigQueryIdentifier [dataset-name ; optional; will use (dataset-name-for-current-query) otherwise
-                                         table-name
-                                         field-name
-                                         alias?]
-  honeysql.format/ToSql
-  (to-sql [{:keys [dataset-name table-name field-name], :as bq-id}]
-    ;; Check to make sure the identifiers are valid and don't contain any sorts of escape characters since we are
-    ;; constructing raw SQL here, and would like to avoid potential SQL injection vectors (even though this is not
-    ;; direct user input, but instead would require someone to go in and purposely corrupt their Table names/Field names
-    ;; to do so)
-    (when dataset-name
-      (assert (valid-bigquery-identifier? dataset-name)
-        (tru "Invalid BigQuery identifier: ''{0}''" dataset-name)))
-    (assert (valid-bigquery-identifier? table-name)
-      (tru "Invalid BigQuery identifier: ''{0}''" table-name))
-    (when (seq field-name)
-      (assert (valid-bigquery-identifier? field-name)
-        (tru "Invalid BigQuery identifier: ''{0}''" field-name)))
-    ;; BigQuery identifiers should look like `dataset.table` or `dataset.table`.`field` (SAD!)
-    (let [dataset-name (or dataset-name (dataset-name-for-current-query))]
-      (str
-       (if alias?
-         (format "`%s`" table-name)
-         (format "`%s.%s`" dataset-name table-name))
-       (when (seq field-name)
-         (format ".`%s`" field-name))))))
-
-(defn- honeysql-form->sql ^String [honeysql-form]
-  {:pre [(map? honeysql-form)]}
+(defn- should-qualify-identifier?
+  "Should we qualify an Identifier with the dataset name?
+
+  Table & Field identifiers (usually) need to be qualified with the current dataset name; this needs to be part of the
+  table e.g.
+
+    `table`.`field` -> `dataset.table`.`field`"
+  [{:keys [identifier-type components]}]
+  (cond
+    ;; If we're currently using a Table alias, don't qualify the alias with the dataset name
+    sql.qp/*table-alias*
+    false
+
+    ;; otherwise always qualify Table identifiers
+    (= identifier-type :table)
+    true
+
+    ;; Only qualify Field identifiers that are qualified by a Table. (e.g. don't qualify stuff inside `CREATE TABLE`
+    ;; DDL statements)
+    (and (= identifier-type :field)
+         (>= (count components) 2))
+    true))
+
+(defmethod sql.qp/->honeysql [:bigquery Identifier]
+  [_ identifier]
+  (cond-> identifier
+    (should-qualify-identifier? identifier)
+    (update :components (fn [[table & more]]
+                          (cons (str (dataset-name-for-current-query) \. table)
+                                more)))))
+
+(s/defn ^:private honeysql-form->sql :- s/Str
+  [honeysql-form :- su/Map]
   (let [[sql & args] (sql.qp/honeysql-form->sql+args :bigquery honeysql-form)]
     (when (seq args)
       (throw (Exception. (str (tru "BigQuery statements can''t be parameterized!")))))
@@ -389,27 +391,13 @@
        (sql.qp/date driver unit)
        hx/->time))
 
-(defmethod sql.qp/->honeysql [Object :datetime-field]
-  [driver [_ field unit]]
-  (sql.qp/date driver unit (sql.qp/->honeysql driver field)))
-
-(defmethod sql.qp/->honeysql [:bigquery (class Field)]
-  [driver field]
-  (let [{table-name :name, :as table} (qp.store/table (:table_id field))
-        field-identifier              (map->BigQueryIdentifier
-                                       {:table-name table-name
-                                        :field-name (:name field)
-                                        :alias?     (:alias? table)})]
-    (sql.qp/cast-unix-timestamp-field-if-needed driver field field-identifier)))
-
-(defmethod sql.qp/field->identifier :bigquery [_ {table-id :table_id, :as field}]
+(defmethod sql.qp/field->identifier :bigquery [_ {table-id :table_id, field-name :name, :as field}]
   ;; TODO - Making a DB call for each field to fetch its Table is inefficient and makes me cry, but this method is
   ;; currently only used for SQL params so it's not a huge deal at this point
   ;;
   ;; TODO - we should make sure these are in the QP store somewhere and then could at least batch the calls
-  (let [table-name (db/select-one-field :name table/Table :id (u/get-id table-id))
-        details    (:details (qp.store/database))]
-    (map->BigQueryIdentifier {:dataset-name (:dataset-id details), :table-name table-name, :field-name (:name field)})))
+  (let [table-name (db/select-one-field :name table/Table :id (u/get-id table-id))]
+    (hx/identifier :field table-name field-name)))
 
 (defmethod sql.qp/apply-top-level-clause [:bigquery :breakout]
   [driver _ honeysql-form {breakout-field-clauses :breakout, fields-field-clauses :fields}]
@@ -424,32 +412,6 @@
       ((partial apply h/merge-select) (for [field-clause breakout-field-clauses
                                             :when        (not (contains? (set fields-field-clauses) field-clause))]
                                         (sql.qp/as driver field-clause)))))
-
-;; Copy of the SQL implementation, but prepends the current dataset ID to the table name.
-(defmethod sql.qp/apply-top-level-clause [:bigquery :source-table] [_ _ honeysql-form {source-table-id :source-table}]
-  (let [{table-name :name} (qp.store/table source-table-id)]
-    (h/from honeysql-form (map->BigQueryIdentifier {:table-name table-name}))))
-
-;; Copy of the SQL implementation, but prepends the current dataset ID to join-alias.
-(defmethod sql.qp/apply-top-level-clause [:bigquery :join-tables]
-  [_ _ honeysql-form {join-tables :join-tables, source-table-id :source-table}]
-  (let [{source-table-name :name} (qp.store/table source-table-id)]
-    (loop [honeysql-form honeysql-form, [{:keys [table-id pk-field-id fk-field-id join-alias]} & more] join-tables]
-      (let [{table-name :name} (qp.store/table table-id)
-            source-field       (qp.store/field fk-field-id)
-            pk-field           (qp.store/field pk-field-id)
-
-            honeysql-form
-            (h/merge-left-join honeysql-form
-              [(map->BigQueryIdentifier {:table-name table-name})
-               (map->BigQueryIdentifier {:table-name join-alias, :alias? true})]
-              [:=
-               (map->BigQueryIdentifier {:table-name source-table-name, :field-name (:name source-field)})
-               (map->BigQueryIdentifier {:table-name join-alias, :field-name (:name pk-field), :alias? true})])]
-        (if (seq more)
-          (recur honeysql-form more)
-          honeysql-form)))))
-
 (defn- ag-ref->alias [[_ index]]
   (let [{{aggregations :aggregation} :query} sql.qp/*query*
         [ag-type :as ag]                     (nth aggregations index)]

modules/drivers/bigquery/test/metabase/driver/bigquery_test.clj

 (ns metabase.driver.bigquery-test
   (:require [clj-time.core :as time]
-            [expectations :refer :all]
+            [expectations :refer [expect]]
             [honeysql.core :as hsql]
             [metabase
              [driver :as driver]
@@ -171,16 +171,6 @@
                                  :breakout     [[:fk-> (data/id :venues :category_id) (data/id :categories :name)]]}})]
         (get-in results [:data :native_form :query] results)))))
 
-;; Make sure the BigQueryIdentifier class works as expected
-(expect
-  ["SELECT `dataset.table`.`field`"]
-  (hsql/format {:select [(#'bigquery/map->BigQueryIdentifier
-                          {:dataset-name "dataset", :table-name "table", :field-name "field"})]}))
-
-(expect
-  ["SELECT `dataset.table`"]
-  (hsql/format {:select [(#'bigquery/map->BigQueryIdentifier {:dataset-name "dataset", :table-name "table"})]}))
-
 (defn- native-timestamp-query [db-or-db-id timestamp-str timezone-str]
   (-> (qp/process-query
         {:database (u/get-id db-or-db-id)

modules/drivers/oracle/src/metabase/driver/oracle.clj

@@ -196,12 +196,11 @@
       col
 
       ;; otherwise we *should* be dealing with an Identifier. If so, take the last component of the Identifier and use
-      ;; that as the alias. Because Identifiers can be nested, check if the last part is an `Identifier` and recurse
-      ;; if needed.
+      ;; that as the alias.
       ;;
-      ;; TODO - could this be done using `->honeysql` instead?
+      ;; TODO - could this be done using `->honeysql` or `field->alias` instead?
       (instance? Identifier col)
-      [col (hx/identifier (last (:components col)))]
+      [col (hx/identifier :field-alias (last (:components col)))]
 
       :else
       (do

modules/drivers/oracle/test/metabase/driver/oracle_test.clj

@@ -72,31 +72,31 @@
 ;; `deduplicate-identifiers` should use the last component of an identifier as the alias if it does not already have
 ;; one
 (expect
-  [[(hx/identifier "A" "B" "C" "D") (hx/identifier "D")]
-   [(hx/identifier "F")             (hx/identifier "G")]]
+  [[(hx/identifier :field "A" "B" "C" "D") (hx/identifier :field-alias "D")]
+   [(hx/identifier :field "F")             (hx/identifier :field-alias "G")]]
   (#'oracle/deduplicate-identifiers
-   [(hx/identifier "A" "B" "C" "D")
-    [(hx/identifier "F")            (hx/identifier "G")]]))
+   [(hx/identifier :field "A" "B" "C" "D")
+    [(hx/identifier :field "F")            (hx/identifier :field-alias "G")]]))
 
 ;; `deduplicate-identifiers` should append numeric suffixes to duplicate aliases
 (expect
-  [[(hx/identifier "A" "B" "C" "D") (hx/identifier "D")]
-   [(hx/identifier "E" "D")         (hx/identifier "D_2")]
-   [(hx/identifier "F")             (hx/identifier "G")]]
+  [[(hx/identifier :field "A" "B" "C" "D") (hx/identifier :field-alias "D")]
+   [(hx/identifier :field "E" "D")         (hx/identifier :field-alias "D_2")]
+   [(hx/identifier :field "F")             (hx/identifier :field-alias "G")]]
   (#'oracle/deduplicate-identifiers
-   [(hx/identifier "A" "B" "C" "D")
-    (hx/identifier "E" "D")
-    [(hx/identifier "F")            (hx/identifier "G")]]))
+   [(hx/identifier :field "A" "B" "C" "D")
+    (hx/identifier :field "E" "D")
+    [(hx/identifier :field "F")            (hx/identifier :field-alias "G")]]))
 
 ;; `deduplicate-identifiers` should handle aliases that are already suffixed gracefully
 (expect
-  [[(hx/identifier "A" "B" "C" "D") (hx/identifier "D")]
-   [(hx/identifier "E" "D")         (hx/identifier "D_2")]
-   [(hx/identifier "F")             (hx/identifier "D_3")]]
+  [[(hx/identifier :field "A" "B" "C" "D") (hx/identifier :field-alias "D")]
+   [(hx/identifier :field "E" "D")         (hx/identifier :field-alias "D_2")]
+   [(hx/identifier :field "F")             (hx/identifier :field-alias "D_3")]]
   (#'oracle/deduplicate-identifiers
-   [(hx/identifier "A" "B" "C" "D")
-    (hx/identifier "E" "D")
-    [(hx/identifier "F")            (hx/identifier "D_2")]]))
+   [(hx/identifier :field "A" "B" "C" "D")
+    (hx/identifier :field "E" "D")
+    [(hx/identifier :field "F")            (hx/identifier :field-alias "D_2")]]))
 
 
 (expect

modules/drivers/presto/src/metabase/driver/presto.clj

@@ -154,7 +154,7 @@
 (s/defmethod driver/can-connect? :presto
   [driver {:keys [catalog] :as details} :- PrestoConnectionDetails]
   (let [{[[v]] :rows} (execute-presto-query! details
-                        (format "SHOW SCHEMAS FROM %s LIKE 'information_schema'" (sql.u/quote-name driver catalog)))]
+                        (format "SHOW SCHEMAS FROM %s LIKE 'information_schema'" (sql.u/quote-name driver :database catalog)))]
     (= v "information_schema")))
 
 (defmethod driver/date-interval :presto
@@ -164,12 +164,12 @@
 (s/defn ^:private database->all-schemas :- #{su/NonBlankString}
   "Return a set of all schema names in this `database`."
   [driver {{:keys [catalog schema] :as details} :details :as database}]
-  (let [sql            (str "SHOW SCHEMAS FROM " (sql.u/quote-name driver catalog))
+  (let [sql            (str "SHOW SCHEMAS FROM " (sql.u/quote-name driver :database catalog))
         {:keys [rows]} (execute-presto-query! details sql)]
     (set (map first rows))))
 
 (defn- describe-schema [driver {{:keys [catalog] :as details} :details} {:keys [schema]}]
-  (let [sql            (str "SHOW TABLES FROM " (sql.u/quote-name driver catalog schema))
+  (let [sql            (str "SHOW TABLES FROM " (sql.u/quote-name driver :schema catalog schema))
         {:keys [rows]} (execute-presto-query! details sql)
         tables         (map first rows)]
     (set (for [table-name tables]
@@ -207,7 +207,7 @@
 
 (defmethod driver/describe-table :presto
   [driver {{:keys [catalog] :as details} :details} {schema :schema, table-name :name}]
-  (let [sql            (str "DESCRIBE " (sql.u/quote-name driver catalog schema table-name))
+  (let [sql            (str "DESCRIBE " (sql.u/quote-name driver :table catalog schema table-name))
         {:keys [rows]} (execute-presto-query! details sql)]
     {:schema schema
      :name   table-name

modules/drivers/presto/test/metabase/test/data/presto.clj

@@ -62,7 +62,7 @@
             (sql.tx/qualify-and-quote driver database-name table-name)
             (str/join \, dummy-values)
             (str/join \, (for [column columns]
-                           (sql.u/quote-name driver (tx/format-name driver column)))))))
+                           (sql.u/quote-name driver :field (tx/format-name driver column)))))))
 
 (defmethod sql.tx/drop-table-if-exists-sql :presto [driver {:keys [database-name]} {:keys [table-name]}]
   (str "DROP TABLE IF EXISTS " (sql.tx/qualify-and-quote driver database-name table-name)))

modules/drivers/snowflake/src/metabase/driver/snowflake.clj

@@ -19,9 +19,7 @@
              [sync :as sql-jdbc.sync]]
             [metabase.driver.sql.query-processor :as sql.qp]
             [metabase.driver.sql.util.unprepare :as unprepare]
-            [metabase.models
-             [field :refer [Field]]
-             [table :refer [Table]]]
+            [metabase.models.table :refer [Table]]
             [metabase.query-processor.store :as qp.store]
             [metabase.util
              [date :as du]
@@ -30,6 +28,7 @@
             [toucan.db :as db])
   (:import java.sql.Time
            java.util.Date
+           metabase.util.honeysql_extensions.Identifier
            net.snowflake.client.jdbc.SnowflakeSQLException))
 
 (driver/register! :snowflake, :parent :sql-jdbc)
@@ -140,18 +139,40 @@
   (or (-> (qp.store/database) db-name)
       (throw (Exception. "Missing DB name"))))
 
-(defmethod sql.qp/->honeysql [:snowflake (class Field)]
-  [driver field]
-  (let [table            (qp.store/table (:table_id field))
-        db-name          (when-not (:alias? table)
-                           (query-db-name))
-        field-identifier (sql.qp/->honeysql driver (hx/identifier db-name (:schema table) (:name table) (:name field)))]
-    (sql.qp/cast-unix-timestamp-field-if-needed driver field field-identifier)))
-
-(defmethod sql.qp/->honeysql [:snowflake (class Table)]
-  [driver table]
-  (let [{table-name :name, schema :schema} table]
-    (sql.qp/->honeysql driver (hx/identifier (query-db-name) schema table-name))))
+;; unless we're currently using a table alias, we need to prepend Table and Field identifiers with the DB name for the
+;; query
+(defn- should-qualify-identifier?
+  "Should we qualify an Identifier with the dataset name?
+
+  Table & Field identifiers (usually) need to be qualified with the current database name; this needs to be part of the
+  table e.g.
+
+    \"table\".\"field\" -> \"database\".\"table\".\"field\""
+  [{:keys [identifier-type components]}]
+  (cond
+    ;; If we're currently using a Table alias, don't qualify the alias with the dataset name
+    sql.qp/*table-alias*
+    false
+
+    ;; already qualified
+    (= (first components) (query-db-name))
+    false
+
+    ;; otherwise always qualify Table identifiers
+    (= identifier-type :table)
+    true
+
+    ;; Only qualify Field identifiers that are qualified by a Table. (e.g. don't qualify stuff inside `CREATE TABLE`
+    ;; DDL statements)
+    (and (= identifier-type :field)
+         (>= (count components) 2))
+    true))
+
+(defmethod sql.qp/->honeysql [:snowflake Identifier]
+  [_ {:keys [identifier-type], :as identifier}]
+  (cond-> identifier
+    (should-qualify-identifier? identifier)
+    (update :components (partial cons (query-db-name)))))
 
 (defmethod sql.qp/->honeysql [:snowflake :time]
   [driver [_ value unit]]

modules/drivers/sparksql/src/metabase/driver/sparksql.clj

@@ -34,14 +34,11 @@
   "Default alias for all source tables. (Not for source queries; those still use the default SQL QP alias of `source`.)"
   "t1")
 
+;; use `source-table-alias` for the source Table, e.g. `t1.field` instead of the normal `schema.table.field`
 (defmethod sql.qp/->honeysql [:sparksql (class Field)]
   [driver field]
-  (let [table            (qp.store/table (:table_id field))
-        table-name       (if (:alias? table)
-                           (:name table)
-                           source-table-alias)
-        field-identifier (sql.qp/->honeysql driver (hx/identifier table-name (:name field)))]
-    (sql.qp/cast-unix-timestamp-field-if-needed driver field field-identifier)))
+  (binding [sql.qp/*table-alias* (or sql.qp/*table-alias* source-table-alias)]
+    ((get-method sql.qp/->honeysql [:hive-like (class Field)]) driver field)))
 
 (defmethod sql.qp/apply-top-level-clause [:sparksql :page] [_ _ honeysql-form {{:keys [items page]} :page}]
   (let [offset (* (dec page) items)]
@@ -61,8 +58,8 @@
 (defmethod sql.qp/apply-top-level-clause [:sparksql :source-table]
   [driver _ honeysql-form {source-table-id :source-table}]
   (let [{table-name :name, schema :schema} (qp.store/table source-table-id)]
-    (h/from honeysql-form [(sql.qp/->honeysql driver (hx/identifier schema table-name))
-                           source-table-alias])))
+    (h/from honeysql-form [(sql.qp/->honeysql driver (hx/identifier :table schema table-name))
+                           (sql.qp/->honeysql driver (hx/identifier :table-alias source-table-alias))])))
 
 
 ;;; ------------------------------------------- Other Driver Method Impls --------------------------------------------
@@ -112,7 +109,7 @@
    (with-open [conn (jdbc/get-connection (sql-jdbc.conn/db->pooled-connection-spec database))]
      (let [results (jdbc/query {:connection conn} [(format
                                                     "describe %s"
-                                                    (sql.u/quote-name driver
+                                                    (sql.u/quote-name driver :table
                                                       (dash-to-underscore schema)
                                                       (dash-to-underscore table-name)))])]
        (set

modules/drivers/sparksql/test/metabase/test/data/sparksql.clj

@@ -88,7 +88,7 @@
 
 (defmethod sql.tx/create-table-sql :sparksql
   [driver {:keys [database-name], :as dbdef} {:keys [table-name field-definitions]}]
-  (let [quote-name    #(sql.u/quote-name driver (tx/format-name driver %))
+  (let [quote-name    #(sql.u/quote-name driver :field (tx/format-name driver %))
         pk-field-name (quote-name (sql.tx/pk-field-name driver))]
     (format "CREATE TABLE %s (%s, %s %s)"
             (sql.tx/qualify-and-quote driver database-name table-name)

src/metabase/driver/sql/query_processor.clj

@@ -21,7 +21,8 @@
             [metabase.query-processor.middleware.annotate :as annotate]
             [metabase.util
              [honeysql-extensions :as hx]
-             [i18n :refer [tru]]]
+             [i18n :refer [tru]]
+             [schema :as su]]
             [schema.core :as s])
   (:import honeysql.format.ToSql
            metabase.util.honeysql_extensions.Identifier))
@@ -180,7 +181,7 @@
                      [*table-alias*]
                      (let [{schema :schema, table-name :name} (qp.store/table table-id)]
                        [schema table-name]))
-        identifier (->honeysql driver (apply hx/identifier (concat qualifiers [field-name])))]
+        identifier (->honeysql driver (apply hx/identifier :field (concat qualifiers [field-name])))]
     (cast-unix-timestamp-field-if-needed driver field identifier)))
 
 (defmethod ->honeysql [:sql :field-id]
@@ -189,7 +190,7 @@
 
 (defmethod ->honeysql [:sql :field-literal]
   [driver [_ field-name]]
-  (->honeysql driver (hx/identifier *table-alias* field-name)))
+  (->honeysql driver (hx/identifier :field *table-alias* field-name)))
 
 (defmethod ->honeysql [:sql :joined-field]
   [driver [_ alias field]]
@@ -316,7 +317,7 @@
                        expression-name      expression-name
                        field                (field->alias driver field)
                        (string? id-or-name) id-or-name)]
-      (->honeysql driver (hx/identifier alias)))))
+      (->honeysql driver (hx/identifier :field-alias alias)))))
 
 (defn as
   "Generate HoneySQL for an `AS` form (e.g. `<form> AS <field>`) using the name information of a `field-clause`. The
@@ -354,6 +355,7 @@
                 form
                 [(->honeysql driver ag)
                  (->honeysql driver (hx/identifier
+                                     :field-alias
                                      (driver/format-custom-field-name driver (annotate/aggregation-name ag))))])]
       (if-not (seq more)
         form
@@ -450,17 +452,20 @@
   [driver table-or-query-expr {:keys [join-alias fk-field-id pk-field-id]}]
   (let [source-field (qp.store/field fk-field-id)
         pk-field     (qp.store/field pk-field-id)]
-    [[table-or-query-expr (keyword join-alias)]
+    [[table-or-query-expr (->honeysql driver (hx/identifier :table-alias join-alias))]
      [:=
       (->honeysql driver source-field)
-      (->honeysql driver (hx/identifier join-alias (:name pk-field)))]]))
+      (binding [*table-alias* join-alias]
+        (->honeysql driver pk-field))]]))
 
 (s/defn ^:private join-info->honeysql
   [driver , {:keys [query table-id], :as info} :- mbql.s/JoinInfo]
   (if query
     (make-honeysql-join-clauses driver (build-honeysql-form driver query) info)
-    (let [table (qp.store/table table-id)]
-      (make-honeysql-join-clauses driver (->honeysql driver table) info))))
+    (let [table            (qp.store/table table-id)
+          table-identifier (binding [*table-alias* nil]
+                             (->honeysql driver table))]
+      (make-honeysql-join-clauses driver table-identifier info))))
 
 (defmethod apply-top-level-clause [:sql :join-tables]
   [driver _ honeysql-form {:keys [join-tables]}]
@@ -496,7 +501,7 @@
 (defmethod ->honeysql [:sql (class Table)]
   [driver table]
   (let [{table-name :name, schema :schema} table]
-    (->honeysql driver (hx/identifier schema table-name))))
+    (->honeysql driver (hx/identifier :table schema table-name))))
 
 (defmethod apply-top-level-clause [:sql :source-table]
   [driver _ honeysql-form {source-table-id :source-table}]
@@ -556,20 +561,14 @@
               (hsql/raw (str "(" (str/replace native #";+\s*$" "") ")")) ; strip off any trailing slashes
               (binding [*nested-query-level* (inc *nested-query-level*)]
                 (apply-clauses driver {} source-query)))
-            source-query-alias]]))
+            (->honeysql driver (hx/identifier :table-alias source-query-alias))]]))
 
 (defn- apply-clauses-with-aliased-source-query-table
   "For queries that have a source query that is a normal MBQL query with a source table, temporarily swap the name of
   that table to the `source` alias and handle other clauses. This is done so `field-id` references and the like
   referring to Fields belonging to the Table in the source query work normally."
   [driver honeysql-form {:keys [source-query], :as inner-query}]
-  (qp.store/with-pushed-store
-    (when-let [source-table-id (:source-table source-query)]
-      (qp.store/store-table! (assoc (qp.store/table source-table-id)
-                               :schema nil
-                               :name   (name source-query-alias)
-                               ;; some drivers like Snowflake need to know this so they don't include Database name
-                               :alias? true)))
+  (binding [*table-alias* source-query-alias]
     (apply-top-level-clauses driver honeysql-form (dissoc inner-query :source-query))))
 
 
@@ -586,10 +585,9 @@
      inner-query)
     (apply-top-level-clauses driver honeysql-form inner-query)))
 
-(defn build-honeysql-form
+(s/defn build-honeysql-form
   "Build the HoneySQL form we will compile to SQL and execute."
-  [driverr {inner-query :query}]
-  {:pre [(map? inner-query)]}
+  [driverr, {inner-query :query} :- su/Map]
   (u/prog1 (apply-clauses driverr {} inner-query)
     (when-not i/*disable-qp-logging*
       (log/debug (tru "HoneySQL Form:") (u/emoji "🍯") "\n" (u/pprint-to-str 'cyan <>)))))
@@ -599,11 +597,10 @@
 ;;; |                                                 MBQL -> Native                                                 |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
-(defn honeysql-form->sql+args
-  "Convert HONEYSQL-FORM to a vector of SQL string and params, like you'd pass to JDBC."
+(s/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)]}
+  [driver, honeysql-form :- su/Map]
   (let [[sql & args] (try (binding [hformat/*subquery?* false]
                             (hsql/format honeysql-form
                               :quoting             (quote-style driver)

src/metabase/driver/sql/util.clj

@@ -2,9 +2,10 @@
   "Utility functions for writing SQL drivers."
   (:require [honeysql.core :as hsql]
             [metabase.driver.sql.query-processor :as sql.qp]
-            [metabase.util.honeysql-extensions :as hx]))
+            [metabase.util.honeysql-extensions :as hx]
+            [schema.core :as s]))
 
-(defn quote-name
+(s/defn quote-name
   "Quote unqualified string or keyword identifier(s) by passing them to `hx/identifier`, then calling HoneySQL `format`
   on the resulting `Identifier`. Uses the `sql.qp/quote-style` of the current driver. You can implement `->honeysql`
   for `Identifier` if you need custom behavior here.
@@ -14,9 +15,9 @@
 
   You should only use this function for places where you are not using HoneySQL, such as queries written directly in
   SQL. For HoneySQL forms, `Identifier` is converted to SQL automatically when it is compiled."
-  {:style/indent 1}
-  [driver & identifiers]
+  {:style/indent 2}
+  [driver :- s/Keyword, identifier-type :- hx/IdentifierType, & components]
   (first
-   (hsql/format (sql.qp/->honeysql driver (apply hx/identifier identifiers))
+   (hsql/format (sql.qp/->honeysql driver (apply hx/identifier identifier-type components))
      :quoting             (sql.qp/quote-style driver)
      :allow-dashed-names? true)))

src/metabase/driver/sql_jdbc.clj

@@ -23,7 +23,7 @@
    (jdbc/query (sql-jdbc.conn/db->pooled-connection-spec database)
                (sql.qp/honeysql-form->sql+args driver honeysql-form)))
   ([driver database table honeysql-form]
-   (query driver database (merge {:from [(sql.qp/->honeysql driver (hx/identifier (:schema table) (:name table)))]}
+   (query driver database (merge {:from [(sql.qp/->honeysql driver (hx/identifier :table (:schema table) (:name table)))]}
                                  honeysql-form))))
 
 

src/metabase/mbql/util.clj

@@ -340,29 +340,6 @@
     (class x)))
 
 
-(s/defn fk-clause->join-info :- (s/maybe mbql.s/JoinInfo)
-  "Return the matching info about the JOINed for the 'destination' Field in an `fk->` clause, for the current level of
-  nesting (`0` meaning this `fk->` clause was found in the top-level query; `1` meaning it was found in the first
-  `source-query`, and so forth.)
-
-     (fk-clause->join-info query [:fk-> [:field-id 1] [:field-id 2]] 0)
-     ;; -> \"orders__via__order_id\""
-  [query :- mbql.s/Query, nested-query-level :- su/NonNegativeInt, [_ source-field-clause :as fk-clause] :- mbql.s/fk->]
-  ;; if we're dealing with something that's not at the top-level go ahead and recurse a level until we get to the
-  ;; query we want to work with
-  (if (pos? nested-query-level)
-    (recur {:query (or (get-in query [:query :source-query])
-                       (throw (Exception. (str (tru "Bad nested-query-level: query does not have a source query")))))}
-           (dec nested-query-level)
-           fk-clause)
-    ;; ok, when we've reached the right level of nesting, look in `:join-tables` to find the appropriate info
-    (let [source-field-id (field-clause->id-or-literal source-field-clause)]
-      (some (fn [{:keys [fk-field-id], :as info}]
-              (when (= fk-field-id source-field-id)
-                info))
-            (-> query :query :join-tables)))))
-
-
 (s/defn expression-with-name :- mbql.s/FieldOrExpressionDef
   "Return the `Expression` referenced by a given `expression-name`."
   [query :- mbql.s/Query, expression-name :- su/NonBlankString]

src/metabase/models/query/permissions.clj

@@ -115,9 +115,9 @@
     (catch Throwable e
       (when throw-exceptions?
         (throw e))
-      (log/warn (tru "Error calculating permissions for query: {0}" (.getMessage e))
-                "\n"
-                (u/pprint-to-str (u/filtered-stacktrace e)))
+      (log/error (tru "Error calculating permissions for query: {0}" (.getMessage e))
+                 "\n"
+                 (u/pprint-to-str (u/filtered-stacktrace e)))
       #{"/db/0/"})))                    ; DB 0 will never exist
 
 (s/defn ^:private perms-set* :- #{perms/ObjectPath}

src/metabase/query_processor/middleware/parameters.clj

@@ -38,7 +38,8 @@
           (assoc-in [:query :source-query :native] (unprepare/unprepare driver/*driver* (cons new-query new-params)))))))
 
 (defn- expand-parameters
-  "Expand parameters in the OUTER-QUERY, and if the query is using a native source query, expand params in that as well."
+  "Expand parameters in the `outer-query`, and if the query is using a native source query, expand params in that as
+  well."
   [outer-query]
   (cond-> (expand-parameters* outer-query)
     (get-in outer-query [:query :source-query :native]) expand-params-in-native-source-query))

src/metabase/query_processor/middleware/parameters/sql.clj

@@ -137,7 +137,7 @@
 ;;                                    :value  "\2015-01-01~2016-09-01"\}}}
 
 (s/defn ^:private param-with-target
-  "Return the param in PARAMS with a matching TARGET. TARGET is something like:
+  "Return the param in `params` with a matching `target`. `target` is something like:
 
      [:dimension [:template-tag <param-name>]] ; for Dimensions (Field Filters)
      [:variable  [:template-tag <param-name>]] ; for other types of params"
@@ -274,7 +274,7 @@
     value))
 
 (s/defn ^:private value-for-tag :- ParamValue
-  "Given a map TAG (a value in the `:template-tags` dictionary) return the corresponding value from the PARAMS
+  "Given a map TAG (a value in the `:template-tags` dictionary) return the corresponding value from the `params`
    sequence. The VALUE is something that can be compiled to SQL via `->replacement-snippet-info`."
   [tag :- TagParam, params :- (s/maybe [DimensionValue])]
   (parse-value-for-type (:type tag) (or (param-value-for-tag tag params)
@@ -356,25 +356,26 @@
     :else                                  (dimension-value->equals-clause-sql value)))
 
 (s/defn ^:private honeysql->replacement-snippet-info :- ParamSnippetInfo
-  "Convert X to a replacement snippet info map by passing it to HoneySQL's `format` function."
+  "Convert `x` to a replacement snippet info map by passing it to HoneySQL's `format` function."
   [x]
   (let [[snippet & args] (hsql/format x, :quoting (sql.qp/quote-style driver/*driver*), :allow-dashed-names? true)]
     {:replacement-snippet     snippet
      :prepared-statement-args args}))
 
 (s/defn ^:private field->identifier :- su/NonBlankString
-  "Return an approprate snippet to represent this FIELD in SQL given its param type.
+  "Return an approprate snippet to represent this `field` in SQL given its param type.
    For non-date Fields, this is just a quoted identifier; for dates, the SQL includes appropriately bucketing based on
-   the PARAM-TYPE."
+   the `param-type`."
   [field param-type]
-  (-> (honeysql->replacement-snippet-info (let [identifier (sql.qp/field->identifier driver/*driver* field)]
-                                            (if (date-params/date-type? param-type)
-                                              (sql.qp/date driver/*driver* :day identifier)
-                                              identifier)))
-      :replacement-snippet))
+  (:replacement-snippet
+   (honeysql->replacement-snippet-info
+    (let [identifier (sql.qp/->honeysql driver/*driver* (sql.qp/field->identifier driver/*driver* field))]
+      (if (date-params/date-type? param-type)
+        (sql.qp/date driver/*driver* :day identifier)
+        identifier)))))
 
 (s/defn ^:private combine-replacement-snippet-maps :- ParamSnippetInfo
-  "Combine multiple REPLACEMENT-SNIPPET-MAPS into a single map using a SQL `AND` clause."
+  "Combine multiple `replacement-snippet-maps` into a single map using a SQL `AND` clause."
   [replacement-snippet-maps :- [ParamSnippetInfo]]
   {:replacement-snippet     (str \( (str/join " AND " (map :replacement-snippet replacement-snippet-maps)) \))
    :prepared-statement-args (reduce concat (map :prepared-statement-args replacement-snippet-maps))})
@@ -575,10 +576,10 @@
   [{:keys [driver database] :as query}]
   (or driver
       (driver.u/database->driver database)
-      (throw (IllegalArgumentException. "Could not resolve driver"))))
+      (throw (IllegalArgumentException. (str (tru "Could not resolve driver."))))))
 
 (defn expand
-  "Expand parameters inside a *SQL* QUERY."
+  "Expand parameters inside a *SQL* `query`."
   [query]
   (binding [driver/*driver* (ensure-driver query)]
     (if (driver/supports? driver/*driver* :native-parameters)

src/metabase/query_processor/store.clj

@@ -28,6 +28,11 @@
   "Dynamic var used as the QP store for a given query execution."
   (delay (throw (Exception. (str (tru "Error: Query Processor store is not initialized."))))))
 
+(defn initialized?
+  "Is the QP store currently initialized?"
+  []
+  (not (delay? *store*)))
+
 (defn do-with-new-store
   "Execute `f` with a freshly-bound `*store*`."
   [f]
@@ -41,36 +46,6 @@
   [& body]
   `(do-with-new-store (fn [] ~@body)))
 
-(defn do-with-pushed-store
-  "Execute bind a *copy* of the current store and execute `f`."
-  [f]
-  (binding [*store* (atom @*store*)]
-    (f)))
-
-(defmacro with-pushed-store
-  "Bind a temporary copy of the current store (presumably so you can make temporary changes) for the duration of `body`.
-  All changes to this 'pushed' copy will be discarded after the duration of `body`.
-
-  This is used to make it easily to write downstream clause-handling functions in driver QP implementations without
-  needing to code them in a way where they are explicitly aware of the context in which they are called. For example,
-  we use this to temporarily give Tables a different `:name` in the SQL QP when we need to use an alias for them in
-  `fk->` forms.
-
-  Pushing stores is cumulative: nesting a `with-pushed-store` form inside another will make a copy of the copy.
-
-    (with-pushed-store
-      ;; store is now a temporary copy of original
-      (store-table! (assoc (table table-id) :name \"Temporary New Name\"))
-      (with-pushed-store
-        ;; store is now a temporary copy of the copy
-        (:name (table table-id)) ; -> \"Temporary New Name\"
-        ...)
-    ...)
-    (:name (table table-id)) ; -> \"Original Name\""
-  {:style/indent 0}
-  [& body]
-  `(do-with-pushed-store (fn [] ~@body)))
-
 (def database-columns-to-fetch
   "Columns you should fetch for the Database referenced by the query before stashing in the store."
   [:id

src/metabase/util.clj

@@ -691,3 +691,17 @@
   {:style/indent 0}
   [& body]
   `(do-with-us-locale (fn [] ~@body)))
+
+(defn xor
+  "Exclusive or. Hopefully this is self-explanatory ;)"
+  [x y & more]
+  (loop [[x y & more] (into [x y] more)]
+    (cond
+      (and x y)
+      false
+
+      (seq more)
+      (recur (cons (or x y) more))
+
+      :else
+      (or x y))))

src/metabase/util/honeysql_extensions.clj

@@ -7,7 +7,8 @@
             [metabase
              [config :as config]
              [util :as u]]
-            [metabase.util.pretty :refer [PrettyPrintable]])
+            [metabase.util.pretty :refer [PrettyPrintable]]
+            [schema.core :as s])
   (:import honeysql.format.ToSql
            java.util.Locale))
 
@@ -54,34 +55,57 @@
   clojure.lang.Ratio
   (to-sql [x] (hformat/to-sql (double x))))
 
-(defrecord Identifier [components]
+(def IdentifierType
+  "Schema for valid Identifier types."
+  (s/enum
+   :database
+   :schema
+   :constraint
+   :index
+   ;; Suppose we have a query like:
+   ;; SELECT my_field f FROM my_table t
+   ;; then:
+   :table          ; is `my_table`
+   :table-alias    ; is `t`
+   :field          ; is `my_field`
+   :field-alias))  ; is `f`
+
+(defrecord Identifier [identifier-type components]
   :load-ns true
   ToSql
   (to-sql [_]
     (binding [hformat/*allow-dashed-names?* true]
       (str/join
        \.
-       (for [component components
-             :when     (some? component)]
+       (for [component components]
          (hformat/quote-identifier component, :split false)))))
   PrettyPrintable
   (pretty [_]
-    (cons 'identifier components)))
-
-(defn identifier
-  "Define an identifer with `components`. Prefer this to using keywords for identifiers, as those do not properly handle
-  identifiers with slashes in them."
-  [& components]
-  (Identifier. (for [component components
-                     component (if (instance? Identifier component)
-                                 (:components component)
-                                 [component])]
-                 component)))
+    (cons 'identifier (cons identifier-type components))))
+
 ;; don't use `->Identifier` or `map->Identifier`. Use the `identifier` function instead, which cleans up its input
 (when-not config/is-prod?
   (alter-meta! #'->Identifier    assoc :private true)
   (alter-meta! #'map->Identifier assoc :private true))
 
+(s/defn identifier :- Identifier
+  "Define an identifer of type with `components`. Prefer this to using keywords for identifiers, as those do not
+  properly handle identifiers with slashes in them.
+
+  `identifier-type` represents the type of identifier in question, which is important context for some drivers, such
+  as BigQuery (which needs to qualify Tables identifiers with their dataset name.)
+
+  This function automatically unnests any Identifiers passed as arguments, removes nils, and converts all args to
+  strings."
+  [identifier-type :- IdentifierType, & components]
+  (Identifier.
+   identifier-type
+   (for [component components
+         component (if (instance? Identifier component)
+                     (:components component)
+                     [component])
+         :when     (some? component)]
+     (u/keyword->qualified-name component))))
 
 ;; Single-quoted string literal
 (defrecord Literal [literal]

test/expectation_options.clj

@@ -73,10 +73,10 @@
 
 ;;; ------------------------------------------------ enforce-timeout -------------------------------------------------
 
-(def ^:private test-timeout-ms (* 60 1000))
+(def ^:private test-timeout-ms (* 5 60 1000))
 
 (defn- enforce-timeout
-  "If any test takes longer that 60 seconds to run print a message and stop running tests. (This usually happens when
+  "If any test takes longer that 5 minutes to run print a message and stop running tests. (This usually happens when
   something is fundamentally broken, and we don't want to continue running thousands of tests that can hang for a
   minute each.)"
   [run]