Consistent error handling for both NQA interfaces (#49023)

This standards on a "just maps" approach to returning disabled, disallowed, or failed query analysis. This is as opposed to the hodgepodge approach of significant `nil`, untagged unions with keywords, and exceptions.

Essentially we have a classic "either" style tagged union, where `:error` is the tag.

src/metabase/query_analysis.clj

@@ -108,7 +108,8 @@
      :native     (try
                    (nqa/references-for-native query)
                    (catch Exception e
-                     (log/debug e "Failed to analyze native query" query)))
+                     (log/debug e "Failed to analyze native query" query)
+                     {:error :query-analysis.error/exception, :context {:exception e}}))
      ;; For now, all model references are resolved transitively to the ultimate field ids.
      ;; We may want to change to record model references directly rather than resolving them.
      ;; This would remove the need to invalidate consuming cards when a given model changes.
@@ -124,36 +125,38 @@
   (let [query-type (lib/normalized-query-type query)]
     (when (enabled-type? query-type)
       (t2/with-transaction [_conn]
-        (let [analysis-id      (t2/insert-returning-pk! :model/QueryAnalysis {:card_id card-id :status "running"})
-              references       (query-references query query-type)
-              table->row       (fn [{:keys [schema table table-id]}]
-                                 {:card_id     card-id
-                                  :analysis_id analysis-id
-                                  :schema      schema
-                                  :table       table
-                                  :table_id    table-id})
-              field->row       (fn [{:keys [schema table column table-id field-id explicit-reference]}]
-                                 {:card_id            card-id
-                                  :analysis_id        analysis-id
-                                  :schema             schema
-                                  :table              table
-                                  :column             column
-                                  :table_id           table-id
-                                  :field_id           field-id
-                                  :explicit_reference explicit-reference})
-              success?         (some? references)]
-
-          (if-not success?
+        (let [analysis-id (t2/insert-returning-pk! :model/QueryAnalysis {:card_id card-id :status "running"})
+              result      (query-references query query-type)
+              table->row  (fn [{:keys [schema table table-id]}]
+                            {:card_id     card-id
+                             :analysis_id analysis-id
+                             :schema      schema
+                             :table       table
+                             :table_id    table-id})
+              field->row  (fn [{:keys [schema table column table-id field-id explicit-reference]}]
+                            {:card_id            card-id
+                             :analysis_id        analysis-id
+                             :schema             schema
+                             :table              table
+                             :column             column
+                             :table_id           table-id
+                             :field_id           field-id
+                             :explicit_reference explicit-reference})]
+
+          (if (contains? result :error)
+            ;; TODO we should track cases where the driver is disabled or not-supported differently.
             (t2/update! :model/QueryAnalysis analysis-id {:status "failed"})
             (do
-              (t2/insert! :model/QueryField (map field->row (:fields references)))
-              (t2/insert! :model/QueryTable (map table->row (:tables references)))
+              (t2/insert! :model/QueryField (map field->row (:fields result)))
+              (t2/insert! :model/QueryTable (map table->row (:tables result)))
               (t2/update! :model/QueryAnalysis analysis-id {:status "complete"})))
 
           (t2/delete! :model/QueryAnalysis
                       {:where [:and
                                [:= :card_id card-id]
-                               [:not= :id analysis-id]]}))))))
+                               [:not= :id analysis-id]]})
+
+          result)))))
 
 (defn- replaced-inner-query-for-native-card
   "Substitute new references for certain fields and tables, based upon the given mappings."

src/metabase/query_analysis/native_query_analyzer.clj

@@ -335,8 +335,9 @@
     ;; For now we are not restricting this as we are for [[tables-for-native]], yet.
     ;; As we don't have any hard dependencies on this data, its useful to gather info on which drivers have errors or
     ;; inaccuracies in practice.
-    (when (isa? driver/hierarchy driver :sql)
-      (references-for-sql driver query))))
+    (if (isa? driver/hierarchy driver :sql)
+      (references-for-sql driver query)
+      {:error :query-analysis.error/driver-not-supported})))
 
 (defn tables-for-native
   "Returns a set of table identifiers that (may) be referenced in the given card's query.

src/metabase/task/analyze_queries.clj

@@ -48,8 +48,9 @@
           (if (failure-map/non-retryable? card)
             (log/warnf "Skipping analysis of Card %s as its query has caused failures in the past." card-id)
             (try
-              (query-analysis/analyze!* card)
-              (failure-map/track-success! card)
+              (if (:error (query-analysis/analyze!* card))
+                (failure-map/track-failure! card)
+                (failure-map/track-success! card))
               (let [taken-ms (Math/ceil (u/since-ms timer))
                     sleep-ms (wait-proportional taken-ms)]
                 (log/debugf "Query analysis for Card %s took %sms (incl. persisting)" card-id taken-ms)

test/metabase/query_analysis/native_query_analyzer_test.clj

@@ -41,8 +41,10 @@
 (defn- refs [sql]
   (-> (mt/native-query {:query sql})
       (#'nqa/references-for-native)
+      (select-keys [:fields :tables])
       (update-vals
-       (partial sort-by (juxt :schema :table :column)))))
+       (partial sort-by (juxt :schema :table :column)))
+      not-empty))
 
 (defn- field-refs [sql]
   (:fields (refs sql)))

test/metabase/task/setup/query_analysis_setup.clj

@@ -17,34 +17,35 @@
         mlv2-query        (-> (lib/query metadata-provider venues)
                               (lib/aggregate (lib/distinct venues-name)))]
 
-    (mt/with-temp [Card c1       {:query_type    "native"
-                                  :dataset_query (mt/native-query {:query "SELECT id FROM venues"})}
-                   Card c2       {:query_type    "native"
-                                  :dataset_query (mt/native-query {:query         "SELECT id FROM venues WHERE name = {{ name }}"
-                                                                   :template-tags {"name" {:id           "_name_"
-                                                                                           :type         :text
-                                                                                           :display-name "name"
-                                                                                           :default      "qwe"}}})}
-                   Card c3       {:query_type    "query"
-                                  :dataset_query (mt/mbql-query venues {:aggregation [[:distinct $name]]})}
-                   Card c4       {:query_type    "query"
-                                  :dataset_query mlv2-query}
-                   Card archived {:archived      true
-                                  :query_type    "native"
-                                  :dataset_query (mt/native-query {:query "SELECT id FROM venues"})}
-                   Card invalid  {:query_type "native"
-                                  :dataset_query (mt/native-query {:query "SELECT boom, FROM"})}]
+    (mt/with-temporary-setting-values [query-analysis-enabled true]
+      (mt/with-temp [Card c1       {:query_type    "native"
+                                    :dataset_query (mt/native-query {:query "SELECT id FROM venues"})}
+                     Card c2       {:query_type    "native"
+                                    :dataset_query (mt/native-query {:query         "SELECT id FROM venues WHERE name = {{ name }}"
+                                                                     :template-tags {"name" {:id           "_name_"
+                                                                                             :type         :text
+                                                                                             :display-name "name"
+                                                                                             :default      "qwe"}}})}
+                     Card c3       {:query_type    "query"
+                                    :dataset_query (mt/mbql-query venues {:aggregation [[:distinct $name]]})}
+                     Card c4       {:query_type    "query"
+                                    :dataset_query mlv2-query}
+                     Card archived {:archived      true
+                                    :query_type    "native"
+                                    :dataset_query (mt/native-query {:query "SELECT id FROM venues"})}
+                     Card invalid  {:query_type "native"
+                                    :dataset_query (mt/native-query {:query "SELECT boom, FROM"})}]
 
-      ;; Make sure there is no existing analysis for the relevant cards
-      (t2/delete! :model/QueryAnalysis :card_id [:in (map :id [c1 c2 c3 c4 archived invalid])])
+        ;; Make sure there is no existing analysis for the relevant cards
+        (t2/delete! :model/QueryAnalysis :card_id [:in (map :id [c1 c2 c3 c4 archived invalid])])
 
-      ;; Make sure some other card has analysis
-      (query-analysis/analyze!* c3)
+        ;; Make sure some other card has analysis
+        (query-analysis/analyze!* c3)
 
-      ;; And attempt to analyze an invalid query
-      (query-analysis/analyze!* invalid)
+        ;; And attempt to analyze an invalid query
+        (query-analysis/analyze!* invalid)
 
-      (mt/call-with-map-params f [c1 c2 c3 c4 archived invalid]))))
+        (mt/call-with-map-params f [c1 c2 c3 c4 archived invalid])))))
 
 (defmacro with-test-setup!
   "Set up the data required to test the Query Analysis related tasks"