Fix crud

src/database/crud.ts

@@ -5,10 +5,9 @@ import Statement from "./statement";
 export type CrudType = `INSERT` | `UPDATE` | `DELETE`;
 
 /**
- * Generates an INSERT, UPDATE or DELETE statement for a table. Every value is a `?` host variable,
- * described in a trailing comment, so the statement is valid SQL as-is. A `bind:` statement with a
- * placeholder value per host variable is appended below it, so running the statement is as easy as
- * editing those values first.
+ * Generates an INSERT, UPDATE or DELETE statement for a table. Every value is a `:name` named
+ * host variable, described in a trailing comment, so the statement is valid SQL once those
+ * variables are declared or bound.
  * @param schema Schema name, as it comes from the catalog
  * @param name Table name, as it comes from the catalog
  * @param columns All columns of the table, in ordinal position order
@@ -38,7 +37,7 @@ function generateInsert(schema: string, name: string, columns: TableColumn[]): s
     ].join(`\n`);
   }
 
-  const values = valuesList(insertColumns);
+  const values = valuesList(insertColumns, new Set());
 
   return [
     ...omissionWarnings(generatedColumns),
@@ -47,8 +46,7 @@ function generateInsert(schema: string, name: string, columns: TableColumn[]): s
     `)`,
     `VALUES (`,
     ...values.lines,
-    `);`,
-    bindStatement(values.bindValues)
+    `);`
   ].join(`\n`);
 }
 
@@ -60,8 +58,11 @@ function generateUpdate(schema: string, name: string, columns: TableColumn[], ke
     throw new Error(`No columns available to update in ${qualify(schema, name)}`);
   }
 
-  const set = setList(setColumns);
-  const where = whereClause(schema, name, columns, keyColumns);
+  // Shared across SET and WHERE so a column used in both places doesn't collide with itself,
+  // and so two columns that sanitize to the same host variable name don't collide either.
+  const usedNames = new Set<string>();
+  const set = setList(setColumns, usedNames);
+  const where = whereClause(schema, name, columns, keyColumns, usedNames);
 
   return [
     ...omissionWarnings(generatedColumns),
@@ -71,38 +72,35 @@ function generateUpdate(schema: string, name: string, columns: TableColumn[], ke
     ...set.lines,
     `WHERE`,
     ...where.lines,
-    `;`,
-    bindStatement([...set.bindValues, ...where.bindValues])
+    `;`
   ].join(`\n`);
 }
 
 function generateDelete(schema: string, name: string, columns: TableColumn[], keyColumns: string[]): string {
-  const where = whereClause(schema, name, columns, keyColumns);
+  const where = whereClause(schema, name, columns, keyColumns, new Set());
 
   return [
     ...where.warnings,
     `DELETE FROM ${qualify(schema, name)}`,
     `WHERE`,
     ...where.lines,
-    `;`,
-    bindStatement(where.bindValues)
+    `;`
   ].join(`\n`);
 }
 
-function whereClause(schema: string, name: string, columns: TableColumn[], keyColumns: string[]) {
+function whereClause(schema: string, name: string, columns: TableColumn[], keyColumns: string[], usedNames: Set<string>) {
   // Without a key, every column is listed so the statement never matches more rows than intended
   const predicateColumns = keyColumns.length ? columns.filter(column => isKeyColumn(column, keyColumns)) : columns;
 
   if (predicateColumns.length === 0) {
     throw new Error(`No columns available to identify a row in ${qualify(schema, name)}`);
   }
 
-  const predicate = wherePredicates(predicateColumns);
+  const predicate = wherePredicates(predicateColumns, usedNames);
 
   return {
     warnings: keyColumns.length ? [] : [`-- No primary or unique key found so every column is listed to identify the row. Adjust as needed.`],
-    lines: predicate.lines,
-    bindValues: predicate.bindValues
+    lines: predicate.lines
   };
 }
 
@@ -154,48 +152,50 @@ function qualify(schema: string, name: string) {
   return `${Statement.delimName(schema)}.${Statement.delimName(name)}`;
 }
 
-/** A `?` host variable per column for a VALUES list, e.g. "?,  -- COL - VARCHAR(20)" */
-function valuesList(columns: TableColumn[]): { lines: string[], bindValues: string[] } {
-  return {
-    lines: columns.map((column, index) => `  ?${index < columns.length - 1 ? `,` : ``}  -- ${describe(column)}`),
-    bindValues: columns.map(placeholderValue)
-  };
+/**
+ * A valid, unquoted identifier to use as a `:name` host variable for a column. Column names from
+ * the catalog can contain characters (quotes, spaces, punctuation) that aren't valid in an
+ * identifier, so those are stripped out. `usedNames` avoids collisions when that stripping makes
+ * two columns resolve to the same name within one statement.
+ */
+function hostVariableName(column: TableColumn, usedNames: Set<string>): string {
+  const stripped = column.COLUMN_NAME.replace(/[^A-Za-z0-9_]/g, ``);
+  const base = !stripped || /^[0-9]/.test(stripped) ? `C${stripped}` : stripped;
+
+  let candidate = base;
+  for (let suffix = 2; usedNames.has(candidate); suffix++) {
+    candidate = `${base}${suffix}`;
+  }
+
+  usedNames.add(candidate);
+  return candidate;
 }
 
-/** A `?` host variable assignment per column for a SET list, e.g. "COL = ?,  -- COL - VARCHAR(20)" */
-function setList(columns: TableColumn[]): { lines: string[], bindValues: string[] } {
+/** A `:name` host variable per column for a VALUES list, e.g. ":NAME,  -- COL - VARCHAR(20)" */
+function valuesList(columns: TableColumn[], usedNames: Set<string>): { lines: string[] } {
   return {
-    lines: columns.map((column, index) => `  ${Statement.delimName(column.COLUMN_NAME)} = ?${index < columns.length - 1 ? `,` : ``}  -- ${describe(column)}`),
-    bindValues: columns.map(placeholderValue)
+    lines: columns.map((column, index) => `  :${hostVariableName(column, usedNames)}${index < columns.length - 1 ? `,` : ``}  -- ${describe(column)}`)
   };
 }
 
-/** A `?` host variable predicate per column for a WHERE clause, joined with AND */
-function wherePredicates(columns: TableColumn[]): { lines: string[], bindValues: string[] } {
+/** A `:name` host variable assignment per column for a SET list, e.g. "COL = :NAME,  -- COL - VARCHAR(20)" */
+function setList(columns: TableColumn[], usedNames: Set<string>): { lines: string[] } {
   return {
-    lines: columns.map((column, index) => `  ${index > 0 ? `AND ` : ``}${Statement.delimName(column.COLUMN_NAME)} = ?  -- ${describe(column)}`),
-    bindValues: columns.map(placeholderValue)
+    lines: columns.map((column, index) => `  ${Statement.delimName(column.COLUMN_NAME)} = :${hostVariableName(column, usedNames)}${index < columns.length - 1 ? `,` : ``}  -- ${describe(column)}`)
   };
 }
 
 /**
- * A value for the `bind:` statement. Must be a string or number literal, so date/time/timestamp
- * columns get a valid literal string rather than an expression like CURRENT DATE.
+ * A `:name` host variable predicate per column for a WHERE clause, joined with AND. A nullable
+ * column is compared with IS NOT DISTINCT FROM, like ACS does, since `=` never matches a null.
  */
-function placeholderValue(column: TableColumn): string {
-  const type = column.DATA_TYPE.toUpperCase();
-
-  if (type === `DATE`) return `'2024-01-01'`;
-  if (type === `TIME`) return `'00:00:00'`;
-  if (type.startsWith(`TIMESTAMP`)) return `'2024-01-01-00.00.00.000000'`;
-  if ([`DECIMAL`, `NUMERIC`, `DECFLOAT`, `FLOAT`, `REAL`, `DOUBLE`, `INTEGER`, `BIGINT`, `SMALLINT`].includes(type)) return `0`;
-
-  // Character types, and anything else not covered above, default to an empty string literal
-  return `''`;
-}
-
-function bindStatement(bindValues: string[]): string {
-  return `bind: ${bindValues.join(`, `)};`;
+function wherePredicates(columns: TableColumn[], usedNames: Set<string>): { lines: string[] } {
+  return {
+    lines: columns.map((column, index) => {
+      const operator = column.IS_NULLABLE === `N` ? `=` : `IS NOT DISTINCT FROM`;
+      return `  ${index > 0 ? `AND ` : ``}${Statement.delimName(column.COLUMN_NAME)} ${operator} :${hostVariableName(column, usedNames)}  -- ${describe(column)}`;
+    })
+  };
 }
 
 function describe(column: TableColumn): string {

src/database/statement.ts

@@ -27,8 +27,8 @@ export default class Statement {
       const validQsysName = new RegExp(`^[A-Z0-9${variant_chars_local}][A-Z0-9_${variant_chars_local}.]{0,9}$`);
       return validQsysName.test(name);
     } else {
-      // Fall back with standard variants
-      return name.match(`[^A-Z0-9_@#$]`);
+      // Fall back with standard variants. Valid when nothing outside that set is present.
+      return !name.match(`[^A-Z0-9_@#$]`);
     }
   }
 
@@ -42,16 +42,17 @@ export default class Statement {
     if (fromUser) { // The name was input by the user
       // If already delimited, return it as-is
       if (name.startsWith(`"`) && name.endsWith(`"`)) return name;
-      // If the value contains a space or decimal it needs to be delimited
-      if (name.includes(` `) || name.includes(`.`) || name.includes(`'`)) return `"${name}"`;
+      // If the value contains a space, decimal, apostrophe or double quote it needs to be delimited
+      if (name.includes(` `) || name.includes(`.`) || name.includes(`'`) || name.includes(`"`)) return `"${name.replace(/"/g, `""`)}"`;
       // Otherwise, fold to uppercase.  The user should have explicitly delimited if that was their intention.
       return name.toUpperCase();
     } else { // The name came from a catalog file query
       // If the name contains characters other than the valid variants, uppercase, digits, or underscores, it must be delimited
       if (Statement.validQsysName(name)) return name;
       else {
-        if (name.includes(` `) || name.includes(`.`) || name.includes(`'`) || name !== name.toUpperCase()) {
-          return `"${name}"`;
+        if (name.includes(` `) || name.includes(`.`) || name.includes(`'`) || name.includes(`"`) || name !== name.toUpperCase()) {
+          // Embedded double quotes must be escaped by doubling them, per the delimited identifier syntax
+          return `"${name.replace(/"/g, `""`)}"`;
         } else {
           return name;
         }

src/tests/suites/crud.test.ts

@@ -29,7 +29,7 @@ const allGenerated: TableColumn[] = [
   column(`C4`, `TIMESTAMP`, { HAS_DEFAULT: `X` }),
 ];
 
-test('Generated columns are left out of an INSERT, and every value is a host variable with a bind statement', () => {
+test('Generated columns are left out of an INSERT, and every value is a named host variable', () => {
   const columns = [
     ...allGenerated,
     column(`NAME`, `VARCHAR`, { CHARACTER_MAXIMUM_LENGTH: 20 }),
@@ -49,10 +49,29 @@ test('Generated columns are left out of an INSERT, and every value is a host var
     `  CREATED`,
     `)`,
     `VALUES (`,
-    `  ?,  -- NAME - VARCHAR(20) NOT NULL`,
-    `  ?  -- CREATED - DATE NOT NULL`,
-    `);`,
-    `bind: '', '2024-01-01';`
+    `  :NAME,  -- NAME - VARCHAR(20) NOT NULL`,
+    `  :CREATED  -- CREATED - DATE NOT NULL`,
+    `);`
+  ].join(`\n`));
+});
+
+test('A column whose catalog name needs delimiting still produces a valid named host variable', () => {
+  const columns = [
+    column(`PARK_NAME`, `VARCHAR`, { CHARACTER_MAXIMUM_LENGTH: 50 }),
+    column(`'thetime"`, `TIMESTAMP`),
+  ];
+
+  const statement = generateCrudStatement(`INSERT`, `annoying`, `annoyingly123`, columns);
+
+  expect(statement).toBe([
+    `INSERT INTO "annoying"."annoyingly123" (`,
+    `  PARK_NAME,`,
+    `  "'thetime"""`,
+    `)`,
+    `VALUES (`,
+    `  :PARK_NAME,  -- PARK_NAME - VARCHAR(50) NOT NULL`,
+    `  :thetime  -- "'thetime""" - TIMESTAMP NOT NULL`,
+    `);`
   ].join(`\n`));
 });
 
@@ -108,7 +127,7 @@ test('An identity column is left out when HAS_DEFAULT says nothing', () => {
   expect(generateCrudStatement(`INSERT`, `RMOELLER`, `MYTABLE`, columns)).toContain(`-- C1 omitted: identity`);
 });
 
-test('Generated columns are left out of the SET list of an UPDATE, which assigns host variables', () => {
+test('Generated columns are left out of the SET list of an UPDATE, which assigns named host variables', () => {
   const columns = [
     ...allGenerated,
     column(`NAME`, `VARCHAR`, { CHARACTER_MAXIMUM_LENGTH: 20 }),
@@ -123,11 +142,10 @@ test('Generated columns are left out of the SET list of an UPDATE, which assigns
     `-- C4 omitted: transaction start ID`,
     `UPDATE RMOELLER.MYTABLE`,
     `SET`,
-    `  NAME = ?  -- NAME - VARCHAR(20) NOT NULL`,
+    `  NAME = :NAME  -- NAME - VARCHAR(20) NOT NULL`,
     `WHERE`,
-    `  C1 = ?  -- C1 - INTEGER`,
-    `;`,
-    `bind: '', 0;`
+    `  C1 IS NOT DISTINCT FROM :C1  -- C1 - INTEGER`,
+    `;`
   ].join(`\n`));
 });
 
@@ -137,9 +155,8 @@ test('A DELETE keeps generated columns in the WHERE clause', () => {
   expect(statement).toBe([
     `DELETE FROM RMOELLER.MYTABLE`,
     `WHERE`,
-    `  C1 = ?  -- C1 - INTEGER`,
-    `;`,
-    `bind: 0;`
+    `  C1 IS NOT DISTINCT FROM :C1  -- C1 - INTEGER`,
+    `;`
   ].join(`\n`));
 });
 
@@ -155,9 +172,40 @@ test('Without a key, an UPDATE/DELETE uses every column in the WHERE clause, joi
     `-- No primary or unique key found so every column is listed to identify the row. Adjust as needed.`,
     `DELETE FROM RMOELLER.MYTABLE`,
     `WHERE`,
-    `  C1 = ?  -- C1 - INTEGER NOT NULL`,
-    `  AND C2 = ?  -- C2 - VARCHAR(10) NOT NULL`,
-    `;`,
-    `bind: 0, '';`
+    `  C1 = :C1  -- C1 - INTEGER NOT NULL`,
+    `  AND C2 = :C2  -- C2 - VARCHAR(10) NOT NULL`,
+    `;`
+  ].join(`\n`));
+});
+
+test('A nullable column is matched with IS NOT DISTINCT FROM in the WHERE clause, but not in the SET list', () => {
+  const columns = [
+    column(`ID`, `INTEGER`),
+    column(`CODE`, `CHAR`, { CHARACTER_MAXIMUM_LENGTH: 3, IS_NULLABLE: `Y` }),
+    column(`NOTE`, `VARCHAR`, { CHARACTER_MAXIMUM_LENGTH: 10, IS_NULLABLE: `Y` }),
+  ];
+
+  const statement = generateCrudStatement(`UPDATE`, `RMOELLER`, `MYTABLE`, columns, [`ID`, `CODE`]);
+
+  expect(statement).toBe([
+    `UPDATE RMOELLER.MYTABLE`,
+    `SET`,
+    `  NOTE = :NOTE  -- NOTE - VARCHAR(10)`,
+    `WHERE`,
+    `  ID = :ID  -- ID - INTEGER NOT NULL`,
+    `  AND CODE IS NOT DISTINCT FROM :CODE  -- CODE - CHAR(3)`,
+    `;`
   ].join(`\n`));
 });
+
+test('Two columns whose names sanitize to the same host variable name get disambiguated', () => {
+  const columns = [
+    column(`A.B`, `INTEGER`),
+    column(`A-B`, `VARCHAR`, { CHARACTER_MAXIMUM_LENGTH: 5 }),
+  ];
+
+  const statement = generateCrudStatement(`INSERT`, `RMOELLER`, `MYTABLE`, columns);
+
+  expect(statement).toContain(`:AB,`);
+  expect(statement).toContain(`:AB2`);
+});