#171: Refactor Submission Record data access layer for new schema

.dev/tech-debt.md

@@ -53,8 +53,51 @@ context: `dictionary_categories.id` is a plain auto-increment `serial`, used dir
 
 ---
 
+### No integration tests exist for the submission edit, delete, or commit endpoints — let alone mixed insert+update+delete scenarios
+standalone: yes
+context: `packages/data-provider/test/integration/routers/submission/` only covers `submissionRouter-submit*` (insert-only file/JSON uploads). There is no integration spec for `editSubmittedData`, `deleteSubmittedDataBySystemId`, or the commit flow (`performCommitSubmissionAsync`/`commitSubmissionWorker.ts`) at all, despite the test infra (`test/integration/dependencies/containers.ts`) already running a real Postgres container capable of exercising the real commit transaction. The new UPDATE/DELETE conflict resolution added 2026-08-12 has unit coverage only (`test/unit/utils/submission/findUpdateDeleteConflicts.spec.ts` and friends) — still no HTTP+DB-level test proving a submission with a staged conflict actually ends up INVALID end-to-end. Fix: add integration specs that stage inserts+updates+deletes in the same active submission (including same-systemId collisions) and assert the committed `submitted_data` table end state and the active submission's status/record states.
+
+### `GET /submission/:submissionId/details` can't filter records by state
+standalone: yes
+context: `submissionController.ts::getSubmissionDetailsById` (294-321) and `submissionService.ts::getSubmissionDetailsById` (326-372) only accept `entityNames`/`actionTypes` filters (`submissionDetailsRequestSchema`, `schemas.ts:339-351`), then call `submissionRecordsRepository.getBySubmissionId` (365-369) without a `states` filter. The repository itself already supports it — `getBySubmissionId`/`getByFileIds` (`submissionRecordsRepository.ts`) both accept `filterOptions.states?: SubmissionRecordState[]` and `SUBMISSION_RECORD_STATE` (`types.ts`) already enumerates `RECEIVED`/`VALID`/`INVALID` — so this is a plumbing gap, not a missing capability. Fix: add a `states`/`state` query param to `submissionDetailsRequestSchema`, thread it through `getSubmissionDetailsById`'s `filterOptions` in both the controller and service, and pass it to `submissionRecordsRepository.getBySubmissionId`.
+
+### API to download the originally uploaded file
+standalone: yes
+context: No original file bytes are stored anywhere today. Uploads land in a multer temp path (`dest: '/tmp'`, `submissionRouter.ts:26`), get streamed and parsed by `collectRows` (`fileUtils.ts:62-80`), and the temp file is deleted immediately after parsing (`fileUtils.ts:78`, `fs.unlink`). `submission_files` (`packages/data-model/src/models/submission_files.ts:7-17`) only keeps metadata (`fileName`, `entityName`, `fileSize`); `submission_records` only keeps parsed row data (jsonb), not the raw file. Needs a scope decision before implementation: (a) persist the raw uploaded bytes somewhere (object store or DB blob) at upload time going forward — adds storage/retention cost and doesn't help for submissions already committed under the old behavior, or (b) reconstruct a file from the stored parsed rows — lossy, won't reproduce original column order, formatting, or any extra/ignored columns, and "recreate" may not satisfy whatever this is needed for (audit, re-upload, external sharing). No download/`Content-Disposition` endpoint exists for submission files today; the only precedent for that response pattern in the codebase is `dictionaryController.ts:74-87` (zips dictionary templates, unrelated data).
+
+### Download error report for a file
+standalone: no
+context: Depends on the storage/identity decision above and needs its own format decision. Per-record errors already exist as `SubmissionRecordError[]` in `submission_records.errors` (jsonb, `packages/data-model/src/models/submission_records.ts:59-73`), and a file already has a stable identity distinct from `entityName` (`submission_files.id`, `submission_records.fileId`) — so per-file error retrieval is possible today via `getBySubmissionId`/`getByFileIds` (`submissionRecordsRepository.ts:34-63,149-177`), just not exposed as a dedicated download endpoint. Open question flagged by the requirement itself: return the raw per-record `errors` JSON as-is, or design a purpose-built report format (e.g. row/field/message table)? Note that line numbers are computed at parse time (`fileUtils.ts:104`, `+1 for header row, +1 for 1-based line numbers`) but aren't currently persisted into the stored `errors` — worth carrying through if the report should reference original file line numbers.
+
+### Download error report as a zip for all files in a submission
+standalone: no
+context: Depends on the single-file error report above being defined first — this is just "download that report N times, zipped." `jszip` is already a dependency (`package.json:48`) and already used for exactly this response shape (`zip.generateAsync` + `Content-Disposition: attachment` + `application/zip`) in `dictionaryController.ts:74-87`, so no new library or pattern work is needed once the per-file report format exists.
+
+### List submissions endpoint needs sorting and filtering
+standalone: yes
+context: `GET /submission/category/:categoryId` (`submissionController.ts:233-276`) only accepts `onlyActive`, `organization`, `username` (`submissionsByCategoryRequestSchema`, `schemas.ts:318-329`), plus `page`/`pageSize`. Sort order is hardcoded to `desc(submissions.createdAt)` (`activeSubmissionRepository.ts:216`) with no sort param at all. The mandatory requirement (reverse creation-date sort) already matches today's fixed default — the gap is that it isn't documented as a stable, guaranteed default, and there's no way to choose anything else. Nice-to-have filters not yet supported: study/category beyond the path param, date range (created/updated), creator, contributing users, statuses. `auditRepository.ts` already has a directly reusable pattern for both pieces: date-range filtering (lines 76-81, `lt`/`gt` on `createdAt` against `startDate`/`endDate`) and configurable-direction `orderBy` (line 126), driven by `AuditFilterOptions` (`types.ts:89-98`) — worth modeling the new filter options after that rather than inventing a new shape. Swagger (`submission-api.yml:144-171`) will need the new params documented too.
+
+### `GET /submission/:submissionId` needs richer per-file details
+standalone: yes
+context: The response (`SubmissionSummaryResponse`, `types.ts:302-305`, built by `createSubmissionSummaryResponse`/`submissionResponseParser.ts:65-78`) groups `inserts`/`updates` by `entityName`, each entry only exposing `batchName` (the original filename), `recordsCount`, and `errors` (a count, not detail) — see `submissionResponseParser.ts:14-58` and the source rows shape in `submissionRecordsRepository.ts:20-26,188-208`. Missing relative to the ask: no `fileId` in the response (so a client can't correlate a listed file to a future per-file download/error-report endpoint, above); no file size, even though `submission_files.fileSize` is already stored in the DB (`submission_files.ts:16`) and just isn't selected by `getRecordsSummaryBySubmissionId` (`submissionRecordsRepository.ts:188-197`); no rolled-up per-file validation status (only an error count — the per-record `state` enum `RECEIVED`/`VALID`/`INVALID` exists at `submission_records.ts:12` but isn't aggregated to file level); `deletes` has no per-file breakdown at all in the current model (`DataDeletesSubmissionSummary` is not batched by file). Also found in passing: swagger's `SubmissionDetailsResult` schema (`schemas.yml:87-111`) documents a shape that doesn't match what `/submission/:submissionId/details` actually returns (`SubmissionRecordWithEntityName[]`, i.e. `{id, actionType, state, fileId, data, errors, entityName}[]`) — worth correcting alongside this work since both touch the same response family.
+
 ## Resolved
 
+### Flaky integration test: dictionary migration force-retry intermittently returned 409 instead of 200
+resolved: 2026-08-12, made the test wait for the background migration worker to reach a terminal status before mutating it directly, removing the race. `dictionaryMigration.spec.ts`'s "should retry migration..." test now calls a `waitForMigrationToFinish` helper (same polling pattern already used in `dictionaryMigrationData.spec.ts`) right before overwriting the migration's status to `FAILED`, instead of assuming `initiateMigration`'s fire-and-forget worker had already finished. Verified with 3 consecutive full integration-suite runs, 380/380 passing each time, no failures.
+
+### Staging a DELETE submission record never checked for or merged with an existing pending record for the same systemId
+resolved: 2026-08-12, added a staging-time check instead of letting one action override the other silently. `resolveDeleteStagingConflicts` (`packages/data-provider/src/utils/submissionUtils.ts`) is now called from `deleteSubmittedDataBySystemId` (`submmittedData.ts`) before any new DELETE record is inserted: it fetches the Active Submission's existing UPDATE/DELETE records and cross-checks them against the systemIds about to be deleted (the target record plus its dependents). A systemId with a pending UPDATE is now rejected outright (`INVALID_SUBMISSION` response, no record staged) — consistent with the "both sides invalid" policy used for the same conflict at validation time — instead of quietly reaching `performDataValidation` later. A systemId that already has a pending DELETE is treated as a duplicate and skipped rather than inserted a second time, addressing the original in-code TODO directly. Previously the function always blindly inserted new DELETE rows regardless of what was already staged.
+
+### Duplicate UPDATE submission records for the same systemId collapsed via undefined last-write-wins at commit time
+resolved: 2026-08-12, added a deterministic `ORDER BY` instead of relying on unspecified Postgres row order. `submissionRecordsRepository.ts::getByFileIds` now does `.orderBy(submissionRecords.id)` (ascending) — `id` is a `serial` primary key, monotonic with insertion order, so `commitSubmissionWorker.ts`'s `record[systemId] = record` reduction now deterministically keeps the most recently inserted UPDATE row for a given systemId instead of whichever row Postgres happened to scan last. Considered adding a `created_at` timestamp column instead, but `id` already gives the same ordering guarantee without a schema migration, so that was dropped in favour of the simpler fix.
+
+### UPDATE/DELETE conflict on the same systemId was silently dropped instead of surfaced as an error
+resolved: 2026-08-12, added explicit conflict detection ahead of dictionary validation. `findUpdateDeleteConflicts` (`packages/data-provider/src/utils/submissionUtils.ts`) scans staged submission records for entityName+systemId pairs with both an UPDATE and a DELETE, before `performDataValidation` (`submissionProcessor.ts`) runs `validateSchemas`. Conflicting records are excluded from validation, both sides are marked `INVALID` with a new `CONFLICTING_ACTION` error (`RecordErrorActionConflict`, added to `SubmissionRecordError` in `@overture-stack/lyric-data-model`), the active submission is marked `INVALID`, and the scenario is `logger.error`-logged. Previously this was only prevented by incidental JS array-filtering order with no error surfaced (see the still-open items above for what remains: the staging-time TODO, and integration coverage).
+
+### `filterDeletesFromUpdates`/`filterRecordsByConflicts` removed as dead code
+resolved: 2026-08-12, same session as the fix above. Once `findUpdateDeleteConflicts` became the real, wired-in conflict detector, the unused `filterRecordsByConflicts`/`filterDeletesFromUpdates` pair (`submissionUtils.ts`, previously lines 234-280) had no remaining reason to exist — deleted both functions and their orphaned spec (`test/unit/utils/submission/filterDeletesFromUpdates.spec.ts`). Confirmed via repo-wide grep that nothing else referenced them before removing.
+
 <!-- Move entries here when addressed, with a note of when and what fixed it -->
 
 ### Kafka publish tracking: no unit tests for `createPublishTracker`

apps/server/swagger/schemas.yml

@@ -95,7 +95,7 @@ components:
               type:
                 type: string
                 description: Type of action
-                enum: ['INSERTS', 'UPDATES', 'DELETES']
+                enum: ['INSERT', 'UPDATE', 'DELETE']
               entity:
                 type: string
                 description: Name of the entity

apps/server/swagger/submission-api.yml

@@ -56,7 +56,7 @@
 
 /submission/{submissionId}/details:
   get:
-    summary: Fetch Submission Data records. Sorted in their original file order and grouped by `inserts`, `updates`, and `deletes`.
+    summary: Fetch Submission Data records. Sorted in their original file order and grouped by `insert`, `update`, and `delete`.
     tags:
       - Submission
     parameters:
@@ -67,9 +67,9 @@
           type: array
           items:
             type: string
-            enum: [inserts, updates, deletes]
+            enum: [insert, update, delete]
         uniqueItems: true
-        description: Filters the Submission Data records by action type. Valid values are `inserts`, `updates`, and `deletes` (case insensitive). If not provided, all action types are returned.
+        description: Filters the Submission Data records by action type. Valid values are `insert`, `update`, and `delete` (case insensitive). If not provided, all action types are returned.
       - name: entityNames
         in: query
         schema:
@@ -98,7 +98,7 @@
       503:
         $ref: '#/components/responses/ServiceUnavailableError'
 
-/submission/{submissionId}/{actionType}:
+/submission/{submissionId}/data:
   delete:
     summary: Clear Active Submission by entity name
     tags:
@@ -109,24 +109,18 @@
         type: string
         required: true
         description: The ID of the Submission
-      - name: actionType
-        in: path
-        required: true
-        schema:
-          type: string
-          enum: [inserts, updates, deletes]
-        description: Parameter to specify the type of record to remove from the Submission. Must be one of `inserts`, `updates`, or `deletes` (case insensitive)
-      - name: entityName
+      - name: recordId
         in: query
-        type: string
-        required: true
-        description: The name of the entity
-      - name: index
+        required: false
+        schema:
+          type: integer
+        description: An optional query parameter used to specify the record ID within the submission to be deleted. <br />Only one of `recordId` or `fileId` can be provided
+      - name: fileId
         in: query
         required: false
         schema:
           type: integer
-        description: An optional query parameter used to specify the index of the item within the submission type to be deleted. <br />If not provided all the items within the submission type will be deleted.
+        description: An optional query parameter used to specify the file ID within the submission to be deleted. <br />Only one of `recordId` or `fileId` can be provided
     responses:
       200:
         description: Submission cleared successfully. Returns the current Active Submission

packages/data-model/docs/schema.dbml

@@ -128,10 +128,8 @@ table submission_records {
 
 table submissions {
   id serial [pk, not null, increment]
-  data jsonb [not null]
   dictionary_category_id integer [not null]
   dictionary_id integer [not null]
-  errors jsonb
   organization varchar [not null]
   status submission_status [not null]
   created_at timestamp [default: `now()`]
@@ -187,7 +185,7 @@ ref: dictionary_migration.to_dictionary_id - dictionaries.id
 
 ref: dictionary_migration.submission_id - submissions.id
 
-ref: submission_files.submission_id - submissions.id
+ref: submission_files.submission_id > submissions.id
 
 ref: submission_records.file_id > submission_files.id
 

packages/data-model/migrations/0017_remove_submission_data.sql

@@ -0,0 +1,2 @@
+ALTER TABLE "submissions" DROP COLUMN IF EXISTS "data";--> statement-breakpoint
+ALTER TABLE "submissions" DROP COLUMN IF EXISTS "errors";