Invalidate cached pooled connection when DB details change (#17307)

Adding new cache to `metabase.driver.sql-jdbc.connection` that tracks the DB details hash values per DB id

Checking if the non-empty hash value of DB details changed in `db->pooled-connection-spec` in order to invalidate an existing pool

Adding test that redefines the log fn to assert that the pool is invalidated on hash change

src/metabase/driver/sql_jdbc/connection.clj

@@ -102,11 +102,25 @@
   database-id->connection-pool
   (atom {}))
 
+(defonce ^:private ^{:doc "A map of DB details hash values, keyed by Database `:id`."}
+  database-id->db-details-hashes
+  (atom {}))
+
+(defn- db-details-hash
+  "Computes a hash value for the given `database`'s `:details` map, for the purpose of determining if details changed
+  and therefore the existing connection pool needs to be invalidated."
+  [database]
+  {:pre [(or nil? (instance? (type Database) database))]}
+  (if (some? database)
+    (hash (:details database))
+    nil))
+
 (defn- set-pool!
-  "Atomically update the current connection pool for Database with `database-id`. Use this function instead of modifying
-  `database-id->connection-pool` directly because it properly closes down old pools in a thread-safe way, ensuring no
-  more than one pool is ever open for a single database."
-  [database-id pool-spec-or-nil]
+  "Atomically update the current connection pool for Database `database` with `database-id`. Use this function instead
+  of modifying database-id->connection-pool` directly because it properly closes down old pools in a thread-safe way,
+  ensuring no more than one pool is ever open for a single database. Also modifies the database-id->db-details-hashes
+  map with the hash value of the given DB's details map."
+  [database-id pool-spec-or-nil database]
   {:pre [(integer? database-id)]}
   (let [[old-id->pool] (if pool-spec-or-nil
                          (swap-vals! database-id->connection-pool assoc database-id pool-spec-or-nil)
@@ -115,12 +129,14 @@
     (when-let [old-pool-spec (get old-id->pool database-id)]
       (when-not (identical? old-pool-spec pool-spec-or-nil)
         (destroy-pool! database-id old-pool-spec))))
+  ;; update the db details hash cache with the new hash value
+  (swap! database-id->db-details-hashes assoc database-id (db-details-hash database))
   nil)
 
 (defn invalidate-pool-for-db!
   "Invalidates the connection pool for the given database by closing it and removing it from the cache."
   [database]
-  (set-pool! (u/the-id database) nil))
+  (set-pool! (u/the-id database) nil nil))
 
 (defn notify-database-updated
   "Default implementation of `driver/notify-database-updated` for JDBC SQL drivers. We are being informed that a
@@ -130,8 +146,14 @@
   (invalidate-pool-for-db! database))
 
 (defn- log-ssh-tunnel-reconnect-msg! [db-id]
-    (log/warn (u/format-color 'red (trs "ssh tunnel for database {0} looks closed; marking pool invalid to reopen it"
-                                        db-id))))
+  (log/warn (u/format-color 'red (trs "ssh tunnel for database {0} looks closed; marking pool invalid to reopen it"
+                                      db-id)))
+  nil)
+
+(defn- log-db-details-hash-change-msg! [db-id]
+  (log/warn (u/format-color 'yellow (trs "Hash of database {0} details changed; marking pool invalid to reopen it"
+                                          db-id)))
+  nil)
 
 (defn db->pooled-connection-spec
   "Return a JDBC connection spec that includes a cp30 `ComboPooledDataSource`. These connection pools are cached so we
@@ -141,19 +163,34 @@
     ;; db-or-id-or-spec is a Database instance or an integer ID
     (u/id db-or-id-or-spec)
     (let [database-id (u/the-id db-or-id-or-spec)
-          get-fn      (fn [db-id log-tunnel-check]
+          ;; 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})))
+          get-fn      (fn [db-id log-invalidation?]
                         (when-let [details (get @database-id->connection-pool db-id)]
-                          (cond (nil? (:tunnel-session details))
-                                ;; no tunnel in use; valid
-                                details
-                                (ssh/ssh-tunnel-open? details)
-                                ;; tunnel in use, and open; valid
-                                details
-                                :default
-                                ;; tunnel in use, and not open; invalid
-                                (do (when log-tunnel-check
-                                      (log-ssh-tunnel-reconnect-msg! db-id))
-                                    nil))))]
+                          (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)))
+                            (if log-invalidation?
+                              (log-db-details-hash-change-msg! db-id)
+                              nil)
+
+                            (nil? (:tunnel-session details)) ; no tunnel in use; valid
+                            details
+
+                            (ssh/ssh-tunnel-open? details) ; tunnel in use, and open; valid
+                            details
+
+                            :else ; tunnel in use, and not open; invalid
+                            (if log-invalidation?
+                              (log-ssh-tunnel-reconnect-msg! db-id)
+                              nil))))]
       (or
        ;; we have an existing pool for this database, so use it
        (get-fn database-id true)
@@ -166,13 +203,8 @@
           ;; check if another thread created the pool while we were waiting to acquire the lock
           (get-fn database-id false)
           ;; create a new pool and add it to our cache, then return it
-          (let [db (or (db/select-one [Database :id :engine :details] :id database-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})))]
-            (u/prog1 (create-pool! db)
-              (set-pool! database-id <>)))))))
+          (u/prog1 (create-pool! db)
+            (set-pool! database-id <> db))))))
 
     ;; already a `clojure.java.jdbc` spec map
     (map? db-or-id-or-spec)

test/metabase/driver/postgres_test.clj

@@ -439,7 +439,7 @@
   (mt/with-temp Database [database {:engine :postgres, :details (enums-test-db-details)}]
     (sync-metadata/sync-db-metadata! database)
     (f database)
-    (#'sql-jdbc.conn/set-pool! (u/id database) nil)))
+    (#'sql-jdbc.conn/set-pool! (u/id database) nil nil)))
 
 (deftest enums-test
   (mt/test-driver :postgres

test/metabase/driver/sql_jdbc/connection_test.clj

@@ -9,8 +9,11 @@
             [metabase.models.database :refer [Database]]
             [metabase.test :as mt]
             [metabase.test.data :as data]
+            [metabase.test.fixtures :as fixtures]
             [metabase.util :as u]))
 
+(use-fixtures :once (fixtures/initialize :db))
+
 (deftest can-connect-with-details?-test
   (is (= true
          (driver.u/can-connect-with-details? :h2 (:details (data/db)))))
@@ -53,7 +56,7 @@
                   (is (contains? @@#'sql-jdbc.conn/database-id->connection-pool
                                  (u/id database)))))
               (testing "and is no longer in our connection map after cleanup"
-                (#'sql-jdbc.conn/set-pool! (u/id database) nil)
+                (#'sql-jdbc.conn/set-pool! (u/id database) nil nil)
                 (is (not (contains? @@#'sql-jdbc.conn/database-id->connection-pool
                                     (u/id database)))))
               (testing "the pool has been destroyed"
@@ -67,3 +70,44 @@
             expected (format "db-%d-%s-%s" (u/the-id db) (name driver/*driver*) (-> db :details :db))]
         (is (= expected
                (get props "dataSourceName")))))))
+
+(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")
+
+                                                         (= "127.0.0.1" (:host details))
+                                                         (assoc :host "localhost")
+
+                                                         :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))))))))))
+