From 45e78b855c0dbab3a23dc5e942908ba101d296ff Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 27 Oct 2026 18:49:11 +1100 Subject: [PATCH] fix(upload): ISO-only date inference; an all-failed upload is an error (#123) - Root cause: the browser inferred timestamptz for anything new Date() parses (e.g. "27 Oct 26"); the API accepts only ISO, so every row failed. inferCell now infers date/timestamptz only for real ISO values. - When every row fails, the upload is an error: the transaction is rolled back (no table, no registry row, an overwrite is undone). - Partial failures: warning toast, and the Migrated files list and Preview show "N of M rows failed". Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019YPoXzBSj89ZHgw7nfTmbB --- api/services/dynamic_loader.py | 12 +++ .../src/__tests__/csv-preview.test.ts | 16 ++++ .../src/__tests__/job-outcome.test.ts | 15 +++- csv-table-hub-main/src/lib/csv-preview.ts | 18 +++-- csv-table-hub-main/src/lib/csv.functions.ts | 2 + csv-table-hub-main/src/lib/job-outcome.ts | 18 ++++- .../src/routes/_authenticated/index.tsx | 6 +- docs/VCRM.md | 2 +- tests/test_failed_uploads.py | 74 +++++++++++++++++++ 9 files changed, 151 insertions(+), 12 deletions(-) create mode 100644 tests/test_failed_uploads.py diff --git a/api/services/dynamic_loader.py b/api/services/dynamic_loader.py index 74fdf9c..c2a86e6 100644 --- a/api/services/dynamic_loader.py +++ b/api/services/dynamic_loader.py @@ -320,6 +320,18 @@ def _do_upload( count=len(to_insert), ) + # #123: if every row failed, nothing was migrated. Roll back the new + # table (and any overwrite) and report a failed upload, not a success. + if data_rows and failed_row_count == len(data_rows): + conn.rollback() + msg = f"None of the {len(data_rows)} rows could be imported, so nothing was saved." + if row_errors: + e = row_errors[0] + msg += f' First problem: row {e["rowNumber"]}, column "{e["column"]}": {e["reason"]}' + _log(logs, "error", msg, "error") + return {"status": "error", "message": msg, "rowErrors": row_errors, + "totalRows": len(data_rows), "failedRows": failed_row_count, "logs": logs} + # Insert inserted = 0 if to_insert: diff --git a/csv-table-hub-main/src/__tests__/csv-preview.test.ts b/csv-table-hub-main/src/__tests__/csv-preview.test.ts index 5826b34..6e91fa0 100644 --- a/csv-table-hub-main/src/__tests__/csv-preview.test.ts +++ b/csv-table-hub-main/src/__tests__/csv-preview.test.ts @@ -125,6 +125,22 @@ describe("parseCsvPreview — type inference", () => { expect(preview.inferredTypes[0]).toBe("date"); }); + // #123: only types the API's cast accepts may be inferred. + it("keeps non-ISO dates such as '27 Oct 26' as text", async () => { + const preview = await parseCsvPreview(makeFile("Date,Processed On\n27 Oct 26,27 Oct 26 10:15\n28 Oct 26,28 Oct 26 11:00\n")); + expect(preview.inferredTypes).toEqual(["text", "text"]); + }); + + it("still infers timestamptz for ISO timestamps", async () => { + const preview = await parseCsvPreview(makeFile("at\n2026-10-27 10:15:00\n2026-10-28T11:00:00Z\n")); + expect(preview.inferredTypes[0]).toBe("timestamptz"); + }); + + it("does not infer date for impossible dates the API would reject", async () => { + const preview = await parseCsvPreview(makeFile("d\n2026-02-30\n")); + expect(preview.inferredTypes[0]).toBe("text"); + }); + it("falls back to text when column has mixed types", async () => { const preview = await parseCsvPreview(makeFile("col\n1\ntwo\n3\n")); expect(preview.inferredTypes[0]).toBe("text"); diff --git a/csv-table-hub-main/src/__tests__/job-outcome.test.ts b/csv-table-hub-main/src/__tests__/job-outcome.test.ts index 0638a7a..17180bb 100644 --- a/csv-table-hub-main/src/__tests__/job-outcome.test.ts +++ b/csv-table-hub-main/src/__tests__/job-outcome.test.ts @@ -8,7 +8,7 @@ import { describe, it, expect } from "vitest"; import { classifyUploadResult, classifyThrownError, buildCancelPatch, - buildCancelToastMessage, decideOverwriteAction, + buildCancelToastMessage, decideOverwriteAction, failedRowsNote, } from "@/lib/job-outcome"; import type { UploadResult } from "@/lib/csv.functions"; import type { Job } from "@/lib/job-types"; @@ -40,6 +40,8 @@ describe("classifyUploadResult", () => { "widgets.csv: overwrote · 8 of 10 rows imported · 2 failed", ); expect(outcome.patch.overwritten).toBe(true); + expect(outcome.toast.type).toBe("warning"); // #123: failed rows are not a plain success + expect(outcome.notify?.title).toBe("Import complete with failed rows"); }); it("status duplicate_file -> duplicate patch, warning toast, no invalidateRoute", () => { @@ -134,3 +136,14 @@ describe("decideOverwriteAction", () => { expect(decideOverwriteAction(undefined)).toEqual({ action: "retry-overwrite" }); }); }); + +describe("failedRowsNote (#123)", () => { + it("names failed rows out of the total", () => { + expect(failedRowsNote({ failed_rows: 51, total_rows: 171 })).toBe("51 of 171 rows failed"); + }); + it("is null when nothing failed or counts predate #89", () => { + expect(failedRowsNote({ failed_rows: 0, total_rows: 10 })).toBeNull(); + expect(failedRowsNote({ failed_rows: null, total_rows: null })).toBeNull(); + expect(failedRowsNote({})).toBeNull(); + }); +}); diff --git a/csv-table-hub-main/src/lib/csv-preview.ts b/csv-table-hub-main/src/lib/csv-preview.ts index 4c0f7c1..b359664 100644 --- a/csv-table-hub-main/src/lib/csv-preview.ts +++ b/csv-table-hub-main/src/lib/csv-preview.ts @@ -69,20 +69,28 @@ const DATE_RE = /^\d{4}-\d{2}-\d{2}$/; const BOOL_TRUE = new Set(["true", "t", "yes", "y", "1"]); const BOOL_FALSE = new Set(["false", "f", "no", "n", "0"]); +// #123: date and timestamptz are inferred only for values the API's cast_value +// accepts (ISO 8601). new Date() also reads "27 Oct 26", which the API rejects, +// so a looser check here made every row of such a file fail on upload. +const ISO_TS_RE = /^\d{4}-\d{2}-\d{2}[T ]\d{2}:\d{2}(:\d{2}(\.\d{1,6})?)?(Z|[+-]\d{2}(:?\d{2})?)?$/; + +function realDate(s: string): boolean { + const d = new Date(`${s.slice(0, 10)}T00:00:00Z`); + return !isNaN(d.getTime()) && d.toISOString().slice(0, 10) === s.slice(0, 10); +} + function inferCell(v: string): Set { const s = v.trim(); if (s === "") return new Set(["int8", "numeric", "date", "timestamptz", "boolean", "text"]); const out = new Set(["text"]); if (INT_RE.test(s)) out.add("int8"); if (NUM_RE.test(s)) out.add("numeric"); - if (DATE_RE.test(s)) out.add("date"); const lower = s.toLowerCase(); if (BOOL_TRUE.has(lower) || BOOL_FALSE.has(lower)) out.add("boolean"); - // timestamptz: anything Date parses that also has a T or space + colon - if (!DATE_RE.test(s) && /[T ]\d/.test(s) && !isNaN(new Date(s).getTime())) { + if (DATE_RE.test(s) && realDate(s)) { + out.add("date"); out.add("timestamptz"); - } else if (DATE_RE.test(s)) { - // date is already added; also timestamptz-compatible + } else if (ISO_TS_RE.test(s) && realDate(s) && !isNaN(new Date(s).getTime())) { out.add("timestamptz"); } return out; diff --git a/csv-table-hub-main/src/lib/csv.functions.ts b/csv-table-hub-main/src/lib/csv.functions.ts index 6850516..1287ae5 100644 --- a/csv-table-hub-main/src/lib/csv.functions.ts +++ b/csv-table-hub-main/src/lib/csv.functions.ts @@ -87,6 +87,8 @@ const csvFileSummarySchema = z.object({ file_name: z.string(), table_name: z.string(), row_count: z.number(), + total_rows: z.number().nullable().optional(), + failed_rows: z.number().nullable().optional(), column_names: z.array(z.string()), created_at: z.string(), }); diff --git a/csv-table-hub-main/src/lib/job-outcome.ts b/csv-table-hub-main/src/lib/job-outcome.ts index 7603fec..ba46c8c 100644 --- a/csv-table-hub-main/src/lib/job-outcome.ts +++ b/csv-table-hub-main/src/lib/job-outcome.ts @@ -43,8 +43,11 @@ export function classifyUploadResult( replacedFileName: res.replacedFileName, headerMapping: mapping, }, - toast: { type: "success", message: msg }, - notify: { title: "Import complete", body: msg }, + toast: { type: res.failedRows ? "warning" : "success", message: msg }, + notify: { + title: res.failedRows ? "Import complete with failed rows" : "Import complete", + body: msg, + }, invalidateRoute: true, }; } @@ -132,3 +135,14 @@ export function decideOverwriteAction(job: Job | undefined): OverwriteAction { if (job?.tableName) return { action: "show-diff", job }; return { action: "retry-overwrite" }; } + +/** "51 of 171 rows failed", for a registered upload with failures (#123); else null. */ +export function failedRowsNote(f: { + failed_rows?: number | null; + total_rows?: number | null; +}): string | null { + if (!f.failed_rows) return null; + return f.total_rows + ? `${f.failed_rows} of ${f.total_rows} rows failed` + : `${f.failed_rows} rows failed`; +} diff --git a/csv-table-hub-main/src/routes/_authenticated/index.tsx b/csv-table-hub-main/src/routes/_authenticated/index.tsx index ae9f15a..b5bad0f 100644 --- a/csv-table-hub-main/src/routes/_authenticated/index.tsx +++ b/csv-table-hub-main/src/routes/_authenticated/index.tsx @@ -36,7 +36,7 @@ import { downloadErrorReport, buildErrorRows, type ExportFormat } from "@/lib/ex import type { Job, JobStatus } from "@/lib/job-types"; import { classifyUploadResult, classifyThrownError, buildCancelPatch, buildCancelToastMessage, - decideOverwriteAction, + decideOverwriteAction, failedRowsNote, } from "@/lib/job-outcome"; import { Button } from "@/components/ui/button"; import { Card, CardContent, CardHeader, CardTitle } from "@/components/ui/card"; @@ -690,7 +690,7 @@ function FilesList({files}:{files:CsvFileSummary[]}){ <>
#FileTableColsRowsUploaded - {visible.map((f,i)=>{startIndex+i+1}{f.file_name}{f.table_name}{f.column_names.length}{f.row_count})} + {visible.map((f,i)=>{startIndex+i+1}{f.file_name}{f.table_name}{f.column_names.length}{f.row_count}{failedRowsNote(f)&&{failedRowsNote(f)}})}
@@ -741,7 +741,7 @@ function PreviewTable({file}:{file:CsvFileSummary}){ {rows.length===0&&No rows.} -

Showing up to 50 rows. Table has {file.row_count} row{file.row_count===1?"":"s"} total.

+

Showing up to 50 rows. Table has {file.row_count} row{file.row_count===1?"":"s"} total.{failedRowsNote(file)&& {failedRowsNote(file)} validation and weren't imported.}

); } diff --git a/docs/VCRM.md b/docs/VCRM.md index 69f6b33..b9db6d8 100644 --- a/docs/VCRM.md +++ b/docs/VCRM.md @@ -152,7 +152,7 @@ Status as assessed on 2026-10-26 at `main` 032b522. None of these is "Verified" | BR-31 | ⚠️ Partial | T&E mode only; overwrite drops tables | Integration | M5 | | BR-32 | ❌ Not started | No code | API + e2e | M6 | | BR-33 | ⚠️ Partial | Single-transaction load; no staging or lineage | Integration incl. failure injection | M6 | -| BR-34 | ⚠️ Partial | Total, inserted, duplicate and failed counts persisted per import (M1: #90, `tests/test_import_counts.py`); over-wide rows reported, not dropped (M1: #88, `tests/test_ragged_rows.py`); excluded and edited rows not yet tracked (M6) | Integration | M1, M6 | +| BR-34 | ⚠️ Partial | Total, inserted, duplicate and failed counts persisted per import (M1: #90, `tests/test_import_counts.py`); an upload where every row fails is reported as failed and saves nothing, and partial failures are shown on the files list (#123, `tests/test_failed_uploads.py`); over-wide rows reported, not dropped (M1: #88, `tests/test_ragged_rows.py`); excluded and edited rows not yet tracked (M6) | Integration | M1, M6 | | BR-35 | ⚠️ Partial | Export formula injection neutralised (M1: #86, `export.test.ts`); audit table still plain, no actor, no filters | DB negative tests + unit | M1, M3, M7 | | BR-36 | ⚠️ Partial | Done in M2:
• local accounts with lockout (#98, #103)
• OIDC (#105)
• per-tenant schemas, T&E tables included (#100, #104)
• role matrix in `docs/ROLES.md` (#101)

Tests: `tests/test_accounts.py`, `test_tenant_isolation.py`, `test_roles.py`, `test_oidc.py`, `test_password_reset.py`. Each failed with its control removed.
• frontend sign-in; the browser no longer holds the shared key (#115)
• password change, admin reset and break-glass reset (#117; `tests/test_password_reset.py`)

Open: the shared key still grants non-tenant access to scripts and services; security review (#112); real-IdP OIDC check (#113) | Cross-tenant negative tests; role matrix tests | M2 | | BR-37 | ⚠️ Partial | No external calls found; no automated check; no offline doc (#60) | Egress-blocked run + offline dry run | M7 | diff --git a/tests/test_failed_uploads.py b/tests/test_failed_uploads.py new file mode 100644 index 0000000..55c358c --- /dev/null +++ b/tests/test_failed_uploads.py @@ -0,0 +1,74 @@ +"""An upload where every row fails is a failed upload, not a migrated file (#123). + +Integration: needs a live PostgreSQL. Fails (never skips) without one. +""" + +from __future__ import annotations + +import os +import unittest +import uuid + +import pytest +from fastapi.testclient import TestClient +from psycopg2 import sql + +from api.config import settings +from api.db import Conn +from api.main import app + + +@pytest.mark.integration +class EveryRowFailed(unittest.TestCase): + @classmethod + def setUpClass(cls): + cls._orig_allow_destructive = settings.allow_destructive + settings.allow_destructive = True + cls.ctx = TestClient(app) + cls.client = cls.ctx.__enter__() + + @classmethod + def tearDownClass(cls): + settings.allow_destructive = cls._orig_allow_destructive + cls.ctx.__exit__(None, None, None) + + def setUp(self): + self.tag = f"{os.getpid()}_{uuid.uuid4().hex[:8]}" + self.name = f"allfail_{self.tag}.csv" + + def _upload(self, content: str, types: list[str], overwrite: bool = False) -> dict: + return self.client.post("/api/csv/upload", json={ + "fileName": self.name, "content": content, "types": types, "overwrite": overwrite}).json() + + def _registered(self) -> list[dict]: + return [f for f in self.client.get("/api/csv/files").json() if f["file_name"] == self.name] + + def _table_exists(self, table: str) -> bool: + with Conn() as conn, conn.cursor() as cur: + cur.execute("SELECT to_regclass(%s)", (sql.Identifier(settings.UPLOADS_SCHEMA, table).as_string(cur),)) + return cur.fetchone()[0] is not None + + def test_every_row_failing_is_an_error_and_saves_nothing(self): + # The #123 file: dates like "27 Oct 26" sent as timestamptz, which the API rejects. + content = f"Date,Ref\n27 Oct 26,{self.tag}a\n28 Oct 26,{self.tag}b\n" + res = self._upload(content, ["timestamptz", "text"]) + self.assertEqual(res["status"], "error", res) + self.assertIn("None of the 2 rows could be imported", res["message"]) + self.assertIn('row 1, column "date"', res["message"]) + self.assertEqual((res["totalRows"], res["failedRows"], len(res["rowErrors"])), (2, 2, 2)) + self.assertEqual(self._registered(), [], "a file with no imported rows must not be listed as migrated") + + def test_failed_overwrite_keeps_the_previous_upload(self): + good = self._upload(f"n\n1\n2\n{self.tag}\n", ["text"]) + self.assertEqual(good["status"], "ok", good) + res = self._upload(f"n\nx{self.tag}\ny{self.tag}\n", ["int8"], overwrite=True) + self.assertEqual(res["status"], "error", res) + files = self._registered() + self.assertEqual([(f["table_name"], f["row_count"]) for f in files], [(good["tableName"], 3)]) + self.assertTrue(self._table_exists(good["tableName"]), "the previous table must survive") + + def test_partly_failed_upload_still_imports_and_reports_the_failures(self): + res = self._upload(f"n,t\n1,{self.tag}\nx,{self.tag}\n", ["int8", "text"]) + self.assertEqual((res["status"], res["insertedRows"], res["failedRows"]), ("ok", 1, 1), res) + [entry] = self._registered() + self.assertEqual((entry["row_count"], entry["total_rows"], entry["failed_rows"]), (1, 2, 1))