Merge branch 'release-0.32.x' into master

frontend/src/metabase/visualizations/components/Visualization.jsx

@@ -425,7 +425,7 @@ export default class Visualization extends Component {
     }
 
     return (
-      <div className={cx(className, "flex flex-column")}>
+      <div className={cx(className, "flex flex-column full-height")}>
         {(showTitle &&
           (settings["card.title"] || extra) &&
           (loading ||

frontend/src/metabase/visualizations/visualizations/Text.css

@@ -7,6 +7,7 @@
   flex-direction: column;
   justify-content: center;
   padding: 1em 0 1em 1.5em;
+  height: 100%;
 }
 :local .Text.dashboard-is-editing {
   padding: 2.8em 1.3em var(--text-card-padding) 1.3em;
@@ -26,6 +27,7 @@
   box-shadow: 0 1px 7px var(--color-shadow);
 }
 :local .Text .text-card-markdown {
+  height: 100%;
   overflow: auto;
   pointer-events: all;
 }

modules/drivers/googleanalytics/src/metabase/driver/googleanalytics/query_processor.clj

@@ -177,24 +177,27 @@
 
 ;;; ----------------------------------------------- filter (intervals) -----------------------------------------------
 
-
-(defn- date-sub-day [[clause time-component unit :as value]]
-  (case clause
-    :absolute-datetime [:absolute-datetime (du/relative-date :day -1 (du/date-trunc unit time-component)) :day]
-    :relative-datetime (if (= unit :day)
-                         [clause (- time-component 1) unit]
-                         [:absolute-datetime
-                          (du/relative-date :day -1 (du/date-trunc unit (du/relative-date unit time-component))) :day])
-    value))
-
-(defn- date-add-day [[clause time-component unit :as value]]
-  (case clause
-    :absolute-datetime [:absolute-datetime (du/relative-date :day 1 (du/date-trunc unit time-component)) unit]
-    :relative-datetime (if (= unit :day)
-                         [clause (+ time-component 1) unit]
-                         [:absolute-datetime
-                          (du/relative-date :day 1 (du/date-trunc unit (du/relative-date unit time-component))) :day])
-    value))
+(defn- date-add-days
+  "Add `n-days` to a datetime clause (`:absolute-datetime` or `:relative-datetime`) Done to fix off-by-one issues with
+  GA. See #9904"
+  [[clause-name time-component unit, :as datetime-clause] n-days]
+  (case clause-name
+    :absolute-datetime
+    [:absolute-datetime (du/relative-date :day n-days (du/date-trunc unit time-component)) unit]
+
+    :relative-datetime
+    (if (= unit :day)
+      [clause-name (+ time-component n-days) unit]
+      [:absolute-datetime
+       (du/relative-date :day n-days (du/date-trunc unit (du/relative-date unit time-component))) :day])
+
+    datetime-clause))
+
+(defn- date-sub-day [datetime-clause]
+  (date-add-days datetime-clause -1))
+
+(defn- date-add-day [datetime-clause]
+  (date-add-days datetime-clause 1))
 
 
 (defmulti ^:private parse-filter:interval mbql.u/dispatch-by-clause-name-or-class)

modules/drivers/redshift/project.clj

@@ -6,7 +6,7 @@
 
 
   :dependencies
-  [[com.amazon.redshift/redshift-jdbc42-no-awssdk "1.2.18.1036"]]
+  [[com.amazon.redshift/redshift-jdbc42-no-awssdk "1.2.27.1051"]]
 
   :profiles
   {:provided

resources/log4j.properties

-log4j.rootLogger=WARN, console, metabase
+log4j.rootLogger=WARN, console
 
 # log to the console
 log4j.appender.console=org.apache.log4j.ConsoleAppender

src/metabase/driver/sql_jdbc/execute.clj

-
 (ns metabase.driver.sql-jdbc.execute
   "Code related to actually running a SQL query against a JDBC database (including setting the session timezone when
   appropriate), and for properly encoding/decoding types going in and out of the database."
@@ -164,7 +163,8 @@
   `(do-with-ensured-connection ~db (fn [~conn-binding] ~@body)))
 
 (defn- cancelable-run-query
-  "Runs `sql` in such a way that it can be interrupted via a `future-cancel`"
+  "Runs JDBC query, canceling it if an InterruptedException is caught (e.g. if there query is canceled before
+  finishing)."
   [db sql params opts]
   (with-ensured-connection [conn db]
     ;; This is normally done for us by java.jdbc as a result of our `jdbc/query` call
@@ -173,19 +173,17 @@
       ;; (Not all drivers support this so ignore Exceptions if they don't)
       (u/ignore-exceptions
         (.closeOnCompletion stmt))
-      ;; Need to run the query in another thread so that this thread can cancel it if need be
       (try
-        (let [query-future (future (jdbc/query conn (into [stmt] params) opts))]
-          ;; This thread is interruptable because it's awaiting the other thread (the one actually running the
-          ;; query). Interrupting this thread means that the client has disconnected (or we're shutting down) and so
-          ;; we can give up on the query running in the future
-          @query-future)
+        (jdbc/query conn (into [stmt] params) opts)
         (catch InterruptedException e
           (log/warn (tru "Client closed connection, canceling query"))
           ;; This is what does the real work of canceling the query. We aren't checking the result of
           ;; `query-future` but this will cause an exception to be thrown, saying the query has been cancelled.
           (.cancel stmt)
-          (throw e))))))
+          (throw e))
+        (catch Exception e
+          (u/ignore-exceptions (.cancel stmt))
+          e)))))
 
 (defn- run-query
   "Run the query itself."
@@ -205,10 +203,12 @@
 ;;; -------------------------- Running queries: exception handling & disabling auto-commit ---------------------------
 
 (defn- exception->nice-error-message ^String [^SQLException e]
-  (or (->> (.getMessage e)     ; error message comes back like 'Column "ZID" not found; SQL statement: ... [error-code]' sometimes
-           (re-find #"^(.*);") ; the user already knows the SQL, and error code is meaningless
-           second)             ; so just return the part of the exception that is relevant
-      (.getMessage e)))
+  ;; error message comes back like 'Column "ZID" not found; SQL statement: ... [error-code]' sometimes
+  ;; the user already knows the SQL, and error code is meaningless
+  ;; so just return the part of the exception that is relevant
+  (->> (.getMessage e)
+       (re-find #"^(.*);")
+       second))
 
 (defn do-with-try-catch
   "Tries to run the function `f`, catching and printing exception chains if SQLException is thrown,
@@ -219,7 +219,10 @@
     (f)
     (catch SQLException e
       (log/error (jdbc/print-sql-exception-chain e))
-      (throw (Exception. (exception->nice-error-message e) e)))))
+      (throw
+       (if-let [nice-error-message (exception->nice-error-message e)]
+         (Exception. nice-error-message e)
+         e)))))
 
 (defn- do-with-auto-commit-disabled
   "Disable auto-commit for this transaction, and make the transaction `rollback-only`, which means when the

src/metabase/sync/sync_metadata/fields/sync_metadata.clj

@@ -27,6 +27,7 @@
         {new-database-type :database-type
          new-base-type     :base-type
          new-field-comment :field-comment} field-metadata
+        new-database-type                  (or new-database-type "NULL")
         new-special-type                   (common/special-type field-metadata)
 
         new-db-type?

test/metabase/driver/sql_jdbc/execute_test.clj

 (ns metabase.driver.sql-jdbc.execute-test
-  (:require [clojure.java.jdbc :as jdbc]
+  (:require [clojure.core.async :as a]
+            [clojure.java.jdbc :as jdbc]
+            [clojure.string :as str]
+            [expectations :refer [expect]]
+            [metabase
+             [query-processor :as qp]
+             [util :as u]]
             [metabase.driver.sql-jdbc-test :as sql-jdbc-test]
-            [metabase.query-processor :as qp]
-            [metabase.test.data :as data]
-            [metabase.test.data.datasets :as datasets])
-  (:import java.sql.PreparedStatement))
+            [metabase.test
+             [data :as data]
+             [util :as tu]]
+            [metabase.test.data.datasets :as datasets]
+            [metabase.test.util.log :as tu.log])
+  (:import [java.sql PreparedStatement ResultSet]))
 
 (defn- do-with-max-rows [f]
   ;; force loading of the test data before swapping out `jdbc/query`, otherwise things might not sync correctly
@@ -59,3 +67,121 @@
                                        :type     :query
                                        :query    {:source-table (data/id :venues)}})
        :constraints {:max-results 15}})))
+
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; |                                   Query Cancelation & Resource Closing Tests                                   |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+(defrecord ^:private FakePreparedStatement [called-cancel? called-close? fake-resultset]
+  java.sql.PreparedStatement
+  (closeOnCompletion [_])               ; no-op
+  (cancel [_] (some-> called-cancel? (deliver true)))
+  (close [_] (some-> called-close? (deliver true)))
+  (executeQuery [_] fake-resultset))
+
+(defrecord ^:private FakeResultSet [called-close?]
+  ResultSet
+  (close [_] (some-> called-close? (deliver true)))
+  (next [_] false)
+  (getMetaData [_]
+    (reify java.sql.ResultSetMetaData
+      (getColumnCount [_] 0))))
+
+(defn- fake-prepare-statement
+  "Returns `fake-value` whenenver the `sql` parameter returns a truthy value when passed to `use-fake-value?`."
+  [& {:keys [use-fake-value? faked-value]
+      :or   {use-fake-value? (constantly false)}}]
+  (let [original-prepare-statement jdbc/prepare-statement]
+    (fn [connection sql options]
+      (if (use-fake-value? sql)
+        faked-value
+        (original-prepare-statement connection sql options)))))
+
+(defn- fake-query
+  "Function to replace the `clojure.java.jdbc/query` function. Will invoke `on-fake-prepared-statement` when passed an
+  instance of `FakePreparedStatement`."
+  [& {:keys [on-fake-prepared-statement]}]
+  (let [original-query jdbc/query]
+    (fn
+      ([conn stmt+params]
+       (conn stmt+params))
+
+      ([conn stmt+params opts]
+       (if (instance? FakePreparedStatement (first stmt+params))
+         (when on-fake-prepared-statement (on-fake-prepared-statement))
+         (original-query conn stmt+params opts))))))
+
+;; make sure queries properly clean up after themselves and close result sets and prepared statements when finished
+(expect
+  {:prepared-statement-closed? true, :result-set-closed? true}
+  (let [closed-prepared-statement? (promise)
+        closed-result-set?         (promise)
+        fake-resultset             (->FakeResultSet closed-result-set?)
+        fake-prepared-statement    (->FakePreparedStatement nil closed-prepared-statement? fake-resultset)
+        placeholder-query          "SELECT 1;"]
+    (with-redefs [jdbc/prepare-statement (fake-prepare-statement
+                                          :use-fake-value? #(str/includes? % placeholder-query)
+                                          :faked-value fake-prepared-statement)]
+      (let [{{:keys [rows]} :data, :as results} (qp/process-query
+                                                  {:database (data/id)
+                                                   :type     :native
+                                                   :native   {:query placeholder-query}})]
+        (when-not rows
+          (throw (ex-info "Query failed to run!" results)))
+        ;; make sure results are fully realized!
+        (mapv vec rows))
+      {:prepared-statement-closed? (deref closed-prepared-statement? 1000 false)
+       :result-set-closed?         (deref closed-result-set?         1000 false)})))
+
+;; make sure a prepared statement is canceled & closed when the async QP channel is closed
+(expect
+  {:prepared-statement-canceled? true, :prepared-statement-closed? true}
+  (let [started?                     (promise)
+        canceled-prepared-statement? (promise)
+        closed-prepared-statement?   (promise)
+        fake-prepared-statement      (->FakePreparedStatement canceled-prepared-statement? closed-prepared-statement? nil)
+        placeholder-query            "SLEEP PLACEHOLDER"]
+    (with-redefs [jdbc/prepare-statement (fake-prepare-statement
+                                          :use-fake-value? #(str/includes? % placeholder-query)
+                                          :faked-value fake-prepared-statement)
+                  jdbc/query             (fake-query
+                                          :on-fake-prepared-statement
+                                          (fn [& _]
+                                            (deliver started? true)
+                                            (Thread/sleep 2000)
+                                            (->FakeResultSet nil)))]
+      (let [chan (qp/process-query
+                   {:database (data/id)
+                    :type     :native
+                    :native   {:query placeholder-query}
+                    :async?   true})]
+        (u/deref-with-timeout started? 1000)
+        (a/close! chan)
+        {:prepared-statement-canceled? (deref canceled-prepared-statement? 3000 false)
+         :prepared-statement-closed?   (deref closed-prepared-statement?   1000 false)}))))
+
+;; The test below sort of tests the same thing (I think). But it is written in a much more confusing manner. If you
+;; dare, try to work out why happens by reading source `call-with-paused-query`. - Cam
+(expect
+  ::tu/success
+  ;; this might dump messages about the connection being closed; we don't need to worry about that
+  (tu.log/suppress-output
+    (tu/call-with-paused-query
+     (fn [query-thunk called-query? called-cancel? pause-query]
+       (let [ ;; This fake prepared statement is cancelable like a prepared statement, but will allow us to tell the
+             ;; difference between our Prepared statement and the real thing
+             fake-prep-stmt  (->FakePreparedStatement called-cancel? nil nil)]
+         (future
+           (try
+             (with-redefs [jdbc/prepare-statement (fake-prepare-statement
+                                                   :use-fake-value? (fn [sql] (re-find #"checkins" (str/lower-case sql)))
+                                                   :faked-value     fake-prep-stmt)
+                           jdbc/query             (fake-query
+                                                   :on-fake-prepared-statement
+                                                   (fn []
+                                                     (deliver called-query? true)
+                                                     @pause-query))]
+               (query-thunk))
+             (catch Throwable e
+               (throw e)))))))))

test/metabase/query_processor_test/failure_test.clj

@@ -4,7 +4,9 @@
             [medley.core :as m]
             [metabase.query-processor :as qp]
             [metabase.query-processor.interface :as qp.i]
-            [metabase.test.data :as data]))
+            [metabase.test.data :as data]
+            [metabase.util.schema :as su]
+            [schema.core :as s]))
 
 (defn- bad-query []
   {:database (data/id)
@@ -27,12 +29,13 @@
                 "LIMIT 1048576")
    :params nil})
 
-(def ^:private ^{:arglists '([stacktrace])} valid-stacktrace? (every-pred seq (partial every? (every-pred string? seq))))
+(def ^:private ^{:arglists '([stacktrace])} valid-stacktrace?
+  (complement (partial s/check [su/NonBlankString])))
 
 ;; running a bad query via `process-query` should return stacktrace, query, preprocessed query, and native query
 (expect
   {:status       :failed
-   :class        java.util.concurrent.ExecutionException
+   :class        Exception
    :error        true
    :stacktrace   true
    ;; `:database` is removed by the catch-exceptions middleware for historical reasons

test/metabase/query_processor_test/query_cancellation_test.clj

-(ns metabase.query-processor-test.query-cancellation-test
-  "TODO - This is sql-jdbc specific, so it should go in a sql-jdbc test namespace."
-  (:require [clojure.java.jdbc :as jdbc]
-            [clojure.string :as str]
-            [expectations :refer [expect]]
-            [metabase.test.util :as tu]
-            [metabase.test.util.log :as tu.log]))
-
-(defrecord ^:private FakePreparedStatement [called-cancel?]
-  java.sql.PreparedStatement
-  (closeOnCompletion [_]) ; no-op
-  (cancel [_] (deliver called-cancel? true))
-  (close [_] true))
-
-(defn- make-fake-prep-stmt
-  "Returns `fake-value` whenenver the `sql` parameter returns a truthy value when passed to `use-fake-value?`."
-  [orig-make-prep-stmt & {:keys [use-fake-value? faked-value]
-                          :or   {use-fake-value? (constantly false)}}]
-  (fn [connection sql options]
-    (if (use-fake-value? sql)
-      faked-value
-      (orig-make-prep-stmt connection sql options))))
-
-(defn- fake-query
-  "Function to replace the `clojure.java.jdbc/query` function. Will invoke `on-fake-prepared-statement` when passed an
-  instance of `FakePreparedStatement`."
-  {:style/indent 1}
-  [orig-jdbc-query & {:keys [on-fake-prepared-statement]}]
-  (fn
-    ([conn stmt+params]
-     (orig-jdbc-query conn stmt+params))
-
-    ([conn stmt+params opts]
-     (if (instance? FakePreparedStatement (first stmt+params))
-       (when on-fake-prepared-statement (on-fake-prepared-statement))
-       (orig-jdbc-query conn stmt+params opts)))))
-
-(expect
-  ::tu/success
-  ;; this might dump messages about the connection being closed; we don't need to worry about that
-  (tu.log/suppress-output
-    (tu/call-with-paused-query
-     (fn [query-thunk called-query? called-cancel? pause-query]
-       (let [ ;; This fake prepared statement is cancelable like a prepared statement, but will allow us to tell the
-             ;; difference between our Prepared statement and the real thing
-             fake-prep-stmt  (->FakePreparedStatement called-cancel?)
-             ;; Much of the underlying plumbing of MB requires a working jdbc/query and jdbc/prepared-statement (such
-             ;; as queryies for the application database). Let binding the original versions of the functions allows
-             ;; us to delegate to them when it's not the query we're trying to test
-             orig-jdbc-query jdbc/query
-             orig-prep-stmt  jdbc/prepare-statement]
-         (future
-           (try
-             (with-redefs [jdbc/prepare-statement (make-fake-prep-stmt
-                                                   orig-prep-stmt
-                                                   :use-fake-value? (fn [sql] (re-find #"checkins" (str/lower-case sql)))
-                                                   :faked-value     fake-prep-stmt)
-                           jdbc/query             (fake-query orig-jdbc-query
-                                                    :on-fake-prepared-statement
-                                                    (fn []
-                                                      (deliver called-query? true)
-                                                      @pause-query))]
-               (query-thunk))
-             (catch Throwable e
-               (throw e)))))))))

test/metabase/sync/sync_metadata/fields/sync_metadata_test.clj

+(ns metabase.sync.sync-metadata.fields.sync-metadata-test
+  (:require [expectations :refer [expect]]
+            [metabase.models.table :refer [Table]]
+            [metabase.sync.sync-metadata.fields.sync-metadata :as sync-metadata]
+            [toucan.db :as db]
+            [toucan.util.test :as tt]))
+
+(defn- updates-that-will-be-performed [new-metadata-from-sync metadata-in-application-db]
+  (tt/with-temp Table [table]
+    (let [update-operations (atom [])]
+      (with-redefs [db/update! (fn [model id updates]
+                                 (swap! update-operations conj [(name model) id updates]))]
+        (#'sync-metadata/update-field-metadata-if-needed!
+         table
+         new-metadata-from-sync
+         metadata-in-application-db)
+        @update-operations))))
+
+;; test that if database-type changes we will update it in the DB
+(expect
+  [["Field" 1 {:database_type "Integer"}]]
+  (updates-that-will-be-performed
+   {:name          "My Field"
+    :database-type "Integer"
+    :base-type     :type/Integer}
+   {:name          "My Field"
+    :database-type "NULL"
+    :base-type     :type/Integer
+    :id            1}))
+
+;; no changes should be made (i.e., no calls to `update!`) if nothing changes
+(expect
+ []
+ (updates-that-will-be-performed
+  {:name          "My Field"
+   :database-type "Integer"
+   :base-type     :type/Integer}
+  {:name          "My Field"
+   :database-type "Integer"
+   :base-type     :type/Integer
+   :id            1}))
+
+;; test that if `database-type` comes back as `nil` in the metadata from the sync process, we won't try to set a `nil`
+;; value in the DB -- this is against the rules -- we should set `NULL` instead. See `TableMetadataField` schema.
+(expect
+  [["Field" 1 {:database_type "NULL"}]]
+  (updates-that-will-be-performed
+   {:name          "My Field"
+    :database-type nil
+    :base-type     :type/Integer}
+   {:name          "My Field"
+    :database-type "Integer"
+    :base-type     :type/Integer
+    :id            1}))
+
+;; if `database-type` comes back as `nil` and was already saved in application DB as `NULL` no changes should be made
+(expect
+  []
+  (updates-that-will-be-performed
+   {:name          "My Field"
+    :database-type nil
+    :base-type     :type/Integer}
+   {:name          "My Field"
+    :database-type "NULL"
+    :base-type     :type/Integer
+    :id            1}))