malli describe fixes (#27602)

* updates + updates tests

- use titles for everything
- repeat "sounds" right
- biding time to remerge upstream

* Update src/metabase/util/malli/describe.clj

* pluralize times ouhgt to be private

* keep error messages in tests in sync 

* linter newline

src/metabase/util/malli/describe.clj

@@ -29,6 +29,20 @@
       max (str " with length >= " max)
       :else "")))
 
+(defn- pluralize-times [n]
+  (when n
+    (if (= 1 n) "time" "times")))
+
+(defn- repeat-suffix [schema]
+  (let [{:keys [min max]} (-> schema mc/properties)
+        min-timez (pluralize-times min)
+        max-timez (pluralize-times max)]
+    (cond
+      (and min max) (str " at least " min " " min-timez ", up to " max " " max-timez)
+      min           (str " at least " min " " min-timez)
+      max           (str " at most " max " " max-timez)
+      :else         "")))
+
 (defn- min-max-suffix-number [schema]
   (let [{:keys [min max]} (-> schema mc/properties)]
     (cond
@@ -116,36 +130,36 @@
 (defmethod accept 'set? [_ schema children _] (str "set" (titled schema) (length-suffix schema) (of-clause children)))
 (defmethod accept :set [_ schema children _] (str "set" (titled schema) (length-suffix schema) (of-clause children)))
 
-(defmethod accept 'string? [_ schema _ _] (str "string" (length-suffix schema)))
-(defmethod accept :string [_ schema _ _] (str "string" (length-suffix schema)))
+(defmethod accept 'string? [_ schema _ _] (str "string" (titled schema) (length-suffix schema)))
+(defmethod accept :string [_ schema _ _] (str "string" (titled schema) (length-suffix schema)))
 
-(defmethod accept 'number? [_ schema _ _] (str "number" (min-max-suffix schema)))
-(defmethod accept :number [_ schema _ _] (str "number" (min-max-suffix schema)))
+(defmethod accept 'number? [_ schema _ _] (str "number" (titled schema) (min-max-suffix schema)))
+(defmethod accept :number [_ schema _ _] (str "number" (titled schema) (min-max-suffix schema)))
 
-(defmethod accept 'pos-int? [_ schema _ _] (str "integer greater than 0" (min-max-suffix schema)))
-(defmethod accept :pos-int [_ schema _ _] (str "integer greater than 0" (min-max-suffix schema)))
+(defmethod accept 'pos-int? [_ schema _ _] (str "integer greater than 0" (titled schema) (min-max-suffix schema)))
+(defmethod accept :pos-int [_ schema _ _] (str "integer greater than 0" (titled schema) (min-max-suffix schema)))
 
-(defmethod accept 'neg-int? [_ schema _ _] (str "integer less than 0" (min-max-suffix schema)))
-(defmethod accept :neg-int [_ schema _ _] (str "integer less than 0" (min-max-suffix schema)))
+(defmethod accept 'neg-int? [_ schema _ _] (str "integer less than 0" (titled schema) (min-max-suffix schema)))
+(defmethod accept :neg-int [_ schema _ _] (str "integer less than 0" (titled schema) (min-max-suffix schema)))
 
-(defmethod accept 'nat-int? [_ schema _ _] (str "natural integer" (min-max-suffix schema)))
-(defmethod accept :nat-int [_ schema _ _] (str "natural integer" (min-max-suffix schema)))
+(defmethod accept 'nat-int? [_ schema _ _] (str "natural integer" (titled schema) (min-max-suffix schema)))
+(defmethod accept :nat-int [_ schema _ _] (str "natural integer" (titled schema) (min-max-suffix schema)))
 
-(defmethod accept 'float? [_ schema _ _] (str "float" (min-max-suffix schema)))
-(defmethod accept :float [_ schema _ _] (str "float" (min-max-suffix schema)))
+(defmethod accept 'float? [_ schema _ _] (str "float" (titled schema) (min-max-suffix schema)))
+(defmethod accept :float [_ schema _ _] (str "float" (titled schema) (min-max-suffix schema)))
 
-(defmethod accept 'pos? [_ schema _ _] (str "number greater than 0" (min-max-suffix schema)))
-(defmethod accept :pos [_ schema _ _] (str "number greater than 0" (min-max-suffix schema)))
+(defmethod accept 'pos? [_ schema _ _] (str "number greater than 0" (titled schema) (min-max-suffix schema)))
+(defmethod accept :pos [_ schema _ _] (str "number greater than 0" (titled schema) (min-max-suffix schema)))
 
-(defmethod accept 'neg? [_ schema _ _] (str "number less than 0" (min-max-suffix schema)))
-(defmethod accept :neg [_ schema _ _] (str "number less than 0" (min-max-suffix schema)))
+(defmethod accept 'neg? [_ schema _ _] (str "number less than 0" (titled schema) (min-max-suffix schema)))
+(defmethod accept :neg [_ schema _ _] (str "number less than 0" (titled schema) (min-max-suffix schema)))
 
-(defmethod accept 'integer? [_ schema _ _] (str "integer" (min-max-suffix-number schema)))
-(defmethod accept 'int? [_ schema _ _] (str "integer" (min-max-suffix-number schema)))
-(defmethod accept :int [_ schema _ _] (str "integer" (min-max-suffix-number schema)))
+(defmethod accept 'integer? [_ schema _ _] (str "integer" (titled schema) (min-max-suffix-number schema)))
+(defmethod accept 'int? [_ schema _ _] (str "integer" (titled schema) (min-max-suffix-number schema)))
+(defmethod accept :int [_ schema _ _] (str "integer" (titled schema) (min-max-suffix-number schema)))
 
-(defmethod accept 'double? [_ schema _ _] (str "double" (min-max-suffix-number schema)))
-(defmethod accept :double [_ schema _ _] (str "double" (min-max-suffix-number schema)))
+(defmethod accept 'double? [_ schema _ _] (str "double" (titled schema) (min-max-suffix-number schema)))
+(defmethod accept :double [_ schema _ _] (str "double" (titled schema) (min-max-suffix-number schema)))
 
 (defmethod accept :merge [_ schema _ options] ((::describe options) (mc/deref schema) options))
 (defmethod accept :union [_ schema _ options] ((::describe options) (mc/deref schema) options))
@@ -192,11 +206,24 @@
 (defmethod accept :function [_ _ _children _] "function")
 (defmethod accept :fn [_ _ _ _] "function")
 
+(defn- tagged [children]
+  (map (fn [[tag _ c]] (str c " (tag: " tag ")" )) children))
+
 (defmethod accept :or [_ _ children _] (str/join ", or " children))
-(defmethod accept :orn [_ _ children _] (str/join ", or " (map (fn [[tag _ c]] (str c " (tag: " tag ")" )) children)))
+(defmethod accept :orn [_ _ children _] (str/join ", or " (tagged children)))
 
 (defmethod accept :cat [_ _ children _] (str/join ", " children))
-(defmethod accept :catn [_ _ children _] (str/join ", or " (map (fn [[tag _ c]] (str c " (tag: " tag ")" )) children)))
+(defmethod accept :catn [_ _ children _] (str/join ", and " (tagged children)))
+
+(defmethod accept :alt [_ _ children _] (str/join ", or " children))
+(defmethod accept :altn [_ _ children _] (str/join ", or " (tagged children)))
+
+(defmethod accept :+ [_ _ children _] (str "one or more " (str/join ", " children)))
+(defmethod accept :* [_ _ children _] (str "zero or more " (str/join ", " children)))
+(defmethod accept :? [_ _ children _] (str "zero or one " (str/join ", " children)))
+
+(defmethod accept :repeat [_ schema children _]
+  (str "repeat " (diamond (first children)) (repeat-suffix schema)))
 
 (defmethod accept 'boolean? [_ _ _ _] "boolean")
 (defmethod accept :boolean [_ _ _ _] "boolean")
@@ -209,10 +236,11 @@
         additional-properties (:closed (mc/properties schema))
         kv-description (str/join ", " (map (fn [[k _ s]] (str k (when (contains? optional  k) " (optional)") " -> " (diamond s))) children))]
     (str/trim
-     (cond-> (str "map ")
+     (cond-> (str "map " (titled schema))
        (seq kv-description) (str "where {" kv-description "} ")
        additional-properties (str "with no other keys ")))))
 
+
 (defmethod accept ::mc/val [_ _ children _] (first children))
 (defmethod accept 'map? [n schema children o] (-map n schema children o))
 (defmethod accept :map [n schema children o] (-map n schema children o))

test/metabase/api/common/internal_test.clj

@@ -95,14 +95,14 @@
                                     :lonlat [0.0 0.0]}}))))
 
     (is (= {:errors
-            {:address "map where {:id -> <string>, :tags -> <vector of string>, :address -> <map where {:street -> <string>, :city -> <string>, :zip -> <integer>, :lonlat -> <vector with exactly 2 items of type: double, double>}>}"},
-            :specific-errors {:address {:id ["missing required key"],
+            {:address "map (titled: ‘Address’) where {:id -> <string>, :tags -> <vector of string>, :address -> <map where {:street -> <string>, :city -> <string>, :zip -> <integer>, :lonlat -> <vector with exactly 2 items of type: double, double>}>}"},
+             :specific-errors {:address {:id ["missing required key"],
                                         :tags ["missing required key"],
                                         :address ["missing required key"]}}}
            (:body (post! "/post/test-address" {:x "1"}))))
 
     (is (= {:errors
-            {:address "map where {:id -> <string>, :tags -> <vector of string>, :address -> <map where {:street -> <string>, :city -> <string>, :zip -> <integer>, :lonlat -> <vector with exactly 2 items of type: double, double>}>}"},
+            {:address "map (titled: ‘Address’) where {:id -> <string>, :tags -> <vector of string>, :address -> <map where {:street -> <string>, :city -> <string>, :zip -> <integer>, :lonlat -> <vector with exactly 2 items of type: double, double>}>}"},
             :specific-errors {:address
                               {:id ["should be a string"]
                                :tags ["invalid type"]
@@ -116,7 +116,7 @@
                                                          :lonlat [0.0 0.0]}}))))
 
     (is (= {:errors
-            {:address "map where {:id -> <string>, :tags -> <vector of string>, :address -> <map where {:street -> <string>, :city -> <string>, :zip -> <integer>, :lonlat -> <vector with exactly 2 items of type: double, double>} with no other keys>} with no other keys"},
+            {:address "map (titled: ‘Address’) where {:id -> <string>, :tags -> <vector of string>, :address -> <map where {:street -> <string>, :city -> <string>, :zip -> <integer>, :lonlat -> <vector with exactly 2 items of type: double, double>} with no other keys>} with no other keys"},
             :specific-errors {:address
                               {:address ["missing required key"],
                                :a ["disallowed key"],

test/metabase/util/malli/describe_test.clj

 (ns metabase.util.malli.describe-test
-  (:require [clojure.test :refer [deftest is]]
+  (:require [clojure.test :refer [deftest is testing]]
             [metabase.util.malli.describe :as umd]))
 
 (deftest descriptor-test
-
-  (is (= "vector" (umd/describe vector?)))
-
-  (is (= "vector of integer" (umd/describe [:vector :int])))
-
-  (is (= "string with length <= 5" (umd/describe [:string {:min 5}])))
-  (is (= "string with length >= 5" (umd/describe [:string {:max 5}])))
-  (is (= "string with length between 3 and 5 inclusive" (umd/describe [:string {:min 3 :max 5}])))
-
-  (is (= "map" (umd/describe map?)))
-
-  (is (= "map where {:x -> <integer>}"
-         (umd/describe [:map [:x int?]])))
-
-  (is (= "map where {:x (optional) -> <integer>, :y -> <boolean>}"
-         (umd/describe [:map [:x {:optional true} int?] [:y :boolean]])))
-
-  (is (= "map where {:x -> <integer>} with no other keys"
-         (umd/describe [:map {:closed true} [:x int?]])))
-
-  (is (= "map where {:x (optional) -> <integer>, :y -> <boolean>} with no other keys"
-         (umd/describe [:map {:closed true} [:x {:optional true} int?] [:y :boolean]])))
-
-  (is (= "function that takes input: [integer] and returns integer"
-         (umd/describe [:=> [:cat int?] int?])))
-
-  (is (= "map where {:j-code -> <keyword, and has length 4>}"
-         (umd/describe [:map [:j-code [:and
-                                          :keyword
-                                          [:fn {:description "has length 4"} #(= 4 (count (name %)))]]]])))
-
-  (is (= (umd/describe [:map-of {:title "dict"} :int :string])
-         "map (titled: ‘dict’) from <integer> to <string>"))
-
-  (is (= (umd/describe [:vector [:sequential [:set :int]]])
-         "vector of sequence of set of integer"))
-
-  (is (= "one of <:dog = map where {:x -> <integer>} | :cat = anything> dispatched by the type of animal"
-         (umd/describe [:multi {:dispatch :type
-                                   :dispatch-description "the type of animal"}
-                           [:dog [:map [:x :int]]]
-                           [:cat :any]])))
-
-  (is (= "one of <:dog = map where {:x -> <integer>} | :cat = anything> dispatched by :type"
-         (umd/describe [:multi {:dispatch :type}
-                           [:dog [:map [:x :int]]]
-                           [:cat :any]])))
-
-  (is (= "Order which is: <Country is map where {:name -> <enum of :FI, :PO>, :neighbors (optional) -> <vector of \"Country\">} with no other keys, Burger is map where {:name -> <string>, :description (optional) -> <string>, :origin -> <nullable Country>, :price -> <integer greater than 0>}, OrderLine is map where {:burger -> <Burger>, :amount -> <integer>} with no other keys, Order is map where {:lines -> <vector of OrderLine>, :delivery -> <map where {:delivered -> <boolean>, :address -> <map where {:street -> <string>, :zip -> <integer>, :country -> <Country>}>} with no other keys>} with no other keys>"
-         (umd/describe [:schema
-                        {:registry
-                         {"Country" [:map
-                                     {:closed true}
-                                     [:name [:enum :FI :PO]]
-                                     [:neighbors
-                                      {:optional true}
-                                      [:vector [:ref "Country"]]]],
-                          "Burger" [:map
-                                    [:name string?]
-                                    [:description {:optional true} string?]
-                                    [:origin [:maybe "Country"]]
-                                    [:price pos-int?]],
-                          "OrderLine" [:map
-                                       {:closed true}
-                                       [:burger "Burger"]
-                                       [:amount int?]],
-                          "Order" [:map
-                                   {:closed true}
-                                   [:lines [:vector "OrderLine"]]
-                                   [:delivery
-                                    [:map
+  (testing "vector"
+    (is (= "vector" (umd/describe vector?)))
+    (is (= "vector of integer" (umd/describe [:vector :int]))))
+
+  (testing "string"
+    (is (= "string with length <= 5" (umd/describe [:string {:min 5}])))
+    (is (= "string with length >= 5" (umd/describe [:string {:max 5}])))
+    (is (= "string with length between 3 and 5 inclusive" (umd/describe [:string {:min 3 :max 5}]))))
+
+  (testing "function"
+    (is (= "function that takes input: [integer] and returns integer"
+           (umd/describe [:=> [:cat int?] int?]))))
+
+  (testing "map"
+    (is (= "map" (umd/describe map?)))
+    (is (= "map where {:x -> <integer>}"
+           (umd/describe [:map [:x int?]])))
+    (is (= "map where {:x (optional) -> <integer>, :y -> <boolean>}"
+           (umd/describe [:map [:x {:optional true} int?] [:y :boolean]])))
+    (is (= "map where {:x -> <integer>} with no other keys"
+           (umd/describe [:map {:closed true} [:x int?]])))
+    (is (= "map where {:x (optional) -> <integer>, :y -> <boolean>} with no other keys"
+           (umd/describe [:map {:closed true} [:x {:optional true} int?] [:y :boolean]])))
+    (is (= "map where {:j-code -> <keyword, and has length 4>}"
+           (umd/describe [:map [:j-code [:and
+                                         :keyword
+                                         [:fn {:description "has length 4"} #(= 4 (count (name %)))]]]])))
+    (is (= "map (titled: ‘dict’) from <integer> to <string>"
+           (umd/describe [:map-of {:title "dict"} :int :string]))))
+
+  (testing "compound schemas"
+    (is (= "vector of sequence of set of integer"
+           (umd/describe [:vector [:sequential [:set :int]]]))))
+
+  (testing "multi"
+    (is (= "one of <:dog = map where {:x -> <integer>} | :cat = anything> dispatched by the type of animal"
+           (umd/describe [:multi {:dispatch :type
+                                  :dispatch-description "the type of animal"}
+                          [:dog [:map [:x :int]]]
+                          [:cat :any]])))
+    (is (= "one of <:dog = map where {:x -> <integer>} | :cat = anything> dispatched by :type"
+           (umd/describe [:multi {:dispatch :type}
+                          [:dog [:map [:x :int]]]
+                          [:cat :any]]))))
+
+  (testing "schema registry"
+    (is (= "Order which is: <Country is map where {:name -> <enum of :FI, :PO>, :neighbors (optional) -> <vector of \"Country\">} with no other keys, Burger is map where {:name -> <string>, :description (optional) -> <string>, :origin -> <nullable Country>, :price -> <integer greater than 0>}, OrderLine is map where {:burger -> <Burger>, :amount -> <integer>} with no other keys, Order is map where {:lines -> <vector of OrderLine>, :delivery -> <map where {:delivered -> <boolean>, :address -> <map where {:street -> <string>, :zip -> <integer>, :country -> <Country>}>} with no other keys>} with no other keys>"
+           (umd/describe [:schema
+                          {:registry
+                           {"Country"
+                            [:map
+                             {:closed true}
+                             [:name [:enum :FI :PO]]
+                             [:neighbors
+                              {:optional true}
+                              [:vector [:ref "Country"]]]],
+                            "Burger" [:map
+                                      [:name string?]
+                                      [:description {:optional true} string?]
+                                      [:origin [:maybe "Country"]]
+                                      [:price pos-int?]],
+                            "OrderLine" [:map
+                                         {:closed true}
+                                         [:burger "Burger"]
+                                         [:amount int?]],
+                            "Order" [:map
                                      {:closed true}
-                                     [:delivered boolean?]
-                                     [:address
+                                     [:lines [:vector "OrderLine"]]
+                                     [:delivery
                                       [:map
-                                       [:street string?]
-                                       [:zip int?]
-                                       [:country "Country"]]]]]]}}
-                        "Order"])))
-
-  (is (= "ConsCell <nullable vector with exactly 2 items of type: integer, \"ConsCell\">"
-         (umd/describe [:schema
-                           {:registry {"ConsCell" [:maybe [:tuple :int [:ref "ConsCell"]]]}}
-                           "ConsCell"])))
-
-  (is (= "integer greater than or equal to 0"
-         (umd/describe [:int {:min 0}])))
-
-  (is (= "integer less than or equal to 1"
-         (umd/describe [:int {:max 1}])))
-
-  (is (= "integer between 0 and 1 inclusive"
-         (umd/describe [:int {:min 0 :max 1}]))))
+                                       {:closed true}
+                                       [:delivered boolean?]
+                                       [:address
+                                        [:map
+                                         [:street string?]
+                                         [:zip int?]
+                                         [:country "Country"]]]]]]}}
+                          "Order"])))
+    (is (= "ConsCell <nullable vector with exactly 2 items of type: integer, \"ConsCell\">"
+           (umd/describe [:schema
+                          {:registry {"ConsCell" [:maybe [:tuple :int [:ref "ConsCell"]]]}}
+                          "ConsCell"]))))
+
+  (testing "int"
+    (is (= "integer greater than or equal to 0"
+           (umd/describe [:int {:min 0}])))
+    (is (= "integer less than or equal to 1"
+           (umd/describe [:int {:max 1}])))
+    (is (= "integer between 0 and 1 inclusive"
+           (umd/describe [:int {:min 0 :max 1}]))))
+
+  (testing "repeat"
+    (is (= "repeat <integer> at least 1 time"
+           (umd/describe [:repeat {:min 1} int?])))
+    (is (= "repeat <integer> at most 7 times"
+           (umd/describe [:repeat {:max 7} int?])))
+    (is (= "repeat <integer> at least 1 time, up to 7 times"
+           (umd/describe [:repeat {:min 1 :max 7} int?])))))