Munge setting names (#15640)

* Watch for munge collisions

* Always have to check for munge collisions

was thinking only in the case where munging made a difference did we
have to check, but if the first one defined is foo? -> foo as the
munge, defining foo later will cause a collision

* Prettier fix

frontend/test/__runner__/backend.js

@@ -68,7 +68,7 @@ export const BackendResource = createSharedResource("BackendResource", {
               (process.env["MB_EDITION"] === "ee" &&
                 process.env["ENTERPRISE_TOKEN"]) ||
               undefined,
-            "MB_FIELD_FILTER_OPERATORS_ENABLED?": "true",
+            MB_FIELD_FILTER_OPERATORS_ENABLED: "true",
           },
           stdio:
             process.env["DISABLE_LOGGING"] ||

project.clj

@@ -237,7 +237,7 @@
 
     :env
     {:mb-run-mode       "dev"
-     :mb-field-filter-operators-enabled? "true"
+     :mb-field-filter-operators-enabled "true"
      :mb-test-setting-1 "ABCDEFG"}
 
     :jvm-opts
@@ -331,7 +331,7 @@
     {:mb-run-mode     "test"
      :mb-db-in-memory "true"
      :mb-jetty-join   "false"
-     :mb-field-filter-operators-enabled? "true"
+     :mb-field-filter-operators-enabled "true"
      :mb-api-key      "test-api-key"
      ;; use a random port between 3001 and 3501. That way if you run multiple sets of tests at the same time locally
      ;; they won't stomp on each other

src/metabase/models/setting.clj

@@ -77,6 +77,7 @@
 
 (def ^:private SettingDefinition
   {:name        s/Keyword
+   :munged-name s/Str
    :namespace   s/Symbol
    :description s/Any            ; description is validated via the macro, not schema
    :default     s/Any
@@ -148,18 +149,28 @@
   (setting-name [this]
     (name this)))
 
+(defn- munge-setting-name
+  "Munge names so that they are legal for bash. Only allows for alphanumeric characters,  underscores, and hyphens."
+  [setting-nm]
+  (str/replace (name setting-nm) #"[^a-zA-Z0-9_-]*" ""))
+
 (defn- env-var-name
   "Get the env var corresponding to `setting-definition-or-name`.
    (This is used primarily for documentation purposes)."
   ^String [setting-definition-or-name]
-  (str "MB_" (str/upper-case (str/replace (setting-name setting-definition-or-name) "-" "_"))))
+  (str "MB_" (-> (setting-name setting-definition-or-name)
+                 munge-setting-name
+                 (str/replace "-" "_")
+                 str/upper-case)))
 
 (defn env-var-value
   "Get the value of `setting-definition-or-name` from the corresponding env var, if any.
-   The name of the Setting is converted to uppercase and dashes to underscores;
-   for example, a setting named `default-domain` can be set with the env var `MB_DEFAULT_DOMAIN`."
+   The name of the Setting is converted to uppercase and dashes to underscores; for example, a setting named
+  `default-domain` can be set with the env var `MB_DEFAULT_DOMAIN`. Note that this strips out characters that are not
+  legal for shells. Setting `foo-bar?` will expect to find the key `:mb-foo-bar` which will be sourced from the
+  environment variable `MB_FOO_BAR`."
   ^String [setting-definition-or-name]
-  (let [v (env/env (keyword (str "mb-" (setting-name setting-definition-or-name))))]
+  (let [v (env/env (keyword (str "mb-" (munge-setting-name (setting-name setting-definition-or-name)))))]
     (when (seq v)
       v)))
 
@@ -443,28 +454,37 @@
   "Register a new Setting with a map of `SettingDefinition` attributes. Returns the map it was passed. This is used
   internally be `defsetting`; you shouldn't need to use it yourself."
   [{setting-name :name, setting-ns :namespace, setting-type :type, default :default, :as setting}]
-  (u/prog1 (let [setting-type         (s/validate Type (or setting-type :string))]
-             (merge
-              {:name        setting-name
-               :namespace   setting-ns
-               :description nil
-               :type        setting-type
-               :default     default
-               :on-change   nil
-               :getter      (partial (default-getter-for-type setting-type) setting-name)
-               :setter      (partial (default-setter-for-type setting-type) setting-name)
-               :tag         (default-tag-for-type setting-type)
-               :visibility  :admin
-               :sensitive?  false
-               :cache?      true}
-              (dissoc setting :name :type :default)))
-    (s/validate SettingDefinition <>)
-    ;; eastwood complains about (setting-name @registered-settings) for shadowing the function `setting-name`
-    (when-let [registered-setting (clojure.core/get @registered-settings setting-name)]
-      (when (not= setting-ns (:namespace registered-setting))
-        (throw (ex-info (tru "Setting {0} already registered in {1}" setting-name (:namespace registered-setting))
-                        {:existing-setting (dissoc registered-setting :on-change :getter :setter)}))))
-    (swap! registered-settings assoc setting-name <>)))
+  (let [munged-name (munge-setting-name (name setting-name))]
+    (u/prog1 (let [setting-type (s/validate Type (or setting-type :string))]
+               (merge
+                {:name        setting-name
+                 :munged-name munged-name
+                 :namespace   setting-ns
+                 :description nil
+                 :type        setting-type
+                 :default     default
+                 :on-change   nil
+                 :getter      (partial (default-getter-for-type setting-type) setting-name)
+                 :setter      (partial (default-setter-for-type setting-type) setting-name)
+                 :tag         (default-tag-for-type setting-type)
+                 :visibility  :admin
+                 :sensitive?  false
+                 :cache?      true}
+                (dissoc setting :name :type :default)))
+      (s/validate SettingDefinition <>)
+      ;; eastwood complains about (setting-name @registered-settings) for shadowing the function `setting-name`
+      (when-let [registered-setting (clojure.core/get @registered-settings setting-name)]
+        (when (not= setting-ns (:namespace registered-setting))
+          (throw (ex-info (tru "Setting {0} already registered in {1}" setting-name (:namespace registered-setting))
+                          {:existing-setting (dissoc registered-setting :on-change :getter :setter)}))))
+      (when-let [same-munge (first (filter (comp #{munged-name} :munged-name)
+                                           (vals @registered-settings)))]
+        (when (not= setting-name (:name same-munge)) ;; redefinitions are fine
+          (throw (ex-info (tru "Setting names in would collide: {0} and {1}"
+                                setting-name (:name same-munge))
+                          {:existing-setting (dissoc same-munge :on-change :getter :setter)
+                           :new-setting      (dissoc <> :on-change :getter :setter)}))))
+      (swap! registered-settings assoc setting-name <>))))
 
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                                defsetting macro                                                |

test/metabase/models/setting_test.clj

 (ns metabase.models.setting-test
   (:require [clojure.test :refer :all]
+            [medley.core :as m]
             [metabase.models.setting :as setting :refer [defsetting Setting]]
             [metabase.models.setting.cache :as cache]
             [metabase.test :as mt]
@@ -532,6 +533,7 @@
                    :cache? true,
                    :default nil,
                    :name :foo,
+                   :munged-name "foo"
                    :type :string,
                    :sensitive? false,
                    :tag 'java.lang.String,
@@ -541,3 +543,52 @@
           (is (= (str "Setting :foo already registered in " current-ns)
                  (ex-message e))))
         (finally (in-ns current-ns))))))
+
+(defsetting ^:private test-setting-with-question-mark?
+  "Test setting - this only shows up in dev (6)"
+  :visibility :internal)
+
+(deftest munged-setting-name-test
+  (testing "Only valid characters used for environment lookup"
+    (is (nil? (test-setting-with-question-mark?)))
+    ;; note now question mark on the environmental setting
+    (with-redefs [environ.core/env {:mb-test-setting-with-question-mark "resolved"}]
+      (binding [setting/*disable-cache* false]
+        (is (= "resolved" (test-setting-with-question-mark?))))))
+  (testing "Setting a setting that would munge the same throws an error"
+    (is (= {:existing-setting
+            {:name :test-setting-with-question-mark?
+             :munged-name "test-setting-with-question-mark"}
+            :new-setting
+            {:name :test-setting-with-question-mark????
+             :munged-name "test-setting-with-question-mark"}}
+           (m/map-vals #(select-keys % [:name :munged-name])
+                       (try (defsetting ^:private test-setting-with-question-mark????
+                              "Test setting - this only shows up in dev (6)"
+                              :visibility :internal)
+                            (catch Exception e (ex-data e)))))))
+  (testing "Munge collision on first definition"
+    (defsetting ^:private test-setting-normal
+      "Test setting - this only shows up in dev (6)"
+      :visibility :internal)
+    (is (= {:existing-setting {:name :test-setting-normal, :munged-name "test-setting-normal"},
+            :new-setting {:name :test-setting-normal??, :munged-name "test-setting-normal"}}
+           (m/map-vals #(select-keys % [:name :munged-name])
+                       (try (defsetting ^:private test-setting-normal??
+                              "Test setting - this only shows up in dev (6)"
+                              :visibility :internal)
+                            (catch Exception e (ex-data e)))))))
+  (testing "Munge collision on second definition"
+    (defsetting ^:private test-setting-normal-1??
+      "Test setting - this only shows up in dev (6)"
+      :visibility :internal)
+    (is (= {:new-setting {:munged-name "test-setting-normal-1", :name :test-setting-normal-1},
+             :existing-setting {:munged-name "test-setting-normal-1", :name :test-setting-normal-1??}}
+           (m/map-vals #(select-keys % [:name :munged-name])
+                       (try (defsetting ^:private test-setting-normal-1
+                              "Test setting - this only shows up in dev (6)"
+                              :visibility :internal)
+                            (catch Exception e (ex-data e)))))))
+  (testing "Removes characters not-compliant with shells"
+    (is (= "aa1aa-b2b_cc3c"
+           (#'setting/munge-setting-name "aa1'aa@#?-b2@b_cc'3?c?")))))