PUT /api/database/:id can now update Database-local Setting values (#22689)

src/metabase/api/database.clj

@@ -693,7 +693,7 @@
 (api/defendpoint PUT "/:id"
   "Update a `Database`."
   [id :as {{:keys [name engine details is_full_sync is_on_demand description caveats points_of_interest schedules
-                   auto_run_queries refingerprint cache_ttl]} :body}]
+                   auto_run_queries refingerprint cache_ttl settings]} :body}]
   {name               (s/maybe su/NonBlankString)
    engine             (s/maybe DBEngineString)
    refingerprint      (s/maybe s/Bool)
@@ -703,7 +703,8 @@
    caveats            (s/maybe s/Str)   ; whether someone sets these to blank strings
    points_of_interest (s/maybe s/Str)
    auto_run_queries   (s/maybe s/Bool)
-   cache_ttl          (s/maybe su/IntGreaterThanZero)}
+   cache_ttl          (s/maybe su/IntGreaterThanZero)
+   settings           (s/maybe su/Map)}
   ;; TODO - ensure that custom schedules and let-user-control-scheduling go in lockstep
   (let [existing-database (api/write-check (Database id))
         details           (driver.u/db-details-client->server engine details)
@@ -745,7 +746,15 @@
 
                                                    ;; if user is controlling schedules
                                                    (:let-user-control-scheduling details)
-                                                   (sync.schedules/schedule-map->cron-strings (sync.schedules/scheduling schedules))))))
+                                                   (sync.schedules/schedule-map->cron-strings (sync.schedules/scheduling schedules))
+
+                                                   ;; upsert settings with a PATCH-style update. `nil` key means unset
+                                                   ;; the Setting.
+                                                   (seq settings)
+                                                   {:settings (into {}
+                                                                    (remove (fn [[_k v]] (nil? v)))
+                                                                    (merge (:settings existing-database)
+                                                                           settings))}))))
         ;; do nothing in the case that user is not in control of
         ;; scheduling. leave them as they are in the db
 

test/metabase/api/database_test.clj

@@ -1230,3 +1230,67 @@
                      (mt/user-http-request :crowberto :get 200 d)
                      (:details d)
                      (select-keys d [:password-source :password-value]))))))))
+
+(deftest database-local-settings-come-back-with-database-test
+  (testing "Database-local Settings should come back with"
+    (mt/with-temp-vals-in-db Database (mt/id) {:settings {:max-results-bare-rows 1337}}
+      ;; only returned for admin users at this point in time. See #22683 -- issue to return them for non-admins as well.
+      (doseq [{:keys [endpoint response]} [{:endpoint "GET /api/database/:id"
+                                            :response (fn []
+                                                        (mt/user-http-request :crowberto :get 200 (format "database/%d" (mt/id))))}
+                                           {:endpoint "GET /api/database"
+                                            :response (fn []
+                                                        (some
+                                                         (fn [database]
+                                                           (when (= (:id database) (mt/id))
+                                                             database))
+                                                         (:data (mt/user-http-request :crowberto :get 200 "database"))))}]]
+        (testing endpoint
+          (let [{:keys [settings], :as response} (response)]
+            (testing (format "\nresponse = %s" (u/pprint-to-str response))
+              (is (map? response))
+              (is (partial= {:max-results-bare-rows 1337}
+                            settings)))))))))
+
+(deftest admins-set-database-local-settings-test
+  (testing "Admins should be allowed to update Database-local Settings (#19409)"
+    (mt/with-temp-vals-in-db Database (mt/id) {:settings nil}
+      (letfn [(settings []
+                (db/select-one-field :settings Database :id (mt/id)))
+              (set-settings! [m]
+                (mt/user-http-request :crowberto :put 200 (format "database/%d" (mt/id))
+                                      {:settings m}))]
+        (testing "Should initially be nil"
+          (is (nil? (settings))))
+        (testing "Set initial value"
+          (testing "response"
+            (is (partial= {:settings {:max-results-bare-rows 1337}}
+                          (set-settings! {:max-results-bare-rows 1337}))))
+          (testing "App DB"
+            (is (= {:max-results-bare-rows 1337}
+                   (settings)))))
+        (testing "Setting a different value should not affect anything not specified (PATCH-style update)"
+          (testing "response"
+            (is (partial= {:settings {:max-results-bare-rows   1337
+                                      :database-enable-actions true}}
+                          (set-settings! {:database-enable-actions true}))))
+          (testing "App DB"
+            (is (= {:max-results-bare-rows   1337
+                    :database-enable-actions true}
+                   (settings)))))
+        (testing "Update existing value"
+          (testing "response"
+            (is (partial= {:settings {:max-results-bare-rows   1337
+                                      :database-enable-actions false}}
+                          (set-settings! {:database-enable-actions false}))))
+          (testing "App DB"
+            (is (= {:max-results-bare-rows   1337
+                    :database-enable-actions false}
+                   (settings)))))
+        (testing "Unset a value"
+          (testing "response"
+            (is (partial= {:settings {:database-enable-actions false}}
+                          (set-settings! {:max-results-bare-rows nil}))))
+          (testing "App DB"
+            (is (= {:database-enable-actions false}
+                   (settings)))))))))