add validation in .offset() (#2908)

lib/query/builder.js

@@ -14,6 +14,7 @@ const {
   isBoolean,
   isEmpty,
   isFunction,
+  isNil,
   isNumber,
   isObject,
   isString,
@@ -856,7 +857,17 @@ assign(Builder.prototype, {
 
   // Only allow a single "offset" to be set for the current query.
   offset(value) {
-    this._single.offset = value;
+    if (isNil(value) || value instanceof Raw || value instanceof Builder) {
+      // Builder for backward compatibility
+      this._single.offset = value;
+    } else {
+      const val = parseInt(value, 10);
+      if (isNaN(val)) {
+        this.client.logger.warn('A valid integer must be provided to offset');
+      } else {
+        this._single.offset = val;
+      }
+    }
     return this;
   },
 

test/unit/query/builder.js

@@ -40,6 +40,35 @@ const clientsWithNullAsDefault = {
   ),
 };
 
+const customLoggerConfig = {
+  log: {
+    warn: function(message) {
+      throw new Error(message);
+    },
+  },
+};
+const clientsWithCustomLoggerForTestWarnings = {
+  mysql: new MySQL_Client(
+    Object.assign({ client: 'mysql' }, customLoggerConfig)
+  ),
+  pg: new PG_Client(Object.assign({ client: 'pg' }, customLoggerConfig)),
+  'pg-redshift': new Redshift_Client(
+    Object.assign({ client: 'redshift' }, customLoggerConfig)
+  ),
+  oracledb: new Oracledb_Client(
+    Object.assign({ client: 'oracledb' }, customLoggerConfig)
+  ),
+  sqlite3: new SQLite3_Client(
+    Object.assign(
+      { client: 'sqlite3' },
+      { ...customLoggerConfig, ...useNullAsDefaultConfig }
+    )
+  ),
+  mssql: new MSSQL_Client(
+    Object.assign({ client: 'mssql' }, customLoggerConfig)
+  ),
+};
+
 // note: as a workaround, we are using postgres here, since that's using the default " field wrapping
 // otherwise subquery cloning would need to be fixed. See: https://github.com/tgriesser/knex/pull/2063
 function qb() {
@@ -3868,6 +3897,39 @@ describe('QueryBuilder', () => {
     );
   });
 
+  it('limits and offsets with raw', () => {
+    testsql(
+      qb()
+        .select('*')
+        .from('users')
+        .offset(raw('5'))
+        .limit(raw('10')),
+      {
+        mysql: {
+          sql: 'select * from `users` limit ? offset 5',
+          bindings: [10],
+        },
+        mssql: {
+          sql: 'select * from [users] offset 5 rows fetch next ? rows only',
+          bindings: [10],
+        },
+        oracledb: {
+          sql:
+            'select * from (select row_.*, ROWNUM rownum_ from (select * from "users") row_ where rownum <= ?) where rownum_ > 5',
+          bindings: [15],
+        },
+        pg: {
+          sql: 'select * from "users" limit ? offset 5',
+          bindings: [10],
+        },
+        'pg-redshift': {
+          sql: 'select * from "users" limit ? offset 5',
+          bindings: [10],
+        },
+      }
+    );
+  });
+
   it('limits and raw selects', () => {
     testsql(
       qb()
@@ -7177,6 +7239,100 @@ describe('QueryBuilder', () => {
     );
   });
 
+  it('should throw warning with null call in limit', function() {
+    try {
+      testsql(
+        qb()
+          .from('test')
+          .limit(null),
+        {
+          mysql: {
+            sql: 'select * from `test`',
+            bindings: [],
+          },
+          mssql: {
+            sql: 'select * from [test]',
+            bindings: [],
+          },
+          pg: {
+            sql: 'select * from "test"',
+            bindings: [],
+          },
+          'pg-redshift': {
+            sql: 'select * from "test"',
+            bindings: [],
+          },
+        },
+        clientsWithCustomLoggerForTestWarnings
+      );
+    } catch (error) {
+      expect(error.message).to.equal(
+        'A valid integer must be provided to limit'
+      );
+    }
+  });
+
+  it('should do nothing with offset when passing null', () => {
+    testsql(
+      qb()
+        .from('test')
+        .limit(10)
+        .offset(null),
+      {
+        mysql: {
+          sql: 'select * from `test` limit ?',
+          bindings: [10],
+        },
+        mssql: {
+          sql: 'select top (?) * from [test]',
+          bindings: [10],
+        },
+        pg: {
+          sql: 'select * from "test" limit ?',
+          bindings: [10],
+        },
+        'pg-redshift': {
+          sql: 'select * from "test" limit ?',
+          bindings: [10],
+        },
+      }
+    );
+  });
+
+  it('should throw warning with wrong value call in offset', function() {
+    try {
+      testsql(
+        qb()
+          .from('test')
+          .limit(10)
+          .offset('$10'),
+        {
+          mysql: {
+            sql: 'select * from `test` limit ?',
+            bindings: [10],
+          },
+          mssql: {
+            sql: 'select top (?) * from [test]',
+            bindings: [10],
+          },
+          pg: {
+            sql: 'select * from "test" limit ?',
+            bindings: [10],
+          },
+          'pg-redshift': {
+            sql: 'select * from "test" limit ?',
+            bindings: [10],
+          },
+        },
+        clientsWithCustomLoggerForTestWarnings
+      );
+    } catch (error) {
+      expect(error.message).to.equal(
+        'A valid integer must be provided to offset'
+      );
+    }
+  });
+
   it('should clear offset when passing null', () => {
     testsql(
       qb()