diff --git a/scripts/pb-add-ask-collections.py b/scripts/pb-add-ask-collections.py index 4ba5939..64c8b57 100644 --- a/scripts/pb-add-ask-collections.py +++ b/scripts/pb-add-ask-collections.py @@ -13,13 +13,32 @@ appending, never delete or replace unrelated schema. All rules are left null (superuser only) — the Next.js server is the only client of these three collections; there is no browser-direct access. + +``created``/``updated`` must be PocketBase ``autodate`` fields (they were +first shipped as plain ``date`` fields, which PocketBase never populates on +its own — every row's ``created`` has been empty since). ``--apply`` also +migrates ``ask_threads``, ``ask_messages`` and ``ask_log`` in place: any +``created``/``updated`` field still typed ``date`` on those three +collections is replaced with an ``autodate`` field of the same name. +Existing rows keep an empty ``created`` either way — PocketBase can't +backfill an autodate field to a historical value — so ``--backfill`` +(only meaningful with ``--apply``) just reports how many rows are affected +per collection rather than attempting a write that would do nothing. """ import sys +import urllib.parse from pb_env import PB, http, superuser_token DRY_RUN = "--dry-run" in sys.argv or "--apply" not in sys.argv APPLY = "--apply" in sys.argv +BACKFILL = "--backfill" in sys.argv + +MIGRATED_COLLECTIONS = ("ask_threads", "ask_messages", "ask_log") +ASK_MESSAGES_THREAD_CREATED_INDEX = ( + "CREATE INDEX `idx_ask_messages_thread_created` " + "ON `ask_messages` (`thread`, `created`)" +) def confirmation_host(): @@ -68,11 +87,15 @@ def select(name, values, required=False, max_select=1): } +def autodate_field(name): + options = {"onCreate": True} + if name == "updated": + options["onUpdate"] = True + return field(name, "autodate", **options) + + def system_fields(): - return [ - field("created", "date", onCreate=True), - field("updated", "date", onCreate=True, onUpdate=True), - ] + return [autodate_field("created"), autodate_field("updated")] def add_fields(token, name, wanted): @@ -112,6 +135,116 @@ def add_indexes(token, name, wanted): sys.exit(f"PATCH {name} indexes failed: {code} {response}") +def stale_date_field_names(fields): + """Names of ``created``/``updated`` fields still typed ``date``. Pure.""" + return sorted( + f["name"] for f in fields + if f.get("name") in ("created", "updated") and f.get("type") == "date" + ) + + +def migration_fields(existing_fields, stale_names): + """The full ``fields`` list for the migration PATCH: every field that + isn't one of ``stale_names`` kept as-is, plus a fresh ``autodate`` + field for each stale name. Omitting the old field's ``id`` here is + what makes PocketBase drop it — this list represents the *entire* + desired field set. Pure — no network calls, easy to unit test. + """ + kept = [f for f in existing_fields if f.get("name") not in stale_names] + return kept + [autodate_field(n) for n in sorted(stale_names)] + + +def migrate_system_fields(token, name): + """On one of the three Ask collections, replace any ``created``/ + ``updated`` field still typed ``date`` with an ``autodate`` field of + the same name. Idempotent — a second run finds nothing stale. Existing + rows keep an empty ``created``/``updated``; see ``--backfill``. + """ + collection = require_collection(token, name) + fields = list(collection.get("fields", [])) + stale = stale_date_field_names(fields) + if not stale: + print(f" {name}: created/updated already autodate") + return + + print(f" {name}: migrate {stale} from date -> autodate") + replaced = migration_fields(fields, set(stale)) + if DRY_RUN: + print(f" {name}: would PATCH fields (delete+recreate {stale})") + return + + code, response = http( + "PATCH", f"/api/collections/{collection['id']}", {"fields": replaced}, token, + ) + if code != 200: + # Some PocketBase versions refuse to delete and add a field with + # the same name in a single PATCH — fall back to two PATCHes: + # delete the stale fields first, then add the autodate ones. + print(f" {name}: combined PATCH failed ({code}: {response}); trying delete then add") + kept = [f for f in fields if f.get("name") not in set(stale)] + code, response = http( + "PATCH", f"/api/collections/{collection['id']}", {"fields": kept}, token, + ) + if code != 200: + sys.exit(f" {name}: delete PATCH failed: {code} {response}") + code, current = http("GET", f"/api/collections/{name}", token=token) + if code != 200: + sys.exit(f" {name}: re-fetch after delete failed: {code} {current}") + current_fields = list(current.get("fields", [])) + code, response = http( + "PATCH", f"/api/collections/{current['id']}", + {"fields": current_fields + [autodate_field(n) for n in stale]}, token, + ) + if code != 200: + sys.exit(f" {name}: add PATCH failed: {code} {response}") + + # The thread/created index is defined on the field we just replaced — + # re-add it if PocketBase dropped it along with the old field. + if name == "ask_messages": + code, current = http("GET", f"/api/collections/{name}", token=token) + if code != 200: + sys.exit(f" {name}: re-fetch failed: {code} {current}") + current_indexes = list(current.get("indexes", []) or []) + if ASK_MESSAGES_THREAD_CREATED_INDEX not in current_indexes: + print(f" {name}: re-adding idx_ask_messages_thread_created (dropped with the field)") + code, response = http( + "PATCH", f"/api/collections/{current['id']}", + {"indexes": current_indexes + [ASK_MESSAGES_THREAD_CREATED_INDEX]}, token, + ) + if code != 200: + sys.exit(f" {name}: index PATCH failed: {code} {response}") + print(f" {name}: migrated") + + +def count_empty_created(token, name): + query = urllib.parse.quote("created = ''") + code, response = http( + "GET", f"/api/collections/{name}/records?perPage=1&filter={query}", token=token, + ) + if code != 200: + sys.exit(f" {name}: could not count empty-created rows: {code} {response}") + return response.get("totalItems", 0) + + +def backfill_report(token): + """``--backfill``: PocketBase only ever sets an ``autodate`` field to + *now*, on create (and on update, if ``onUpdate`` is set) — never to an + arbitrary historical value, so there is nothing a script can write + into an already-empty ``created`` that would be correct. This just + counts the affected rows per collection; it never issues a write. + """ + print("\nbackfill (--backfill): reporting only, autodate can't be set to a past value") + for name in MIGRATED_COLLECTIONS: + total = count_empty_created(token, name) + if total == 0: + print(f" {name}: no rows with empty created") + else: + print( + f" {name}: {total} row(s) have empty created; left as-is. " + "The read side orders these by id (see threads.ts)." + ) + + def create_collection(token, name, fields, indexes): existing = get_collection(token, name) if existing is not None: @@ -142,13 +275,39 @@ def self_test(): assert f == {"name": "text", "type": "text", "required": False, "max": 100000} s = select("kind", ["user", "assistant", "attachment", "image"]) assert s["values"] == ["user", "assistant", "attachment", "image"] - print("self-test: field builders passed") + + assert system_fields() == [ + {"name": "created", "type": "autodate", "required": False, "onCreate": True}, + {"name": "updated", "type": "autodate", "required": False, "onCreate": True, "onUpdate": True}, + ] + + stale_fields = [ + {"id": "f1", "name": "created", "type": "date", "onCreate": True}, + {"id": "f2", "name": "updated", "type": "date", "onCreate": True, "onUpdate": True}, + {"id": "f3", "name": "title", "type": "text"}, + ] + assert stale_date_field_names(stale_fields) == ["created", "updated"] + assert stale_date_field_names([{"name": "created", "type": "autodate"}]) == [] + + migrated = migration_fields(stale_fields, {"created", "updated"}) + assert migrated == [ + {"id": "f3", "name": "title", "type": "text"}, + {"name": "created", "type": "autodate", "required": False, "onCreate": True}, + {"name": "updated", "type": "autodate", "required": False, "onCreate": True, "onUpdate": True}, + ] + # No stale "id" leaks through on the replacement fields — that is what + # tells PocketBase to drop the old field and create a new one. + assert "id" not in migrated[1] and "id" not in migrated[2] + + print("self-test: field builders and migration helpers passed") return 0 def main(): if "--self-test" in sys.argv: return self_test() + if BACKFILL and not APPLY: + sys.exit("--backfill requires --apply") if APPLY and confirmation_host() != PB: sys.exit( "REFUSING --apply: pass --confirm-host with the exact configured " @@ -166,6 +325,13 @@ def main(): print(f"target: {PB} mode: {'DRY RUN' if DRY_RUN else 'APPLY'}") + print("\nmigrate created/updated to autodate:") + for collection_name in MIGRATED_COLLECTIONS: + if get_collection(token, collection_name) is None: + print(f" {collection_name}: does not exist yet, nothing to migrate") + continue + migrate_system_fields(token, collection_name) + users = require_collection(token, "users") sites = require_collection(token, "sites") users_id = users["id"] @@ -248,6 +414,9 @@ def main(): ], ) + if BACKFILL: + backfill_report(token) + print("\nDRY RUN. No schema changes written." if DRY_RUN else "\nSchema applied.") return 0 diff --git a/src/app/api/chat/threads/[id]/route.test.ts b/src/app/api/chat/threads/[id]/route.test.ts index 476cd8f..2c341a4 100644 --- a/src/app/api/chat/threads/[id]/route.test.ts +++ b/src/app/api/chat/threads/[id]/route.test.ts @@ -67,6 +67,13 @@ describe("/api/chat/threads/[id]", () => { expect(body.rows).toHaveLength(1); }); + it("GET sorts messages by created then id, so equal-created rows are stable", async () => { + getOneThreadMock.mockResolvedValue({ id: "t1", owner: "u1" }); + getFullListMessagesMock.mockResolvedValue([]); + await GET(req("GET", { "x-ask-user": "u1" }), { params }); + expect(getFullListMessagesMock.mock.calls[0][0]).toMatchObject({ sort: "created,id" }); + }); + it("PATCH renames and archives", async () => { getOneThreadMock.mockResolvedValue({ id: "t1", owner: "u1" }); updateThreadMock.mockResolvedValue({ id: "t1", title: "Renamed", archived: true }); diff --git a/src/app/api/chat/threads/[id]/route.ts b/src/app/api/chat/threads/[id]/route.ts index a476493..a021677 100644 --- a/src/app/api/chat/threads/[id]/route.ts +++ b/src/app/api/chat/threads/[id]/route.ts @@ -41,9 +41,12 @@ export async function GET( const thread = await ownedThread(pb, id, actor.id); if (!thread) return NOT_FOUND; + // `,id` is a stable tiebreaker for rows sharing the same `created` (in + // particular pre-autodate-migration rows, which all have an empty + // `created`) — see the matching note in lib/chat/threads.ts. const rows = await pb.collection("ask_messages").getFullList({ filter: pb.filter("thread = {:thread}", { thread: id }), - sort: "created", + sort: "created,id", }); return Response.json({ thread, rows }); diff --git a/src/lib/chat/threads.test.ts b/src/lib/chat/threads.test.ts index 7736622..accef37 100644 --- a/src/lib/chat/threads.test.ts +++ b/src/lib/chat/threads.test.ts @@ -128,7 +128,7 @@ describe("loadThreadEntries", () => { { role: "user", text: "hi" }, { role: "assistant", text: "hello", toolCalls: [] }, ]); - expect(getFullListMock.mock.calls[0][0]).toMatchObject({ sort: "created" }); + expect(getFullListMock.mock.calls[0][0]).toMatchObject({ sort: "created,id" }); }); }); diff --git a/src/lib/chat/threads.ts b/src/lib/chat/threads.ts index da97b90..b616f3d 100644 --- a/src/lib/chat/threads.ts +++ b/src/lib/chat/threads.ts @@ -124,12 +124,19 @@ export async function getRecentActionIds(threadId: string): Promise { return last?.action_ids ?? []; } -/** Loads a thread's rows from PocketBase, ordered by `created`, and maps them. */ +/** + * Loads a thread's rows from PocketBase, ordered by `created`, and maps + * them. `,id` is a stable tiebreaker for rows sharing the same `created` — + * in particular the pre-autodate-migration rows, which all have an empty + * `created` and would otherwise come back in an undefined order. Old test + * threads from before the migration may still show slightly out of order + * among themselves; there's no recoverable insertion order for those. + */ export async function loadThreadEntries(threadId: string): Promise { const pb = await pbAdmin(); const rows = (await pb.collection("ask_messages").getFullList({ filter: pb.filter("thread = {:threadId}", { threadId }), - sort: "created", + sort: "created,id", })) as unknown as ThreadMessageRow[]; return mapRowsToEntries(rows); }