Keep result-metadata of native queries if possible

Part of #37009.

src/metabase/api/card.clj

@@ -517,7 +517,8 @@
           timed-out?            (= port metadata-timeout)
           card-updates          (cond-> card-updates
                                   (not timed-out?)
-                                  (assoc :result_metadata fresh-metadata))]
+                                  (assoc :result_metadata fresh-metadata
+                                         :verified-result-metadata? true))]
       (u/prog1 (-> (card/update-card! {:card-before-update card-before-update
                                        :card-updates       card-updates
                                        :actor              @api/*current-user*})

src/metabase/models/card.clj

@@ -547,16 +547,22 @@
     (parameter-card/upsert-or-delete-from-parameters! "card" (:id card) (:parameters card))))
 
 (t2/define-before-update :model/Card
-  [card]
+  [{:keys [verified-result-metadata?] :as card}]
   ;; remove all the unchanged keys from the map, except for `:id`, so the functions below can do the right thing since
   ;; they were written pre-Toucan 2 and don't know about [[t2/changes]]...
   ;;
   ;; We have to convert this to a plain map rather than a Toucan 2 instance at this point to work around upstream bug
   ;; https://github.com/camsaul/toucan2/issues/145 .
-  (-> (into {:id (:id card)} (t2/changes card))
+  (-> (into {:id (:id card)} (t2/changes (dissoc card :verified-result-metadata?)))
       ensure-type-and-dataset-are-consistent
       maybe-normalize-query
-      populate-result-metadata
+      ;; If we have fresh result_metadata, we don't have to populate it anew. When result_metadata doesn't
+      ;; change for a native query, populate-result-metadata removes it (set to nil) unless prevented by the
+      ;; verified-result-metadata? flag (see #37009).
+      (cond-> #_changes
+        (or (empty? (:result_metadata card))
+            (not verified-result-metadata?))
+        populate-result-metadata)
       pre-update
       populate-query-fields
       maybe-populate-initially-published-at
@@ -857,12 +863,14 @@ saved later when it is ready."
                                            :text                (tru "Unverified due to edit")}))
       ;; ok, now save the Card
       (t2/update! Card (:id card-before-update)
-                  ;; `collection_id` and `description` can be `nil` (in order to unset them). Other values should only be
-                  ;; modified if they're passed in as non-nil
+                  ;; `collection_id` and `description` can be `nil` (in order to unset them).
+                  ;; Other values should only be modified if they're passed in as non-nil
                   (u/select-keys-when card-updates
-                                      :present #{:collection_id :collection_position :description :cache_ttl :dataset :type}
-                                      :non-nil #{:dataset_query :display :name :visualization_settings :archived :enable_embedding
-                                                 :parameters :parameter_mappings :embedding_params :result_metadata :collection_preview}))))
+                                      :present #{:collection_id :collection_position :description :cache_ttl :dataset
+                                                 :type}
+                                      :non-nil #{:dataset_query :display :name :visualization_settings :archived
+                                                 :enable_embedding :parameters :parameter_mappings :embedding_params
+                                                 :result_metadata :collection_preview :verified-result-metadata?}))))
   ;; Fetch the updated Card from the DB
   (let [card (t2/select-one Card :id (:id card-before-update))]
     (delete-alerts-if-needed! :old-card card-before-update, :new-card card, :actor actor)

test/metabase/api/card_test.clj

@@ -50,9 +50,9 @@
    [toucan2.core :as t2]
    [toucan2.tools.with-temp :as t2.with-temp])
   (:import
-    (java.io ByteArrayInputStream)
-    (org.apache.poi.ss.usermodel DataFormatter)
-    (org.quartz.impl StdSchedulerFactory)))
+   (java.io ByteArrayInputStream)
+   (org.apache.poi.ss.usermodel DataFormatter)
+   (org.quartz.impl StdSchedulerFactory)))
 
 (set! *warn-on-reflection* true)
 
@@ -916,6 +916,23 @@
             (is (= ["UPDATED" "UPDATED"]
                    (map :display_name (:result_metadata updated))))))))))
 
+(deftest updating-native-card-preserves-metadata
+  (testing "A trivial change in a native question should not remove result_metadata (#37009)"
+    (let [query (to-native (updating-card-updates-metadata-query))
+          updated-query (update-in query [:native :query] str/replace #"\d+$" "1000")]
+      ;; sanity check
+      (is (not= query updated-query))
+      ;; tthe actual test
+      (mt/with-model-cleanup [:model/Card]
+        (let [card (mt/user-http-request :rasta :post 200 "card"
+                                         (card-with-name-and-query "card-name"
+                                                                   query))
+              metadata (:result_metadata card)]
+          (is (some? metadata))
+          (let [updated (mt/user-http-request :rasta :put 200 (str "card/" (:id card))
+                                              {:dataset_query updated-query})]
+            (is (= metadata (:result_metadata updated)))))))))
+
 (deftest fetch-results-metadata-test
   (testing "Check that the generated query to fetch the query result metadata includes user information in the generated query"
     (mt/with-non-admin-groups-no-root-collection-perms

test/metabase/models/card_test.clj

@@ -272,7 +272,16 @@
         (f {:dataset_query (mt/native-query {:native "SELECT * FROM VENUES"})}
            (fn [metadata]
              (is (= nil
-                    metadata))))))))
+                    metadata)))))
+      (testing "Shouldn't remove verified result metadata from native queries (#37009)"
+        (let [metadata (qp.preprocess/query->expected-cols (mt/mbql-query checkins))]
+          (f (cond-> {:dataset_query (mt/native-query {:native "SELECT * FROM CHECKINS"})
+                      :result_metadata metadata}
+               (= creating-or-updating "updating")
+               (assoc :verified-result-metadata? true))
+             (fn [new-metadata]
+               (is (= (mt/derecordize metadata)
+                      (mt/derecordize new-metadata))))))))))
 
 ;; this is a separate function so we can use the same tests for DashboardCards as well
 (defn test-visualization-settings-normalization [f]