From 1a957da0282b6e9bab1ab965d2f7e19819ea1250 Mon Sep 17 00:00:00 2001 From: Ethan Hur Date: Thu, 15 Nov 2018 17:25:07 +0900 Subject: [PATCH 01/13] add validation in `.offset()` --- src/query/builder.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/query/builder.js b/src/query/builder.js index 3151876a65..fbfe76885a 100644 --- a/src/query/builder.js +++ b/src/query/builder.js @@ -825,7 +825,12 @@ assign(Builder.prototype, { // Only allow a single "offset" to be set for the current query. offset(value) { - this._single.offset = value; + const val = parseInt(value, 10); + if (isNaN(val)) { + this.client.logger.warn('A valid integer must be provided to offset'); + } else { + this._single.offset = value; + } return this; }, From 3ea763734c6cbb6ecdc72d3818baa91f33deecc2 Mon Sep 17 00:00:00 2001 From: Ethan Hur Date: Thu, 15 Nov 2018 17:28:21 +0900 Subject: [PATCH 02/13] fix typo --- src/query/builder.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/query/builder.js b/src/query/builder.js index fbfe76885a..97b44e699a 100644 --- a/src/query/builder.js +++ b/src/query/builder.js @@ -829,7 +829,7 @@ assign(Builder.prototype, { if (isNaN(val)) { this.client.logger.warn('A valid integer must be provided to offset'); } else { - this._single.offset = value; + this._single.offset = val; } return this; }, From 9d04306021dd2eeffd60a581b709cc8ba4673afd Mon Sep 17 00:00:00 2001 From: hursungyun Date: Fri, 16 Nov 2018 21:55:58 +0900 Subject: [PATCH 03/13] add test of limit warning message --- test/unit/query/builder.js | 59 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/test/unit/query/builder.js b/test/unit/query/builder.js index 77bef5ddfd..de26622616 100644 --- a/test/unit/query/builder.js +++ b/test/unit/query/builder.js @@ -40,6 +40,32 @@ var clientsWithNullAsDefault = { ), }; +var customLoggerConfig = { + log: { + warn: function(message) { + throw new Error(message); + }, + }, +}; +var 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) + ), + 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() { @@ -6476,6 +6502,39 @@ describe('QueryBuilder', function() { ); }); + 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('allows passing builder into where clause, #162', function() { var chain = qb() .from('chapter') From 9476420ce4ac298370fadfcbab0c5d5b7b64ea98 Mon Sep 17 00:00:00 2001 From: hursungyun Date: Fri, 16 Nov 2018 22:15:12 +0900 Subject: [PATCH 04/13] skip sqlite --- test/unit/query/builder.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/unit/query/builder.js b/test/unit/query/builder.js index de26622616..c5adc215d3 100644 --- a/test/unit/query/builder.js +++ b/test/unit/query/builder.js @@ -58,9 +58,9 @@ var clientsWithCustomLoggerForTestWarnings = { oracledb: new Oracledb_Client( Object.assign({ client: 'oracledb' }, customLoggerConfig) ), - sqlite3: new SQLite3_Client( - Object.assign({ client: 'sqlite3' }, customLoggerConfig) - ), + // sqlite3: new SQLite3_Client( + // Object.assign({ client: 'sqlite3' }, customLoggerConfig) + // ), mssql: new MSSQL_Client( Object.assign({ client: 'mssql' }, customLoggerConfig) ), From b593d404755342de23f88977babea73ffd90f4ba Mon Sep 17 00:00:00 2001 From: hursungyun Date: Fri, 16 Nov 2018 22:22:26 +0900 Subject: [PATCH 05/13] add test for checking offset warning message --- test/unit/query/builder.js | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/test/unit/query/builder.js b/test/unit/query/builder.js index c5adc215d3..06891acbc5 100644 --- a/test/unit/query/builder.js +++ b/test/unit/query/builder.js @@ -6535,6 +6535,40 @@ describe('QueryBuilder', function() { } }); + it('should throw warning with null call in offset', function() { + try { + testsql( + qb() + .from('test') + .limit(10) + .offset(null), + { + mysql: { + sql: 'select * from `test` limit ?', + bindings: [10], + }, + mssql: { + sql: 'select * from [test] limit ?', + 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('allows passing builder into where clause, #162', function() { var chain = qb() .from('chapter') From 61b7a68bcfce2f352d2f2b3273287624ece632d5 Mon Sep 17 00:00:00 2001 From: hursungyun Date: Fri, 16 Nov 2018 22:45:46 +0900 Subject: [PATCH 06/13] fix mssql query --- test/unit/query/builder.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/query/builder.js b/test/unit/query/builder.js index 06891acbc5..fd0f9f8cb4 100644 --- a/test/unit/query/builder.js +++ b/test/unit/query/builder.js @@ -6548,7 +6548,7 @@ describe('QueryBuilder', function() { bindings: [10], }, mssql: { - sql: 'select * from [test] limit ?', + sql: 'select top (?) * from [test]', bindings: [10], }, pg: { From d7e17ec50e1a97f5ed07337c8fe5415504f42bc9 Mon Sep 17 00:00:00 2001 From: hursungyun Date: Fri, 16 Nov 2018 22:52:29 +0900 Subject: [PATCH 07/13] add unreachable point in test --- test/unit/query/builder.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/unit/query/builder.js b/test/unit/query/builder.js index fd0f9f8cb4..d2edff19e8 100644 --- a/test/unit/query/builder.js +++ b/test/unit/query/builder.js @@ -6528,6 +6528,7 @@ describe('QueryBuilder', function() { }, clientsWithCustomLoggerForTestWarnings ); + throw new Error('Should not reach this point'); } catch (error) { expect(error.message).to.equal( 'A valid integer must be provided to limit' @@ -6562,6 +6563,7 @@ describe('QueryBuilder', function() { }, clientsWithCustomLoggerForTestWarnings ); + throw new Error('Should not reach this point'); } catch (error) { expect(error.message).to.equal( 'A valid integer must be provided to offset' From 9de3e144c52ffe16ecd8a84ad84a1c3f58aaeb72 Mon Sep 17 00:00:00 2001 From: hursungyun Date: Fri, 16 Nov 2018 23:00:28 +0900 Subject: [PATCH 08/13] Revert "add unreachable point in test" This reverts commit d7e17ec50e1a97f5ed07337c8fe5415504f42bc9. --- test/unit/query/builder.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/unit/query/builder.js b/test/unit/query/builder.js index d2edff19e8..fd0f9f8cb4 100644 --- a/test/unit/query/builder.js +++ b/test/unit/query/builder.js @@ -6528,7 +6528,6 @@ describe('QueryBuilder', function() { }, clientsWithCustomLoggerForTestWarnings ); - throw new Error('Should not reach this point'); } catch (error) { expect(error.message).to.equal( 'A valid integer must be provided to limit' @@ -6563,7 +6562,6 @@ describe('QueryBuilder', function() { }, clientsWithCustomLoggerForTestWarnings ); - throw new Error('Should not reach this point'); } catch (error) { expect(error.message).to.equal( 'A valid integer must be provided to offset' From b04282ee216f51fff1cc6866438e8a6c34b4aa80 Mon Sep 17 00:00:00 2001 From: Ethan Hur Date: Thu, 13 Dec 2018 19:57:02 +0900 Subject: [PATCH 09/13] add nil check for backward-compatible --- src/query/builder.js | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/query/builder.js b/src/query/builder.js index 97b44e699a..09d112974c 100644 --- a/src/query/builder.js +++ b/src/query/builder.js @@ -14,6 +14,7 @@ import { isBoolean, isEmpty, isFunction, + isNil, isNumber, isObject, isString, @@ -825,11 +826,15 @@ assign(Builder.prototype, { // Only allow a single "offset" to be set for the current query. offset(value) { - const val = parseInt(value, 10); - if (isNaN(val)) { - this.client.logger.warn('A valid integer must be provided to offset'); + if (isNil(value)) { + this._single.offset = value; } else { - this._single.offset = val; + 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; }, From d941cc3fff96ecf25b4d060b9bbc1c9c9d9dc0b8 Mon Sep 17 00:00:00 2001 From: ethanhur Date: Tue, 8 Oct 2019 23:10:12 +0900 Subject: [PATCH 10/13] offset works with Raw --- lib/query/builder.js | 3 ++- test/unit/query/builder.js | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/lib/query/builder.js b/lib/query/builder.js index a4f87b6ad0..ed645e97df 100644 --- a/lib/query/builder.js +++ b/lib/query/builder.js @@ -857,7 +857,8 @@ assign(Builder.prototype, { // Only allow a single "offset" to be set for the current query. offset(value) { - if (isNil(value)) { + if (isNil(value) || value instanceof Raw || value instanceof Builder) { + // Builder for backward compatibility this._single.offset = value; } else { const val = parseInt(value, 10); diff --git a/test/unit/query/builder.js b/test/unit/query/builder.js index 2ba69ee819..94d9856bea 100644 --- a/test/unit/query/builder.js +++ b/test/unit/query/builder.js @@ -3894,6 +3894,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() From 5e74c3ac5c99c7df586c4f9d2000b63364152f2e Mon Sep 17 00:00:00 2001 From: Ethan Hur Date: Wed, 9 Oct 2019 00:29:46 +0900 Subject: [PATCH 11/13] add tests for code coverage --- test/unit/query/builder.js | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/test/unit/query/builder.js b/test/unit/query/builder.js index 94d9856bea..4fa6100c91 100644 --- a/test/unit/query/builder.js +++ b/test/unit/query/builder.js @@ -7269,13 +7269,40 @@ describe('QueryBuilder', () => { } }); - it('should throw warning with null call in offset', function() { + 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(null), + .offset('$10'), { mysql: { sql: 'select * from `test` limit ?', From 4a8bcb97d16469d0add75467d6ec8761530faaf2 Mon Sep 17 00:00:00 2001 From: Ethan Hur Date: Tue, 15 Oct 2019 16:30:04 +0900 Subject: [PATCH 12/13] var to const, uncomment sqlite --- test/unit/query/builder.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/unit/query/builder.js b/test/unit/query/builder.js index 4fa6100c91..bdc3160514 100644 --- a/test/unit/query/builder.js +++ b/test/unit/query/builder.js @@ -40,14 +40,14 @@ const clientsWithNullAsDefault = { ), }; -var customLoggerConfig = { +const customLoggerConfig = { log: { warn: function(message) { throw new Error(message); }, }, }; -var clientsWithCustomLoggerForTestWarnings = { +const clientsWithCustomLoggerForTestWarnings = { mysql: new MySQL_Client( Object.assign({ client: 'mysql' }, customLoggerConfig) ), @@ -58,9 +58,9 @@ var clientsWithCustomLoggerForTestWarnings = { oracledb: new Oracledb_Client( Object.assign({ client: 'oracledb' }, customLoggerConfig) ), - // sqlite3: new SQLite3_Client( - // Object.assign({ client: 'sqlite3' }, customLoggerConfig) - // ), + sqlite3: new SQLite3_Client( + Object.assign({ client: 'sqlite3' }, customLoggerConfig) + ), mssql: new MSSQL_Client( Object.assign({ client: 'mssql' }, customLoggerConfig) ), From cdde64e16b7319ecc24118666fed594b4d2f5b01 Mon Sep 17 00:00:00 2001 From: Ethan Hur Date: Tue, 15 Oct 2019 16:54:46 +0900 Subject: [PATCH 13/13] add useNullAsDefault to avoid error --- test/unit/query/builder.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/unit/query/builder.js b/test/unit/query/builder.js index bdc3160514..eba8f0867d 100644 --- a/test/unit/query/builder.js +++ b/test/unit/query/builder.js @@ -59,7 +59,10 @@ const clientsWithCustomLoggerForTestWarnings = { Object.assign({ client: 'oracledb' }, customLoggerConfig) ), sqlite3: new SQLite3_Client( - Object.assign({ client: 'sqlite3' }, customLoggerConfig) + Object.assign( + { client: 'sqlite3' }, + { ...customLoggerConfig, ...useNullAsDefaultConfig } + ) ), mssql: new MSSQL_Client( Object.assign({ client: 'mssql' }, customLoggerConfig)