From eaa25b49d6aefd508ef8b2d4a6b3c0a4d7af9d07 Mon Sep 17 00:00:00 2001
From: Cam Saul <cammsaul@gmail.com>
Date: Mon, 19 Nov 2018 18:53:16 -0800
Subject: [PATCH] Fail gracefully when migrating invalid BigQuery legacy SQL
questions :skull:
---
src/metabase/db/migrations.clj | 31 ++++++++++++++++++----------
test/metabase/db/migrations_test.clj | 13 ++++++++++++
2 files changed, 33 insertions(+), 11 deletions(-)
diff --git a/src/metabase/db/migrations.clj b/src/metabase/db/migrations.clj
index d9938492df0..918ce928076 100644
--- a/src/metabase/db/migrations.clj
+++ b/src/metabase/db/migrations.clj
@@ -279,17 +279,26 @@
(doseq [database-id (db/select-ids Database :engine "bigquery")]
;; For each Card belonging to that BigQuery database...
(doseq [{query :dataset_query, card-id :id} (db/select [Card :id :dataset_query] :database_id database-id)]
- ;; If the Card isn't native, ignore it
- (when (= (keyword (:type query)) :native)
- (let [sql (get-in query [:native :query])]
- ;; if the Card already contains a #standardSQL or #legacySQL (both are case-insenstive) directive, ignore it
- (when-not (re-find #"(?i)#(standard|legacy)sql" sql)
- ;; if it doesn't have a directive it would have (under old behavior) defaulted to legacy SQL, so give it a
- ;; #legacySQL directive...
- (let [updated-sql (str "#legacySQL\n" sql)]
- ;; and save the updated dataset_query map
- (db/update! Card (u/get-id card-id)
- :dataset_query (assoc-in query [:native :query] updated-sql)))))))))
+ (try
+ ;; If the Card isn't native, ignore it
+ (when (= (keyword (:type query)) :native)
+ ;; there apparently are cases where we have a `:native` query with no `:query`. See #8924
+ (when-let [sql (get-in query [:native :query])]
+ ;; if the Card already contains a #standardSQL or #legacySQL (both are case-insenstive) directive, ignore it
+ (when-not (re-find #"(?i)#(standard|legacy)sql" sql)
+ ;; if it doesn't have a directive it would have (under old behavior) defaulted to legacy SQL, so give it a
+ ;; #legacySQL directive...
+ (let [updated-sql (str "#legacySQL\n" sql)]
+ ;; and save the updated dataset_query map
+ (db/update! Card (u/get-id card-id)
+ :dataset_query (assoc-in query [:native :query] updated-sql))))))
+ ;; if for some reason something above fails (as in #8924) let's log the error and proceed. It's not mission
+ ;; critical that we migrate existing queries anyway, and for ones that are impossible to migrate (e.g. ones
+ ;; that are invalid in the first place) it's best to fail gracefully and proceed rather than nuke someone's MB
+ ;; instance
+ (catch Throwable e
+ (log/error e (trs "Error adding legacy SQL directive to BigQuery saved Question")))))))
+
;; Before 0.30.0, we were storing the LDAP user's password in the `core_user` table (though it wasn't used). This
;; migration clears those passwords and replaces them with a UUID. This is similar to a new account setup, or how we
diff --git a/test/metabase/db/migrations_test.clj b/test/metabase/db/migrations_test.clj
index 3c8c3b5e14e..c6430ed6a25 100644
--- a/test/metabase/db/migrations_test.clj
+++ b/test/metabase/db/migrations_test.clj
@@ -47,6 +47,19 @@
(->> (db/select-field->field :name :dataset_query Card :id [:in (map u/get-id [card-1 card-2])])
(m/map-vals #(update % :database integer?)))))
+;; if for some reason we have a BigQuery native query that does not actually have any SQL, ignore it rather than
+;; barfing (#8924) (No idea how this was possible, but clearly it was)
+(expect
+ {:database true, :type :native, :native {:query 1000}}
+ (tt/with-temp* [Database [database {:engine "bigquery"}]
+ Card [card {:database_id (u/get-id database)
+ :dataset_query {:database (u/get-id database)
+ :type :native
+ :native {:query 1000}}}]]
+ (#'migrations/add-legacy-sql-directive-to-bigquery-sql-cards)
+ (-> (db/select-one-field :dataset_query Card :id (u/get-id card))
+ (update :database integer?))))
+
;; Test clearing of LDAP user local passwords
(expect
[false true]
--
GitLab