Repository: knex/knex
Pull request #5530 (2023-03-28 23:18:39)

Add readonly option to Better-SQLite3
Better-SQLite3 [supports a `readonly` option](https://github.com/WiseLibs/better-sqlite3/blob/master/docs/api.md#new-databasepath-options) on initialization. This PR lets Knex users set that option.

(The automatic code formatter wanted to reflow some lines unrelated to the new code. I've split those automated changes out into a separate commit for clarity and to preserve author attribution on the changed lines.)

Docs PR is in knex/documentation#505.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/58346556/badge)](https://coveralls.io/builds/58346556)

Coverage: 92.385% (-0.007%) from 92.392% when pulling **68578c8e223f4d0953f31f95b89584ce392515f8 on undees:add-readonly-option-to-better-sqlite3** into **de2a8a2df4c9450cb637177c4b13fe0ce948fe9c on knex:master**.


Patch:
diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml
index 0cbb8f0f..d32a422b 100644
--- a/.github/workflows/codeql-analysis.yml
+++ b/.github/workflows/codeql-analysis.yml
@@ -8,8 +8,15 @@ on:
   schedule:
     - cron: '36 2 * * 0'
 
+permissions:
+  contents: read
+
 jobs:
   analyze:
+    permissions:
+      actions: read  # for github/codeql-action/init to get workflow details
+      contents: read  # for actions/checkout to fetch code
+      security-events: write  # for github/codeql-action/analyze to upload SARIF results
     name: Analyze
     runs-on: ubuntu-latest
 
diff --git a/.github/workflows/coverage.yml b/.github/workflows/coverage.yml
index fec04817..f5ff4003 100644
--- a/.github/workflows/coverage.yml
+++ b/.github/workflows/coverage.yml
@@ -4,8 +4,14 @@ name: Coverage
 
 on: ["push", "pull_request"]
 
+permissions:
+  contents: read
+
 jobs:
   build:
+    permissions:
+      checks: write  # for coverallsapp/github-action to create new checks
+      contents: read  # for actions/checkout to fetch code
     runs-on: ubuntu-latest
 
     name: Coverage
diff --git a/.github/workflows/linting.yml b/.github/workflows/linting.yml
index 7d6bcebe..2e451413 100644
--- a/.github/workflows/linting.yml
+++ b/.github/workflows/linting.yml
@@ -10,6 +10,9 @@ on:
     branches:
       - master
 
+permissions:
+  contents: read
+
 jobs:
   build:
     runs-on: ubuntu-latest
diff --git a/lib/dialects/better-sqlite3/index.js b/lib/dialects/better-sqlite3/index.js
index 18cf1576..85a5ba69 100644
--- a/lib/dialects/better-sqlite3/index.js
+++ b/lib/dialects/better-sqlite3/index.js
@@ -13,6 +13,7 @@ class Client_BetterSQLite3 extends Client_SQLite3 {
 
     return new this.driver(this.connectionSettings.filename, {
       nativeBinding: options.nativeBinding,
+      readonly: !!options.readonly,
     });
   }
 
diff --git a/lib/dialects/mssql/query/mssql-querycompiler.js b/lib/dialects/mssql/query/mssql-querycompiler.js
index 4dbe18de..74c9b8ae 100644
--- a/lib/dialects/mssql/query/mssql-querycompiler.js
+++ b/lib/dialects/mssql/query/mssql-querycompiler.js
@@ -11,6 +11,7 @@ const {
 } = require('../../../formatter/wrappingFormatter');
 
 const components = [
+  'comments',
   'columns',
   'join',
   'lock',
diff --git a/lib/dialects/mysql2/index.js b/lib/dialects/mysql2/index.js
index 63bfa4f0..a3c71394 100644
--- a/lib/dialects/mysql2/index.js
+++ b/lib/dialects/mysql2/index.js
@@ -14,6 +14,26 @@ class Client_MySQL2 extends Client_MySQL {
   _driver() {
     return require('mysql2');
   }
+
+  initializeDriver() {
+    try {
+      this.driver = this._driver();
+    } catch (e) {
+      let message = `Knex: run\n$ npm install ${this.driverName}`;
+
+      const nodeMajorVersion = process.version.replace(/^v/, '').split('.')[0];
+      if (nodeMajorVersion <= 12) {
+        message += `@3.2.0`;
+        this.logger.error(
+          'Mysql2 version 3.2.0 is the latest version to support Node.js 12 or lower.'
+        );
+      }
+      message += ` --save`;
+      this.logger.error(`${message}\n${e.message}\n${e.stack}`);
+      throw new Error(`${message}\n${e.message}`);
+    }
+  }
+
   validateConnection(connection) {
     return (
       connection &&
diff --git a/lib/dialects/oracle/query/oracle-querycompiler.js b/lib/dialects/oracle/query/oracle-querycompiler.js
index 032281e7..ee934cb4 100644
--- a/lib/dialects/oracle/query/oracle-querycompiler.js
+++ b/lib/dialects/oracle/query/oracle-querycompiler.js
@@ -12,6 +12,7 @@ const { ReturningHelper } = require('../utils');
 const { isString } = require('../../../util/is');
 
 const components = [
+  'comments',
   'columns',
   'join',
   'where',
diff --git a/lib/dialects/oracledb/schema/oracledb-columncompiler.js b/lib/dialects/oracledb/schema/oracledb-columncompiler.js
index 117a74f4..df4bb058 100644
--- a/lib/dialects/oracledb/schema/oracledb-columncompiler.js
+++ b/lib/dialects/oracledb/schema/oracledb-columncompiler.js
@@ -4,7 +4,7 @@ const { isObject } = require('../../../util/is');
 class ColumnCompiler_Oracledb extends ColumnCompiler_Oracle {
   constructor() {
     super(...arguments);
-    this.modifiers = ['defaultTo', 'nullable', 'comment'];
+    this.modifiers = ['defaultTo', 'nullable', 'comment', 'checkJson'];
     this._addCheckModifiers();
   }
 
@@ -38,14 +38,20 @@ class ColumnCompiler_Oracledb extends ColumnCompiler_Oracle {
   }
 
   json() {
-    return `varchar2(4000) check (${this.formatter.columnize(
-      this.getColumnName()
-    )} is json)`;
+    // implicitly add the check for json
+    this.columnBuilder._modifiers.checkJson = [
+      this.formatter.columnize(this.getColumnName()),
+    ];
+    return 'varchar2(4000)';
   }
 
   jsonb() {
     return this.json();
   }
+
+  checkJson(column) {
+    return `check (${column} is json)`;
+  }
 }
 
 ColumnCompiler_Oracledb.prototype.time = 'timestamp with local time zone';
diff --git a/lib/dialects/sqlite3/schema/internal/sqlite-ddl-operations.js b/lib/dialects/sqlite3/schema/internal/sqlite-ddl-operations.js
index 4a66f1ae..73371cf0 100644
--- a/lib/dialects/sqlite3/schema/internal/sqlite-ddl-operations.js
+++ b/lib/dialects/sqlite3/schema/internal/sqlite-ddl-operations.js
@@ -15,7 +15,7 @@ function renameTable(tableName, alteredName) {
 }
 
 function getTableSql(tableName) {
-  return `SELECT type, sql FROM sqlite_master WHERE (type='table' OR (type='index' AND sql IS NOT NULL)) AND tbl_name='${tableName}'`;
+  return `SELECT type, sql FROM sqlite_master WHERE (type='table' OR (type='index' AND sql IS NOT NULL)) AND lower(tbl_name)='${tableName.toLowerCase()}'`;
 }
 
 function isForeignCheckEnabled() {
diff --git a/lib/execution/runner.js b/lib/execution/runner.js
index 8bb23216..ed016529 100644
--- a/lib/execution/runner.js
+++ b/lib/execution/runner.js
@@ -72,6 +72,24 @@ class Runner {
       this.client.releaseConnection(this.connection);
     });
 
+    // If the stream is manually destroyed, the close event is not
+    // propagated to the top of the pipe chain. We need to manually verify
+    // that the source stream is closed and if not, manually destroy it.
+    stream.on('pipe', (sourceStream) => {
+      const cleanSourceStream = () => {
+        if (!sourceStream.closed) {
+          sourceStream.destroy();
+        }
+      };
+
+      // Stream already closed, cleanup immediately
+      if (stream.closed) {
+        cleanSourceStream();
+      } else {
+        stream.on('close', cleanSourceStream);
+      }
+    });
+
     const connectionAcquirePromise = this.ensureConnection(
       ensureConnectionStreamCallback,
       {
diff --git a/lib/query/querybuilder.js b/lib/query/querybuilder.js
index 547a6040..87a8cc3b 100644
--- a/lib/query/querybuilder.js
+++ b/lib/query/querybuilder.js
@@ -61,6 +61,7 @@ class Builder extends EventEmitter {
     this.client = client;
     this.and = this;
     this._single = {};
+    this._comments = [];
     this._statements = [];
     this._method = 'select';
     if (client.config) {
@@ -88,6 +89,7 @@ class Builder extends EventEmitter {
     const cloned = new this.constructor(this.client);
     cloned._method = this._method;
     cloned._single = clone(this._single);
+    cloned._comments = clone(this._comments);
     cloned._statements = clone(this._statements);
     cloned._debug = this._debug;
 
@@ -235,6 +237,21 @@ class Builder extends EventEmitter {
     return this;
   }
 
+  // Adds a comment to the query
+  comment(txt) {
+    if (!isString(txt)) {
+      throw new Error('Comment must be a string');
+    }
+    const forbiddenChars = ['/*', '*/', '?'];
+    if (forbiddenChars.some((chars) => txt.includes(chars))) {
+      throw new Error(`Cannot include ${forbiddenChars.join(', ')} in comment`);
+    }
+    this._comments.push({
+      comment: txt,
+    });
+    return this;
+  }
+
   // Allow for a sub-select to be explicitly aliased as a column,
   // without needing to compile the query in a where.
   as(column) {
diff --git a/lib/query/querycompiler.js b/lib/query/querycompiler.js
index a4a09d27..ca79dac9 100644
--- a/lib/query/querycompiler.js
+++ b/lib/query/querycompiler.js
@@ -28,6 +28,7 @@ const {
 const debugBindings = debug('knex:bindings');
 
 const components = [
+  'comments',
   'columns',
   'join',
   'where',
@@ -50,6 +51,7 @@ class QueryCompiler {
     this.method = builder._method || 'select';
     this.options = builder._options;
     this.single = builder._single;
+    this.queryComments = builder._comments;
     this.timeout = builder._timeout || false;
     this.cancelOnTimeout = builder._cancelOnTimeout || false;
     this.grouped = groupBy(builder._statements, 'grouping');
@@ -138,6 +140,7 @@ class QueryCompiler {
         case 'union':
           unionStatement = statement;
           break;
+        case 'comments':
         case 'columns':
         case 'join':
         case 'where':
@@ -317,6 +320,14 @@ class QueryCompiler {
     );
   }
 
+  // Add comments to the query
+  comments() {
+    if (!this.queryComments.length) return '';
+    return this.queryComments
+      .map((comment) => `/* ${comment.comment} */`)
+      .join(' ');
+  }
+
   _aggregate(stmt, { aliasSeparator = ' as ', distinctParentheses } = {}) {
     const value = stmt.value;
     const method = stmt.method;
diff --git a/package.json b/package.json
index 399c458e..b814701c 100644
--- a/package.json
+++ b/package.json
@@ -144,7 +144,7 @@
     "toxiproxy-node-client": "^2.0.6",
     "ts-node": "^10.9.1",
     "tsd": "^0.28.1",
-    "typescript": "4.8.3"
+    "typescript": "5.0.4"
   },
   "buildDependencies": [
     "rimraf"
@@ -251,7 +251,8 @@
     ],
     "exclude": [
       "lib/dialects/oracle",
-      "lib/dialects/oracledb"
+      "lib/dialects/oracledb",
+      "test/**/*.spec.js"
     ]
   },
   "tsd": {
diff --git a/types/index.d.ts b/types/index.d.ts
index 119f9aa1..5bd7ea5c 100644
--- a/types/index.d.ts
+++ b/types/index.d.ts
@@ -597,6 +597,7 @@ export declare namespace Knex {
     as: As<TRecord, TResult>;
     columns: Select<TRecord, TResult>;
     column: Select<TRecord, TResult>;
+    comment: Comment<TRecord, TResult>;
     hintComment: HintComment<TRecord, TResult>;
     from: Table<TRecord, TResult>;
     fromRaw: Table<TRecord, TResult>;
@@ -803,6 +804,11 @@ export declare namespace Knex {
       columnName: string,
       amount?: number
     ): QueryBuilder<TRecord, number>;
+    decrement(
+      columns: {
+        [column in keyof TRecord]: number
+      }
+    ): QueryBuilder<TRecord, number>;
 
     // Analytics
     rank: AnalyticFunction<TRecord, TResult>;
@@ -1359,6 +1365,10 @@ export declare namespace Knex {
     ): QueryBuilder<TRecord, TResult>;
   }
 
+  interface Comment<TRecord extends {} = any, TResult = any> {
+    (comment: string): QueryBuilder<TRecord, TResult>;
+  }
+
   interface HintComment<TRecord extends {} = any, TResult = any> {
     (hint: string): QueryBuilder<TRecord, TResult>;
     (hints: readonly string[]): QueryBuilder<TRecord, TResult>;
@@ -2733,6 +2743,7 @@ export declare namespace Knex {
     asyncStackTraces?: boolean;
     log?: Logger;
     compileSqlOnError?: boolean;
+    fetchAsString?: string[];
   }
 
   type StaticConnectionConfig =
@@ -3065,6 +3076,7 @@ export declare namespace Knex {
     filename: string;
     options?: {
       nativeBinding?: string;
+      readonly?: boolean;
     };
   }
 


Test patch:
diff --git a/.github/workflows/integration-tests.yml b/.github/workflows/integration-tests.yml
index b0800c78..72a865f7 100644
--- a/.github/workflows/integration-tests.yml
+++ b/.github/workflows/integration-tests.yml
@@ -10,6 +10,9 @@ on:
     branches:
       - master
 
+permissions:
+  contents: read
+
 jobs:
   build:
     runs-on: ubuntu-latest
diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml
index 89aa9e1e..a57dfd69 100644
--- a/.github/workflows/unit-tests.yml
+++ b/.github/workflows/unit-tests.yml
@@ -10,6 +10,9 @@ on:
     branches:
       - master
 
+permissions:
+  contents: read
+
 jobs:
   build:
     runs-on: ubuntu-latest
@@ -34,6 +37,10 @@ jobs:
       - name: Run npm install
         run: npm install
 
+      - name: Run npm install mysql2@3.2.0 if Node 12.x
+        run: npm install mysql2@3.2.0
+        if: matrix.node-version == '12.x'
+
       - run: npm run build
 
       - name: Run Tests
diff --git a/test/db-less-test-suite.js b/test/db-less-test-suite.js
index 8cac5955..1100a293 100644
--- a/test/db-less-test-suite.js
+++ b/test/db-less-test-suite.js
@@ -50,6 +50,10 @@ if (config.mysql) {
   require('./unit/dialects/mysql');
 }
 
+if (config['better-sqlite3']) {
+  require('./unit/dialects/better-sqlite3');
+}
+
 describe('CLI tests', function () {
   this.timeout(process.env.KNEX_TEST_TIMEOUT || 5000);
   require('./cli/help.spec');
diff --git a/test/integration2/migrate/alter-table-ignore-case-sensitive/01_create.js b/test/integration2/migrate/alter-table-ignore-case-sensitive/01_create.js
new file mode 100644
index 00000000..4d96064c
--- /dev/null
+++ b/test/integration2/migrate/alter-table-ignore-case-sensitive/01_create.js
@@ -0,0 +1,9 @@
+exports.up = async (knex) => {
+  await knex.schema.createTableIfNotExists('some_table', (table) => {
+    table.increments();
+  });
+};
+
+exports.down = async (knex) => {
+  await knex.schema.dropTableIfExists('some_table');
+};
diff --git a/test/integration2/migrate/alter-table-ignore-case-sensitive/02_add_column.js b/test/integration2/migrate/alter-table-ignore-case-sensitive/02_add_column.js
new file mode 100644
index 00000000..946d5221
--- /dev/null
+++ b/test/integration2/migrate/alter-table-ignore-case-sensitive/02_add_column.js
@@ -0,0 +1,13 @@
+exports.up = async (knex) => {
+  // Use the same table but different case.
+  await knex.schema.table('Some_Table', (table) => {
+    table.string('other_id').after('id');
+    table.string('name').after('other_id');
+  });
+};
+
+exports.down = async (knex) => {
+  await knex.schema.table('Some_Table', (table) => {
+    table.dropColumns('other_id', 'name');
+  });
+};
diff --git a/test/integration2/migrate/migration-integration.spec.js b/test/integration2/migrate/migration-integration.spec.js
index 599c0f3e..ffb4b06c 100644
--- a/test/integration2/migrate/migration-integration.spec.js
+++ b/test/integration2/migrate/migration-integration.spec.js
@@ -115,6 +115,31 @@ describe('Migrations', function () {
             await db.migrate.rollback();
             await knexInstance.destroy();
           });
+
+          it('should not fail alter-table-ignore-case-sensitive', async () => {
+            await knex.migrate.latest({
+              directory:
+                'test/integration2/migrate/alter-table-ignore-case-sensitive',
+            });
+            expect(await knex.schema.hasColumn('Some_Table', 'id')).to.be.true;
+            expect(await knex.schema.hasColumn('Some_Table', 'other_id')).to.be
+              .true;
+            expect(await knex.schema.hasColumn('Some_Table', 'name')).to.be
+              .true;
+            // try different case
+            expect(await knex.schema.hasColumn('some_table', 'id')).to.be.true;
+            expect(await knex.schema.hasColumn('some_table', 'other_id')).to.be
+              .true;
+            expect(await knex.schema.hasColumn('some_table', 'name')).to.be
+              .true;
+
+            await knex.migrate.rollback({
+              directory:
+                'test/integration2/migrate/alter-table-ignore-case-sensitive',
+            });
+            expect(await knex.schema.hasTable('Some_Table')).to.be.false;
+            expect(await knex.schema.hasTable('some_table')).to.be.false;
+          });
         }
 
         it('should not fail drop-and-recreate-column operation when using async/await', () => {
diff --git a/test/integration2/query/misc/additional.spec.js b/test/integration2/query/misc/additional.spec.js
index 52002b62..73d9d74d 100644
--- a/test/integration2/query/misc/additional.spec.js
+++ b/test/integration2/query/misc/additional.spec.js
@@ -128,7 +128,7 @@ describe('Additional', function () {
           });
         });
 
-        it('should close the connection when a stream query iteration errors', async function () {
+        it('should release the connection when a stream query iteration errors', async function () {
           const spy = sinon.spy(knex.client, 'releaseConnection');
 
           const stream = knex.raw('VALUES (1), (2), (3)').stream();
@@ -146,6 +146,26 @@ describe('Additional', function () {
           spy.restore();
         });
 
+        it('should close the db connection when prematurely closing a stream', async function () {
+          await knex('accounts').truncate();
+          await insertAccounts(knex, 'accounts');
+
+          // We limit to only one row at a time, to keep the cursor open
+          const stream = knex('accounts').stream({ highWaterMark: 1 });
+
+          // eslint-disable-next-line no-unused-vars
+          for await (const _ of stream) {
+            stream.destroy();
+            break;
+          }
+
+          await new Promise((res) => setTimeout(res, 50));
+
+          // Will timeout if the connection is in the pool, but not closed
+          this.timeout(1000);
+          await knex('accounts').limit(1);
+        });
+
         it('should process response done through a stream', async () => {
           await knex('accounts').truncate();
           await insertAccounts(knex, 'accounts');
diff --git a/test/integration2/query/select/selects.spec.js b/test/integration2/query/select/selects.spec.js
index ee4212be..dc7a2d28 100644
--- a/test/integration2/query/select/selects.spec.js
+++ b/test/integration2/query/select/selects.spec.js
@@ -863,6 +863,16 @@ describe('Selects', function () {
         });
       });
 
+      it('#1982 - Allow comments in db', async function () {
+        await knex('accounts')
+          .comment('Integration Comment')
+          .select('first_name', 'email')
+          .limit(1)
+          .then(function(results) {
+            expect(results).to.have.length(1);
+          });
+      });
+
       describe('recursive CTE support', function () {
         before(async function () {
           await knex.schema.dropTableIfExists('rcte');
diff --git a/test/integration2/query/select/unions.spec.js b/test/integration2/query/select/unions.spec.js
index 1c56a012..12eede35 100644
--- a/test/integration2/query/select/unions.spec.js
+++ b/test/integration2/query/select/unions.spec.js
@@ -29,8 +29,6 @@ describe('All Union functions [.union(), .unionAll(), .intersect(), .except()]',
   getAllDbs().forEach((db) => {
     describe(db, () => {
       let knex;
-      const unionCols = ['last_name', 'phone'];
-
       before(async () => {
         knex = logger(getKnexForDb(db));
 
diff --git a/test/unit/dialects/better-sqlite3.js b/test/unit/dialects/better-sqlite3.js
index dbf3dfe2..8f16cd71 100644
--- a/test/unit/dialects/better-sqlite3.js
+++ b/test/unit/dialects/better-sqlite3.js
@@ -12,12 +12,18 @@ describe('better-sqlite3 unit tests', () => {
         connection: {
           filename: ':memory:',
           options: {
-            nativeBinding: path.resolve(__dirname, '../../../node_modules/better-sqlite3/build/Release/better_sqlite3.node'),
+            nativeBinding: path.resolve(
+              __dirname,
+              '../../../node_modules/better-sqlite3/build/Release/better_sqlite3.node'
+            ),
           },
         },
       });
 
-      const result = await knexInstance.select(knexInstance.raw('2 + 2 as answer')).first().catch(err => err);
+      const result = await knexInstance
+        .select(knexInstance.raw('2 + 2 as answer'))
+        .first()
+        .catch((err) => err);
       expect(result.answer).to.equal(4);
     });
 
@@ -33,10 +39,61 @@ describe('better-sqlite3 unit tests', () => {
         },
       });
 
-      const result = await knexInstance.select(knexInstance.raw('2 + 2 as answer')).first().catch(err => err);
+      const result = await knexInstance
+        .select(knexInstance.raw('2 + 2 as answer'))
+        .first()
+        .catch((err) => err);
       console.dir(result);
       expect(result).to.be.an('error');
       expect(result.code).to.equal('MODULE_NOT_FOUND');
     });
   });
+
+  describe('readonly', () => {
+    it('should initialize the DB with the passed-in `readonly` option', async () => {
+      const knexInstance = knex({
+        client: 'better-sqlite3',
+        useNullAsDefault: true,
+        connection: {
+          filename: __dirname + '/../test.sqlite3',
+          options: {
+            readonly: true,
+          },
+        },
+      });
+
+      const connection = await knexInstance.client.acquireConnection();
+      expect(connection.readonly).to.equal(true);
+    });
+
+    it('should prevent writing to the DB', async () => {
+      const knexInstance = knex({
+        client: 'better-sqlite3',
+        useNullAsDefault: true,
+        connection: {
+          filename: __dirname + '/../test.sqlite3',
+          options: {
+            readonly: true,
+          },
+        },
+      });
+
+      await expect(
+        knexInstance.raw('create table shouldFail (x integer)')
+      ).to.eventually.be.rejectedWith('attempt to write a readonly database');
+    });
+
+    it('should fall back on `readonly` = `false`', async () => {
+      const knexInstance = knex({
+        client: 'better-sqlite3',
+        useNullAsDefault: true,
+        connection: {
+          filename: __dirname + '/../test.sqlite3',
+        },
+      });
+
+      const connection = await knexInstance.client.acquireConnection();
+      expect(connection.readonly).to.equal(false);
+    });
+  });
 });
diff --git a/test/unit/query/builder.js b/test/unit/query/builder.js
index 5fc31e81..c1e86f84 100644
--- a/test/unit/query/builder.js
+++ b/test/unit/query/builder.js
@@ -9440,6 +9440,156 @@ describe('QueryBuilder', () => {
     );
   });
 
+  it('#1982 - should allow query comments in querybuilder', () => {
+    testsql(
+      qb()
+        .from('testtable')
+        .comment('Added comment 1')
+        .comment('Added comment 2'),
+      {
+        mysql: {
+          sql:
+            '/* Added comment 1 */ /* Added comment 2 */ select * from `testtable`',
+          bindings: [],
+        },
+        oracledb: {
+          sql:
+            '/* Added comment 1 */ /* Added comment 2 */ select * from "testtable"',
+          bindings: [],
+        },
+        mssql: {
+          sql:
+            '/* Added comment 1 */ /* Added comment 2 */ select * from [testtable]',
+          bindings: [],
+        },
+        pg: {
+          sql:
+            '/* Added comment 1 */ /* Added comment 2 */ select * from "testtable"',
+          bindings: [],
+        },
+        'pg-redshift': {
+          sql:
+            '/* Added comment 1 */ /* Added comment 2 */ select * from "testtable"',
+          bindings: [],
+        },
+      }
+    );
+  });
+
+  it('#1982 (2) - should throw error on non string', () => {
+    try {
+      testsql(
+        qb()
+          .from('testtable')
+          .comment({ prop: 'val' }),
+        {
+          mysql: {
+            sql: '',
+            bindings: [],
+          },
+          oracledb: {
+            sql: '',
+            bindings: [],
+          },
+          mssql: {
+            sql: '',
+            bindings: [],
+          },
+          pg: {
+            sql: '',
+            bindings: [],
+          },
+          'pg-redshift': {
+            sql: '',
+            bindings: [],
+          },
+        }
+      );
+      expect(true).to.equal(
+        false,
+        'Expected to throw error in compilation about non-string'
+      );
+    } catch (error) {
+      expect(error.message).to.contain('Comment must be a string');
+    }
+  });
+
+  it('#1982 (3) - should throw error when there is subcomments', () => {
+    try {
+      testsql(
+        qb()
+          .from('testtable')
+          .comment('/* Hello world'),
+        {
+          mysql: {
+            sql: '',
+            bindings: [],
+          },
+          oracledb: {
+            sql: '',
+            bindings: [],
+          },
+          mssql: {
+            sql: '',
+            bindings: [],
+          },
+          pg: {
+            sql: '',
+            bindings: [],
+          },
+          'pg-redshift': {
+            sql: '',
+            bindings: [],
+          },
+        }
+      );
+      expect(true).to.equal(
+        false,
+        'Expected to throw error in compilation about non-string'
+      );
+    } catch (error) {
+      expect(error.message).to.contain('Cannot include /*, */, ? in comment');
+    }
+  });
+
+  it('#1982 (4) - should throw error when there is question mark', () => {
+    try {
+      testsql(
+        qb()
+          .from('testtable')
+          .comment('?'),
+        {
+          mysql: {
+            sql: '',
+            bindings: [],
+          },
+          oracledb: {
+            sql: '',
+            bindings: [],
+          },
+          mssql: {
+            sql: '',
+            bindings: [],
+          },
+          pg: {
+            sql: '',
+            bindings: [],
+          },
+          'pg-redshift': {
+            sql: '',
+            bindings: [],
+          },
+        }
+      );
+      expect(true).to.equal(
+        false,
+        'Expected to throw error in compilation about non-string'
+      );
+    } catch (error) {
+      expect(error.message).to.contain('Cannot include /*, */, ? in comment');
+    }
+  });
+
   it('#4199 - allows hint comments in subqueries', () => {
     testsql(
       qb()
diff --git a/test/unit/schema-builder/oracledb.js b/test/unit/schema-builder/oracledb.js
index 4b43064d..66795948 100644
--- a/test/unit/schema-builder/oracledb.js
+++ b/test/unit/schema-builder/oracledb.js
@@ -1096,6 +1096,30 @@ describe('OracleDb SchemaBuilder', function () {
     }).to.throw(TypeError);
   });
 
+  it('allows adding default json objects when the column is json', function () {
+    tableSql = client
+      .schemaBuilder()
+      .table('user', function (t) {
+        t.json('preferences').defaultTo({}).notNullable();
+      })
+      .toSQL();
+    expect(tableSql[0].sql).to.equal(
+      'alter table "user" add "preferences" varchar2(4000) default \'{}\' not null check ("preferences" is json)'
+    );
+  });
+
+  it('allows adding default jsonb objects when the column is json', function () {
+    tableSql = client
+      .schemaBuilder()
+      .table('user', function (t) {
+        t.jsonb('preferences').defaultTo({}).notNullable();
+      })
+      .toSQL();
+    expect(tableSql[0].sql).to.equal(
+      'alter table "user" add "preferences" varchar2(4000) default \'{}\' not null check ("preferences" is json)'
+    );
+  });
+
   it('is possible to set raw statements in defaultTo, #146', function () {
     tableSql = client
       .schemaBuilder()