Fix a footgun in before-update

src/toucan2/tools/before_update.clj

@@ -49,20 +49,22 @@
      ;; After going back and forth on this I've concluded that it's probably best to just realize the entire row here.
      ;; There are a lot of situations where we don't need to do this, but it means we have to step on eggshells
      ;; everywhere else in order to make things work nicely. Maybe we can revisit this in the future.
-     (let [row         (realize/realize row)
-           row         (merge row changes)
-           row         (before-update model row)
+     (let [realized-row (realize/realize row)
+           row          (merge realized-row changes)
+           row          (before-update model row)
            ;; if the `before-update` method returned a plain map then consider that to be the changes.
            ;; `protocols/changes` will return `nil` for non-instances. TODO -- does that behavior make sense? Clearly,
            ;; it's easy to use wrong -- it took me hours to figure out why something was working and that I needed to
            ;; make this change :sad:
-           row-changes (if (instance/instance? row)
-                         (protocols/changes row)
-                         row)]
+           row-changes  (if (instance/instance? row)
+                          (protocols/changes row)
+                          row)
+           pks          (model/primary-key-values-map model realized-row)]
        (log/tracef "The following values have changed: %s" changes)
+       (assert (seq pks) "No primary key(s) were found on the realized row")
        (cond-> changes->pks
-         (seq row-changes) (update row-changes (fn [pks]
-                                                 (conj (set pks) (model/primary-key-values-map model row)))))))))
+         (seq row-changes) (update row-changes (fn [pks*]
+                                                 (conj (set pks*) pks))))))))
 
 (defn- fetch-changes->pk-maps [model {:keys [changes], :as parsed-args} resolved-query]
   (not-empty

test/toucan2/tools/after_update_test.clj

@@ -137,7 +137,7 @@
                  ::venues.exception.db-land      (case (test/current-db-type)
                                                    :postgres #"ERROR: column \"venue_name\" of relation \"venues\" does not exist"
                                                    :h2       #"Column \"VENUE_NAME\" not found"
-                                                   :mariadb  #"Unknown column 'venue_name' in 'field list'"))
+                                                   :mariadb  #"Unknown column 'venue_name' in 'SET'"))
                (update/update! model 2 {:category "store", :name "My Store"})))
           (testing "\nShould be done inside a transaction"
             (is (= [(instance/instance model

test/toucan2/tools/before_update_test.clj

@@ -65,6 +65,34 @@
                 :updated-at (LocalDateTime/parse "2021-06-09T15:18:00")}
                (select/select-one ::venues.update-updated-at 1)))))))
 
+(derive ::venues.with-removed-primary-key ::venues.before-update)
+
+(before-update/define-before-update ::venues.with-removed-primary-key
+  [venue]
+  (dissoc venue :id))
+
+(deftest ^:synchronized missing-id-test
+  (testing "Removing the ID in the `before-update` method isn't catastrophic"
+    (test/with-discarded-table-changes :venues
+      (test.track-realized/with-realized-columns [_realized-columns]
+        ;; The bad case tested here only happened in very precise conditions:
+        ;; - your `updated-at` method's result did not have a primary key
+        ;; - your "where" matches multiple rows
+        ;; - *part* of your update will change all the matched rows
+        ;; - another part of your update will change a subset of matched rows
+        ;;
+        ;; In this test:
+        ;; - we're matching rows 1 and 3
+        ;; - `:updated-at` will change for both rows 1 and 3
+        ;; - `:category` will only change for row 1 (3 is already a "store")
+        (let [row-2 {:id 2 :name "Ho's Tavern" :category "bar"}]
+          (testing "sanity check"
+            (is (= row-2 (select/select-one [::venues.with-removed-primary-key :id :name :category] :id 2))))
+          (testing "we only update 2 rows"
+            (is (= 2 (update/update! ::venues.with-removed-primary-key :id [:in [1 3]] {:category "store" :updated-at (LocalDateTime/parse "2024-11-22T00:00")}))))
+          (testing "the other row is unchanged"
+            (is (= row-2 (select/select-one [::venues.with-removed-primary-key :id :name :category] :id 2)))))))))
+
 (derive ::venues.discard-category-change ::venues.before-update)
 
 (before-update/define-before-update ::venues.discard-category-change