Fix bad error message; better trs/tru validation (#12885)

src/metabase/public_settings.clj

@@ -153,8 +153,10 @@
                            global-max-caching-kb))
                (throw (IllegalArgumentException.
                        (str
-                        (deferred-tru "Failed setting `query-caching-max-kb` to {0}." new-value)
-                        (deferred-tru "Values greater than {1} are not allowed." global-max-caching-kb)))))
+                        (tru "Failed setting `query-caching-max-kb` to {0}." new-value)
+                        " "
+                        (tru "Values greater than {0} ({1}) are not allowed."
+                             global-max-caching-kb (u/format-bytes (* global-max-caching-kb 1024)))))))
              (setting/set-integer! :query-caching-max-kb new-value)))
 
 (defsetting query-caching-max-ttl
@@ -215,8 +217,8 @@
                          u/lower-case-en)))
 
 (defn remove-public-uuid-if-public-sharing-is-disabled
-  "If public sharing is *disabled* and OBJECT has a `:public_uuid`, remove it so people don't try to use it (since it
-   won't work). Intended for use as part of a `post-select` implementation for Cards and Dashboards."
+  "If public sharing is *disabled* and `object` has a `:public_uuid`, remove it so people don't try to use it (since it
+  won't work). Intended for use as part of a `post-select` implementation for Cards and Dashboards."
   [object]
   (if (and (:public_uuid object)
            (not (enable-public-sharing)))

src/metabase/util/i18n.clj

 (ns metabase.util.i18n
   "i18n functionality."
   (:require [cheshire.generate :as json-gen]
+            [clojure
+             [string :as str]
+             [walk :as walk]]
             [clojure.tools.logging :as log]
-            [clojure.walk :as walk]
             [metabase.util.i18n.impl :as impl]
             [potemkin :as p]
             [potemkin.types :as p.types]
@@ -93,14 +95,21 @@
   "Make sure the right number of args were passed to `trs`/`tru` and related forms during macro expansion."
   [format-string args]
   (assert (string? format-string)
-    "The first arg to (deferred-)trs/tru must be a String! `gettext` does not eval Clojure files.")
-  (let [message-format    (MessageFormat. format-string)
-        expected-num-args (count (.getFormats message-format))
-        actual-num-args   (count args)]
+          "The first arg to (deferred-)trs/tru must be a String! `gettext` does not eval Clojure files.")
+  (let [message-format             (MessageFormat. format-string)
+        ;; number of {n} placeholders in format string including any you may have skipped. e.g. "{0} {2}" -> 3
+        expected-num-args-by-index (count (.getFormatsByArgumentIndex message-format))
+        ;; number of {n} placeholders in format string *not* including ones you make have skipped. e.g. "{0} {2}" -> 2
+        expected-num-args          (count (.getFormats message-format))
+        actual-num-args            (count args)]
+    (assert (= expected-num-args expected-num-args-by-index)
+            (format "(deferred-)trs/tru with format string %s is missing some {} placeholders. Expected %s. Did you skip any?"
+                    (pr-str (.toPattern message-format))
+                    (str/join ", " (map (partial format "{%d}") (range expected-num-args-by-index)))))
     (assert (= expected-num-args actual-num-args)
-      (format (str "(deferred-)trs/tru with format string \"%s\" expects %d args, got %d. "
-                   "Did you forget to escape a single quote?")
-              (.toPattern message-format) expected-num-args actual-num-args))))
+            (str (format (str "(deferred-)trs/tru with format string %s expects %d args, got %d.")
+                         (pr-str (.toPattern message-format)) expected-num-args actual-num-args)
+                 " Did you forget to escape a single quote?"))))
 
 (defmacro deferred-tru
   "Similar to `tru` but creates a `UserLocalizedString` instance so that conversion to the correct locale can be delayed

test/metabase/public_settings_test.clj

@@ -111,11 +111,19 @@
       (is (= "HOST"
              (tru "Host"))))))
 
-(deftest max-cache-entry
-  (testing "Make sure Max Cache Entry Size can be set via with a string value, which is what comes back from the API (#9143)"
+(deftest query-caching-max-kb-test
+  (testing (str "Make sure Max Cache Entry Size can be set via with a string value, which is what comes back from the "
+                "API (#9143)")
     (mt/discard-setting-changes [query-caching-max-kb]
       (is (= "1000"
-             (public-settings/query-caching-max-kb "1000"))))))
+             (public-settings/query-caching-max-kb "1000")))))
+
+  (testing "query-caching-max-kb should throw an error if you try to put in a huge value"
+    (mt/discard-setting-changes [query-caching-max-kb]
+      (is (thrown?
+           IllegalArgumentException
+           #"Values greater than 204,800 \(200\.0 MB\) are not allowed"
+           (public-settings/query-caching-max-kb (* 1024 1024)))))))
 
 (deftest site-locale-test
   (testing "site-locale Setting"

test/metabase/util/i18n_test.clj

 (ns metabase.util.i18n-test
-  (:require [clojure.test :refer :all]
+  (:require [clojure
+             [test :refer :all]
+             [walk :as walk]]
             [metabase.test :as mt]
             [metabase.util.i18n :as i18n]))
 
@@ -53,3 +55,42 @@
          (i18n/localized-string? (i18n/deferred-trs "WOW"))))
   (is (= false
          (i18n/localized-string? "WOW"))))
+
+(deftest validate-number-of-args-test
+  (testing "`trs` and `tru` should validate that the are being called with the correct number of args\n"
+    (testing "not enough args"
+      (is (thrown?
+           clojure.lang.Compiler$CompilerException
+           (walk/macroexpand-all `(i18n/trs "{0} {1}" 0))))
+      (is (thrown-with-msg?
+           AssertionError
+           #"expects 2 args, got 1"
+           (#'i18n/validate-number-of-args "{0} {1}" [0]))))
+
+    (testing "too many args"
+      (is (thrown?
+           clojure.lang.Compiler$CompilerException
+           (walk/macroexpand-all `(i18n/trs "{0} {1}" 0 1 2))))
+      (is (thrown-with-msg?
+           AssertionError
+           #"expects 2 args, got 3"
+           (#'i18n/validate-number-of-args "{0} {1}" [0 1 2]))))
+
+    (testing "Missing format specifiers (e.g. {1} but no {0})"
+      (testing "num args match num specifiers"
+        (is (thrown?
+             clojure.lang.Compiler$CompilerException
+             (walk/macroexpand-all `(i18n/trs "{1}" 0))))
+        (is (thrown-with-msg?
+             AssertionError
+             #"missing some \{\} placeholders\. Expected \{0\}, \{1\}"
+             (#'i18n/validate-number-of-args "{1}" [0]))))
+
+      (testing "num args match num specifiers if none were missing"
+        (is (thrown?
+             clojure.lang.Compiler$CompilerException
+             (walk/macroexpand-all `(i18n/trs "{1}" 0 1))))
+        (is (thrown-with-msg?
+             AssertionError
+             #"missing some \{\} placeholders\. Expected \{0\}, \{1\}"
+             (#'i18n/validate-number-of-args "{1}" [0 1])))))))