fix(pg-sql2): use same placeholder for same sql.value node (#705)

Author: benjie

packages/pg-sql2/__tests__/general.test.js

@@ -94,14 +94,47 @@ describe("sql.compile", () => {
     expect(sql.compile(node)).toEqual({ text: "select 1", values: [] });
   });
 
-  it("with values", () => {
+  it("with value", () => {
     const node = sql.query`select ${sql.value(1)}::integer`;
     expect(sql.compile(node)).toEqual({
       text: "select $1::integer",
       values: [1],
     });
   });
 
+  it("with two values", () => {
+    const node = sql.query`select ${sql.value(1)}::integer + ${sql.value(
+      2
+    )}::integer`;
+    expect(sql.compile(node)).toEqual({
+      text: "select $1::integer + $2::integer",
+      values: [1, 2],
+    });
+  });
+
+  it("with similar value twice", () => {
+    // This _should not_ use the same placeholder as it makes the compiled SQL
+    // dependent on the values used; better for the SQL to be stable.
+    const node = sql.query`select ${sql.value(1)}::integer + ${sql.value(
+      1
+    )}::integer`;
+    expect(sql.compile(node)).toEqual({
+      text: "select $1::integer + $2::integer",
+      values: [1, 1],
+    });
+  });
+
+  it("with exact same value node twice", () => {
+    // This _should_ use the same placeholder because we've explicitly used the
+    // same `sql.value` node.
+    const sqlValue = sql.value(1);
+    const node = sql.query`select ${sqlValue}::integer + ${sqlValue}::integer`;
+    expect(sql.compile(node)).toEqual({
+      text: "select $1::integer + $1::integer",
+      values: [1],
+    });
+  });
+
   it("with sub-sub-sub query", () => {
     const node = sql.query`select ${sql.query`1 ${sql.query`from ${sql.query`foo`}`}`}`;
     expect(sql.compile(node)).toEqual({

packages/pg-sql2/src/index.ts

@@ -102,6 +102,10 @@ export function compile(sql: SQLQuery | SQLNode): QueryConfig {
   // compile time.
   const values = [];
 
+  // If we use the exact same `sql.value` node more than once, we should use
+  // the same placeholder for both (for performance and efficiency)
+  const valueNodeToPlaceholder = new Map();
+
   // When we come accross a symbol in our identifier, we create a unique
   // alias for it that shouldn’t be in the users schema. This helps maintain
   // sanity when constructing large Sql queries with many aliases.
@@ -145,10 +149,19 @@ export function compile(sql: SQLQuery | SQLNode): QueryConfig {
         );
         break;
       }
-      case "VALUE":
-        values.push(item.value);
-        sqlFragments.push(`$${values.length}`);
+      case "VALUE": {
+        let placeholder = valueNodeToPlaceholder.get(item);
+
+        // If there's no placeholder for this value node, create one
+        if (!placeholder) {
+          values.push(item.value);
+          placeholder = `$${values.length}`;
+          valueNodeToPlaceholder.set(item, placeholder);
+        }
+
+        sqlFragments.push(placeholder);
         break;
+      }
       default:
       // This cannot happen
     }