Don't log stack trace when opportunistically trying out SSL (#21280)

* Revert "Log database connection error before humanizing it (#10695)"

This reverts commit 426df163.

This is no longer needed. The original exception message is kept
in the inner exception and everywhere this is used, the whole
stack trace is logged anyway.

* Don't log exception when opportunistically trying SSL

src/metabase/api/database.clj

@@ -421,8 +421,9 @@
 (defn test-database-connection
   "Try out the connection details for a database and useful error message if connection fails, returns `nil` if
    connection succeeds."
-  [engine {:keys [host port] :as details}, & {:keys [invalid-response-handler]
-                                              :or   {invalid-response-handler invalid-connection-response}}]
+  [engine {:keys [host port] :as details}, & {:keys [invalid-response-handler log-exception]
+                                              :or   {invalid-response-handler invalid-connection-response
+                                                     log-exception            true}}]
   {:pre [(some? engine)]}
   (let [engine  (keyword engine)
         details (assoc details :engine engine)]
@@ -445,7 +446,8 @@
         :else
         (invalid-response-handler :db (tru "Unable to connect to database.")))
       (catch Throwable e
-        (log/error e (trs "Cannot connect to Database"))
+        (when log-exception
+          (log/error e (trs "Cannot connect to Database")))
         (invalid-response-handler :dbname (.getMessage e))))))
 
 ;; TODO - Just make `:ssl` a `feature`
@@ -465,21 +467,19 @@
   the details used to successfully connect. Otherwise returns a map with the connection error message. (This map will
   also contain the key `:valid` = `false`, which you can use to distinguish an error from valid details.)"
   [engine :- DBEngineString, details :- su/Map]
-  (if (and (supports-ssl? (keyword engine))
-           (true? (:ssl details)))
-    (let [error (test-database-connection engine details)]
-      (or error details))
-    (let [details (if (supports-ssl? (keyword engine))
-                    (assoc details :ssl true)
-                    details)]
-    ;; this loop tries connecting over ssl and non-ssl to establish a connection
-    ;; if it succeeds it returns the `details` that worked, otherwise it returns an error
-      (loop [details details]
-        (let [error (test-database-connection engine details)]
-          (if (and error
-                   (true? (:ssl details)))
-            (recur (assoc details :ssl false))
-            (or error details)))))))
+  (let [;; Try SSL first if SSL is supported and not already enabled
+        ;; If not successful or not applicable, details-with-ssl will be nil
+        details-with-ssl (assoc details :ssl true)
+        details-with-ssl (when (and (supports-ssl? (keyword engine))
+                                    (not (true? (:ssl details)))
+                                    (nil? (test-database-connection engine details-with-ssl :log-exception false)))
+                           details-with-ssl)]
+    (or
+      ;; Opportunistic SSL
+      details-with-ssl
+      ;; Try with original parameters
+      (test-database-connection engine details)
+      details)))
 
 (api/defendpoint POST "/"
   "Add a new `Database`."

src/metabase/driver/util.clj

@@ -49,7 +49,6 @@
       ;; actually if we are going to `throw-exceptions` we'll rethrow the original but attempt to humanize the message
       ;; first
       (catch Throwable e
-        (log/error e (trs "Database connection error"))
         (throw (Exception. (str (driver/humanize-connection-error-message driver (.getMessage e))) e))))
     (try
       (can-connect-with-details? driver details-map :throw-exceptions)

test/metabase/api/database_test.clj

@@ -737,49 +737,41 @@
     (is (= "You don't have permissions to do that."
            (mt/user-http-request :rasta :post 403 (format "database/%d/discard_values" (mt/id)))))))
 
-
-;; For some stupid reason the *real* version of `test-database-connection` is set up to do nothing for tests. I'm
-;; guessing it's done that way so we can save invalid DBs for some silly tests. Instead of doing it the right way
-;; and using `with-redefs` to disable it in the few tests where it makes sense, we actually have to use `with-redefs`
-;; here to simulate its *normal* behavior. :unamused:
-(defn- test-database-connection [engine details]
-  (if (driver.u/can-connect-with-details? (keyword engine) details)
-    nil
-    {:valid false, :message "Error!"}))
-
 (deftest validate-database-test
   (testing "POST /api/database/validate"
-    (with-redefs [database-api/test-database-connection test-database-connection]
-      (testing "Should require superuser permissions"
-        (is (= "You don't have permissions to do that."
-               (mt/user-http-request :rasta :post 403 "database/validate"
-                                     {:details {:engine :h2, :details (:details (mt/db))}}))))
-
-      (testing "Underlying `test-connection-details` function should work"
-        (is (= (:details (mt/db))
-               (#'database-api/test-connection-details "h2" (:details (mt/db))))))
-
-      (testing "Valid database connection details"
-        (is (= {:valid true}
+    (testing "Should require superuser permissions"
+      (is (= "You don't have permissions to do that."
+             (mt/user-http-request :rasta :post 403 "database/validate"
+                                   {:details {:engine :h2, :details (:details (mt/db))}}))))
+
+    (testing "Underlying `test-connection-details` function should work"
+      (is (= (:details (mt/db))
+             (#'database-api/test-connection-details "h2" (:details (mt/db))))))
+
+    (testing "Valid database connection details"
+      (is (= {:valid true}
+             (mt/user-http-request :crowberto :post 200 "database/validate"
+                                   {:details {:engine :h2, :details (:details (mt/db))}}))))
+
+    (testing "invalid database connection details"
+      (testing "calling test-connection-details directly"
+        (is (= {:dbname  "Hmm, we couldn't connect to the database. Make sure your host and port settings are correct"
+                :message "Hmm, we couldn't connect to the database. Make sure your host and port settings are correct"
+                :valid   false}
+               (#'database-api/test-connection-details "h2" {:db "ABC"}))))
+
+      (testing "via the API endpoint"
+        (is (= {:valid false}
                (mt/user-http-request :crowberto :post 200 "database/validate"
-                                     {:details {:engine :h2, :details (:details (mt/db))}}))))
-
-      (testing "invalid database connection details"
-        (testing "calling test-connection-details directly"
-          (is (= {:valid false, :message "Error!"}
-                 (#'database-api/test-connection-details "h2" {:db "ABC"}))))
-
-        (testing "via the API endpoint"
-          (is (= {:valid false}
-                 (mt/user-http-request :crowberto :post 200 "database/validate"
-                                       {:details {:engine :h2, :details {:db "ABC"}}}))))))
+                                     {:details {:engine :h2, :details {:db "ABC"}}})))))
 
     (let [call-count (atom 0)
-          ssl-values (atom [])]
-      (with-redefs [database-api/test-database-connection (fn [_ details]
+          ssl-values (atom [])
+          valid?     (atom false)]
+      (with-redefs [database-api/test-database-connection (fn [_ details & _]
                                                             (swap! call-count inc)
                                                             (swap! ssl-values conj (:ssl details))
-                                                            {:valid true})]
+                                                            (if @valid? nil {:valid false}))]
         (testing "with SSL enabled, do not allow non-SSL connections"
           (#'database-api/test-connection-details "postgres" {:ssl true})
           (is (= 1 @call-count))
@@ -799,7 +791,17 @@
         (testing "with SSL unspecified, try twice (once with, once without SSL)"
           (#'database-api/test-connection-details "postgres" {})
           (is (= 2 @call-count))
-          (is (= [true false] @ssl-values)))))))
+          (is (= [true nil] @ssl-values)))
+
+        (reset! call-count 0)
+        (reset! ssl-values [])
+        (reset! valid? true)
+
+        (testing "with SSL disabled, but working try once (since SSL work we don't try without SSL)"
+          (is (= {:ssl true}
+                 (#'database-api/test-connection-details "postgres" {:ssl false})))
+          (is (= 1 @call-count))
+          (is (= [true] @ssl-values)))))))
 
 
 ;;; +----------------------------------------------------------------------------------------------------------------+