From 6afe95c271c79492dff988128faeb05c13e3f1d7 Mon Sep 17 00:00:00 2001 From: heeeione <68272931+heeoneie@users.noreply.github.com> Date: Tue, 15 Sep 2026 15:18:57 +0900 Subject: [PATCH 1/2] =?UTF-8?q?Separate=20the=20JSON=20probe=20from=20the?= =?UTF-8?q?=20schema=20DDL=20in=20the=20key=E2=80=93value=20store?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `PostgresKvStore`'s `initialized` option set the very flag that `initialize()` checks before returning early, so passing it skipped all of the method — not only the `CREATE UNLOGGED TABLE` statement but the `driverSerializesJson()` probe after it. `#driverSerializesJson` stayed `false`, `#json()` ran `JSON.stringify()` before handing the value to postgres.js, and the driver serialized it a second time. Values were stored as JSONB strings rather than JSONB objects. Unlike the queue's version of this bug, the bad row stays in the table. Every later read returns a string, including reads from a store that never passed the option, so one misconfigured writer poisons entries that correctly configured readers go on to consume, and `get()` no longer returns what `set()` was given. Gate only the DDL on the option, and let the probe and the initialized flag run unconditionally. `get()`, `set()`, `delete()` and `list()` already await `initialize()` before touching the table, so the probe runs on first use either way and no caller contract changes. Cover both halves of the contract by regression test: that `initialized: true` stores a JSONB object, and that it still does not create the table on its own. Fixes https://github.com/fedify-dev/fedify/issues/1031 Assisted-by: Claude Code:claude-opus-5 --- CHANGES.md | 7 ++ .../postgres/kv-initialized-json-probe.md | 6 + packages/postgres/src/kv.test.ts | 103 ++++++++++++++++++ packages/postgres/src/kv.ts | 23 ++-- 4 files changed, 132 insertions(+), 7 deletions(-) create mode 100644 changes.d/postgres/kv-initialized-json-probe.md diff --git a/CHANGES.md b/CHANGES.md index a19d43814..0f1fb9153 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -10,6 +10,12 @@ To be released. ### @fedify/postgres + - Fixed `PostgresKvStore` storing values as JSONB strings rather than JSONB + objects when it was constructed with the `initialized: true` option. The + option skipped the driver's JSON serialization probe along with the table's + schema DDL, so every value was serialized twice and every later read of the + row returned a string, including reads from a store that never passed the + option. The option now skips only the DDL. [[#1031] by Heewon Chae\] - Fixed `PostgresMessageQueue` storing messages as JSONB strings rather than JSONB objects when it was constructed with the `initialized: true` option. The option skipped the driver's JSON serialization probe along with the @@ -19,6 +25,7 @@ To be released. [[#1014], [#1032] by Heewon Chae\] [#1014]: https://github.com/fedify-dev/fedify/issues/1014 +[#1031]: https://github.com/fedify-dev/fedify/issues/1031 [#1032]: https://github.com/fedify-dev/fedify/issues/1032 diff --git a/changes.d/postgres/kv-initialized-json-probe.md b/changes.d/postgres/kv-initialized-json-probe.md new file mode 100644 index 000000000..6e118dead --- /dev/null +++ b/changes.d/postgres/kv-initialized-json-probe.md @@ -0,0 +1,6 @@ + - Fixed `PostgresKvStore` storing values as JSONB strings rather than JSONB + objects when it was constructed with the `initialized: true` option. The + option skipped the driver's JSON serialization probe along with the table's + schema DDL, so every value was serialized twice and every later read of the + row returned a string, including reads from a store that never passed the + option. The option now skips only the DDL. [[#1031] by Heewon Chae] diff --git a/packages/postgres/src/kv.test.ts b/packages/postgres/src/kv.test.ts index aaee02954..cb1e2062e 100644 --- a/packages/postgres/src/kv.test.ts +++ b/packages/postgres/src/kv.test.ts @@ -249,4 +249,107 @@ test( }, ); +// Regression test for the driver JSON serialization probe being skipped +// together with the schema DDL when `initialized: true` is passed. +// +// `initialize()` does two unrelated things: it runs the `CREATE UNLOGGED +// TABLE` statement, and it sets `#driverSerializesJson` from the +// `driverSerializesJson()` probe. Because the constructor assigned +// `options.initialized` straight into `#initialized`, `initialize()` returned +// at its first line and reached neither, so the flag stayed `false`, `#json()` +// called `JSON.stringify()` before handing the value to postgres.js, and the +// driver serialized it a second time. The value was stored as a JSONB string +// instead of a JSONB object, and unlike the queue's version of this bug the +// bad row stays in the table: every later `get()` returns a string, including +// one from a store that never passed the option. +// +// See: https://github.com/fedify-dev/fedify/issues/1031 +test( + "PostgresKvStore stores JSONB objects when initialized is true", + { skip: dbUrl == null }, + async () => { + if (dbUrl == null) return; // Bun does not support skip option + + const sql = postgres(dbUrl!); + const tableName = `fedify_kv_test_${Math.random().toString(36).slice(5)}`; + const store = new PostgresKvStore(sql, { tableName, initialized: true }); + try { + // Create the table up front, which is the situation `initialized: true` + // describes. The DDL is the same one `initialize()` would have run. + await sql` + CREATE UNLOGGED TABLE IF NOT EXISTS ${sql(tableName)} ( + key text[] PRIMARY KEY, + value jsonb NOT NULL, + created timestamp with time zone DEFAULT CURRENT_TIMESTAMP, + ttl interval + ); + `; + + const value = { keyPair: { id: "https://example.com/actor#main-key" } }; + await store.set(["cache", "a"], value); + + const [row] = await sql` + SELECT value, jsonb_typeof(value) AS json_type + FROM ${sql(tableName)} + WHERE key = ${["cache", "a"]}; + `; + assert.strictEqual( + row.json_type, + "object", + "initialized: true should still store the value as a JSONB object", + ); + assert.deepStrictEqual( + row.value, + value, + "the stored value should round-trip as the original object", + ); + assert.deepStrictEqual( + await store.get(["cache", "a"]), + value, + "get() should return the object that set() was given", + ); + } finally { + await store.drop(); + await sql.end(); + } + }, +); + +// The other half of the same contract: running the probe unconditionally must +// not drag the DDL along with it. If `initialized: true` ever starts creating +// the table again, callers that pass it precisely because they manage their own +// schema would silently get a table they did not ask for, so the missing table +// has to surface as an error instead. +test( + "PostgresKvStore initialized true still skips the schema DDL", + { skip: dbUrl == null }, + async () => { + if (dbUrl == null) return; // Bun does not support skip option + + const sql = postgres(dbUrl!); + const tableName = `fedify_kv_test_${Math.random().toString(36).slice(5)}`; + const store = new PostgresKvStore(sql, { tableName, initialized: true }); + try { + // The table is deliberately never created. + await assert.rejects( + () => store.set(["cache", "a"], { n: 1 }), + (error: unknown) => + error instanceof postgres.PostgresError && error.code === "42P01", + "initialized: true should not create the table on its own", + ); + + const rows = await sql` + SELECT 1 + FROM pg_tables + WHERE schemaname = current_schema() + AND tablename = ${tableName}; + `; + assert.strictEqual(rows.length, 0, "no table should have been created"); + } finally { + await store.drop(); + await sql.end(); + } + }, +); + // cSpell: ignore regclass diff --git a/packages/postgres/src/kv.ts b/packages/postgres/src/kv.ts index e289bca3a..2f075b2a1 100644 --- a/packages/postgres/src/kv.ts +++ b/packages/postgres/src/kv.ts @@ -23,6 +23,10 @@ export interface PostgresKvStoreOptions { /** * Whether the table has been initialized. `false` by default. + * + * This skips only the table's schema DDL. Driver-specific runtime setup, + * such as detecting whether the driver serializes JSON parameters on its + * own, still runs before the first key is read or written. * @default `false` */ readonly initialized?: boolean; @@ -47,7 +51,8 @@ export class PostgresKvStore implements KvStore { // deno-lint-ignore ban-types readonly #sql: Sql<{}>; readonly #tableName: string; - #initialized: boolean; + readonly #skipDdl: boolean; + #initialized = false; #driverSerializesJson = false; /** @@ -62,7 +67,7 @@ export class PostgresKvStore implements KvStore { ) { this.#sql = sql; this.#tableName = options.tableName ?? "fedify_kv_v2"; - this.#initialized = options.initialized ?? false; + this.#skipDdl = options.initialized ?? false; } async #expire(): Promise { @@ -156,6 +161,15 @@ export class PostgresKvStore implements KvStore { logger.debug("Initializing the key–value store table {tableName}...", { tableName: this.#tableName, }); + if (!this.#skipDdl) await this.#initializeTable(); + this.#driverSerializesJson = await driverSerializesJson(this.#sql); + this.#initialized = true; + logger.debug("Initialized the key–value store table {tableName}.", { + tableName: this.#tableName, + }); + } + + async #initializeTable(): Promise { await this.#sql` CREATE UNLOGGED TABLE IF NOT EXISTS ${this.#sql(this.#tableName)} ( key text[] PRIMARY KEY, @@ -164,11 +178,6 @@ export class PostgresKvStore implements KvStore { ttl interval ); `; - this.#driverSerializesJson = await driverSerializesJson(this.#sql); - this.#initialized = true; - logger.debug("Initialized the key–value store table {tableName}.", { - tableName: this.#tableName, - }); } /** From 5b42740a7ebca863cd71a71df37a1462f3e3bdd7 Mon Sep 17 00:00:00 2001 From: heeeione <68272931+heeoneie@users.noreply.github.com> Date: Tue, 15 Sep 2026 15:31:15 +0900 Subject: [PATCH 2/2] Reference the pull request in the changelog fragment The fragment could only cite the issue when it was written, since the pull request did not exist yet. Released entries in this changelog carry both numbers. Assisted-by: Claude Code:claude-opus-5 --- CHANGES.md | 4 +++- changes.d/postgres/kv-initialized-json-probe.md | 3 ++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 0f1fb9153..e007bd4c4 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -15,7 +15,8 @@ To be released. option skipped the driver's JSON serialization probe along with the table's schema DDL, so every value was serialized twice and every later read of the row returned a string, including reads from a store that never passed the - option. The option now skips only the DDL. [[#1031] by Heewon Chae\] + option. The option now skips only the DDL. + [[#1031], [#1033] by Heewon Chae\] - Fixed `PostgresMessageQueue` storing messages as JSONB strings rather than JSONB objects when it was constructed with the `initialized: true` option. The option skipped the driver's JSON serialization probe along with the @@ -27,6 +28,7 @@ To be released. [#1014]: https://github.com/fedify-dev/fedify/issues/1014 [#1031]: https://github.com/fedify-dev/fedify/issues/1031 [#1032]: https://github.com/fedify-dev/fedify/issues/1032 +[#1033]: https://github.com/fedify-dev/fedify/issues/1033 Version 2.0.26 diff --git a/changes.d/postgres/kv-initialized-json-probe.md b/changes.d/postgres/kv-initialized-json-probe.md index 6e118dead..a1c8d17f8 100644 --- a/changes.d/postgres/kv-initialized-json-probe.md +++ b/changes.d/postgres/kv-initialized-json-probe.md @@ -3,4 +3,5 @@ option skipped the driver's JSON serialization probe along with the table's schema DDL, so every value was serialized twice and every later read of the row returned a string, including reads from a store that never passed the - option. The option now skips only the DDL. [[#1031] by Heewon Chae] + option. The option now skips only the DDL. + [[#1031], [#1033] by Heewon Chae]