diff --git a/src/metabase/driver/sql_jdbc/connection.clj b/src/metabase/driver/sql_jdbc/connection.clj
index eb9878e2915051af74c9eadcfc852848fb4f33fb..969726e864c9c05fac0c170666e8a60cddc8f3a9 100644
--- a/src/metabase/driver/sql_jdbc/connection.clj
+++ b/src/metabase/driver/sql_jdbc/connection.clj
@@ -172,18 +172,23 @@
(let [database-id (u/the-id db-or-id-or-spec)
;; we need the Database instance no matter what (in order to compare details hash with cached value)
db (or (and (instance? (type Database) db-or-id-or-spec) db-or-id-or-spec) ; passed in
- (db/select-one [Database :id :engine :details] :id database-id) ; look up by ID
- (throw (ex-info (tru "Database {0} does not exist." database-id)
- {:status-code 404
- :type qp.error-type/invalid-query
- :database-id database-id})))
+ (db/select-one [Database :id :engine :details] :id database-id) ; look up by ID
+ (throw (ex-info (tru "Database {0} does not exist." database-id)
+ {:status-code 404
+ :type qp.error-type/invalid-query
+ :database-id database-id})))
get-fn (fn [db-id log-invalidation?]
(when-let [details (get @database-id->connection-pool db-id)]
(cond
;; details hash changed from what is cached; invalid
(let [curr-hash (get @database-id->db-details-hashes db-id)
new-hash (db-details-hash db)]
- (and (some? curr-hash) (not= curr-hash new-hash)))
+ (when (and (some? curr-hash) (not= curr-hash new-hash))
+ ;; the hash didn't match, but it's possible that a stale instance of `DatabaseInstance`
+ ;; was passed in (ex: from a long-running sync operation); fetch the latest one from
+ ;; our app DB, and see if it STILL doesn't match
+ (not= curr-hash (-> (db/select-one [Database :id :engine :details] :id database-id)
+ db-details-hash))))
(if log-invalidation?
(log-db-details-hash-change-msg! db-id)
nil)
diff --git a/test/metabase/driver/sql_jdbc/connection_test.clj b/test/metabase/driver/sql_jdbc/connection_test.clj
index 4617cefc3ba5fc8e269bfc6fa5518e9541fd1872..e6f76e71b6fc2a79e6d026780461a9ede983ca4e 100644
--- a/test/metabase/driver/sql_jdbc/connection_test.clj
+++ b/test/metabase/driver/sql_jdbc/connection_test.clj
@@ -7,10 +7,12 @@
[metabase.driver.sql-jdbc.test-util :as sql-jdbc.tu]
[metabase.driver.util :as driver.u]
[metabase.models.database :refer [Database]]
+ [metabase.sync :as sync]
[metabase.test :as mt]
[metabase.test.data :as data]
[metabase.test.fixtures :as fixtures]
- [metabase.util :as u]))
+ [metabase.util :as u]
+ [toucan.db :as db]))
(use-fixtures :once (fixtures/initialize :db))
@@ -76,39 +78,65 @@
(deftest connection-pool-invalidated-on-details-change
(mt/test-drivers (sql-jdbc.tu/sql-jdbc-drivers)
(testing "db->pooled-connection-spec marks a connection pool invalid if the db details map changes"
- (let [db (mt/db)
- hash-change-called (atom false)
- hash-change-fn (fn [db-id]
- (is (= (u/the-id db) db-id))
- (reset! hash-change-called true)
- nil)
- perturb-db-details (fn [db]
- (update db :details (fn [details]
- (cond-> details
- ;; swap localhost and 127.0.0.1
- (= "localhost" (:host details))
- (assoc :host "127.0.0.1")
+ (let [db (mt/db)
+ hash-change-called-times (atom 0)
+ hash-change-fn (fn [db-id]
+ (is (= (u/the-id db) db-id))
+ (swap! hash-change-called-times inc)
+ nil)
+ perturb-db-details (fn [db]
+ (update db
+ :details
+ (fn [details]
+ (case driver/*driver*
+ :redshift
+ (assoc details :additional-options "defaultRowFetchSize=1000")
- (= "127.0.0.1" (:host details))
- (assoc :host "localhost")
+ (cond-> details
+ ;; swap localhost and 127.0.0.1
+ (= "localhost" (:host details))
+ (assoc :host "127.0.0.1")
- :else
- (assoc :new-config "something")))))]
- (sql-jdbc.conn/invalidate-pool-for-db! db)
- ;; a little bit hacky to redefine the log fn, but it's the most direct way to test
- (with-redefs [sql-jdbc.conn/log-db-details-hash-change-msg! hash-change-fn]
- (let [pool-spec-1 (sql-jdbc.conn/db->pooled-connection-spec db)
- db-hash-1 (get @@#'sql-jdbc.conn/database-id->db-details-hashes (u/the-id db))]
- (testing "hash value calculated correctly for new pooled conn"
- (is (some? pool-spec-1))
- (is (integer? db-hash-1))
- (is (not= db-hash-1 0)))
- (testing "changing DB details results in hash value changing and connection being invalidated"
- (let [db-perturbed (perturb-db-details db)
- pool-spec-2 (sql-jdbc.conn/db->pooled-connection-spec db-perturbed)
- db-hash-2 (get @@#'sql-jdbc.conn/database-id->db-details-hashes (u/the-id db))]
- (is (some? pool-spec-2))
- (is (true? @hash-change-called))
- (is (integer? db-hash-2))
- (is (not= db-hash-2 0))
- (is (not= db-hash-1 db-hash-2))))))))))
+ (= "127.0.0.1" (:host details))
+ (assoc :host "localhost")
+
+ :else
+ (assoc :new-config "something"))))))]
+ (try
+ (sql-jdbc.conn/invalidate-pool-for-db! db)
+ ;; a little bit hacky to redefine the log fn, but it's the most direct way to test
+ (with-redefs [sql-jdbc.conn/log-db-details-hash-change-msg! hash-change-fn]
+ (let [pool-spec-1 (sql-jdbc.conn/db->pooled-connection-spec db)
+ db-hash-1 (get @@#'sql-jdbc.conn/database-id->db-details-hashes (u/the-id db))]
+ (testing "hash value calculated correctly for new pooled conn"
+ (is (some? pool-spec-1))
+ (is (integer? db-hash-1))
+ (is (not= db-hash-1 0)))
+ (testing "changing DB details results in hash value changing and connection being invalidated"
+ (let [db-perturbed (perturb-db-details db)]
+ (db/update! Database (mt/id) :details (:details db-perturbed))
+ (let [;; this call should result in the connection pool becoming invalidated, and the new hash value
+ ;; being stored based upon these updated details
+ pool-spec-2 (sql-jdbc.conn/db->pooled-connection-spec db-perturbed)
+ db-hash-2 (get @@#'sql-jdbc.conn/database-id->db-details-hashes (u/the-id db))]
+ ;; to throw a wrench into things, kick off a sync of the original db (unperturbed); this
+ ;; simulates a long running sync that began before the perturbed details were saved to the app DB
+ ;; the sync steps SHOULD NOT invalidate the connection pool, because doing so could cause a seesaw
+ ;; effect that continuously invalidates the connection pool on every sync step and query, which
+ ;; wreaks havoc (#18499)
+ ;; instead, the connection pool code will simply fetch the newest DatabaseInstance it
+ ;; can find in the app DB, in the case of a hash mismatch, and check AGAIN to see whether the hash
+ ;; still doesn't match (in this test case, it should actually match this time, because we updated
+ ;; the app DB with the perturbed DatabaseInstance above here)
+ ;; this should still see a hash mismatch in the case that the DB details were updated external to
+ ;; this process (i.e. by a different instance), since our in-memory hash value still wouldn't match
+ ;; even after getting the latest `DatabaseInstance`
+ (sync/sync-database! db {:scan :schema})
+ (is (some? pool-spec-2))
+ (is (= 1 @hash-change-called-times))
+ (is (integer? db-hash-2))
+ (is (not= db-hash-2 0))
+ (is (not= db-hash-1 db-hash-2)))))))
+ (finally
+ ;; restore the original test DB details, no matter what just happened
+ (db/update! Database (mt/id) :details (:details db))))))))