User parameter value json fix (#44420)

* UserParameterValue transforms wrapped to properly escape string vals

The `json-in` and `json-out` functions used for toucan model transforms do not perform any string escaping logic. This
makes sense as we don't want to make assumptions about the shape of data flowing in/out of the db. But, this did mean
that for the User paramter values table, string values were causing an error to be logged. This doesn't break
anything, as the function will still correctly return the string, but it can clutter up logs.

So, this PR wraps all incoming values in a map and unwraps it for outgoing values.

* Add migration to wrap existing user param values with ::wrapper key

Since the in/out transform for UserParameterValues is updated, we need to migrate any existing values to have this wrapping.

* Simplify the solution to the problem.

Since we're already getting what we need from the json-in/json-out *except* that it's logging a parse error, we can
create a json-out that works the same way (tries to parse and returns the string as-is if it fails) without logging an
error at all.

* take away the arg and keywordize inside the json-out fn

* remove irrelevant comment

Signed-off-by: Adam James <adam.vermeer2@gmail.com>

* Add a little more test coverage for different value types

* faster test thanks to Dan!

* Add a comment about the data being tested

---------

Signed-off-by: Adam James <adam.vermeer2@gmail.com>

src/metabase/models/user_parameter_value.clj

 (ns metabase.models.user-parameter-value
   (:require
+   [cheshire.core :as json]
    [metabase.api.common :as api]
    [metabase.models.interface :as mi]
    [metabase.util.malli :as mu]
@@ -14,8 +15,20 @@
 (doto :model/UserParameterValue
   (derive :metabase/model))
 
+(defn- json-out
+  "A version of `metabase.models.interface/json-out` that does not log a parse error.
+  This is otherwise the same. It returns the string as expected in this case."
+  [s]
+  (if (string? s)
+    (try
+      (json/parse-string s true)
+      (catch Throwable _e
+        s))
+    s))
+
 (t2/deftransforms :model/UserParameterValue
-  {:value mi/transform-json})
+  {:value {:in  mi/json-in
+           :out json-out}})
 
 (mu/defn upsert!
   "Upsert or delete parameter value set by the user."

test/metabase/models/user_parameter_value_test.clj

@@ -6,17 +6,38 @@
    [toucan2.core :as t2]))
 
 (deftest user-parameter-value-crud-test
-  (mt/with-empty-db
-    (let [user-id   (mt/user->id :rasta)
-          upv-count (t2/count :model/UserParameterValue)]
-      (testing "Upsert creates new user parameter value entry if the param_id user_id pair doesn't exist"
-        (upv/upsert! user-id "some-param" "A")
-        (is (= (inc upv-count) (t2/count :model/UserParameterValue)))
-        (is (= "A" (:value (t2/select-one :model/UserParameterValue :user_id user-id :parameter_id "some-param")))))
-      (testing "Upsert updates user parameter value entry if the param_id user_id pair already exists"
-        (upv/upsert! user-id "some-param" "B")
-        (is (= (inc upv-count) (t2/count :model/UserParameterValue)))
-        (is (= "B" (:value (t2/select-one :model/UserParameterValue :user_id user-id :parameter_id "some-param")))))
-      (testing "Upsert deletes user parameter value entry if value is `nil`."
-        (upv/upsert! user-id "some-param" nil)
-        (is (= upv-count (count (t2/select :model/UserParameterValue))))))))
+  (let [user-id        (mt/user->id :rasta)
+        original-count (t2/count :model/UserParameterValue)
+        param-name     (str (random-uuid))
+        value!         (fn
+                         ([] (->> param-name
+                                  (t2/select-one :model/UserParameterValue :user_id user-id :parameter_id)
+                                  :value))
+                         ([v] (upv/upsert! user-id param-name v)))]
+    (try
+      ;; UserParameterValue stores `:user_id`, `:parameter_id`, and `:value`
+      ;; The value is looked up per user and param-id, and is stored as a string in the app db.
+      ;; When it's selected, we try to parse it as json, since the parameter values can be strings and lists,
+      ;; and perhaps other values like keys. We just test that these different types are succesfully added/selected
+      (doseq [[test-str value-in value-out value-update value-update-out] [["string" "A" "A" "B" "B"]
+                                                                           ["key" :A "A" :B "B"]
+                                                                           ["vectors"
+                                                                            ["A" "B" "C"] ["A" "B" "C"]
+                                                                            ["A" "B" "C" "D"] ["A" "B" "C" "D"]]]]
+        (testing (format "User Parameter Value for %s values" test-str)
+          (testing (format "Upsert creates new user parameter value entry if the param_id user_id pair doesn't exist")
+            (value! value-in)
+            (is (= (inc original-count) (t2/count :model/UserParameterValue)))
+            (is (= value-out (value!))))
+
+          (testing "Upsert updates user parameter value entry if the param_id user_id pair already exists"
+            (value! value-update)
+            (is (= (inc original-count) (t2/count :model/UserParameterValue)))
+            (is (= value-update-out (value!))))
+
+          (testing "Upsert deletes user parameter value entry if value is `nil`."
+            (value! nil)
+            (is (= original-count (count (t2/select :model/UserParameterValue))))
+            (is (= nil (value!))))))
+      (finally
+        (t2/delete! :model/UserParameterValue :parameter_id param-name)))))