Use location-id for Storage (#817)

client/sandbox/admin-sdk-express/src/index.ts

@@ -218,14 +218,40 @@ async function testDeleteFileTransactFails() {
   }
 }
 
-// testUploadFile("circle_blue.jpg", "circle_blue.jpg", "image/jpeg");
+async function testDeleteAllowedInTx(
+  src: string,
+  dest: string,
+  contentType?: string,
+) {
+  const buffer = fs.readFileSync(path.join(__dirname, src));
+  const { data } = await db.storage.uploadFile(dest, buffer, {
+    contentType: contentType,
+  });
+  const fileId = data.id;
+  const q = {
+    $files: {
+      $: {
+        where: { id: fileId },
+      },
+    },
+  };
+  const before = await query(q);
+  console.log('Before', JSON.stringify(before, null, 2));
+
+  await transact(tx['$files'][fileId].delete());
+
+  const after = await query(q);
+  console.log('After', JSON.stringify(after, null, 2));
+}
+
+// testUploadFile('circle_blue.jpg', 'circle_blue.jpg', 'image/jpeg');
 // testUploadFile("circle_blue.jpg", "circle_blue2.jpg", "image/jpeg");
 // testQueryFiles()
 // testDeleteSingleFile("circle_blue.jpg");
 // testDeleteBulkFile(["circle_blue.jpg", "circle_blue2.jpg"]);
 // testUpdateFileFails()
 // testMergeFileFails()
-// testDeleteFileTransactFails()
+// testDeleteAllowedInTx('circle_blue.jpg', 'circle_blue.jpg', 'image/jpeg');
 
 /**
  * Legacy Storage API tests (deprecated Jan 2025)

server/flags-config/instant.schema.ts

@@ -54,6 +54,7 @@ const graph = i.graph(
     }),
     "storage-migration": i.entity({
       "disableLegacy?": i.boolean(),
+      "useLocationId?": i.boolean(),
     }),
   },
   // You can define links here.

server/resources/migrations/52_add_app_files_to_sweep.down.sql

@@ -0,0 +1,7 @@
+DROP TRIGGER insert_files_to_sweep_trigger ON triples;
+DROP TRIGGER update_files_to_sweep_trigger ON triples;
+
+DROP FUNCTION create_file_to_sweep();
+DROP FUNCTION create_file_to_sweep_on_update();
+
+DROP TABLE app_files_to_sweep;

server/resources/migrations/52_add_app_files_to_sweep.up.sql

@@ -0,0 +1,57 @@
+CREATE TABLE app_files_to_sweep (
+    id uuid PRIMARY KEY DEFAULT gen_random_uuid(),
+    app_id uuid NOT NULL,
+    location_id text NOT NULL,
+    created_at timestamp NOT NULL DEFAULT NOW(),
+    UNIQUE (app_id, location_id)
+);
+
+-- Whenever we delete file triples we want to ensure they are scheduled for
+-- deletion in S3.
+CREATE FUNCTION create_file_to_sweep()
+RETURNS trigger AS $$
+BEGIN
+    -- This should match the attr_id for $files.location-id
+    IF OLD.attr_id = '96653230-13ff-ffff-2a34-b40fffffffff' THEN
+        INSERT INTO app_files_to_sweep (app_id, location_id)
+        VALUES (
+            OLD.app_id,
+            OLD.value #>> '{}'  -- Extract from JSON
+        )
+        ON CONFLICT DO NOTHING;
+    END IF;
+    RETURN OLD;
+END;
+$$ LANGUAGE plpgsql;
+
+CREATE TRIGGER insert_files_to_sweep_trigger
+    AFTER DELETE ON triples
+    FOR EACH ROW
+    EXECUTE FUNCTION create_file_to_sweep();
+
+-- Whenever we upload a file with the same path we create a new location-id for
+-- it. We add the old location-id to the sweep table to ensure that the old file
+-- is cleaned up
+CREATE FUNCTION create_file_to_sweep_on_update()
+RETURNS trigger AS $$
+BEGIN
+    -- Check if we're updating from the file location attribute
+    IF OLD.attr_id = '96653230-13ff-ffff-2a34-b40fffffffff' THEN
+        -- Only schedule for deletion if the value actually changed
+        IF OLD.value != NEW.value THEN
+            INSERT INTO app_files_to_sweep (app_id, location_id)
+            VALUES (
+                OLD.app_id,
+                OLD.value #>> '{}'  -- Extract from JSON
+            )
+            ON CONFLICT DO NOTHING;
+        END IF;
+    END IF;
+    RETURN NEW;
+END;
+$$ LANGUAGE plpgsql;
+
+CREATE TRIGGER update_files_to_sweep_trigger
+    AFTER UPDATE ON triples
+    FOR EACH ROW
+    EXECUTE FUNCTION create_file_to_sweep_on_update();

server/src/instant/admin/routes.clj

@@ -24,6 +24,7 @@
    [instant.model.schema :as schema-model]
    [clojure.string :as string]
    [instant.storage.coordinator :as storage-coordinator]
+   [instant.storage.s3 :as instant-s3]
    [clojure.walk :as w])
   (:import
    (java.util UUID)))
@@ -453,19 +454,22 @@
 
 ;; Legacy StorageFile format that was only used by the list() endpoint
 (defn legacy-storage-file-format
-  [app-id object-metadata]
-  {:key (str app-id "/" (:path object-metadata))
-   :name (:path object-metadata)
-   :size (:content-length object-metadata)
-   :etag (:etag object-metadata)
-   :last_modified (:last-modified object-metadata)})
+  [app-id file]
+  (let [object-key (if (instant-s3/migrating?)
+                     (instant-s3/->path-object-key app-id (:path file))
+                     (instant-s3/->object-key app-id (:location-id file)))]
+    {:key object-key
+     :name (:path file)
+     :size (:size file)
+     :etag nil
+     :last_modified nil}))
 
 (defn files-get [req]
   (let [{app-id :app_id} (req->admin-token! req)
         res (query-post (assoc-in req [:body :query] {:$files {}}))
         files (get-in res [:body "$files"])
         data (map (fn [item]
-                    (->> (get item "metadata")
+                    (->> item
                          w/keywordize-keys
                          (legacy-storage-file-format app-id)))
                   files)]

server/src/instant/db/instaql.clj

@@ -5,7 +5,6 @@
    [clojure.string :as string]
    [clojure.walk :as walk]
    [honey.sql :as hsql]
-   [instant.flags :as flags]
    [instant.data.constants :refer [zeneca-app-id]]
    [instant.data.resolvers :as resolvers]
    [instant.db.cel :as cel]
@@ -881,15 +880,23 @@
 
 (defn compute-$files-triples [{:keys [app-id]} join-rows]
   (when-let [[eid _ _ t] (ffirst join-rows)]
-    (when-let [path-value (some-> (find-row-by-ident-name join-rows $system-attrs "$files" "path")
-                                  first
-                                  (nth 2))]
-      (let [url-aid (attr-model/resolve-attr-id $system-attrs "$files" "url")
-            {:keys [disableLegacy?]} (flags/storage-migration)
-            url (if disableLegacy?
-                  (instant-s3/create-signed-download-url! app-id path-value)
-                  (instant-s3/create-legacy-signed-download-url! app-id path-value))]
-        [[[eid url-aid url t]]]))))
+    (let [path (some-> (find-row-by-ident-name
+                        join-rows
+                        $system-attrs
+                        "$files"
+                        "path")
+                       first
+                       (nth 2))
+          location-id (some-> (find-row-by-ident-name
+                               join-rows
+                               $system-attrs
+                               "$files"
+                               "location-id")
+                              first
+                              (nth 2))
+          url-aid (attr-model/resolve-attr-id $system-attrs "$files" "url")
+          url (instant-s3/create-signed-download-url! app-id path location-id)]
+      [[[eid url-aid url t]]])))
 
 (def compute-triples-handler
   {"$files" compute-$files-triples})

server/src/instant/db/transaction.clj

@@ -136,18 +136,8 @@
      [op t]
      [{:message (format "update or merge is not allowed on $files in transact.")}])))
 
-(defn prevent-$files-deletes! [op tx-steps]
-  (doseq [t tx-steps
-          :let [[_op _eid etype] t]
-          :when (= etype "$files")]
-    (ex/throw-validation-err!
-     :tx-step
-     [op t]
-     [{:message (format "delete is not allowed on $files in transact.")}])))
-
 (defn prevent-$files-updates
-  "With the exception of linking/unlinking, we prevent most updates unless it's
-  gone through an explicitly allowed code path"
+  "Files support delete, link/unlink, but not update or merge"
   [attrs grouped-tx-steps opts]
   (when (not (:allow-$files-update? opts))
     (doseq [batch grouped-tx-steps
@@ -156,9 +146,6 @@
         (:add-triple :deep-merge-triple :retract-triple)
         (prevent-$files-add-retract! op attrs tx-steps)
 
-        :delete-entity
-        (prevent-$files-deletes! op tx-steps)
-
         nil))))
 
 (defn resolve-lookups