From c3d95cba6a4cd752460dc9ba84f352e289ea84f7 Mon Sep 17 00:00:00 2001 From: Bryce Date: Sat, 15 Aug 2026 15:39:04 -0700 Subject: [PATCH] feat(square): make the re-key complete, and measure whether it is MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The migration had been run over a chosen subset — the ten clients that share a Square location today, within a date window. Measuring the whole database showed how partial that was: charges 259,763 scoped · 14,241,890 legacy · 2,122,161 with no owner refunds, payouts, shifts: complete So 1.6% of charges carried a client-scoped key. The importer's tolerance of both key schemes was not easing a transition, it was holding the system together. unscoped-report counts, per entity type, how many keys are scoped, how many are still legacy, and how many have no owner to scope by. That is the gate: while legacy is above zero the database is in a mixed state and a stray unscoped record can still be adopted by whichever client imports it first. At zero, the fallback lookup in square.core3/existing-id can be removed and the guarantee becomes structural rather than conventional. migrate-all! runs it over every order rather than a subset. Splitting has to be driven from orders because a payment's rightful owner is whichever order refers to it — and nine client pairs contended in the past without sharing a location today, so a migration scoped to the current configuration misses all of them. Two changes were needed to run at that scale. The split no longer remembers every charge it has seen; whether a charge is claimed is read from the charge, which needs a map of the whole table otherwise. And claiming a charge now records the claiming client on it, which is how a later order recovers the Square id from an already-scoped key — the regression test caught the omission immediately. Re-running is now a true no-op rather than a repeated rename. Co-Authored-By: Claude Opus 5 --- .../jobs/rekey_square_external_ids.clj | 109 +++++++++++++++--- .../jobs/rekey_square_external_ids_test.clj | 4 +- 2 files changed, 97 insertions(+), 16 deletions(-) diff --git a/src/clj/auto_ap/jobs/rekey_square_external_ids.clj b/src/clj/auto_ap/jobs/rekey_square_external_ids.clj index 2203c9b8..7984b893 100644 --- a/src/clj/auto_ap/jobs/rekey_square_external_ids.clj +++ b/src/clj/auto_ap/jobs/rekey_square_external_ids.clj @@ -138,9 +138,17 @@ :else old))) (defn- charge-plan-for-order - "What one order needs doing to its charges: `:keep` where the charge is already this order's - alone, `:clone` where it is shared with another order and this order needs its own copy." - [db order-eid seen] + "What one order needs doing to its charges. + + `:keep` — no other order has claimed this charge yet, so this order takes it and it is renamed + in place. `:clone` — another order already owns it, so this order needs its own copy. + + Whether a charge is already claimed is read from the charge itself: an unclaimed one still + carries the bare `square/charge/` form. Deciding it that way rather than by remembering + every charge seen so far is what lets this run across all 16 million of them — the alternative + needs a map of the entire table in memory. `batch-seen` covers only the orders inside one + transaction, where the database snapshot cannot yet show a claim made moments earlier." + [db order-eid batch-seen] (let [oe (dc/entity db order-eid) code (:client/code (:sales-order/client oe)) loc (:sales-order/location oe)] @@ -148,12 +156,15 @@ (for [d (dc/datoms db :eavt order-eid :sales-order/charges) :let [charge (:v d) old (:v (first (dc/datoms db :eavt charge :charge/external-id)))] - :when old] - (let [raw (or (get @seen charge) (raw-square-id db charge old)) - new-key (str charge-prefix code "-" loc "-" raw)] - {:order order-eid :charge charge :new-key new-key :raw raw - :client (:db/id (:sales-order/client oe)) :location loc - :action (if (contains? @seen charge) :clone :keep)}))))) + :when old + :let [raw (raw-square-id db charge old) + new-key (str charge-prefix code "-" loc "-" raw) + unclaimed? (and (= old (str charge-prefix raw)) + (not (contains? @batch-seen charge)))] + :when (not= old new-key)] + {:order order-eid :charge charge :new-key new-key :raw raw + :client (:db/id (:sales-order/client oe)) :location loc + :action (if unclaimed? :keep :clone)})))) (defn split-and-rekey-charges! "Gives every order its own charge entity, keyed by that order's client and location. @@ -172,19 +183,24 @@ `orders` is the collection of order entity ids to process — typically every order belonging to the clients that share, or have ever shared, a Square location." [orders batch-size] - (let [seen (atom {}) - cloned (atom 0) + (let [cloned (atom 0) rekeyed (atom 0)] (doseq [batch (partition-all batch-size orders)] (let [db (dc/db conn) + batch-seen (atom #{}) tx (doall (for [o batch - plan (charge-plan-for-order db o seen) + plan (charge-plan-for-order db o batch-seen) :let [{:keys [charge new-key action client location]} plan] tx-item (if (= :keep action) - (do (swap! seen assoc charge (:raw plan)) + (do (swap! batch-seen conj charge) (swap! rekeyed inc) - [{:db/id charge :charge/external-id new-key}]) + ;; record who claimed it: the owner is how a later order + ;; recovers the Square id from an already-scoped key + [{:db/id charge + :charge/external-id new-key + :charge/client client + :charge/location location}]) (let [ent (dc/entity db charge) copy (reduce (fn [m a] (if-some [v (get ent a)] (assoc m a (if (map? v) (:db/id v) v)) @@ -213,6 +229,71 @@ (filter (fn [c] (> (reduce (fn [n _] (inc n)) 0 (dc/datoms db :vaet c :sales-order/charges)) 1))) count)) +(def scoped-attrs + "Every Square-imported entity whose key must carry its client, with the prefix and where to + read the owner from." + [{:attr :sales-refund/external-id :prefix refund-prefix + :client :sales-refund/client :location :sales-refund/location} + {:attr :charge/external-id :prefix charge-prefix + :client :charge/client :location :charge/location} + {:attr :expected-deposit/external-id :prefix deposit-prefix + :client :expected-deposit/client :location :expected-deposit/location} + {:attr :cash-drawer-shift/external-id :prefix shift-prefix + :client :cash-drawer-shift/client :location :cash-drawer-shift/location}]) + +(defn unscoped-report + "Counts, per entity type, how many keys are already client-scoped, how many still carry the + legacy unscoped form, and how many have no owner to scope by. + + This is the completeness gate. The importer tolerates both key schemes on purpose, so that the + change can be deployed before the migration finishes — but that tolerance is a transition, not + a resting place. While `:legacy` is above zero the database is in a mixed state and a stray + unscoped record can still be adopted by whichever client imports it first. Once every count + reads zero the fallback lookup in `square.core3/existing-id` can be removed and the guarantee + becomes structural rather than conventional." + [db] + (into {} + (for [{:keys [attr prefix client location]} scoped-attrs] + [attr (reduce (fn [acc d] + (let [e (dc/entity db (:e d)) + code (:client/code (client e)) + loc (location e) + scoped (when (and code loc) (str prefix code "-" loc "-"))] + (cond + (and scoped (.startsWith ^String (:v d) ^String scoped)) (update acc :scoped inc) + (nil? scoped) (update acc :no-owner inc) + :else (update acc :legacy inc)))) + {:scoped 0 :legacy 0 :no-owner 0} + (dc/datoms db :aevt attr))]))) + +(defn all-order-ids + "Every sales order in the database, streamed." + [db] + (map :e (dc/datoms db :aevt :sales-order/external-id))) + +(defn migrate-all! + "The complete migration, over the whole database rather than a chosen subset. + + Splitting is driven from orders, because a payment's rightful owner is whichever order refers + to it — so every order has to be walked, not merely the clients that share a location today. + Nine client pairs contended in the past and no longer share one; their records are still mixed, + and a migration scoped to the current configuration would miss every one of them. + + Returns the split counts and the completeness report, which should read zero legacy across the + board when this finishes." + [batch-size] + (let [split (split-and-rekey-charges! (all-order-ids (dc/db conn)) batch-size)] + (doseq [{:keys [attr prefix]} scoped-attrs + :when (not= attr :charge/external-id)] + (let [p (plan (dc/db conn) attr prefix)] + (when-let [c (seq (collisions (:new-keys p)))] + (throw (ex-info "two entities would take the same key" {:attr attr :collisions (count c)}))) + (migrate! attr (:new-keys p) batch-size))) + ;; charges no order refers to — payout stubs — are scoped from the deposit that holds them + (let [p (plan (dc/db conn) :charge/external-id charge-prefix)] + (when (seq (:new-keys p)) (migrate! :charge/external-id (:new-keys p) batch-size))) + {:split split :completeness (unscoped-report (dc/db conn))})) + (defn counts "Entity totals, for the before/after assertion that is this migration's real safety net: if either number moves, the re-key created duplicates instead of updating in place." diff --git a/test/clj/auto_ap/jobs/rekey_square_external_ids_test.clj b/test/clj/auto_ap/jobs/rekey_square_external_ids_test.clj index 78a459e4..64595f13 100644 --- a/test/clj/auto_ap/jobs/rekey_square_external_ids_test.clj +++ b/test/clj/auto_ap/jobs/rekey_square_external_ids_test.clj @@ -104,8 +104,8 @@ (let [{:keys [order-a order-b]} (two-orders-sharing-one-charge)] (sut/split-and-rekey-charges! [order-a order-b] 100) (let [after-first (charge-count)] - (is (= {:rekeyed 2 :cloned 0} (sut/split-and-rekey-charges! [order-a order-b] 100)) - "each order now owns its charge outright, so both are simple re-keys") + (is (= {:rekeyed 0 :cloned 0} (sut/split-and-rekey-charges! [order-a order-b] 100)) + "every charge already carries the key its order expects, so there is nothing to do") (is (= after-first (charge-count)) "and no further entities appear"))))) (deftest clone-is-not-double-scoped-across-batches