Merge pull request #8980 from metabase/fail-gracefully-when-migrating-invalid-bigquery-cards

Fail gracefully when migrating invalid BigQuery legacy SQL questions

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

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]