diff --git a/functions/src/__tests__/metadata-derived-files.test.js b/functions/src/__tests__/metadata-derived-files.test.js index 9837beb..0138dc5 100644 --- a/functions/src/__tests__/metadata-derived-files.test.js +++ b/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); } }); diff --git a/functions/src/__tests__/metadata-derived-upload-emulator.test.js b/functions/src/__tests__/metadata-derived-upload-emulator.test.js new file mode 100644 index 0000000..d93e54b --- /dev/null +++ b/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"]); + }); +}); diff --git a/functions/src/__tests__/metadata-production.test.js b/functions/src/__tests__/metadata-production.test.js index 4e18df2..cdd0694 100644 --- a/functions/src/__tests__/metadata-production.test.js +++ b/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'); + }); }); diff --git a/functions/src/__tests__/put-file-osf.test.js b/functions/src/__tests__/put-file-osf.test.js index c21c5ef..1ff75d9 100644 --- a/functions/src/__tests__/put-file-osf.test.js +++ b/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')); diff --git a/functions/src/__tests__/scheduled-pending-recovery-emulator.test.js b/functions/src/__tests__/scheduled-pending-recovery-emulator.test.js new file mode 100644 index 0000000..5c13a0f --- /dev/null +++ b/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); + }); +}); diff --git a/functions/src/api-data.ts b/functions/src/api-data.ts index 98abc4d..c9c36f9 100644 --- a/functions/src/api-data.ts +++ b/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/ 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 { diff --git a/functions/src/metadata-block.ts b/functions/src/metadata-block.ts index f365843..5dce4ae 100644 --- a/functions/src/metadata-block.ts +++ b/functions/src/metadata-block.ts @@ -11,13 +11,13 @@ import buildDerivedFiles, { DerivedFile } from "./metadata-derived-files.js"; import { queueDerivedFiles } from "./metadata-derived-upload.js"; import { ExperimentData, Metadata, MetadataResponse } from './interfaces'; -export type MetadataBlockResult = MetadataResponse & { derivedFiles?: DerivedFile[] }; - -// Sentinel thrown inside the transaction when the merge needs the OSF copy of -// the metadata as its base. The OSF download must happen outside the -// transaction (Firestore retries the transaction callback on contention, which -// would re-run any network call inside it), so we abort, download, and re-run. -const NEEDS_OSF_METADATA = new Error("needs-osf-metadata"); +// Discriminated on `success` so the compiler guarantees derivedFiles can only +// ride on a success — a failure response structurally cannot carry them, so +// callers (api-data's 400 path) can send it verbatim with no risk of leaking a +// half-built derived-file set. +type MetadataSuccess = Omit & { success: true; derivedFiles?: DerivedFile[] }; +type MetadataFailure = Omit & { success: false }; +export type MetadataBlockResult = MetadataSuccess | MetadataFailure; export default async function blockMetadata( exp_data: ExperimentData, @@ -35,7 +35,7 @@ try { //Only run if metadata collection is enabled. if (!exp_data.metadataActive) { metadataMessage = MESSAGES.METADATA_NOT_ACTIVE; - const metadataResponse: MetadataResponse = {success: true, ...metadataMessage}; + const metadataResponse: MetadataSuccess = {success: true, ...metadataMessage}; return metadataResponse; } @@ -54,13 +54,33 @@ try { //dataset_description.json exists in the OSF project. const osfMetadataId: string | undefined = (await processMetadata(exp_data.osfFilesLink, osfToken)).metadataId; - //Populated only when Firestore has no metadata but OSF does (see sentinel above). + //Non-transactional pre-read to decide whether the OSF copy of the metadata + //needs downloading as the merge base (the bootstrap case: Firestore empty, + //OSF populated). Done outside the transaction below since Firestore retries + //a transaction callback on contention, which would otherwise repeat this + //network call on every retry. + const preReadFirestoreMetadata: Metadata | undefined = (await metadata_doc_ref.get()).data()?.metadata; + + //Record which of the four states we are in before the download below (which + //can throw) so error responses still report the state, same as a successful + //response would. + if (preReadFirestoreMetadata) { + metadataMessage = osfMetadataId ? MESSAGES.METADATA_IN_OSF_AND_FIRESTORE : MESSAGES.METADATA_IN_FIRESTORE_NOT_IN_OSF; + } else { + metadataMessage = osfMetadataId ? MESSAGES.METADATA_IN_OSF_NOT_IN_FIRESTORE : MESSAGES.METADATA_NOT_IN_FIRESTORE_OR_OSF; + } + let osfMetadata: Metadata | undefined; + if (!preReadFirestoreMetadata && osfMetadataId) { + osfMetadata = (await downloadMetadata(exp_data.osfFilesLink, osfToken, osfMetadataId)).metadata; + } //The transaction is Firestore-only: read the metadata doc, merge, write it - //back. All OSF network I/O happens before or after, so a transaction retry - //can never repeat an OSF call. - const runMergeTransaction = () => db.runTransaction(async (t) => { + //back. All OSF network I/O happened above, so a transaction retry (e.g. a + //concurrent submission populating Firestore between the pre-read and here) + //can never repeat an OSF call — firestoreMetadata is re-read here, so that + //race still resolves correctly via firestoreMetadata ?? osfMetadata. + const updatedMetadata = await db.runTransaction(async (t) => { const firestoreMetadata: Metadata | undefined = (await t.get(metadata_doc_ref)).data()?.metadata; //Record which of the four states we are in. This is set before any @@ -71,32 +91,17 @@ try { metadataMessage = osfMetadataId ? MESSAGES.METADATA_IN_OSF_NOT_IN_FIRESTORE : MESSAGES.METADATA_NOT_IN_FIRESTORE_OR_OSF; } - if (!firestoreMetadata && osfMetadataId && !osfMetadata) { - throw NEEDS_OSF_METADATA; - } - //When Firestore has metadata, updating is done with respect to Firestore. //When only OSF has metadata, the downloaded OSF copy is the base instead. //When neither has metadata, the incoming metadata is used as-is. const baseMetadata: Metadata | undefined = firestoreMetadata ?? osfMetadata; - const updatedMetadata = baseMetadata ? await updateMetadata(baseMetadata, incomingMetadata) : incomingMetadata; + const updated = baseMetadata ? await updateMetadata(baseMetadata, incomingMetadata) : incomingMetadata; - t.set(metadata_doc_ref, {metadata: updatedMetadata}, {merge: true}); + t.set(metadata_doc_ref, {metadata: updated}, {merge: true}); - return updatedMetadata; + return updated; }); - let updatedMetadata; - try { - updatedMetadata = await runMergeTransaction(); - } catch (error) { - if (error !== NEEDS_OSF_METADATA) throw error; - //Metadata is in OSF as evidenced by the metadata ID, so it is downloaded - //to serve as the merge base, and the transaction is re-run. - osfMetadata = (await downloadMetadata(exp_data.osfFilesLink, osfToken, osfMetadataId as string)).metadata; - updatedMetadata = await runMergeTransaction(); - } - //Firestore is now up to date; mirror the merged metadata to OSF. The mirror //is best-effort: Firestore is the source of truth and every submission //re-merges and re-mirrors, so a failure here must not reject the @@ -109,18 +114,25 @@ try { osfFilesLink: exp_data.osfFilesLink, }; - try { - //If a metadata file exists in OSF, it is updated. Otherwise it is created. - if (osfMetadataId) { - //Result intentionally unchecked: the queue can only PUT (which would 409 - //against the existing file), and the next submission updates OSF anyway. + //If a metadata file exists in OSF, it is updated. Otherwise it is created. + if (osfMetadataId) { + try { await updateFileOSF( exp_data.osfFilesLink, osfToken, metadataFileContents, osfMetadataId ); - } else { + } catch { + //Result intentionally unchecked and NOT queued: the uploadQueue only + //PUTs (create), which is guaranteed to 409 against a file that already + //exists — queueing here would just leave a dead entry the retry worker + //marks completed without ever applying the update. Firestore is the + //source of truth and every submission re-merges and re-mirrors, so the + //next submission repairs OSF instead. + } + } else { + try { const response = await putFileOSF( exp_data.osfFilesLink, osfToken, @@ -134,14 +146,14 @@ try { await queueDerivedFiles([{ filename: "dataset_description.json", content: metadataFileContents }], queueTarget, `dataset_description OSF error ${response.errorCode}: ${response.errorText}`); } + } catch (error) { + const detail = error instanceof Error ? error.message : "Unknown error"; + await queueDerivedFiles([{ filename: "dataset_description.json", content: metadataFileContents }], + queueTarget, `dataset_description upload exception: ${detail}`); } - } catch (error) { - const detail = error instanceof Error ? error.message : "Unknown error"; - await queueDerivedFiles([{ filename: "dataset_description.json", content: metadataFileContents }], - queueTarget, `dataset_description upload exception: ${detail}`); } - const metadataResponse: MetadataBlockResult = {success: true, ...metadataMessage, derivedFiles}; + const metadataResponse: MetadataSuccess = {success: true, ...metadataMessage, derivedFiles}; return metadataResponse; } catch (error) { @@ -154,7 +166,7 @@ catch (error) { console.error("Metadata block error:", errorMessage); - const metadataResponse: MetadataResponse = {success: false, ...MESSAGES.METADATA_ERROR, message: errorMessage, ...metadataMessage}; + const metadataResponse: MetadataFailure = {success: false, ...MESSAGES.METADATA_ERROR, message: errorMessage, ...metadataMessage}; return metadataResponse; //METADATA BLOCK END }; diff --git a/functions/src/metadata-derived-files.ts b/functions/src/metadata-derived-files.ts index 4b9965c..9da4af2 100644 --- a/functions/src/metadata-derived-files.ts +++ b/functions/src/metadata-derived-files.ts @@ -26,13 +26,15 @@ export interface DerivedFileSource extends ExtractionResult { /** * Researcher-supplied folder prefixes (e.g. "condition-A/abc.json") are - * flattened away in the Psych-DS layout: the CLI converts whole directories - * into a flat data/ folder, and DataPipe matches it, so only the last path - * segment names the file. Grouping by subfolder is lost under data/. + * flattened into the Psych-DS layout: the CLI converts whole directories into + * a flat data/ folder, and DataPipe matches it, so the path is encoded into a + * single filename rather than nested. Encoding (instead of discarding) the + * prefix keeps two submissions with the same leaf name in different + * subfolders from colliding at data/raw/ (and keeps the derived main + * CSV/sidecar stems, which follow the same encoded name, collision-free too). */ function flattenName(dataFilename: string): string { - const slashIndex = dataFilename.lastIndexOf('/'); - return slashIndex === -1 ? dataFilename : dataFilename.slice(slashIndex + 1); + return dataFilename.replace(/[/\\]+/g, '-'); } /** @@ -44,6 +46,16 @@ export function rawDataPath(dataFilename: string): string { return `data/raw/${flattenName(dataFilename)}`; } +/** + * The OSF path a raw submission should actually be uploaded to: `data/raw/` + * when metadata is on, unchanged at the root otherwise. Callers that key + * queue/dedup entries off the upload filename (api-data's request path and + * scheduled-pending-recovery) must agree on this, so the rule lives here once. + */ +export function uploadPathFor(metadataActive: boolean | undefined, dataFilename: string): string { + return metadataActive ? rawDataPath(dataFilename) : dataFilename; +} + /** * Builds the full set of Psych-DS files derived from one submission, mirroring * what the @jspsych/metadata CLI writes per data file: the main data table as diff --git a/functions/src/metadata-derived-upload.ts b/functions/src/metadata-derived-upload.ts index 9586d74..b640d0d 100644 --- a/functions/src/metadata-derived-upload.ts +++ b/functions/src/metadata-derived-upload.ts @@ -3,6 +3,9 @@ import queueUpload from "./queue-upload.js"; import writeLog from "./write-log.js"; import MESSAGES from "./api-messages.js"; import { DerivedFile } from "./metadata-derived-files.js"; +import resolveFolder from "./subfolder.js"; + +const DATA_PREFIX = "data/"; export interface DerivedUploadTarget { experimentID: string; @@ -23,16 +26,42 @@ export async function uploadDerivedFiles( target: DerivedUploadTarget, osfToken: string, ): Promise { - for (const file of files) { + // Every derived file under data/ shares that one folder; resolve it once up + // front instead of each of the N uploads below independently walking (and + // possibly racing to create) the same path. Folder-create races among + // concurrent submissions still resolve safely via subfolder.ts's own + // 409-re-list branch. + // + // This resolution is best-effort: if it throws (OSF list/create error or a + // network failure), fall back to undefined so each under-data/ file re-walks + // the path itself inside its own per-file try/catch below — that path still + // queues on failure. Letting the throw escape here would instead lose the + // derived files entirely (never uploaded, never queued) and fail an already- + // successful submission, breaking this function's best-effort contract. + const needsDataFolder = files.some((file) => file.filename.startsWith(DATA_PREFIX)); + let dataFolderLink: string | undefined; + if (needsDataFolder) { + try { + dataFolderLink = await resolveFolder(target.osfFilesLink, osfToken, "data"); + } catch { + dataFolderLink = undefined; + } + } + + await Promise.allSettled(files.map(async (file) => { + const underData = file.filename.startsWith(DATA_PREFIX); + const uploadFilename = underData ? file.filename.slice(DATA_PREFIX.length) : file.filename; + const startUrl = underData ? dataFolderLink : undefined; + try { - const result = await putFileOSF(target.osfFilesLink, osfToken, file.content, file.filename); - if (result.success || result.errorCode === 409) continue; + const result = await putFileOSF(target.osfFilesLink, osfToken, file.content, uploadFilename, startUrl); + if (result.success || result.errorCode === 409) return; await queueDerivedFiles([file], target, `Derived file OSF error ${result.errorCode}: ${result.errorText}`); } catch (e) { const detail = e instanceof Error ? e.message : "Unknown error"; await queueDerivedFiles([file], target, `Derived file upload exception: ${detail}`); } - } + })); } /** diff --git a/functions/src/metadata-production.ts b/functions/src/metadata-production.ts index efae6a4..0904c0c 100644 --- a/functions/src/metadata-production.ts +++ b/functions/src/metadata-production.ts @@ -19,44 +19,62 @@ export interface ProducedMetadata extends ExtractionResult { mainContent?: string; } +// Internal marker: the payload parsed as JSON but wasn't a trial array. Kept +// private so it can only be thrown/caught here, never matched by message text. +class NotATrialArrayError extends Error {} + export default async function produceMetadata(data: string, options: object | null = null): Promise { // Initializes the metadata object. var metadata = new jsPsychMetadata(); // eslint-disable-line no-var - // Checks if the data is in CSV format. - const isCsv = (str: string) => { try { JSON.parse(str); return false; } catch (e) { return true; } }; - - const csvFlag: boolean = isCsv(data); + // Parse the payload exactly once. parseJsonData is the library's own + // parser (the CLI and frontend run it too): a bare trial array — the + // standard jsPsych/DataPipe payload — passes through unchanged, and the + // nonstandard-but-possible { "trials": [...] } wrapper is unwrapped to its + // array. If that parse fails, the payload is CSV text instead. + let csvFlag: boolean; + let jsonRows: Array> | undefined; + try { + const parsed = parseJsonData(data); + if (!Array.isArray(parsed)) { + // Valid JSON, but not a trial array (e.g. a bare object). This is a + // real input error, not a "fall back to CSV" signal, so flag it with a + // dedicated marker rather than a message string — that way a coincidental + // parse error from the library carrying the same text can't be mistaken + // for it below. + throw new NotATrialArrayError(); + } + jsonRows = parsed as Array>; + csvFlag = false; + } catch (e) { + if (e instanceof NotATrialArrayError) throw new Error('Data must be an array of trials'); + csvFlag = true; + } - // Parses the data if it is JSON in string format. parseJsonData is the - // library's own parser (the CLI and frontend run it too): a bare trial - // array — the standard jsPsych/DataPipe payload — passes through unchanged, - // and the nonstandard-but-possible { "trials": [...] } wrapper is unwrapped - // to its array, keeping DataPipe's parsing at parity with the CLI's. - if(!csvFlag) data = parseJsonData(data); + // For CSV, parse once up front too and hand generate() the same rows used + // for mainRows below, instead of generate() re-parsing the text itself. + const csvRows: Array> | undefined = csvFlag + ? (await parseCSV(data)) as Array> + : undefined; // Generates the metadata, using the options if they are provided. // The vendored @jspsych/metadata (see functions/metadata/) changed generate()'s // signature to generate(data, metadata={}, ext='json'|'csv', options={}) — the 3rd // arg is now a string extension, not the boolean csv flag the old fork used. + // Passing a pre-parsed array (rather than the raw string) skips generate()'s + // own internal parse for both formats; ext is still passed for its other + // format-dependent behavior (e.g. id-column detection). const ext: 'json' | 'csv' = csvFlag ? 'csv' : 'json'; - options ? await metadata.generate(data, options, ext) : await metadata.generate(data, {}, ext); + const rows = (csvFlag ? csvRows : jsonRows) as Array>; + options ? await metadata.generate(rows, options, ext) : await metadata.generate(rows, {}, ext); const incomingMetadata: Metadata = metadata.getMetadata() as Metadata; - if (!incomingMetadata.variableMeasured || !incomingMetadata.variableMeasured[0].name) { + if (!incomingMetadata.variableMeasured?.length || !incomingMetadata.variableMeasured[0].name) { throw new Error('Invalid metadata generated'); } - // Main data rows for the Psych-DS main CSV. For JSON, `data` is the parsed - // array (nested columns left intact — see mainRows doc above); for CSV, - // parse the original text (the string `data` is untouched — generate() - // parses its own copy internally). - const mainRows: Array> = csvFlag - ? (await parseCSV(data)) as Array> - : (data as unknown as Array>); - // Nested array/object columns that generate() expanded into dotted // sub-variables; their per-row data is returned so callers can write // sidecar CSVs (see metadata-derived-files.ts). @@ -65,8 +83,9 @@ export default async function produceMetadata(data: string, options: object | nu extractedArrays: metadata.getExtractedArrays(), extractedObjects: metadata.getExtractedObjects(), joinKeys: metadata.getArrayJoinKeys(), - mainRows, - // For CSV, `data` was never reassigned and is still the original text. - mainContent: csvFlag ? (data as string) : undefined, + // Same rows array passed to generate() above — see the mainRows doc + // comment for why sharing (rather than re-parsing) is intentional. + mainRows: rows, + mainContent: csvFlag ? data : undefined, }; } diff --git a/functions/src/put-file-osf.ts b/functions/src/put-file-osf.ts index ea4aeac..66b38e6 100644 --- a/functions/src/put-file-osf.ts +++ b/functions/src/put-file-osf.ts @@ -4,18 +4,23 @@ export default async function putFileOSF( osfComponent: string, osfToken: string, filedata: string | Buffer, - filename: string + filename: string, + // Optional pre-resolved link for the folder `filename`'s remaining segments + // are relative to (e.g. already resolved "data/" for a caller uploading many + // files under data/). Skips re-walking that portion of the path. Defaults to + // osfComponent (the component root) when omitted. + startUrl?: string, ) { // A filename may carry a path prefix (e.g. "data/raw/abc123.json"). Split it // into folder segments and the file name; each folder level is found-or-created // in turn (WaterButler has no atomic deep-path create), walking down to the // folder that will hold the file. A bare "abc123.json" has no segments and - // uploads straight to the storage root. + // uploads straight to the storage root (or to startUrl, if given). const segments = filename.split('/'); const fileName = segments.pop() as string; - let targetUrl = osfComponent; + let targetUrl = startUrl ?? osfComponent; for (const folder of segments) { targetUrl = await resolveFolder(targetUrl, osfToken, folder); } diff --git a/functions/src/queue-upload.ts b/functions/src/queue-upload.ts index 286de8c..7e75621 100644 --- a/functions/src/queue-upload.ts +++ b/functions/src/queue-upload.ts @@ -24,15 +24,39 @@ export default async function queueUpload(params: QueueUploadParams): Promise processing) or finish (-> completed/failed, which + // deletes the storage object) the doc in between. Re-read to confirm. + // Still pending/processing: the worker either hasn't started or now owns + // the doc and will read the payload we just wrote — either way we're + // done. Otherwise the doc finished out from under us and our fresh + // payload is orphaned, so fall through to re-queue a clean pending doc. + const recheck = await docRef.get(); + const recheckStatus = recheck.exists ? recheck.data()?.status : undefined; + if (recheckStatus === "pending" || recheckStatus === "processing") { + return docId; + } + } } // Write data to Cloud Storage diff --git a/functions/src/scheduled-pending-recovery.ts b/functions/src/scheduled-pending-recovery.ts index 15c9800..f17d5f9 100644 --- a/functions/src/scheduled-pending-recovery.ts +++ b/functions/src/scheduled-pending-recovery.ts @@ -3,6 +3,7 @@ import { Timestamp } from "firebase-admin/firestore"; import { db, storage } from "./app.js"; import { readPendingEnvelope, cleanupPending } from "./persist-pending.js"; import { ExperimentData } from "./interfaces.js"; +import { uploadPathFor } from "./metadata-derived-files.js"; const PENDING_PREFIX = "pending-data/"; @@ -87,7 +88,7 @@ async function recoverPendingUploads() { * 4. Create an uploadQueue Firestore document * 5. Clean up the pending-data/ file */ -async function promoteToQueue( +export async function promoteToQueue( file: ReturnType["file"]> ) { // Read the envelope @@ -119,9 +120,21 @@ async function promoteToQueue( return; } + // Layout-aware upload path: metadata-active experiments store their raw + // file at data/raw/, same as api-data.ts's live-submission path. Recovered + // sessions get no metadata/derived files regenerated here (recovery has no + // metadata pipeline and the raw file is the source of truth; the next live + // submission re-merges Firestore metadata into dataset_description.json + // anyway) — full parity would be separate work. + const uploadFilename = uploadPathFor(expData.metadataActive, filename); + // Check for deduplication and atomically create the queue entry via transaction. - // This prevents duplicate entries if two recovery runs overlap. - const deduplicationKey = `${experimentID}:${filename}`; + // This prevents duplicate entries if two recovery runs overlap. Keyed off + // uploadFilename (not the envelope's original filename) so this matches the + // key api-data.ts uses for the same eventual OSF path — otherwise a crash + // between queueUpload and cleanupPending could upload the same submission + // twice, once under each filename. + const deduplicationKey = `${experimentID}:${uploadFilename}`; const docId = deduplicationKey.replace(/[/\\]/g, "_"); const docRef = db.collection("uploadQueue").doc(docId); @@ -142,7 +155,7 @@ async function promoteToQueue( transaction.set(docRef, { experimentID, owner: expData.owner, - filename, + filename: uploadFilename, storagePath, dataType: "data", osfFilesLink: expData.osfFilesLink, diff --git a/pages/faq.js b/pages/faq.js index ab0c9df..76c5454 100644 --- a/pages/faq.js +++ b/pages/faq.js @@ -13,18 +13,26 @@ export default function FAQ() { const [openItems, setOpenItems] = useState(["item-0"]); useEffect(() => { - const hash = window.location.hash.replace("#", ""); - if (!hash) return; - setOpenItems((prev) => (prev.includes(hash) ? prev : [...prev, hash])); - // Wait for the accordion to expand, then bring the item's trigger into view. - // Chakra doesn't forward `id` to the DOM, so target the trigger via its - // data-controls attribute and offset for the fixed navbar. - setTimeout(() => { - const el = document.querySelector(`[data-controls$=":content:${hash}"]`); - if (!el) return; - const top = el.getBoundingClientRect().top + window.scrollY - 80; - window.scrollTo({ top, behavior: "smooth" }); - }, 350); + function scrollToHash() { + const hash = window.location.hash.replace("#", ""); + if (!hash) return; + setOpenItems((prev) => (prev.includes(hash) ? prev : [...prev, hash])); + // Each FAQItem owns a Box with id={value} that isn't part of the + // collapsible content, so it's always in the DOM to target — no need to + // wait on the accordion's expand animation. Offset for the fixed navbar. + requestAnimationFrame(() => { + const el = document.getElementById(hash); + if (!el) return; + const top = el.getBoundingClientRect().top + window.scrollY - 80; + window.scrollTo({ top, behavior: "smooth" }); + }); + } + + scrollToHash(); + // Also handle hash changes while already on this page (e.g. clicking + // another #item-N link without a full navigation/mount). + window.addEventListener("hashchange", scrollToHash); + return () => window.removeEventListener("hashchange", scrollToHash); }, []); return ( @@ -325,18 +333,20 @@ export default function FAQ() { function FAQItem({ question, children, value }) { return ( - - - - {question} - - - - - - {children} - - - + + + + + {question} + + + + + + {children} + + + + ); }