From 622b5e6d09148ffa7cbeef3ab307079b283b114f Mon Sep 17 00:00:00 2001 From: Braden Shepherdson <braden@metabase.com> Date: Fri, 18 Nov 2022 15:32:30 -0500 Subject: [PATCH] Serdes v2: Add more human-friendly labels to more models (#26508) Card, Dashboard, Dimension, Metric, NativeQuerySnippet, Segment and Timeline all now using their `:name` fields for the file label. File names should now be more human-friendly. --- .../serialization/v2/extract_test.clj | 84 ++++++++++--------- .../serialization/v2/load_test.clj | 6 +- src/metabase/models/collection.clj | 5 +- src/metabase/models/metric.clj | 6 -- src/metabase/models/native_query_snippet.clj | 4 - src/metabase/models/segment.clj | 6 -- src/metabase/models/serialization/base.clj | 19 ++++- src/metabase/util.clj | 37 +++++--- test/metabase/util_test.clj | 19 +++++ 9 files changed, 112 insertions(+), 74 deletions(-) diff --git a/enterprise/backend/test/metabase_enterprise/serialization/v2/extract_test.clj b/enterprise/backend/test/metabase_enterprise/serialization/v2/extract_test.clj index b2a310f610f..87ab352519c 100644 --- a/enterprise/backend/test/metabase_enterprise/serialization/v2/extract_test.clj +++ b/enterprise/backend/test/metabase_enterprise/serialization/v2/extract_test.clj @@ -211,7 +211,7 @@ {(str "[\"ref\",[\"field\"," field2-id ",null]]") {:column_title "Locus"}}}}]] (testing "table and database are extracted as [db schema table] triples" (let [ser (serdes.base/extract-one "Card" {} (db/select-one 'Card :id c1-id))] - (is (schema= {:serdes/meta (s/eq [{:model "Card" :id c1-eid}]) + (is (schema= {:serdes/meta (s/eq [{:model "Card" :id c1-eid :label "some_question"}]) :table_id (s/eq ["My Database" nil "Schemaless Table"]) :creator_id (s/eq "mark@direstrai.ts") :collection_id (s/eq coll-eid) @@ -235,7 +235,7 @@ (set (serdes.base/serdes-dependencies ser)))))) (let [ser (serdes.base/extract-one "Card" {} (db/select-one 'Card :id c2-id))] - (is (schema= {:serdes/meta (s/eq [{:model "Card" :id c2-eid}]) + (is (schema= {:serdes/meta (s/eq [{:model "Card" :id c2-eid :label "second_question"}]) :table_id (s/eq ["My Database" "PUBLIC" "Schema'd Table"]) :creator_id (s/eq "mark@direstrai.ts") :collection_id (s/eq coll-eid) @@ -266,7 +266,7 @@ (set (serdes.base/serdes-dependencies ser)))))) (let [ser (serdes.base/extract-one "Card" {} (db/select-one 'Card :id c3-id))] - (is (schema= {:serdes/meta (s/eq [{:model "Card" :id c3-eid}]) + (is (schema= {:serdes/meta (s/eq [{:model "Card" :id c3-eid :label "third_question"}]) :table_id (s/eq ["My Database" "PUBLIC" "Schema'd Table"]) :creator_id (s/eq "mark@direstrai.ts") :collection_id (s/eq coll-eid) @@ -314,7 +314,7 @@ (testing "Cards can be based on other cards" (let [ser (serdes.base/extract-one "Card" {} (db/select-one 'Card :id c5-id))] - (is (schema= {:serdes/meta (s/eq [{:model "Card" :id c5-eid}]) + (is (schema= {:serdes/meta (s/eq [{:model "Card" :id c5-eid :label "dependent_question"}]) :table_id (s/eq ["My Database" "PUBLIC" "Schema'd Table"]) :creator_id (s/eq "mark@direstrai.ts") :collection_id (s/eq coll-eid) @@ -337,7 +337,7 @@ (testing "Dashboards include their Dashcards" (let [ser (serdes.base/extract-one "Dashboard" {} (db/select-one 'Dashboard :id other-dash-id))] - (is (schema= {:serdes/meta (s/eq [{:model "Dashboard" :id other-dash}]) + (is (schema= {:serdes/meta (s/eq [{:model "Dashboard" :id other-dash :label "dave_s_dash"}]) :entity_id (s/eq other-dash) :ordered_cards [{:visualization_settings (s/eq {:table.pivot_column "SOURCE" @@ -435,7 +435,7 @@ :human_readable_field_id target-id}]] (testing "vanilla user-created dimensions" (let [ser (serdes.base/extract-one "Dimension" {} (db/select-one 'Dimension :id dim1-id))] - (is (schema= {:serdes/meta (s/eq [{:model "Dimension" :id dim1-eid}]) + (is (schema= {:serdes/meta (s/eq [{:model "Dimension" :id dim1-eid :label "vanilla_dimension"}]) :field_id (s/eq ["My Database" nil "Schemaless Table" "email"]) :human_readable_field_id (s/eq nil) :created_at LocalDateTime @@ -451,7 +451,7 @@ (testing "foreign key dimensions" (let [ser (serdes.base/extract-one "Dimension" {} (db/select-one 'Dimension :id dim2-id))] - (is (schema= {:serdes/meta (s/eq [{:model "Dimension" :id dim2-eid}]) + (is (schema= {:serdes/meta (s/eq [{:model "Dimension" :id dim2-eid :label "foreign_dimension"}]) :field_id (s/eq ["My Database" "PUBLIC" "Schema'd Table" "foreign_id"]) :human_readable_field_id (s/eq ["My Database" "PUBLIC" "Foreign Table" "real_field"]) :created_at LocalDateTime @@ -487,7 +487,7 @@ :aggregation [[:sum [:field field-id nil]]]}}]] (testing "metrics" (let [ser (serdes.base/extract-one "Metric" {} (db/select-one 'Metric :id m1-id))] - (is (schema= {:serdes/meta (s/eq [{:model "Metric" :id m1-eid :label "My Metric"}]) + (is (schema= {:serdes/meta (s/eq [{:model "Metric" :id m1-eid :label "my_metric"}]) :table_id (s/eq ["My Database" nil "Schemaless Table"]) :creator_id (s/eq "ann@heart.band") :definition (s/eq {:source-table ["My Database" nil "Schemaless Table"] @@ -529,7 +529,7 @@ (let [ser (serdes.base/extract-one "NativeQuerySnippet" {} (db/select-one 'NativeQuerySnippet :id s1-id))] (is (schema= {:serdes/meta (s/eq [{:model "NativeQuerySnippet" :id s1-eid - :label "Snippet 1"}]) + :label "snippet_1"}]) :collection_id (s/eq coll-eid) :creator_id (s/eq "ann@heart.band") :created_at OffsetDateTime @@ -545,7 +545,7 @@ (let [ser (serdes.base/extract-one "NativeQuerySnippet" {} (db/select-one 'NativeQuerySnippet :id s2-id))] (is (schema= {:serdes/meta (s/eq [{:model "NativeQuerySnippet" :id s2-eid - :label "Snippet 2"}]) + :label "snippet_2"}]) (s/optional-key :collection_id) (s/eq nil) :creator_id (s/eq "ann@heart.band") :created_at OffsetDateTime @@ -579,7 +579,7 @@ (testing "timelines" (testing "with no events" (let [ser (serdes.base/extract-one "Timeline" {} (db/select-one 'Timeline :id empty-id))] - (is (schema= {:serdes/meta (s/eq [{:model "Timeline" :id empty-eid}]) + (is (schema= {:serdes/meta (s/eq [{:model "Timeline" :id empty-eid :label "empty_timeline"}]) :collection_id (s/eq coll-eid) :creator_id (s/eq "ann@heart.band") :created_at OffsetDateTime @@ -594,7 +594,7 @@ (testing "with events" (let [ser (serdes.base/extract-one "Timeline" {} (db/select-one 'Timeline :id line-id)) stamp "2020-04-11T00:00:00Z"] - (is (schema= {:serdes/meta (s/eq [{:model "Timeline" :id line-eid}]) + (is (schema= {:serdes/meta (s/eq [{:model "Timeline" :id line-eid :label "populated_timeline"}]) :collection_id (s/eq coll-eid) :creator_id (s/eq "ann@heart.band") :created_at OffsetDateTime @@ -628,7 +628,7 @@ :filter [:< [:field field-id nil] 18]}}]] (testing "segment" (let [ser (serdes.base/extract-one "Segment" {} (db/select-one 'Segment :id s1-id))] - (is (schema= {:serdes/meta (s/eq [{:model "Segment" :id s1-eid :label "My Segment"}]) + (is (schema= {:serdes/meta (s/eq [{:model "Segment" :id s1-eid :label "my_segment"}]) :table_id (s/eq ["My Database" nil "Schemaless Table"]) :creator_id (s/eq "ann@heart.band") :definition (s/eq {:source-table ["My Database" nil "Schemaless Table"] @@ -718,7 +718,9 @@ :dashboard_id dash-id}]] (testing "pulse with neither collection nor dashboard" (let [ser (serdes.base/extract-one "Pulse" {} (db/select-one 'Pulse :id p-none-id))] - (is (schema= {:serdes/meta (s/eq [{:model "Pulse" :id p-none-eid}]) + (is (schema= {:serdes/meta (s/eq [{:model "Pulse" + :id p-none-eid + :label "pulse_w_o_collection_or_dashboard"}]) :creator_id (s/eq "ann@heart.band") (s/optional-key :dashboard_id) (s/eq nil) (s/optional-key :collection_id) (s/eq nil) @@ -733,7 +735,9 @@ (testing "pulse with just collection" (let [ser (serdes.base/extract-one "Pulse" {} (db/select-one 'Pulse :id p-coll-id))] - (is (schema= {:serdes/meta (s/eq [{:model "Pulse" :id p-coll-eid}]) + (is (schema= {:serdes/meta (s/eq [{:model "Pulse" + :id p-coll-eid + :label "pulse_with_only_collection"}]) :creator_id (s/eq "ann@heart.band") (s/optional-key :dashboard_id) (s/eq nil) :collection_id (s/eq coll-eid) @@ -748,7 +752,9 @@ (testing "pulse with just dashboard" (let [ser (serdes.base/extract-one "Pulse" {} (db/select-one 'Pulse :id p-dash-id))] - (is (schema= {:serdes/meta (s/eq [{:model "Pulse" :id p-dash-eid}]) + (is (schema= {:serdes/meta (s/eq [{:model "Pulse" + :id p-dash-eid + :label "pulse_with_only_dashboard"}]) :creator_id (s/eq "ann@heart.band") :dashboard_id (s/eq dash-eid) (s/optional-key :collection_id) (s/eq nil) @@ -763,7 +769,9 @@ (testing "pulse with both collection and dashboard" (let [ser (serdes.base/extract-one "Pulse" {} (db/select-one 'Pulse :id p-both-id))] - (is (schema= {:serdes/meta (s/eq [{:model "Pulse" :id p-both-eid}]) + (is (schema= {:serdes/meta (s/eq [{:model "Pulse" + :id p-both-eid + :label "pulse_with_both_collection_and_dashboard"}]) :creator_id (s/eq "ann@heart.band") :dashboard_id (s/eq dash-eid) :collection_id (s/eq coll-eid) @@ -969,45 +977,45 @@ (testing "selecting a dashboard gets all cards its dashcards depend on" (testing "grandparent dashboard" - (is (= #{[{:model "Dashboard" :id dash1-eid}] - [{:model "Card" :id c1-1-eid}] - [{:model "Card" :id c1-2-eid}]} + (is (= #{[{:model "Dashboard" :id dash1-eid :label "dashboard_1"}] + [{:model "Card" :id c1-1-eid :label "question_1_1"}] + [{:model "Card" :id c1-2-eid :label "question_1_2"}]} (->> (extract/extract-subtrees {:targets [["Dashboard" dash1-id]]}) (map serdes.base/serdes-path) set)))) (testing "middle dashboard" - (is (= #{[{:model "Dashboard" :id dash2-eid}] - [{:model "Card" :id c2-1-eid}] - [{:model "Card" :id c2-2-eid}]} + (is (= #{[{:model "Dashboard" :id dash2-eid :label "dashboard_2"}] + [{:model "Card" :id c2-1-eid :label "question_2_1"}] + [{:model "Card" :id c2-2-eid :label "question_2_2"}]} (->> (extract/extract-subtrees {:targets [["Dashboard" dash2-id]]}) (map serdes.base/serdes-path) set)))) (testing "grandchild dashboard" - (is (= #{[{:model "Dashboard" :id dash3-eid}] - [{:model "Card" :id c3-1-eid}] - [{:model "Card" :id c3-2-eid}]} + (is (= #{[{:model "Dashboard" :id dash3-eid :label "dashboard_3"}] + [{:model "Card" :id c3-1-eid :label "question_3_1"}] + [{:model "Card" :id c3-2-eid :label "question_3_2"}]} (->> (extract/extract-subtrees {:targets [["Dashboard" dash3-id]]}) (map serdes.base/serdes-path) set))))) (testing "selecting a collection gets all its contents" (let [grandchild-paths #{[{:model "Collection" :id coll3-eid :label "grandchild_collection"}] - [{:model "Dashboard" :id dash3-eid}] - [{:model "Card" :id c3-1-eid}] - [{:model "Card" :id c3-2-eid}] - [{:model "Card" :id c3-3-eid}]} + [{:model "Dashboard" :id dash3-eid :label "dashboard_3"}] + [{:model "Card" :id c3-1-eid :label "question_3_1"}] + [{:model "Card" :id c3-2-eid :label "question_3_2"}] + [{:model "Card" :id c3-3-eid :label "question_3_3"}]} middle-paths #{[{:model "Collection" :id coll2-eid :label "nested_collection"}] - [{:model "Dashboard" :id dash2-eid}] - [{:model "Card" :id c2-1-eid}] - [{:model "Card" :id c2-2-eid}] - [{:model "Card" :id c2-3-eid}]} + [{:model "Dashboard" :id dash2-eid :label "dashboard_2"}] + [{:model "Card" :id c2-1-eid :label "question_2_1"}] + [{:model "Card" :id c2-2-eid :label "question_2_2"}] + [{:model "Card" :id c2-3-eid :label "question_2_3"}]} grandparent-paths #{[{:model "Collection" :id coll1-eid :label "some_collection"}] - [{:model "Dashboard" :id dash1-eid}] - [{:model "Card" :id c1-1-eid}] - [{:model "Card" :id c1-2-eid}] - [{:model "Card" :id c1-3-eid}]}] + [{:model "Dashboard" :id dash1-eid :label "dashboard_1"}] + [{:model "Card" :id c1-1-eid :label "question_1_1"}] + [{:model "Card" :id c1-2-eid :label "question_1_2"}] + [{:model "Card" :id c1-3-eid :label "question_1_3"}]}] (testing "grandchild collection has all its own contents" (is (= grandchild-paths ; Includes the third card not found in the collection (->> (extract/extract-subtrees {:targets [["Collection" coll3-id]]}) diff --git a/enterprise/backend/test/metabase_enterprise/serialization/v2/load_test.clj b/enterprise/backend/test/metabase_enterprise/serialization/v2/load_test.clj index 974a6586d8f..ec0153ad3ad 100644 --- a/enterprise/backend/test/metabase_enterprise/serialization/v2/load_test.clj +++ b/enterprise/backend/test/metabase_enterprise/serialization/v2/load_test.clj @@ -671,13 +671,15 @@ timeline1 (first (filter #(= (:entity_id %) (:entity_id @timeline1s)) timelines)) timeline2 (first (filter #(= (:entity_id %) (:entity_id @timeline2s)) timelines))] (testing "with inline :events" - (is (schema= {:archived (s/eq false) + (is (schema= {:serdes/meta (s/eq [{:model "Timeline" + :id (:entity_id timeline1) + :label "some_events"}]) + :archived (s/eq false) :collection_id (s/eq (:entity_id @coll1s)) :name (s/eq "Some events") :creator_id (s/eq "tom@bost.on") (s/optional-key :updated_at) OffsetDateTime :created_at OffsetDateTime - :serdes/meta (s/eq [{:model "Timeline" :id (:entity_id timeline1)}]) :entity_id (s/eq (:entity_id timeline1)) (s/optional-key :icon) (s/maybe s/Str) :description (s/maybe s/Str) diff --git a/src/metabase/models/collection.clj b/src/metabase/models/collection.clj index c2b9d0ac54d..e8392f57a06 100644 --- a/src/metabase/models/collection.clj +++ b/src/metabase/models/collection.clj @@ -951,9 +951,8 @@ [[{:model "Collection" :id parent_id}]] [])) -(defmethod serdes.base/serdes-generate-path "Collection" [_ {:keys [slug] :as coll}] - [(cond-> (serdes.base/infer-self-path "Collection" coll) - slug (assoc :label slug))]) +(defmethod serdes.base/serdes-generate-path "Collection" [_ coll] + (serdes.base/maybe-labeled "Collection" coll :slug)) (defmethod serdes.base/serdes-descendants "Collection" [_model-name id] (let [location (db/select-one-field :location Collection :id id) diff --git a/src/metabase/models/metric.clj b/src/metabase/models/metric.clj index be44b7dac50..8273bbf23ac 100644 --- a/src/metabase/models/metric.clj +++ b/src/metabase/models/metric.clj @@ -76,12 +76,6 @@ ;;; ------------------------------------------------- SERIALIZATION -------------------------------------------------- - -(defmethod serdes.base/serdes-generate-path "Metric" - [_ metric] - (let [base (serdes.base/infer-self-path "Metric" metric)] - [(assoc base :label (:name metric))])) - (defmethod serdes.base/extract-one "Metric" [_model-name _opts metric] (-> (serdes.base/extract-one-basics "Metric" metric) diff --git a/src/metabase/models/native_query_snippet.clj b/src/metabase/models/native_query_snippet.clj index fe6d41f7c81..b605e810ebb 100644 --- a/src/metabase/models/native_query_snippet.clj +++ b/src/metabase/models/native_query_snippet.clj @@ -80,10 +80,6 @@ (when (seq collection-set) (db/select-reducible NativeQuerySnippet :collection_id [:in collection-set]))])) -(defmethod serdes.base/serdes-generate-path "NativeQuerySnippet" [_ snippet] - [(assoc (serdes.base/infer-self-path "NativeQuerySnippet" snippet) - :label (:name snippet))]) - (defmethod serdes.base/extract-one "NativeQuerySnippet" [_model-name _opts snippet] (-> (serdes.base/extract-one-basics "NativeQuerySnippet" snippet) diff --git a/src/metabase/models/segment.clj b/src/metabase/models/segment.clj index b867de22614..0f446e9541c 100644 --- a/src/metabase/models/segment.clj +++ b/src/metabase/models/segment.clj @@ -76,12 +76,6 @@ ;;; ------------------------------------------------ Serialization --------------------------------------------------- - -(defmethod serdes.base/serdes-generate-path "Segment" - [_ segment] - [(assoc (serdes.base/infer-self-path "Segment" segment) - :label (:name segment))]) - (defmethod serdes.base/extract-one "Segment" [_model-name _opts segment] (-> (serdes.base/extract-one-basics "Segment" segment) diff --git a/src/metabase/models/serialization/base.clj b/src/metabase/models/serialization/base.clj index 1d4e9019bde..d2fac6c0ab8 100644 --- a/src/metabase/models/serialization/base.clj +++ b/src/metabase/models/serialization/base.clj @@ -12,6 +12,7 @@ (:require [clojure.tools.logging :as log] [metabase.models.interface :as mi] [metabase.models.serialization.hash :as serdes.hash] + [metabase.util :as u] [toucan.db :as db] [toucan.models :as models])) @@ -69,7 +70,8 @@ against existing entities. The default implementation is a single level, using the model name provided and the ID from either - [[serdes-entity-id]] or [[serdes.hash/identity-hash]]. + [[serdes-entity-id]] or [[serdes.hash/identity-hash]], and any `:name` field as the `:label`. + This default implementation is factored out as [[maybe-labeled]] for reuse. Implementation notes: - `:serdes/meta` might be defined - if so it's coming from ingestion and might have truncated values in it, and should @@ -98,9 +100,22 @@ (throw (ex-info "Could not infer-self-path on this entity - maybe implement serdes-entity-id ?" {:model model-name :entity entity})))})) +(defn maybe-labeled + "Common helper for defining [[serdes-generate-path]] for an entity that is + (1) top-level, ie. a one layer path; + (2) labeled by a single field, slugified. + + For example, a Card's or Dashboard's `:name` field." + [model-name entity slug-key] + (let [self (infer-self-path model-name entity) + label (get entity slug-key)] + [(if label + (assoc self :label (u/slugify label {:unicode? true})) + self)])) + (defmethod serdes-generate-path :default [model-name entity] ;; This default works for most models, but needs overriding for nested ones. - [(infer-self-path model-name entity)]) + (maybe-labeled model-name entity :name)) ;;; +----------------------------------------------------------------------------------------------------------------+ ;;; | Serialization Process | diff --git a/src/metabase/util.clj b/src/metabase/util.clj index 5e060e58c6e..6d3a436aced 100644 --- a/src/metabase/util.clj +++ b/src/metabase/util.clj @@ -412,25 +412,36 @@ \_}) ;; unfortunately it seems that this doesn't fully-support Emoji :(, they get encoded as "??" -(defn- slugify-char [^Character c] - (cond - (> (int c) 128) (codec/url-encode c) ; for non-ASCII characters, URL-encode them - (contains? slugify-valid-chars c) c ; for ASCII characters, if they're in the allowed set of characters, keep them - :else \_)) ; otherwise replace them with underscores +(defn- slugify-char [^Character c url-encode?] + (if (< (int c) 128) + ;; ASCII characters must be in the valid list, or they get replaced with underscores. + (if (contains? slugify-valid-chars c) + c + \_) + ;; Non-ASCII characters are URL-encoded or preserved, based on the option. + (if url-encode? + (codec/url-encode c) + c))) (defn slugify "Return a version of String `s` appropriate for use as a URL slug. - Downcase the name, remove diacritcal marks, and replace non-alphanumeric *ASCII* characters with underscores; - URL-encode non-ASCII characters. (Non-ASCII characters are encoded rather than replaced with underscores in order - to support languages that don't use the Latin alphabet; see metabase#3818). + Downcase the name and remove diacritcal marks, and replace non-alphanumeric *ASCII* characters with underscores. + + If `unicode?` is falsy (the default), URL-encode non-ASCII characters. With `unicode?` truthy, non-ASCII characters + are preserved. + (Even when we want full ASCII output for eg. URL slugs, non-ASCII characters should be encoded rather than + replaced with underscores in order to support languages that don't use the Latin alphabet; see metabase#3818). - Optionally specify `max-length` which will truncate the slug after that many characters." + Optionally specify `:max-length` which will truncate the slug after that many characters." (^String [^String s] + (slugify s {})) + (^String [s {:keys [max-length unicode?]}] (when (seq s) - (str/join (for [c (remove-diacritical-marks (str/lower-case s))] - (slugify-char c))))) - (^String [s max-length] - (str/join (take max-length (slugify s))))) + (let [slug (str/join (for [c (remove-diacritical-marks (str/lower-case s))] + (slugify-char c (not unicode?))))] + (if max-length + (str/join (take max-length slug)) + slug))))) (defn full-exception-chain "Gather the full exception chain into a sequence." diff --git a/test/metabase/util_test.clj b/test/metabase/util_test.clj index 580b5f9f4e5..9f3e11e2f7e 100644 --- a/test/metabase/util_test.clj +++ b/test/metabase/util_test.clj @@ -132,6 +132,25 @@ (is (= expected (u/slugify s)))))))) +(deftest ^:parallel slugify-unicode-test + (doseq [[group s->expected] + {nil + {"ToucanFest 2017" "toucanfest_2017" + "Cam's awesome toucan emporium" "cam_s_awesome_toucan_emporium" + "Frequently-Used Cards" "frequently_used_cards"} + + "check that diactrics get removed" + {"Cam Saul's Toucannery" "cam_saul_s_toucannery" + "toucans dislike piñatas :(" "toucans_dislike_pinatas___" } + + "check that non-ASCII characters are preserved" + {"勇士" "勇士"}}] + (testing group + (doseq [[s expected] s->expected] + (testing (list 'u/slugify s {:unicode? true}) + (is (= expected + (u/slugify s {:unicode? true})))))))) + (deftest ^:parallel full-exception-chain-test (testing "Not an Exception" (is (= nil -- GitLab