Last Used Param Value doesn't fill in with default value if cleared (#46468)

* Last Used Param Value doesn't fill in with default value if cleared

Fixes #46368

The Last used parameter value is cleared when the frontend sends `nil` as the parameter's value. This works except in
cases where a user clears a parameter with a default value. In this case, we want to indicate that the parameter is
cleared, so we store `nil` in the appdb in such a case.

* Update src/metabase/query_processor/dashboard.clj

Co-authored-by: metamben <103100869+metamben@users.noreply.github.com>

* add assertion showing that 'nil' with default is still stored

---------

Co-authored-by: metamben <103100869+metamben@users.noreply.github.com>

src/metabase/models/user_parameter_value.clj

@@ -34,22 +34,22 @@
   "Upsert or delete parameter value set by the user."
   [user-id      :- ms/PositiveInt
    dashboard-id :- ms/PositiveInt
-   parameter-id :- ms/NonBlankString
-   value]
-  (if value
-    (t2/with-transaction [_conn]
-        (or (pos? (t2/update! :model/UserParameterValue {:user_id      user-id
-                                                         :dashboard_id dashboard-id
-                                                         :parameter_id parameter-id}
-                              {:value value}))
-            (t2/insert! :model/UserParameterValue {:user_id      user-id
-                                                   :dashboard_id dashboard-id
-                                                   :parameter_id parameter-id
-                                                   :value        value})))
-    (t2/delete! :model/UserParameterValue
-                :user_id      user-id
-                :dashboard_id dashboard-id
-                :parameter_id parameter-id)))
+   parameter]
+  (let [{:keys [value default] parameter-id :id} parameter]
+    (if (or value default)
+        (t2/with-transaction [_conn]
+          (or (pos? (t2/update! :model/UserParameterValue {:user_id      user-id
+                                                           :dashboard_id dashboard-id
+                                                           :parameter_id parameter-id}
+                                {:value value}))
+              (t2/insert! :model/UserParameterValue {:user_id      user-id
+                                                     :dashboard_id dashboard-id
+                                                     :parameter_id parameter-id
+                                                     :value        value})))
+        (t2/delete! :model/UserParameterValue
+                    :user_id      user-id
+                    :dashboard_id dashboard-id
+                    :parameter_id parameter-id))))
 
 ;; hydration
 

src/metabase/query_processor/dashboard.clj

@@ -126,9 +126,6 @@
    request-params :- [:maybe [:sequential :map]]]
   (log/tracef "Resolving Dashboard %d Card %d query request parameters" dashboard-id card-id)
   (let [request-params            (mbql.normalize/normalize-fragment [:parameters] request-params)
-        ;; ignore default values in request params as well. (#20516)
-        request-params            (for [param request-params]
-                                    (dissoc param :default))
         dashboard                 (-> (t2/select-one Dashboard :id dashboard-id)
                                       (t2/hydrate :resolved-params)
                                       (api/check-404))
@@ -142,12 +139,13 @@
                                         (map (fn [[param-id param]]
                                                [param-id (dissoc param :default)]))
                                         (:resolved-params dashboard))
-        request-param-id->param   (into {} (map (juxt :id identity)) request-params)
+        ;; ignore default values in request params as well. (#20516)
+        request-param-id->param   (into {} (map (juxt :id #(dissoc % :default))) request-params)
         merged-parameters         (vals (merge (dashboard-param-defaults dashboard-param-id->param card-id)
                                                request-param-id->param))]
     (when-let [user-id api/*current-user-id*]
-      (doseq [{:keys [id value]} request-params]
-        (user-parameter-value/upsert! user-id dashboard-id id value)))
+      (doseq [parameter request-params]
+        (user-parameter-value/upsert! user-id dashboard-id parameter)))
     (log/tracef "Dashboard parameters:\n%s\nRequest parameters:\n%s\nMerged:\n%s"
                 (u/pprint-to-str (update-vals dashboard-param-id->param
                                               (fn [param]

test/metabase/api/dashboard_test.clj

@@ -397,14 +397,13 @@
     (mt/with-column-remappings [orders.user_id people.name]
       (binding [api/*current-user-permissions-set* (atom #{"/"})]
         (t2.with-temp/with-temp
-          [Dashboard {dashboard-a-id :id} {:name             "Test Dashboard"
-                                           :creator_id       (mt/user->id :crowberto)
-                                           :embedding_params {:id "enabled"}
-                                           :parameters       [{:name "Name", :slug "name", :id "a" :type :string/contains}]}
-           Dashboard {dashboard-b-id :id} {:name             "Test Dashboard"
-                                           :creator_id       (mt/user->id :crowberto)
-                                           :embedding_params {:id "enabled"}
-                                           :parameters       [{:name "Name", :slug "name", :id "a" :type :string/contains}]}
+          [Dashboard {dashboard-a-id :id} {:name       "Test Dashboard"
+                                           :creator_id (mt/user->id :crowberto)
+                                           :parameters [{:name    "Name", :slug "name", :id "a" :type :string/contains
+                                                         :default ["default_value"]}]}
+           Dashboard {dashboard-b-id :id} {:name       "Test Dashboard"
+                                           :creator_id (mt/user->id :crowberto)
+                                           :parameters [{:name "Name", :slug "name", :id "a" :type :string/contains}]}
            Card {card-id :id} {:database_id   (mt/id)
                                :query_type    :native
                                :name          "test question"
@@ -416,8 +415,7 @@
                                                                    :display-name "name"
                                                                    :type         :dimension
                                                                    :dimension    [:field (mt/id :people :name) nil]
-                                                                   :widget-type  :string/contains
-                                                                   :default      nil}}}
+                                                                   :widget-type  :string/contains}}}
                                                :database (mt/id)}}
            DashboardCard {dashcard-a-id :id} {:parameter_mappings [{:parameter_id "a", :card_id card-id, :target [:dimension [:template-tag "id"]]}]
                                               :card_id            card-id
@@ -436,7 +434,17 @@
             (is (= {:dashboard-a {:a ["new value"]}
                     :dashboard-b {:a ["initial value"]}}
                    {:dashboard-a (:last_used_param_values (mt/user-http-request :rasta :get 200 (format "dashboard/%d" dashboard-a-id)))
-                    :dashboard-b (:last_used_param_values (mt/user-http-request :rasta :get 200 (format "dashboard/%d" dashboard-b-id)))}))))))))
+                    :dashboard-b (:last_used_param_values (mt/user-http-request :rasta :get 200 (format "dashboard/%d" dashboard-b-id)))})))
+          (testing "If a User unsets a parameter's value, the default is NOT used."
+            ;; api request mimicking a user clearing parameter value, and no default exists
+            (is (some? (mt/user-http-request :rasta :post (format "dashboard/%d/dashcard/%s/card/%s/query" dashboard-a-id dashcard-a-id card-id)
+                                             {:parameters [{:id "a" :value nil}]})))
+            (is (= {}
+                   (:last_used_param_values (mt/user-http-request :rasta :get 200 (format "dashboard/%d" dashboard-a-id)))))
+            (is (some? (mt/user-http-request :rasta :post (format "dashboard/%d/dashcard/%s/card/%s/query" dashboard-a-id dashcard-a-id card-id)
+                                             {:parameters [{:id "a" :value nil :default ["default value"]}]})))
+            (is (= {:a nil}
+                   (:last_used_param_values (mt/user-http-request :rasta :get 200 (format "dashboard/%d" dashboard-a-id)))))))))))
 
 (deftest fetch-dashboard-test
   (testing "GET /api/dashboard/:id"

test/metabase/models/user_parameter_value_test.clj

@@ -14,7 +14,9 @@
                            (->> param-name
                                 (t2/select-one :model/UserParameterValue :user_id user-id :parameter_id)
                                 :value))
-          value!         (fn value! [v] (upv/upsert! user-id dashboard-id param-name v))]
+          value!         (fn value!
+                           ([v] (value! v nil))
+                           ([v default] (upv/upsert! user-id dashboard-id {:id param-name :value v :default default})))]
       (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.
@@ -40,5 +42,14 @@
               (value! nil)
               (is (= original-count (count (t2/select :model/UserParameterValue))))
               (is (= nil (value-fn))))))
+        (testing "Parameters with default values can store `nil` (#46368)"
+          (testing (format "Upsert creates new user parameter value entry if the param_id user_id pair doesn't exist")
+            (value! 10 5)
+            (is (= (inc original-count) (t2/count :model/UserParameterValue)))
+            (is (= 10 (value-fn))))
+          (testing "Upsert deletes user parameter value entry if value is `nil` only when there is no default value."
+            (value! nil 5)
+            (is (= (inc original-count) (count (t2/select :model/UserParameterValue))))
+            (is (= nil (value-fn)))))
         (finally
           (t2/delete! :model/UserParameterValue :parameter_id param-name))))))