From 3fb962b9b816112a33f94079b63abe862dc7fe3f Mon Sep 17 00:00:00 2001
From: Cam Saul <1455846+camsaul@users.noreply.github.com>
Date: Tue, 4 Apr 2023 10:21:05 -0700
Subject: [PATCH] Fix metric & segment description logic when definitions don't
have `:source-table` (#29767)
---
src/metabase/models/metric.clj | 14 +++++++++++---
src/metabase/models/segment.clj | 13 ++++++++++---
test/metabase/models/metric_test.clj | 25 +++++++++++++++++++++++--
test/metabase/models/segment_test.clj | 23 +++++++++++++++++++++--
4 files changed, 65 insertions(+), 10 deletions(-)
diff --git a/src/metabase/models/metric.clj b/src/metabase/models/metric.clj
index e3b069736ec..3100253640f 100644
--- a/src/metabase/models/metric.clj
+++ b/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)
diff --git a/src/metabase/models/segment.clj b/src/metabase/models/segment.clj
index df0e2cf6f4a..89a5b85e4db 100644
--- a/src/metabase/models/segment.clj
+++ b/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)
diff --git a/test/metabase/models/metric_test.clj b/test/metabase/models/metric_test.clj
index e866190c62c..b4c37b26869 100644
--- a/test/metabase/models/metric_test.clj
+++ b/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)))))))
diff --git a/test/metabase/models/segment_test.clj b/test/metabase/models/segment_test.clj
index 02f3a4e3d68..b9eeae877cb 100644
--- a/test/metabase/models/segment_test.clj
+++ b/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)))))))
--
GitLab