Post-merge fixes from the #150/#151/#152 reviews

functions/src/__tests__/metadata-derived-files.test.js

@@ -33,9 +33,13 @@ describe('rawDataPath', () => {
     expect(rawDataPath('abc123.json')).toBe('data/raw/abc123.json');
   });
 
-  it('flattens researcher subfolders', () => {
-    expect(rawDataPath('condition-A/abc123.json')).toBe('data/raw/abc123.json');
-    expect(rawDataPath('a/b/abc123.json')).toBe('data/raw/abc123.json');
+  it('encodes researcher subfolders into the flattened name', () => {
+    expect(rawDataPath('condition-A/abc123.json')).toBe('data/raw/condition-A-abc123.json');
+    expect(rawDataPath('a/b/abc123.json')).toBe('data/raw/a-b-abc123.json');
+  });
+
+  it('keeps two same-leaf-name submissions from different subfolders collision-free', () => {
+    expect(rawDataPath('condition-A/data.json')).not.toBe(rawDataPath('condition-B/data.json'));
   });
 });
 
@@ -96,13 +100,26 @@ describe('buildDerivedFiles', () => {
     expect(rows[0]).toContain('hello');
   });
 
-  it('flattens researcher subfolders into the same flat data/ layout', () => {
+  it('encodes researcher subfolders into distinct, flat data/ filenames', () => {
     const flat = buildDerivedFiles('abc123.json', source());
     const nested = buildDerivedFiles('session1/abc123.json', source());
 
-    expect(nested.map((f) => f.filename)).toEqual(flat.map((f) => f.filename));
-    for (const file of nested) {
-      expect(file.filename).not.toContain('session1');
+    expect(nested.map((f) => f.filename)).not.toEqual(flat.map((f) => f.filename));
+    for (const file of nested.filter((f) => f.filename !== '.psychds-ignore')) {
+      expect(file.filename).toContain('session1');
+      expect(file.filename).not.toContain('/session1/');
+    }
+  });
+
+  it('two submissions with the same leaf name in different subfolders no longer collide', () => {
+    const a = buildDerivedFiles('condition-A/data.json', source());
+    const b = buildDerivedFiles('condition-B/data.json', source());
+
+    const aNames = new Set(a.map((f) => f.filename));
+    const bNames = new Set(b.map((f) => f.filename));
+    for (const name of aNames) {
+      if (name === '.psychds-ignore') continue; // shared file, not a collision
+      expect(bNames.has(name)).toBe(false);
     }
   });
 

functions/src/__tests__/metadata-derived-upload-emulator.test.js

@@ -0,0 +1,103 @@
+/**
+ * @jest-environment node
+ */
+
+process.env.FIRESTORE_EMULATOR_HOST = "localhost:8080";
+process.env.FIREBASE_STORAGE_EMULATOR_HOST = "localhost:9199";
+process.env.GCLOUD_PROJECT = "datapipe-test";
+process.env.FIREBASE_CONFIG = JSON.stringify({
+  projectId: "datapipe-test",
+  storageBucket: "datapipe-test.appspot.com",
+});
+
+const { initializeApp, getApp } = require("firebase-admin/app");
+const { getFirestore } = require("firebase-admin/firestore");
+const { uploadDerivedFiles } = require("../../lib/metadata-derived-upload.js");
+
+let app;
+try {
+  app = getApp();
+} catch {
+  app = initializeApp();
+}
+
+jest.setTimeout(30000);
+
+const db = getFirestore(app);
+
+const ROOT = "https://files.osf.io/v1/resources/abc/providers/osfstorage/";
+const TOKEN = "test-token";
+
+const move = (name) => `https://files.osf.io/v1/folder/${name}/`;
+
+const listing = (folderNames) => Promise.resolve({
+  ok: true,
+  json: () => Promise.resolve({
+    data: folderNames.map((n) => ({ attributes: { name: n, kind: "folder" }, links: { move: move(n) } })),
+  }),
+});
+const fileOk = () => Promise.resolve({ status: 201 });
+const fileFail = (status, statusText) => Promise.resolve({
+  status,
+  statusText,
+  headers: { get: () => null },
+});
+
+const experimentID = "derived-upload-test-exp";
+const target = { experimentID, owner: "derived-upload-test-user", osfFilesLink: ROOT };
+
+beforeEach(() => {
+  global.fetch = jest.fn();
+});
+
+afterEach(async () => {
+  const docs = await db.collection("uploadQueue").where("experimentID", "==", experimentID).get();
+  const batch = db.batch();
+  docs.forEach((doc) => batch.delete(doc.ref));
+  await batch.commit();
+});
+
+describe("uploadDerivedFiles", () => {
+  it("resolves data/ once and queues exactly the file that failed", async () => {
+    const files = [
+      { filename: "data/subject-abc_data.csv", content: "main" },
+      { filename: "data/measure-x_data.csv", content: "sidecar" },
+      { filename: ".psychds-ignore", content: "ignore" },
+    ];
+
+    fetch
+      .mockReturnValueOnce(listing(["data"])) // resolve data/ once, up front
+      .mockReturnValueOnce(fileOk())           // main CSV upload
+      .mockReturnValueOnce(fileFail(500, "Server Error")) // sidecar CSV upload fails
+      .mockReturnValueOnce(fileOk());          // .psychds-ignore upload
+
+    await uploadDerivedFiles(files, target, TOKEN);
+
+    // Only one "data/"-folder resolution call, despite two files living under data/.
+    const dataMetaCalls = fetch.mock.calls.filter(([url]) => url === `${ROOT}?meta=`);
+    expect(dataMetaCalls).toHaveLength(1);
+
+    const docs = await db.collection("uploadQueue").where("experimentID", "==", experimentID).get();
+    expect(docs.docs).toHaveLength(1);
+    expect(docs.docs[0].data().filename).toBe("data/measure-x_data.csv");
+  });
+
+  it("does not throw when the up-front data/ resolution fails — every file is queued instead", async () => {
+    const files = [
+      { filename: "data/subject-abc_data.csv", content: "main" },
+      { filename: "data/measure-x_data.csv", content: "sidecar" },
+      { filename: ".psychds-ignore", content: "ignore" },
+    ];
+
+    // OSF is unreachable: the up-front resolveFolder rejects, and so does every
+    // per-file re-walk. The best-effort contract requires this never throws and
+    // every file ends up queued for retry rather than lost.
+    fetch.mockRejectedValue(new Error("network down"));
+
+    await expect(uploadDerivedFiles(files, target, TOKEN)).resolves.toBeUndefined();
+
+    const docs = await db.collection("uploadQueue").where("experimentID", "==", experimentID).get();
+    const queued = docs.docs.map((d) => d.data().filename).sort();
+    expect(queued).toEqual([".psychds-ignore", "data/measure-x_data.csv", "data/subject-abc_data.csv"]);
+  });
+});

functions/src/__tests__/metadata-production.test.js

@@ -164,4 +164,13 @@ describe('produceMetadata', () => {
     expect(objectRows).toHaveLength(1);
     expect(objectRows[0]).toMatchObject({ trial_index: 0, 'response.Q0': 'hello', 'response.Q1': 'world' });
   });
+
+  it('throws a clean error instead of a TypeError when the trial array is empty', async () => {
+    await expect(produceMetadata('[]')).rejects.toThrow('Invalid metadata generated');
+  });
+
+  it('throws a clean error for a bare JSON object instead of letting it reach generate()', async () => {
+    await expect(produceMetadata('{"trial_type": "html-keyboard-response"}'))
+      .rejects.toThrow('Data must be an array of trials');
+  });
 });

functions/src/__tests__/put-file-osf.test.js

@@ -131,6 +131,30 @@ describe('putFileOSF', () => {
     ]);
   });
 
+  it('uploads straight into a pre-resolved startUrl, skipping the walk entirely', async () => {
+    fetch.mockReturnValueOnce(fileOk());
+
+    const result = await putFileOSF(ROOT, TOKEN, 'data', 'abc123.json', move('data'));
+
+    expect(result.success).toBe(true);
+    expect(callUrls()).toEqual([`${move('data')}?kind=file&name=abc123.json`]);
+  });
+
+  it('walks remaining segments starting from a pre-resolved startUrl', async () => {
+    fetch
+      .mockReturnValueOnce(listing([]))
+      .mockReturnValueOnce(folderCreated('raw'))
+      .mockReturnValueOnce(fileOk());
+
+    await putFileOSF(ROOT, TOKEN, '{}', 'raw/abc123.json', move('data'));
+
+    expect(callUrls()).toEqual([
+      `${move('data')}?meta=`,
+      `${move('data')}?kind=folder&name=raw`,
+      `${move('raw')}?kind=file&name=abc123.json`,
+    ]);
+  });
+
   it('returns the OSF error when the file upload fails', async () => {
     fetch.mockReturnValueOnce(fileFail(409, 'Conflict'));
 

functions/src/__tests__/scheduled-pending-recovery-emulator.test.js

@@ -0,0 +1,83 @@
+/**
+ * @jest-environment node
+ */
+
+process.env.FIRESTORE_EMULATOR_HOST = "localhost:8080";
+process.env.FIREBASE_STORAGE_EMULATOR_HOST = "localhost:9199";
+process.env.GCLOUD_PROJECT = "datapipe-test";
+// app.js (imported transitively by the lib modules below) calls
+// initializeApp() with no args, which reads the default bucket from
+// FIREBASE_CONFIG — set it before those imports run so storage.bucket()
+// resolves to the same emulator bucket this test uses directly.
+process.env.FIREBASE_CONFIG = JSON.stringify({
+  projectId: "datapipe-test",
+  storageBucket: "datapipe-test.appspot.com",
+});
+
+const { getFirestore } = require("firebase-admin/firestore");
+const { getStorage } = require("firebase-admin/storage");
+const { promoteToQueue } = require("../../lib/scheduled-pending-recovery.js");
+const { persistPending } = require("../../lib/persist-pending.js");
+
+jest.setTimeout(30000);
+
+const db = getFirestore();
+const bucket = getStorage().bucket();
+
+async function seedExperiment(experimentID, metadataActive) {
+  await db.collection("experiments").doc(experimentID).set({
+    active: true,
+    metadataActive,
+    owner: "recovery-test-user",
+    osfFilesLink: "http://localhost:0/endpoint",
+  });
+}
+
+afterEach(async () => {
+  const docs = await db.collection("uploadQueue").get();
+  const batch = db.batch();
+  docs.forEach((doc) => batch.delete(doc.ref));
+  await batch.commit();
+});
+
+describe("scheduled-pending-recovery layout awareness", () => {
+  it("queues the raw-data path and matching dedup key when metadata is active", async () => {
+    const experimentID = "recovery-test-metadata-on";
+    await seedExperiment(experimentID, true);
+
+    const storagePath = await persistPending(
+      experimentID,
+      "condition-A/data.json",
+      "[]"
+    );
+    const file = bucket.file(storagePath);
+
+    await promoteToQueue(file);
+
+    const expectedDedupKey = `${experimentID}:data/raw/condition-A-data.json`;
+    const docId = expectedDedupKey.replace(/[/\\]/g, "_");
+    const doc = await db.collection("uploadQueue").doc(docId).get();
+
+    expect(doc.exists).toBe(true);
+    expect(doc.data().filename).toBe("data/raw/condition-A-data.json");
+    expect(doc.data().deduplicationKey).toBe(expectedDedupKey);
+  });
+
+  it("queues the original filename and matching dedup key when metadata is off", async () => {
+    const experimentID = "recovery-test-metadata-off";
+    await seedExperiment(experimentID, false);
+
+    const storagePath = await persistPending(experimentID, "data.json", "[]");
+    const file = bucket.file(storagePath);
+
+    await promoteToQueue(file);
+
+    const expectedDedupKey = `${experimentID}:data.json`;
+    const docId = expectedDedupKey.replace(/[/\\]/g, "_");
+    const doc = await db.collection("uploadQueue").doc(docId).get();
+
+    expect(doc.exists).toBe(true);
+    expect(doc.data().filename).toBe("data.json");
+    expect(doc.data().deduplicationKey).toBe(expectedDedupKey);
+  });
+});

functions/src/api-data.ts

@@ -7,7 +7,7 @@ import { db } from "./app.js";
 import writeLog from "./write-log.js";
 import MESSAGES from "./api-messages.js";
 import blockMetadata from "./metadata-block.js";
-import { DerivedFile, rawDataPath } from "./metadata-derived-files.js";
+import { DerivedFile, uploadPathFor } from "./metadata-derived-files.js";
 import { uploadDerivedFiles, queueDerivedFiles } from "./metadata-derived-upload.js";
 import resolveToken from "./resolve-token.js";
 import queueUpload from "./queue-upload.js";
@@ -140,8 +140,10 @@ export const apiData = onRequest({ cors: true, memory: "512MiB", concurrency: 1
     const metadataResponse = await blockMetadata(exp_data, token, metadata_doc_ref, data, filename, metadataOptions);
 
     if (metadataResponse.success === false) {
-      await cleanupPending(pendingPath);
-      res.status(400).json({...metadataResponse, derivedFiles: undefined});
+      // The pending-data copy is deliberately kept (not cleaned up) here: the
+      // participant's raw data never made it to OSF, so scheduled-pending-recovery
+      // salvages it later instead of losing it outright.
+      res.status(400).json(metadataResponse);
       await writeLog(experimentID, "logError", {...MESSAGES.METADATA_ERROR, detail: metadataResponse.message});
       return;
     }
@@ -158,7 +160,7 @@ export const apiData = onRequest({ cors: true, memory: "512MiB", concurrency: 1
   //data/raw/<original name> in the Psych-DS layout (the CSVs above are derived
   //from it). Session counting and queue-on-failure key off this file. With
   //metadata off, the layout is unchanged: the raw file goes to the root.
-  const uploadFilename = exp_data.metadataActive ? rawDataPath(filename) : filename;
+  const uploadFilename = uploadPathFor(exp_data.metadataActive, filename);
 
   let result: OSFResult;
   try {