Repository: knex/knex
Pull request #5738 (2023-11-14 15:55:48)

Missing comments on delete, update and insert
Few monthes ago comments where included in query, but only in select statement. Here is the code to make delete, update and insert compatible with comments

kibertoad: @sebastienroul could you please add tests?
sebastienroul: @kibertoad  Hi, yes sure, but can you give me pointer to the test written for the select statement please, I cant find them (µI'd like to use them as template). For your info PR is working like a charm on 10 of our projects :)
sebastienroul: @kibertoad : Finally, unit test + integration test are there.
I clear my first attempt to modify all dialect, and focus only on MYSQL. Everything is green... let pull :)
coveralls: 
[![Coverage Status](https://coveralls.io/builds/63968294/badge)](https://coveralls.io/builds/63968294)

coverage: 92.818% (+0.004%) from 92.814%
when pulling **30cc14ea2cb8b348406c139ca9fae2884a252112 on sebastienroul:patch-1**
into **82f43d53abd2b6215015c11061a9793ed68e8611 on knex:master**.


Patch:
diff --git a/lib/dialects/mysql/index.js b/lib/dialects/mysql/index.js
index ddace51d..fe316c53 100644
--- a/lib/dialects/mysql/index.js
+++ b/lib/dialects/mysql/index.js
@@ -6,6 +6,7 @@ const { promisify } = require('util');
 const Client = require('../../client');
 
 const Transaction = require('./transaction');
+const QueryBuilder = require('./query/mysql-querybuilder');
 const QueryCompiler = require('./query/mysql-querycompiler');
 const SchemaCompiler = require('./schema/mysql-compiler');
 const TableCompiler = require('./schema/mysql-tablecompiler');
@@ -23,6 +24,10 @@ class Client_MySQL extends Client {
     return require('mysql');
   }
 
+  queryBuilder() {
+    return new QueryBuilder(this);
+  }
+
   queryCompiler(builder, formatter) {
     return new QueryCompiler(this, builder, formatter);
   }
diff --git a/lib/dialects/mysql/query/mysql-querybuilder.js b/lib/dialects/mysql/query/mysql-querybuilder.js
new file mode 100644
index 00000000..28acd40d
--- /dev/null
+++ b/lib/dialects/mysql/query/mysql-querybuilder.js
@@ -0,0 +1,14 @@
+const QueryBuilder = require('../../../query/querybuilder');
+const isEmpty = require('lodash/isEmpty');
+
+module.exports = class QueryBuilder_MySQL extends QueryBuilder {
+  upsert(values, returning, options) {
+    this._method = 'upsert';
+    if (!isEmpty(returning)) {
+      this.returning(returning, options);
+    }
+
+    this._single.upsert = values;
+    return this;
+  }
+};
diff --git a/lib/dialects/mysql/query/mysql-querycompiler.js b/lib/dialects/mysql/query/mysql-querycompiler.js
index 1c1a1c09..e694c994 100644
--- a/lib/dialects/mysql/query/mysql-querycompiler.js
+++ b/lib/dialects/mysql/query/mysql-querycompiler.js
@@ -27,12 +27,21 @@ class QueryCompiler_MySQL extends QueryCompiler {
 
     this._emptyInsertValue = '() values ()';
   }
+  // Compiles an `delete` allowing comments
+  del() {
+    const sql = super.del();
+    if (sql === '') return sql;
+    const comments = this.comments();
+    return (comments === '' ? '' : comments + ' ') + sql;
+  }
 
   // Compiles an `insert` query, allowing for multiple
   // inserts using a single query statement.
   insert() {
     let sql = super.insert();
     if (sql === '') return sql;
+    const comments = this.comments();
+    sql = (comments === '' ? '' : comments + ' ') + sql;
 
     const { ignore, merge, insert } = this.single;
     if (ignore) sql = sql.replace('insert into', 'insert ignore into');
@@ -49,6 +58,13 @@ class QueryCompiler_MySQL extends QueryCompiler {
     return sql;
   }
 
+  upsert() {
+    const upsertValues = this.single.upsert || [];
+    const sql = this.with() + `replace into ${this.tableName} `;
+    const body = this._insertBody(upsertValues);
+    return body === '' ? '' : sql + body;
+  }
+
   // Compiles merge for onConflict, allowing for different merge strategies
   _merge(updates, insert) {
     const sql = ' on duplicate key update ';
@@ -86,6 +102,7 @@ class QueryCompiler_MySQL extends QueryCompiler {
 
   // Update method, including joins, wheres, order & limits.
   update() {
+    const comments = this.comments();
     const withSQL = this.with();
     const join = this.join();
     const updates = this._prepUpdate(this.single.update);
@@ -93,6 +110,7 @@ class QueryCompiler_MySQL extends QueryCompiler {
     const order = this.order();
     const limit = this.limit();
     return (
+      (comments === '' ? '' : comments + ' ') +
       withSQL +
       `update ${this.tableName}` +
       (join ? ` ${join}` : '') +
diff --git a/lib/migrations/migrate/stub/ts.stub b/lib/migrations/migrate/stub/ts.stub
index 649ffce4..c0c213fe 100644
--- a/lib/migrations/migrate/stub/ts.stub
+++ b/lib/migrations/migrate/stub/ts.stub
@@ -1,4 +1,4 @@
-import { Knex } from "knex";
+import type { Knex } from "knex";
 
 <% if (d.tableName) { %>
 export async function up(knex: Knex): Promise<Knex.SchemaBuilder> {
diff --git a/types/index.d.ts b/types/index.d.ts
index d794efae..ff64a0d3 100644
--- a/types/index.d.ts
+++ b/types/index.d.ts
@@ -561,7 +561,7 @@ declare namespace Knex {
   type ResolveTableType<
     TCompositeTableType,
     TScope extends TableInterfaceScope = 'base'
-  > = TCompositeTableType extends CompositeTableType<unknown>
+  > = TCompositeTableType extends CompositeTableType<{}>
     ? TCompositeTableType[TScope]
     : TCompositeTableType;
 


Test patch:
diff --git a/test/integration/query/deletes.js b/test/integration/query/deletes.js
index 9f76c5dd..ba920751 100644
--- a/test/integration/query/deletes.js
+++ b/test/integration/query/deletes.js
@@ -86,6 +86,21 @@ module.exports = function (knex) {
         });
     });
 
+    it('#5738 should handle deletes with comments', function () {
+      return knex('accounts')
+        .where('id', 1)
+        .del()
+        .comment('removing acccount')
+        .testSql(function (tester) {
+          tester(
+            'mysql',
+            '/* removing acccount */ delete from `accounts` where `id` = ?',
+            [1],
+            0
+          );
+        });
+    });
+
     describe('Delete with join', function () {
       it('should handle basic delete with join', async function () {
         const query = knex('test_table_two')
diff --git a/test/integration2/dialects/mysql.spec.js b/test/integration2/dialects/mysql.spec.js
new file mode 100644
index 00000000..b9111f77
--- /dev/null
+++ b/test/integration2/dialects/mysql.spec.js
@@ -0,0 +1,152 @@
+const { expect } = require('chai');
+const {
+  getKnexForDb,
+  getAllDbs,
+  Db,
+} = require('../util/knex-instance-provider');
+const { isArray } = require('lodash');
+
+describe('MySQL dialect', () => {
+  getAllDbs()
+    .filter((db) => db === Db.MySQL)
+    .forEach((db) => {
+      let knex;
+      before(async () => {
+        knex = getKnexForDb(db);
+      });
+
+      describe('Upsert operation', () => {
+        const testTable = 'test_users_mysql';
+
+        before(async () => {
+          await knex.schema.createTable(testTable, function () {
+            this.increments('id').primary();
+            this.decimal('balance');
+            this.string('username');
+          });
+        });
+
+        after(async () => {
+          await knex.schema.dropTable(testTable);
+          await knex.destroy();
+        });
+
+        it('should insert row using an object', async () => {
+          const initValues = { id: 1, username: 'user 1', balance: 10 };
+          await checkNoRecords(initValues.id);
+
+          const inserted = await knex(testTable).upsert(initValues);
+          expect(inserted[0].affectedRows).equals(1);
+          expect(inserted[0].insertId).equals(initValues.id);
+
+          const results = await selectTableData(initValues.id);
+          expect(results.length).equals(1);
+          expect(results[0].username).equals(initValues.username);
+          expect(results[0].balance).equals(initValues.balance);
+        });
+
+        it('should update row using an object', async () => {
+          const id = 2;
+          const initValues = { id, username: 'user 2', balance: 12 };
+          const changedValues = { id, username: 'second', balance: 100 };
+          await checkNoRecords(initValues.id);
+
+          // insert
+          const inserted = await knex(testTable).upsert(initValues);
+          expect(inserted[0].affectedRows).to.equal(1);
+
+          let results = await selectTableData(initValues.id);
+          expect(results[0].username).equals(initValues.username);
+          expect(results[0].balance).equals(initValues.balance);
+
+          // update
+          const updated = await knex(testTable).upsert(changedValues);
+          expect(updated[0].affectedRows).to.equal(2);
+
+          results = await selectTableData(changedValues.id);
+          expect(results[0].username).equals(changedValues.username);
+          expect(results[0].balance).equals(changedValues.balance);
+        });
+
+        it('should insert rows using an array', async () => {
+          const initValues = [
+            { id: 3, username: 'user 3', balance: 13 },
+            { id: 4, username: 'user 4', balance: 14 },
+          ];
+          const ids = initValues.map((el) => el.id);
+          await checkNoRecords(ids);
+
+          const inserted = await knex(testTable).upsert(initValues);
+          expect(inserted[0].affectedRows).equals(2);
+
+          const results = await selectTableData(ids);
+          results.forEach((result, idx) => {
+            expect(result).deep.equals(initValues[idx]);
+          });
+        });
+
+        it('should update rows using an array', async () => {
+          const id1 = 5;
+          const id2 = 6;
+          const initValues = [
+            { id: id1, username: 'user 3', balance: 15 },
+            { id: id2, username: 'user 4', balance: 16 },
+          ];
+          await checkNoRecords([id1, id2]);
+
+          // insert
+          const inserted = await knex(testTable).upsert(initValues);
+          expect(inserted[0].affectedRows).equals(2);
+          let results = await selectTableData([id1, id2]);
+          results.forEach((result, idx) => {
+            expect(result).deep.equals(initValues[idx]);
+          });
+
+          const changedValues = [
+            { id: id1, username: 'third user', balance: 15 },
+            { id: id2, username: 'user 4', balance: 106 },
+          ];
+
+          // update
+          const updated = await knex(testTable).upsert(changedValues);
+          expect(updated[0].affectedRows).equals(4);
+          results = await selectTableData([id1, id2]);
+          results.forEach((result, idx) => {
+            expect(result).deep.equals(changedValues[idx]);
+          });
+        });
+
+        it('should insert row using an object with a default value', async () => {
+          const initValues = { id: 7, balance: 17 };
+          await checkNoRecords(initValues.id);
+
+          const inserted = await knex(testTable).upsert(initValues);
+          expect(inserted[0].affectedRows).equals(1);
+          expect(inserted[0].insertId).equals(initValues.id);
+
+          const results = await selectTableData(initValues.id);
+          expect(results.length).equals(1);
+          expect(results[0].username).to.be.null;
+          expect(results[0].balance).equals(initValues.balance);
+        });
+
+        async function checkNoRecords(id) {
+          if (!isArray(id)) {
+            id = [id];
+          }
+          const results = await knex(testTable).whereIn('id', id).select('id');
+          expect(results.length).to.equal(0);
+        }
+
+        async function selectTableData(id) {
+          if (!isArray(id)) {
+            id = [id];
+          }
+
+          return knex(testTable)
+            .whereIn('id', id)
+            .select('id', 'username', 'balance');
+        }
+      });
+    });
+});
diff --git a/test/integration2/query/insert/inserts.spec.js b/test/integration2/query/insert/inserts.spec.js
index d6e523bb..31ecdab9 100644
--- a/test/integration2/query/insert/inserts.spec.js
+++ b/test/integration2/query/insert/inserts.spec.js
@@ -914,6 +914,20 @@ describe('Inserts', function () {
           });
       });
 
+      it('#5738 should handle insert with comments', async function () {
+        await knex('test_default_table')
+          .insert({}, 'id')
+          .comment('insert into test_default_table')
+          .testSql(function (tester) {
+            tester(
+              'mysql',
+              '/* insert into test_default_table */ insert into `test_default_table` () values ()',
+              [],
+              [2]
+            );
+          });
+      });
+
       it('should handle empty arrays inserts', async function () {
         await knex.schema.createTable('test_default_table2', function (qb) {
           qb.increments().primary();
diff --git a/test/integration2/query/update/updates.spec.js b/test/integration2/query/update/updates.spec.js
index e699870c..6d72fb9e 100644
--- a/test/integration2/query/update/updates.spec.js
+++ b/test/integration2/query/update/updates.spec.js
@@ -84,6 +84,25 @@ describe('Updates', function () {
           });
       });
 
+      it('#5738 should handle update with comments', async function () {
+        await knex('accounts')
+          .where('id', 1)
+          .update({
+            first_name: 'User',
+            last_name: 'Test',
+            email: 'test100@example.com',
+          })
+          .comment('update in account')
+          .testSql(function (tester) {
+            tester(
+              'mysql',
+              '/* update in account */ update `accounts` set `first_name` = ?, `last_name` = ?, `email` = ? where `id` = ?',
+              ['User', 'Test', 'test100@example.com', 1],
+              1
+            );
+          });
+      });
+
       it('should allow for null updates', async function () {
         await knex('accounts')
           .where('id', 1000)
diff --git a/test/unit/query/builder.js b/test/unit/query/builder.js
index 3a2d11d7..cc3b160d 100644
--- a/test/unit/query/builder.js
+++ b/test/unit/query/builder.js
@@ -9508,6 +9508,55 @@ describe('QueryBuilder', () => {
     );
   });
 
+  it('#5738 - should allow query comments in querybuilder for insert statement', () => {
+    testsql(
+      qb()
+        .into('users')
+        .insert({ email: 'foo' })
+        .comment('Added comment 1')
+        .comment('Added comment 2'),
+
+      {
+        mysql: {
+          sql: '/* Added comment 1 */ /* Added comment 2 */ insert into `users` (`email`) values (?)',
+          bindings: ['foo'],
+        },
+      }
+    );
+  });
+
+  it('#5738 - should allow query comments in querybuilder for update statement', () => {
+    testsql(
+      qb()
+        .from('users')
+        .update({ email: 'foo' })
+        .comment('Added comment 1')
+        .comment('Added comment 2'),
+      {
+        mysql: {
+          sql: '/* Added comment 1 */ /* Added comment 2 */ update `users` set `email` = ?',
+          bindings: ['foo'],
+        },
+      }
+    );
+  });
+
+  it('#5738 - should allow query comments in querybuilder for delete statement', () => {
+    testsql(
+      qb()
+        .from('users')
+        .delete()
+        .comment('Added comment 1')
+        .comment('Added comment 2'),
+      {
+        mysql: {
+          sql: '/* Added comment 1 */ /* Added comment 2 */ delete from `users`',
+          bindings: [],
+        },
+      }
+    );
+  });
+
   it('#1982 (2) - should throw error on non string', () => {
     try {
       testsql(qb().from('testtable').comment({ prop: 'val' }), {
@@ -11626,4 +11675,165 @@ describe('QueryBuilder', () => {
       });
     });
   });
+
+  describe('Upsert', () => {
+    function mysqlQb() {
+      return clients.mysql.queryBuilder();
+    }
+
+    const mysqlClient = {
+      mysql: clients.mysql,
+    };
+
+    const mysqlClientWithNullAsDefault = {
+      mysql: clientsWithNullAsDefault.mysql,
+    };
+
+    describe('toSQL method', () => {
+      it('upsert: one record with one field', () => {
+        testsql(
+          mysqlQb().from('users').upsert({ email: 'foo' }),
+          {
+            mysql: {
+              sql: 'replace into `users` (`email`) values (?)',
+              bindings: ['foo'],
+            },
+          },
+          mysqlClient
+        );
+      });
+
+      it('upsert: multiple records with multiple fields', () => {
+        testsql(
+          mysqlQb()
+            .from('users')
+            .upsert([
+              { email: 'foo', name: 'taylor' },
+              { email: 'bar', name: 'dayle' },
+            ]),
+          {
+            mysql: {
+              sql: 'replace into `users` (`email`, `name`) values (?, ?), (?, ?)',
+              bindings: ['foo', 'taylor', 'bar', 'dayle'],
+            },
+          },
+          mysqlClient
+        );
+      });
+
+      it('upsert: one record with returning', () => {
+        // check that returning doesn't break anything
+        testsql(
+          mysqlQb()
+            .from('users')
+            .upsert({ email: 'foo', name: 'taylor' }, 'id'),
+          {
+            mysql: {
+              sql: 'replace into `users` (`email`, `name`) values (?, ?)',
+              bindings: ['foo', 'taylor'],
+            },
+          },
+          mysqlClient
+        );
+      });
+
+      it('upsert: one record with multiple returnings', () => {
+        // check that returning doesn't break anything
+        testsql(
+          mysqlQb()
+            .from('users')
+            .upsert({ email: 'foo', name: 'taylor' }, ['id', 'name']),
+          {
+            mysql: {
+              sql: 'replace into `users` (`email`, `name`) values (?, ?)',
+              bindings: ['foo', 'taylor'],
+            },
+          },
+          mysqlClient
+        );
+      });
+
+      it('upsert: respects raw bindings', () => {
+        testsql(
+          mysqlQb()
+            .from('users')
+            .upsert({ email: raw('CURRENT TIMESTAMP'), name: 'taylor' }),
+          {
+            mysql: {
+              sql: 'replace into `users` (`email`, `name`) values (CURRENT TIMESTAMP, ?)',
+              bindings: ['taylor'],
+            },
+          },
+          mysqlClient
+        );
+      });
+
+      it('upsert: empty query', () => {
+        testsql(
+          mysqlQb().from('users').upsert(),
+          {
+            mysql: {
+              sql: '',
+              bindings: [],
+            },
+          },
+          mysqlClient
+        );
+      });
+
+      it('upsert: empty array', () => {
+        testsql(
+          mysqlQb().from('users').upsert([]),
+          {
+            mysql: {
+              sql: '',
+              bindings: [],
+            },
+          },
+          mysqlClient
+        );
+      });
+
+      it('upsert: empty object in an array', () => {
+        testsql(
+          mysqlQb().from('users').upsert([{}]),
+          {
+            mysql: {
+              sql: 'replace into `users` () values ()',
+              bindings: [],
+            },
+          },
+          mysqlClient
+        );
+      });
+    });
+
+    describe('toQuery method', () => {
+      it('upsert: multiple records with missing fields and no nullAsDefault setting', () => {
+        testquery(
+          mysqlQb()
+            .from('users')
+            .upsert([{ email: 'foo' }, { name: 'dayle' }]),
+          {
+            mysql:
+              "replace into `users` (`email`, `name`) values ('foo', DEFAULT), (DEFAULT, 'dayle')",
+          },
+          mysqlClient
+        );
+      });
+
+      it('upsert: multiple records with missing fields and nullAsDefault setting is true', () => {
+        testquery(
+          mysqlQb()
+            .from('users')
+            .upsert([{ name: 'taylor' }, { email: 'bar' }]),
+          {
+            mysql:
+              "replace into `users` (`email`, `name`) values (NULL, 'taylor'), ('bar', NULL)",
+          },
+          mysqlClientWithNullAsDefault
+        );
+      });
+    });
+  });
 });