From 334322e6dbac6d17f476f424b3f5c80c7a60942c Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Fri, 11 Sep 2026 17:25:03 +0000 Subject: [PATCH] :recycle: Simplify storage target helpers and refusal logging Drop the bang from the pure storage-routes validators, inline the refusal log into park-unresolvable in both GC tasks, and read the storage target from object metadata only. Tests now use the production object shape. AI-assisted-by: muse-spark-1.3-contributor --- backend/src/app/storage/config.clj | 8 +-- backend/src/app/storage/gc_deleted.clj | 13 ++--- backend/src/app/storage/pending_gc.clj | 13 ++--- backend/src/app/storage/s3.clj | 3 +- backend/test/backend_tests/storage_test.clj | 60 +++++++++------------ 5 files changed, 37 insertions(+), 60 deletions(-) diff --git a/backend/src/app/storage/config.clj b/backend/src/app/storage/config.clj index 22c4f72eb6..b8dc49a879 100644 --- a/backend/src/app/storage/config.clj +++ b/backend/src/app/storage/config.clj @@ -65,7 +65,7 @@ :path (str path) :cause cause)))) -(defn- assert-backend-s3! +(defn- assert-backend-s3 "The routing only makes sense on top of the S3 backend." [] (let [backend (or (sto/get-legacy-backend) @@ -77,7 +77,7 @@ :hint "storage routes file requires the :s3 backend" :backend (keyword backend))))) -(defn- validate! +(defn- validate [data path] (when-not (valid-file? data) (ex/raise :type :validation @@ -135,8 +135,8 @@ configuration key is unset." [] (if-let [path (not-empty (cf/get :objects-storage-s3-routes-file))] - (let [data (-> (read-file path) (validate! path))] - (assert-backend-s3!) + (let [data (-> (read-file path) (validate path))] + (assert-backend-s3) {:targets (normalize-targets (:targets data)) :routes (:routes data)}) {:targets nil diff --git a/backend/src/app/storage/gc_deleted.clj b/backend/src/app/storage/gc_deleted.clj index 141e964c4c..ed5a0a8d20 100644 --- a/backend/src/app/storage/gc_deleted.clj +++ b/backend/src/app/storage/gc_deleted.clj @@ -78,14 +78,6 @@ (let [ids (db/create-array conn "uuid" ids)] (db/exec-one! conn [sql:delete-give-up ids max-attempts]))) -(defn- log-refusal! - "Indirection over the error log so tests can capture the refusal payload." - [backend-id target ids] - (l/err :hint "storage target is not configured, deletion refused" - :backend (name backend-id) - :target target - :ids (mapv str ids))) - (def ^:private sql:defer-unresolvable "UPDATE storage_object SET deleted_at = NOW() + INTERVAL '1 day' @@ -97,7 +89,10 @@ selection window without being deleted and without counting a deletion attempt (they are therefore never subject to the give-up window)." [conn backend-id target ids] - (log-refusal! backend-id target ids) + (l/err :hint "storage target is not configured, deletion refused" + :backend (name backend-id) + :target target + :ids (mapv str ids)) (let [ids (db/create-array conn "uuid" ids)] (db/exec-one! conn [sql:defer-unresolvable ids]))) diff --git a/backend/src/app/storage/pending_gc.clj b/backend/src/app/storage/pending_gc.clj index 3e2f8b9ca2..d9d5645781 100644 --- a/backend/src/app/storage/pending_gc.clj +++ b/backend/src/app/storage/pending_gc.clj @@ -38,14 +38,6 @@ (def ^:private sql:delete-pending-sobject "DELETE FROM storage_object WHERE id = ? AND status = 'pending'") -(defn- log-refusal! - "Indirection over the error log so tests can capture the refusal payload." - [backend-id target ids] - (l/err :hint "storage target is not configured, deletion refused" - :backend (name backend-id) - :target target - :ids (mapv str ids))) - (def ^:private sql:park-unresolvable "UPDATE storage_object SET deleted_at = NOW() + INTERVAL '1 day' @@ -57,7 +49,10 @@ misconfiguration and pushes `deleted_at` forward so the rows are excluded from the next selection without being deleted." [conn backend-id target ids] - (log-refusal! backend-id target ids) + (l/err :hint "storage target is not configured, deletion refused" + :backend (name backend-id) + :target target + :ids (mapv str ids)) (let [ids (db/create-array conn "uuid" ids)] (db/exec-one! conn [sql:park-unresolvable ids]))) diff --git a/backend/src/app/storage/s3.clj b/backend/src/app/storage/s3.clj index 9e350a0641..4b4c8ac08c 100644 --- a/backend/src/app/storage/s3.clj +++ b/backend/src/app/storage/s3.clj @@ -209,8 +209,7 @@ "Returns the configured target id stored on the object, or the default target name when the object predates the routing feature." [backend object] - (or (:storage-target object) - (some-> (meta object) :storage-target) + (or (some-> (meta object) :storage-target) (name (::default-target backend)))) (defn- resolve-target-by-id diff --git a/backend/test/backend_tests/storage_test.clj b/backend/test/backend_tests/storage_test.clj index 21a77c9111..c73c0ed16a 100644 --- a/backend/test/backend_tests/storage_test.clj +++ b/backend/test/backend_tests/storage_test.clj @@ -897,8 +897,8 @@ (t/deftest s3-get-object-url-rejects-unknown-target (let [backend (fake-s3-backend-with-targets) ex (try - (impl/get-object-url backend {:id (uuid/next) - :storage-target "ghost"} {}) + (impl/get-object-url backend (with-meta {:id (uuid/next)} + {:storage-target "ghost"}) {}) nil (catch Throwable cause cause))] (t/is (some? ex)) @@ -907,9 +907,8 @@ (t/deftest s3-get-object-data-rejects-unknown-target (let [backend (fake-s3-backend-with-targets) ex (try - (impl/get-object-data backend {:id (uuid/next) - :size 1 - :storage-target "ghost"}) + (impl/get-object-data backend (with-meta {:id (uuid/next) :size 1} + {:storage-target "ghost"})) nil (catch Throwable cause cause))] (t/is (some? ex)) @@ -1183,8 +1182,7 @@ (t/deftest gc-deleted-refuses-unknown-target-and-keeps-row (let [storage (storage-with-s3-targets) cfg {::db/pool th/*pool* ::sto/storage storage} - id (uuid/next) - logged (atom nil)] + id (uuid/next)] (th/db-exec! ["insert into storage_object (id, size, backend, metadata, deleted_at, status) values (?, 1, 's3', ?, ?, 'valid')" id @@ -1192,20 +1190,16 @@ (ct/in-past {:minutes 1})]) (with-mocks [mock {:target 'app.storage.impl/del-objects-in-bulk :return (fn [_ _ _] #{})}] - (with-redefs [app.storage.gc-deleted/log-refusal! - (fn [backend-id target ids] - (reset! logged [backend-id target (vec ids)]))] - (let [result (#'sto.gc-deleted/clean-deleted! cfg) - row (th/db-exec-one! - ["select status, deleted_at, deletion_attempts + (let [result (#'sto.gc-deleted/clean-deleted! cfg) + row (th/db-exec-one! + ["select status, deleted_at, deletion_attempts from storage_object where id = ?" id])] - (t/is (= 0 (:deleted result))) - (t/is (= 1 (:parked result))) - (t/is (= 0 (:call-count @mock))) - (t/is (= "valid" (:status row))) - (t/is (ct/is-after? (:deleted-at row) (ct/now))) - (t/is (= 0 (:deletion-attempts row))) - (t/is (= [:s3 "ghost" [id]] @logged))))))) + (t/is (= 0 (:deleted result))) + (t/is (= 1 (:parked result))) + (t/is (= 0 (:call-count @mock))) + (t/is (= "valid" (:status row))) + (t/is (ct/is-after? (:deleted-at row) (ct/now))) + (t/is (= 0 (:deletion-attempts row))))))) (t/deftest gc-deleted-give-up-not-applied-to-unknown-target (let [storage (storage-with-s3-targets) @@ -1218,8 +1212,7 @@ (ct/in-past {:minutes 1})]) (with-mocks [_mock {:target 'app.storage.impl/del-objects-in-bulk :return (fn [_ _ _] #{})}] - (with-redefs [app.storage.gc-deleted/log-refusal! (fn [& _] nil)] - (#'sto.gc-deleted/clean-deleted! cfg))) + (#'sto.gc-deleted/clean-deleted! cfg)) (let [row (th/db-exec-one! ["select count(*) from storage_object where id = ?" id])] (t/is (= 1 (:count row)))))) @@ -1278,7 +1271,6 @@ (let [storage (storage-with-s3-targets) cfg {::db/pool th/*pool* ::sto/storage storage} id (uuid/next) - logged (atom nil) captured (atom nil)] (th/db-exec! ["insert into storage_object (id, size, backend, metadata, created_at, status) values (?, 1, 's3', ?, ?, 'pending')" @@ -1289,19 +1281,15 @@ :return (fn [_ object] (reset! captured object) nil)}] - (with-redefs [app.storage.pending-gc/log-refusal! - (fn [backend-id target ids] - (reset! logged [backend-id target (vec ids)]))] - (let [result (#'sto.pending-gc/process! cfg) - row (th/db-exec-one! - ["select status, deleted_at from storage_object where id = ?" id])] - (t/is (= 0 (:processed result))) - (t/is (= 1 (:parked result))) - (t/is (= 0 (:call-count @mock))) - (t/is (nil? @captured)) - (t/is (= "pending" (:status row))) - (t/is (ct/is-after? (:deleted-at row) (ct/now))) - (t/is (= [:s3 "ghost" [id]] @logged))))))) + (let [result (#'sto.pending-gc/process! cfg) + row (th/db-exec-one! + ["select status, deleted_at from storage_object where id = ?" id])] + (t/is (= 0 (:processed result))) + (t/is (= 1 (:parked result))) + (t/is (= 0 (:call-count @mock))) + (t/is (nil? @captured)) + (t/is (= "pending" (:status row))) + (t/is (ct/is-after? (:deleted-at row) (ct/now))))))) (t/deftest gc-deleted-legacy-rows-delete-from-default-target (let [storage (storage-with-s3-targets)