diff --git a/CHANGES.md b/CHANGES.md index a19d43814..e007bd4c4 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -10,6 +10,13 @@ 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], [#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 @@ -19,7 +26,9 @@ 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 +[#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 new file mode 100644 index 000000000..a1c8d17f8 --- /dev/null +++ b/changes.d/postgres/kv-initialized-json-probe.md @@ -0,0 +1,7 @@ + - 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], [#1033] 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, - }); } /**