Fix metric & segment description logic when definitions don't have `:source-table` (#29767)

src/metabase/models/metric.clj

@@ -17,6 +17,7 @@
    [metabase.models.serialization :as serdes]
    [metabase.util :as u]
    [metabase.util.i18n :refer [tru]]
+   [metabase.util.log :as log]
    [metabase.util.malli :as mu]
    [methodical.core :as methodical]
    [toucan.models :as models]
@@ -55,9 +56,16 @@
   [metadata-provider :- lib.metadata/MetadataProvider
    {:keys [definition], table-id :table_id, :as _metric}]
   (when (seq definition)
-    (when-let [{database-id :db_id} (lib.metadata.protocols/table metadata-provider table-id)]
-      (let [query (lib.query/query-from-legacy-inner-query metadata-provider database-id definition)]
-        (lib/describe-query query)))))
+    (when-let [{database-id :db_id} (when table-id
+                                      (lib.metadata.protocols/table metadata-provider table-id))]
+      (try
+        (let [definition (merge {:source-table table-id}
+                                definition)
+              query      (lib.query/query-from-legacy-inner-query metadata-provider database-id definition)]
+          (lib/describe-query query))
+        (catch Throwable e
+          (log/error e (tru "Error calculating Metric description: {0}" (ex-message e)))
+          nil)))))
 
 (defn- warmed-metadata-provider [metrics]
   (let [metadata-provider (doto (lib.metadata.jvm/application-database-metadata-provider)

src/metabase/models/segment.clj

@@ -16,6 +16,7 @@
    [metabase.models.serialization :as serdes]
    [metabase.util :as u]
    [metabase.util.i18n :refer [tru]]
+   [metabase.util.log :as log]
    [metabase.util.malli :as mu]
    [methodical.core :as methodical]
    [toucan.models :as models]
@@ -55,9 +56,15 @@
   [metadata-provider :- lib.metadata/MetadataProvider
    {table-id :table_id, :keys [definition], :as _segment}]
   (when (seq definition)
-    (when-let [{database-id :db_id} (lib.metadata.protocols/table metadata-provider table-id)]
-      (let [query (lib.query/query-from-legacy-inner-query metadata-provider database-id definition)]
-        (lib/describe-top-level-key query :filter)))))
+    (when-let [{database-id :db_id} (when table-id (lib.metadata.protocols/table metadata-provider table-id))]
+      (try
+        (let [definition (merge {:source-table table-id}
+                                definition)
+              query      (lib.query/query-from-legacy-inner-query metadata-provider database-id definition)]
+          (lib/describe-top-level-key query :filter))
+        (catch Throwable e
+          (log/error e (tru "Error calculating Segment description: {0}" (ex-message e)))
+          nil)))))
 
 (defn- warmed-metadata-provider [segments]
   (let [metadata-provider (doto (lib.metadata.jvm/application-database-metadata-provider)

test/metabase/models/metric_test.clj

@@ -125,12 +125,14 @@
                (serdes/raw-hash ["measurement" (serdes/identity-hash table) now])
                (serdes/identity-hash metric)))))))
 
-(deftest definition-descriptions-test
+(deftest definition-description-missing-definition-test
   (testing ":definition_description should hydrate to nil if :definition is missing"
     (t2.with-temp/with-temp [Metric metric {:name     "Metric A"
                                             :table_id (mt/id :users)}]
       (is (= nil
-             (:definition_description (t2/hydrate metric :definition_description))))))
+             (:definition_description (t2/hydrate metric :definition_description)))))))
+
+(deftest definition-description-test
   (t2.with-temp/with-temp [Segment {segment-id :id} {:name       "Checkins with ID = 1"
                                                      :table_id   (mt/id :checkins)
                                                      :definition (:query (mt/mbql-query checkins
@@ -144,3 +146,22 @@
                                                                                [:segment segment-id]]}))}]
     (is (= "Venues, Sum of Name, Filtered by Price equals 4 and Checkins with ID = 1"
            (:definition_description (t2/hydrate metric :definition_description))))))
+
+(deftest definition-description-missing-source-table-test
+  (testing "Should work if `:definition` does not include `:source-table`"
+    (t2.with-temp/with-temp [Metric metric {:name       "Metric B"
+                                            :table_id   (mt/id :venues)
+                                            :definition (mt/$ids venues
+                                                          {:aggregation [[:sum $category_id->categories.name]]
+                                                           :filter      [:= $price 4]})}]
+      (is (= "Venues, Sum of Name, Filtered by Price equals 4"
+             (:definition_description (t2/hydrate metric :definition_description)))))))
+
+(deftest definition-description-invalid-query-test
+  (testing "Should return `nil` if query is invalid"
+    (t2.with-temp/with-temp [Metric metric {:name       "Metric B"
+                                            :table_id   (mt/id :venues)
+                                            :definition (mt/$ids venues
+                                                          {:aggregation [[:sum $category_id->categories.name]]
+                                                           :filter      [:= [:field Integer/MAX_VALUE nil] 4]})}]
+      (is (nil? (:definition_description (t2/hydrate metric :definition_description)))))))

test/metabase/models/segment_test.clj

@@ -117,12 +117,14 @@
                (serdes/raw-hash ["big customers" (serdes/identity-hash table) now])
                (serdes/identity-hash segment)))))))
 
-(deftest definition-description-test
+(deftest definition-description-missing-definition-test
   (testing "Do not hydrate definition description if definition is nil"
     (t2.with-temp/with-temp [Segment segment {:name     "Segment"
                                               :table_id (mt/id :users)}]
       (is (=? {:definition_description nil}
-              (t2/hydrate segment :definition_description)))))
+              (t2/hydrate segment :definition_description))))))
+
+(deftest definition-description-test
   (t2.with-temp/with-temp [Segment segment {:name       "Expensive BBQ Spots"
                                             :definition (:query (mt/mbql-query venues
                                                                   {:filter
@@ -140,3 +142,20 @@
                                                                           [:not-null $id]]}))}]
         (is (= "Filtered by Expensive BBQ Spots and ID is not empty"
                (:definition_description (t2/hydrate segment-2 :definition_description))))))))
+
+(deftest definition-description-missing-source-table-test
+  (testing "Should work if `:definition` does not include `:source-table`"
+    (t2.with-temp/with-temp [Segment segment {:name       "Expensive BBQ Spots"
+                                              :definition (mt/$ids venues
+                                                            {:filter
+                                                             [:= $price 4]})}]
+      (is (= "Filtered by Price equals 4"
+             (:definition_description (t2/hydrate segment :definition_description)))))))
+
+(deftest definition-description-invalid-query-test
+  (testing "Should return `nil` if query is invalid"
+    (t2.with-temp/with-temp [Segment segment {:name       "Expensive BBQ Spots"
+                                              :definition (:query (mt/mbql-query venues
+                                                                    {:filter
+                                                                     [:= [:field Integer/MAX_VALUE nil] 4]}))}]
+      (is (nil? (:definition_description (t2/hydrate segment :definition_description)))))))