Implement Transaction.withSQLRowMode() method

Uncover two bugs in the process:

- SQL codecs were accessed in different places by two different names
  "_private_sql_row" and "sql_row", so really SQL return configuration
  only worked once :)

- There was a pathway when options object leaked codecs from one
  version to another (because we want to make things fast!)

Both bugs are fixed as part of this commit.

packages/driver/src/baseClient.ts

@@ -48,7 +48,7 @@ import type { BaseRawConnection } from "./baseConn";
 import type { ConnectWithTimeout } from "./retry";
 import { retryingConnect } from "./retry";
 import { util } from "./reflection/util";
-import { Transaction } from "./transaction";
+import { Transaction, TransactionImpl } from "./transaction";
 import { sleep } from "./utils";
 
 export class ClientConnectionHolder {
@@ -131,12 +131,13 @@ export class ClientConnectionHolder {
   ): Promise<T> {
     let result: T | void;
     for (let iteration = 0; ; ++iteration) {
-      const transaction = await Transaction._startTransaction(this);
+      const transaction = await TransactionImpl._startTransaction(this);
+      const clientTx = new Transaction(transaction, this.options);
 
       let commitFailed = false;
       try {
         result = await Promise.race([
-          action(transaction),
+          action(clientTx),
           transaction._waitForConnAbort(),
         ]);
         try {

packages/driver/src/options.ts

@@ -269,21 +269,32 @@ export class Options {
       clone.moduleAliases = this.moduleAliases;
     }
 
+    let originalCodecsMap = false;
     if (mergeOptions.codecs != null) {
       clone.codecs = new Map([
         ...this.codecs,
         ...Object.entries(mergeOptions.codecs),
       ]) as ReadonlyCodecMap;
     } else {
       clone.codecs = this.codecs;
+      originalCodecsMap = true;
     }
 
-    if (mergeOptions._dropSQLRowCodec && clone.codecs.has("sql_row")) {
+    if (mergeOptions._dropSQLRowCodec && clone.codecs.has("_private_sql_row")) {
       // This is an optimization -- if "sql_row" is the only codec defined
       // and it's set to "object mode", the we want the codec mapping to be
       // empty instead. Why? Empty codec mapping short circuits a lot of
       // custom codec code, and object is the default behavior anyway.
-      (clone.codecs as MutableCodecMap).delete("sql_row");
+      if (originalCodecsMap) {
+        // "codecs" map isn't a full-blown read-only mapping, we're just
+        // treating it as such and protecting from the user to mutate
+        // directly. This allows for optimizing away pointless copies.
+        // However, in this case, if we just "inherited" our codecs
+        // mapping from another options object, we have to clone it
+        // before deleting any keys or mutating it in any way.
+        clone.codecs = new Map(clone.codecs);
+      }
+      (clone.codecs as MutableCodecMap).delete("_private_sql_row");
     }
 
     clone.module = mergeOptions.module ?? this.module;

packages/driver/src/transaction.ts

@@ -26,6 +26,7 @@ import {
   Language,
   type SQLQueryArgs,
 } from "./ifaces";
+import type { Options } from "./options";
 
 export enum TransactionState {
   ACTIVE = 0,
@@ -34,7 +35,7 @@ export enum TransactionState {
   FAILED = 3,
 }
 
-export class Transaction implements Executor {
+export class TransactionImpl {
   protected _holder: ClientConnectionHolder;
   private _rawConn: BaseRawConnection;
 
@@ -55,7 +56,7 @@ export class Transaction implements Executor {
   /** @internal */
   static async _startTransaction(
     holder: ClientConnectionHolder,
-  ): Promise<Transaction> {
+  ): Promise<TransactionImpl> {
     const rawConn = await holder._getConnection();
 
     await rawConn.resetState();
@@ -72,7 +73,7 @@ export class Transaction implements Executor {
       true,
     );
 
-    return new Transaction(holder, rawConn);
+    return new TransactionImpl(holder, rawConn);
   }
 
   /** @internal */
@@ -90,7 +91,7 @@ export class Transaction implements Executor {
     }
   }
 
-  private async _runOp<T>(
+  async _runOp<T>(
     opname: string,
     op: () => Promise<T>,
     errMessage?: string,
@@ -121,7 +122,7 @@ export class Transaction implements Executor {
     }
   }
 
-  private async _runFetchOp(
+  async _runFetchOp(
     opName: string,
     ...args: Parameters<BaseRawConnection["fetch"]>
   ) {
@@ -171,144 +172,158 @@ export class Transaction implements Executor {
       "A query is still in progress after transaction block has returned.",
     );
   }
+}
+
+export class Transaction implements Executor {
+  private impl: TransactionImpl;
+  private options: Options;
+
+  constructor(impl: TransactionImpl, options: Options) {
+    this.impl = impl;
+    this.options = options;
+  }
+
+  withSQLRowMode(mode: "array" | "object"): Transaction {
+    return new Transaction(this.impl, this.options.withSQLRowMode(mode));
+  }
 
   async execute(query: string, args?: QueryArgs): Promise<void> {
-    await this._runFetchOp(
+    await this.impl._runFetchOp(
       "execute",
       query,
       args,
       OutputFormat.NONE,
       Cardinality.NO_RESULT,
-      this._holder.options,
+      this.options,
     );
   }
 
   async executeSQL(query: string, args?: SQLQueryArgs): Promise<void> {
-    await this._runFetchOp(
+    await this.impl._runFetchOp(
       "execute",
       query,
       args,
       OutputFormat.NONE,
       Cardinality.NO_RESULT,
-      this._holder.options,
+      this.options,
       false /* privilegedMode */,
       Language.SQL,
     );
   }
 
   async query<T = unknown>(query: string, args?: QueryArgs): Promise<T[]> {
-    return this._runFetchOp(
+    return this.impl._runFetchOp(
       "query",
       query,
       args,
       OutputFormat.BINARY,
       Cardinality.MANY,
-      this._holder.options,
+      this.options,
     );
   }
 
   async querySQL<T = unknown>(
     query: string,
     args?: SQLQueryArgs,
   ): Promise<T[]> {
-    return this._runFetchOp(
+    return this.impl._runFetchOp(
       "query",
       query,
       args,
       OutputFormat.BINARY,
       Cardinality.MANY,
-      this._holder.options,
+      this.options,
       false /* privilegedMode */,
       Language.SQL,
     );
   }
 
   async queryJSON(query: string, args?: QueryArgs): Promise<string> {
-    return this._runFetchOp(
+    return this.impl._runFetchOp(
       "queryJSON",
       query,
       args,
       OutputFormat.JSON,
       Cardinality.MANY,
-      this._holder.options,
+      this.options,
     );
   }
 
   async querySingle<T = unknown>(
     query: string,
     args?: QueryArgs,
   ): Promise<T | null> {
-    return this._runFetchOp(
+    return this.impl._runFetchOp(
       "querySingle",
       query,
       args,
       OutputFormat.BINARY,
       Cardinality.AT_MOST_ONE,
-      this._holder.options,
+      this.options,
     );
   }
 
   async querySingleJSON(query: string, args?: QueryArgs): Promise<string> {
-    return this._runFetchOp(
+    return this.impl._runFetchOp(
       "querySingleJSON",
       query,
       args,
       OutputFormat.JSON,
       Cardinality.AT_MOST_ONE,
-      this._holder.options,
+      this.options,
     );
   }
 
   async queryRequired<T = unknown>(
     query: string,
     args?: QueryArgs,
   ): Promise<[T, ...T[]]> {
-    return this._runFetchOp(
+    return this.impl._runFetchOp(
       "queryRequired",
       query,
       args,
       OutputFormat.BINARY,
       Cardinality.AT_LEAST_ONE,
-      this._holder.options,
+      this.options,
     );
   }
 
   async queryRequiredJSON(query: string, args?: QueryArgs): Promise<string> {
-    return this._runFetchOp(
+    return this.impl._runFetchOp(
       "queryRequiredJSON",
       query,
       args,
       OutputFormat.JSON,
       Cardinality.AT_LEAST_ONE,
-      this._holder.options,
+      this.options,
     );
   }
 
   async queryRequiredSingle<T = unknown>(
     query: string,
     args?: QueryArgs,
   ): Promise<T> {
-    return this._runFetchOp(
+    return this.impl._runFetchOp(
       "queryRequiredSingle",
       query,
       args,
       OutputFormat.BINARY,
       Cardinality.ONE,
-      this._holder.options,
+      this.options,
     );
   }
 
   async queryRequiredSingleJSON(
     query: string,
     args?: QueryArgs,
   ): Promise<string> {
-    return this._runFetchOp(
+    return this.impl._runFetchOp(
       "queryRequiredSingleJSON",
       query,
       args,
       OutputFormat.JSON,
       Cardinality.ONE,
-      this._holder.options,
+      this.options,
     );
   }
 }

packages/driver/test/client.test.ts

@@ -2602,6 +2602,30 @@ if (getEdgeDBVersion().major >= 6) {
     }
   });
 
+  test("withSQLRowMode in tx", async () => {
+    let client = getClient().withSQLRowMode("array");
+
+    try {
+      let res = await client.querySQL("select 1 as c");
+      expect(JSON.stringify(res)).toEqual("[[1]]");
+
+      await client.transaction(async (tx) => {
+        const tx2 = tx.withSQLRowMode("object");
+
+        let res = await tx.querySQL("select 1 AS foo, 2 AS bar");
+        expect(JSON.stringify(res)).toEqual("[[1,2]]");
+
+        res = await tx2.querySQL("select 1 AS foo, 2 AS bar");
+        expect(JSON.stringify(res)).toEqual('[{"foo":1,"bar":2}]');
+      });
+
+      res = await client.querySQL("select 1 + $1::int8", [41]);
+      expect(JSON.stringify(res)).toEqual("[[42]]");
+    } finally {
+      await client.close();
+    }
+  });
+
   test("querySQL codec", async () => {
     let client = getClient();