Separate the JSON serialization probe from the schema DDL in PostgresKvStore

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

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]

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

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<void> {
@@ -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<void> {
     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,
-    });
   }
 
   /**