From f4e49dbd3398b976dcaf60507b9a36e4e1c60071 Mon Sep 17 00:00:00 2001
From: Case Nelson <case@metabase.com>
Date: Wed, 27 Apr 2022 14:55:45 -0600
Subject: [PATCH] Validate datasets are found when checking bigquery (#22144)
* Validate datasets are found when checking bigquery
Fixes #19709
* Address PR feedback
Made a general mechanism to pass expected messages to users in
api/database via ex-info. This allows us to suppress logging for
"unexceptional" exceptions that one can expect to hit while setting up
drivers.
* Only validate when filters are set
Also removed the dataset list from the exception as it's not surfaced to
users.
---
.../metabase/driver/bigquery_cloud_sdk.clj | 49 +++++++++++--------
.../driver/bigquery_cloud_sdk_test.clj | 12 +++--
src/metabase/api/database.clj | 2 +-
src/metabase/driver.clj | 3 +-
4 files changed, 41 insertions(+), 25 deletions(-)
diff --git a/modules/drivers/bigquery-cloud-sdk/src/metabase/driver/bigquery_cloud_sdk.clj b/modules/drivers/bigquery-cloud-sdk/src/metabase/driver/bigquery_cloud_sdk.clj
index cc97df9a304..e92d8941134 100644
--- a/modules/drivers/bigquery-cloud-sdk/src/metabase/driver/bigquery_cloud_sdk.clj
+++ b/modules/drivers/bigquery-cloud-sdk/src/metabase/driver/bigquery_cloud_sdk.clj
@@ -55,25 +55,32 @@
(defn- ^Iterable list-tables
"Fetch all tables (new pages are loaded automatically by the API)."
- (^Iterable [{{:keys [project-id dataset-filters-type dataset-filters-patterns]} :details, :as database}]
- (list-tables (database->client database)
- (or project-id (bigquery.common/database-details->credential-project-id (:details database)))
- dataset-filters-type
- dataset-filters-patterns))
-
- (^Iterable [^BigQuery client ^String project-id ^String filter-type ^String filter-patterns]
- (let [datasets (.listDatasets client project-id (u/varargs BigQuery$DatasetListOption))
- inclusion-patterns (when (= "inclusion" filter-type) filter-patterns)
- exclusion-patterns (when (= "exclusion" filter-type) filter-patterns)]
- (apply concat (for [^Dataset dataset (.iterateAll datasets)
- :let [^DatasetId dataset-id (.. dataset getDatasetId)]
- :when (driver.s/include-schema? inclusion-patterns
- exclusion-patterns
- (.getDataset dataset-id))]
- (-> (.listTables client dataset-id (u/varargs BigQuery$TableListOption))
- .iterateAll
- .iterator
- iterator-seq))))))
+ (^Iterable [database]
+ (list-tables database false))
+ (^Iterable [{{:keys [project-id dataset-filters-type dataset-filters-patterns]} :details, :as database} validate-dataset?]
+ (list-tables (database->client database)
+ (or project-id (bigquery.common/database-details->credential-project-id (:details database)))
+ dataset-filters-type
+ dataset-filters-patterns
+ (boolean validate-dataset?)))
+ (^Iterable [^BigQuery client ^String project-id ^String filter-type ^String filter-patterns ^Boolean validate-dataset?]
+ (let [datasets (.listDatasets client project-id (u/varargs BigQuery$DatasetListOption))
+ inclusion-patterns (when (= "inclusion" filter-type) filter-patterns)
+ exclusion-patterns (when (= "exclusion" filter-type) filter-patterns)
+ dataset-iter (for [^Dataset dataset (.iterateAll datasets)
+ :let [^DatasetId dataset-id (.. dataset getDatasetId)]
+ :when (driver.s/include-schema? inclusion-patterns
+ exclusion-patterns
+ (.getDataset dataset-id))]
+ dataset-id)]
+ (when (and (not= filter-type "all") validate-dataset? (zero? (count dataset-iter)))
+ (throw (ex-info (tru "Looks like we cannot find any matching datasets.")
+ {::driver/can-connect-message? true})))
+ (apply concat (for [^DatasetId dataset-id dataset-iter]
+ (-> (.listTables client dataset-id (u/varargs BigQuery$TableListOption))
+ .iterateAll
+ .iterator
+ iterator-seq))))))
(defmethod driver/describe-database :bigquery-cloud-sdk
[_ database]
@@ -86,8 +93,10 @@
(defmethod driver/can-connect? :bigquery-cloud-sdk
[_ details-map]
;; check whether we can connect by seeing whether listing tables succeeds
- (try (some? (list-tables {:details details-map}))
+ (try (some? (list-tables {:details details-map} ::validate-dataset))
(catch Exception e
+ (when (::driver/can-connect-message? (ex-data e))
+ (throw e))
(log/errorf e (trs "Exception caught in :bigquery-cloud-sdk can-connect?"))
false)))
diff --git a/modules/drivers/bigquery-cloud-sdk/test/metabase/driver/bigquery_cloud_sdk_test.clj b/modules/drivers/bigquery-cloud-sdk/test/metabase/driver/bigquery_cloud_sdk_test.clj
index 3fc363cb382..0a46234cf42 100644
--- a/modules/drivers/bigquery-cloud-sdk/test/metabase/driver/bigquery_cloud_sdk_test.clj
+++ b/modules/drivers/bigquery-cloud-sdk/test/metabase/driver/bigquery_cloud_sdk_test.clj
@@ -20,8 +20,9 @@
(deftest can-connect?-test
(mt/test-driver :bigquery-cloud-sdk
- (let [db-details (:details (mt/db))
- fake-proj-id "definitely-not-a-real-project-id-way-no-how"]
+ (let [db-details (:details (mt/db))
+ fake-proj-id "definitely-not-a-real-project-id-way-no-how"
+ fake-dataset-id "definitely-not-a-real-dataset-id-way-no-how"]
(testing "can-connect? returns true in the happy path"
(is (true? (driver/can-connect? :bigquery-cloud-sdk db-details))))
(testing "can-connect? returns false for bogus credentials"
@@ -29,7 +30,12 @@
(testing "can-connect? returns true for a valid dataset-id even with no tables"
(with-redefs [bigquery/list-tables (fn [& _]
[])]
- (is (true? (driver/can-connect? :bigquery-cloud-sdk db-details))))))))
+ (is (true? (driver/can-connect? :bigquery-cloud-sdk db-details)))))
+ (testing "can-connect? returns an appropriate exception message if no datasets are found"
+ (is (thrown-with-msg? Exception
+ #"Looks like we cannot find any matching datasets."
+ (driver/can-connect? :bigquery-cloud-sdk
+ (assoc db-details :dataset-filters-patterns fake-dataset-id))))))))
(deftest table-rows-sample-test
(mt/test-driver :bigquery-cloud-sdk
diff --git a/src/metabase/api/database.clj b/src/metabase/api/database.clj
index 664ca189648..13faedcb413 100644
--- a/src/metabase/api/database.clj
+++ b/src/metabase/api/database.clj
@@ -528,7 +528,7 @@
:else
(invalid-response-handler :db (tru "Unable to connect to database.")))
(catch Throwable e
- (when log-exception
+ (when (and log-exception (not (some->> e ex-cause ex-data ::driver/can-connect-message?)))
(log/error e (trs "Cannot connect to Database")))
(invalid-response-handler :dbname (.getMessage e))))))
diff --git a/src/metabase/driver.clj b/src/metabase/driver.clj
index e1c04b6ed63..6c15d31efbb 100644
--- a/src/metabase/driver.clj
+++ b/src/metabase/driver.clj
@@ -247,7 +247,8 @@
"Check whether we can connect to a `Database` with `details-map` and perform a simple query. For example, a SQL
database might try running a query like `SELECT 1;`. This function should return truthy if a connection to the DB
can be made successfully, otherwise it should return falsey or throw an appropriate Exception. Exceptions if a
- connection cannot be made."
+ connection cannot be made. Throw an `ex-info` containing a truthy `::can-connect-message?` in `ex-data`
+ in order to suppress logging expected driver validation messages during setup."
{:arglists '([driver details])}
dispatch-on-initialized-driver
:hierarchy #'hierarchy)
--
GitLab