From 222c0b1c29cc05d1569e517ff9cfc9d0ae3a2369 Mon Sep 17 00:00:00 2001 From: "n.siegfried" Date: Tue, 1 Sep 2026 15:26:37 -0700 Subject: [PATCH] CR-005/D6 fix - a bad CSV row rejects by line number instead of 500ing Postgres Nick's real location list hit the production import and got 'Internal Server Error' with no line number - BL-027's class again, three days after the migration outage: Postgres enforces VARCHAR lengths and refuses control bytes, SQLite shrugs at both, and the importers were only ever rehearsed on SQLite. Reproduced both hazards locally (an over-long value and a NUL byte import cleanly on SQLite; either 500s Postgres wholesale). Both importers now validate per row, before any INSERT, so every dialect answers the same way - with the line number and a reason: - locations: control characters; names over 200; codes over 60; combined paths over 200 (checked where the path exists, with read-counts taken before the loop so a mid-loop rejection is not counted twice) - materials: control characters; description/unit/code over 300/20/80 And the client stops lying about it: wp-list-import.js read every response with r.json(), so a plain-text 500 threw mid-parse and surfaced as 'Could not reach the server' while the server was answering fine. One tolerant reader (text -> parse if it parses -> keep status) now serves import, add and patch; a real error reads 'Import refused - HTTP 500'. Pins: materials_check +2 (over-long and control-byte rows reject at line, 20/20), locations_check +1 (over-long name rejects at line, 59/59). Items: CR-005, D6, BL-027 (second instance of its class; the probe-side dialect guard it proposes is still open). Co-Authored-By: Claude Fable 5 --- docs/reference/file-map.md | 2 +- html/wp-list-import.js | 18 +++++++++++++++--- server/app.py | 37 ++++++++++++++++++++++++++++++++++++- tests/locations_check.py | 7 +++++++ tests/materials_check.py | 14 ++++++++++++++ 5 files changed, 73 insertions(+), 5 deletions(-) diff --git a/docs/reference/file-map.md b/docs/reference/file-map.md index b513009..54e6428 100644 --- a/docs/reference/file-map.md +++ b/docs/reference/file-map.md @@ -292,7 +292,7 @@ Wave 8 adds these: ```bash python tests/kitting_check.py # CR-009/010/012 - statuses, owner, delivery 26 checks python tests/kitting_notify_check.py # CR-011 - kitting mail, coalesced, gated 17 checks -python tests/materials_check.py # D6 - material list, the CR-005 pattern 17 checks +python tests/materials_check.py # D6 - material list, the CR-005 pattern 20 checks python tests/mreq_check.py # CR-013 - lightweight request, end to end 19 checks ``` diff --git a/html/wp-list-import.js b/html/wp-list-import.js index 412e25e..ae96c11 100644 --- a/html/wp-list-import.js +++ b/html/wp-list-import.js @@ -105,6 +105,18 @@ say(bits.join(''), problem); } + // A 500 answers plain text ("Internal Server Error"), and r.json() on that + // throws - which used to land in catch() and read as "could not reach the + // server" while the server was answering fine (found 2026-08-23, the + // production locations import). Read text, parse if it parses, keep status. + function readJson(r) { + return r.text().then(function (t) { + var j = null; + try { j = t ? JSON.parse(t) : null; } catch (e) { /* not JSON: a proxy or 500 page */ } + return { ok: r.ok, status: r.status, body: j }; + }); + } + function importText(dryRun) { var text = (el(p + '-paste') || {}).value || ''; if (!text.trim()) { say('Paste some rows or choose a CSV file first.', true); return; } @@ -114,7 +126,7 @@ method: 'POST', headers: { 'Content-Type': 'application/json', 'Accept': 'application/json' }, body: JSON.stringify({ text: text, dry_run: !!dryRun }), }) - .then(function (r) { return r.json().then(function (j) { return { ok: r.ok, status: r.status, body: j }; }); }) + .then(readJson) .then(function (res) { if (!res.ok) { say('⚠ Import refused — ' + esc((res.body && res.body.detail) || ('HTTP ' + res.status)), true); @@ -142,7 +154,7 @@ method: 'POST', headers: { 'Content-Type': 'application/json', 'Accept': 'application/json' }, body: JSON.stringify(read.payload), }) - .then(function (r) { return r.json().then(function (j) { return { ok: r.ok, status: r.status, body: j }; }); }) + .then(readJson) .then(function (res) { if (!res.ok) { setAddError((res.body && res.body.detail) || ('Could not add it (HTTP ' + res.status + ')')); @@ -161,7 +173,7 @@ method: 'PATCH', headers: { 'Content-Type': 'application/json', 'Accept': 'application/json' }, body: JSON.stringify(patchBody), }) - .then(function (r) { return r.json().then(function (j) { return { ok: r.ok, status: r.status, body: j }; }); }) + .then(readJson) .then(function (res) { if (!res.ok) { say('⚠ ' + esc((res.body && res.body.detail) || ('HTTP ' + res.status)), true); diff --git a/server/app.py b/server/app.py index 1a381fe..c12afa3 100644 --- a/server/app.py +++ b/server/app.py @@ -2388,6 +2388,24 @@ def parse_location_rows(text: str) -> tuple[list[tuple[int, list[str]]], list[di rejected.append({"line": i, "text": line, "reason": "no letters or digits to make a code from"}) continue + # Postgres enforces VARCHAR lengths and refuses NUL/control bytes; + # SQLite shrugs at both - which is how ONE bad CSV line 500'd the whole + # production import (2026-08-23, BL-027's class again) instead of coming + # back as a rejection with its line number. Validate per row, here, + # so every dialect answers the same way: with a reason. + if any(any(ord(ch) < 32 for ch in p) for p in parts): + rejected.append({"line": i, "text": line[:120], + "reason": "contains control characters — re-save the file as plain CSV (UTF-8)"}) + continue + long_p = next((p for p in parts if len(p) > 200), None) + if long_p is not None: + rejected.append({"line": i, "text": line[:120], + "reason": "a name is longer than 200 characters (%d)" % len(long_p)}) + continue + if any(len(location_slug(p)) > 60 for p in parts): + rejected.append({"line": i, "text": line[:120], + "reason": "a code would be longer than 60 characters"}) + continue rows.append((i, parts)) return rows, rejected @@ -2454,6 +2472,7 @@ def import_locations(project_id: str, body: LocationImportIn, require_project_writable(db, user, project_id, "The location list cannot be changed") rows, rejected = parse_location_rows(body.text) + read_total = len(rows) + len(rejected) existing = {n.path: n for n in db.scalars( select(models.LocationNode).where(models.LocationNode.project_id == project_id) @@ -2468,6 +2487,10 @@ def import_locations(project_id: str, body: LocationImportIn, for line_no, parts in rows: segs = [location_slug(p) for p in parts] full = "/".join(segs) + if len(full) > 200: + rejected.append({"line": line_no, "text": "/".join(parts)[:120], + "reason": "the combined path is longer than 200 characters"}) + continue if full in seen_in_file: duplicates.append({"line": line_no, "path": full, "names": parts, "reason": "already on line %d of this import" % seen_in_file[full]}) @@ -2510,7 +2533,7 @@ def import_locations(project_id: str, body: LocationImportIn, result = { "project_id": project_id, "dry_run": bool(body.dry_run), - "read": len(rows) + len(rejected), + "read": read_total, "created": created, "duplicates": duplicates, "reactivated": reactivated, "rejected": rejected, } @@ -3007,6 +3030,18 @@ def parse_material_rows(text: str): rejected.append({"line": i, "text": raw.strip()[:120], "reason": "more than three columns - description, unit, code is the whole shape"}) continue + # Same guard as parse_location_rows: reject what Postgres would refuse + # (VARCHAR limits, control bytes) with the line number, never a 500. + if any(any(ord(ch) < 32 for ch in p) for p in parts): + rejected.append({"line": i, "text": raw.strip()[:120], + "reason": "contains control characters - re-save the file as plain CSV (UTF-8)"}) + continue + caps = ((300, "description"), (20, "unit"), (80, "code")) + long_col = next((("%s is longer than %d characters (%d)" % (label, cap, len(p))) + for (cap, label), p in zip(caps, parts) if len(p) > cap), None) + if long_col: + rejected.append({"line": i, "text": raw.strip()[:120], "reason": long_col}) + continue rows.append((i, parts)) return rows, rejected diff --git a/tests/locations_check.py b/tests/locations_check.py index bee13c8..42d2321 100644 --- a/tests/locations_check.py +++ b/tests/locations_check.py @@ -323,6 +323,13 @@ def run(page, base, tok): {"text": "Probe Building One,Probe Level 1,Probe Sector B"}) chk("the import reports it as reactivated, not created or duplicate", len(again["body"]["reactivated"]) == 1 and not again["body"]["created"], again["body"]) + # The 2026-08-23 production 500, pinned (locations side): Postgres-refused + # values reject by line, on every dialect, never crash the request. + hz = api(page, "POST", "/api/projects/projA/locations/import", + {"text": "Probe Building One," + "Y" * 220 + ",S1", "dry_run": True}) + chk("an over-long name is a line rejection, not a 500", + hz["status"] == 200 and hz["body"]["rejected"] + and "200 characters" in hz["body"]["rejected"][0]["reason"], hz["body"]) same = [n for n in api(page, "GET", "/api/projects/projA/locations?include_inactive=true")["body"]["nodes"] if n["path"] == sec["path"]] diff --git a/tests/materials_check.py b/tests/materials_check.py index c0a1c89..4ec7847 100644 --- a/tests/materials_check.py +++ b/tests/materials_check.py @@ -105,6 +105,20 @@ def main(): _, listing = api(base, "/api/projects/projA/materials", root) chk("...and nothing was written", listing["items"] == []) + # The 2026-08-23 production 500, pinned: what Postgres refuses (VARCHAR + # overflow, control bytes) must come back as a per-line rejection - on + # EVERY dialect - never crash the request. + code, rep = api(base, "/api/projects/projA/materials/import", root, "POST", + {"text": "Sample " + "x" * 300 + ",EA", "dry_run": True}) + chk("an over-long description is a line rejection, not a 500", + code == 200 and rep["rejected"] and "300 characters" in rep["rejected"][0]["reason"] + and not rep["created"], ascii_(rep)) + code, rep = api(base, "/api/projects/projA/materials/import", root, "POST", + {"text": "Sample widget\u0000,EA", "dry_run": True}) + chk("a control byte is a line rejection, not a 500", + code == 200 and rep["rejected"] + and "control characters" in rep["rejected"][0]["reason"], ascii_(rep)) + code, rep = api(base, "/api/projects/projA/materials/import", root, "POST", {"text": text, "dry_run": False}) _, listing = api(base, "/api/projects/projA/materials", root)