From 8729d150212d5bb2dcb15c87b6dbcd8179f073e9 Mon Sep 17 00:00:00 2001 From: Drethic Date: Thu, 16 Dec 2021 12:39:06 -0500 Subject: [PATCH 1/6] fix(abstract): dialog query-generator for JSON in PG Added check to traverseJSON that checks if value is JSON and passes result to jsonPathExtrationQuery Added isJson param to jsonPathExtractionQuery Updated postgres JSON join arrow to change if value is JSON with '#' will be ignored, and an empty message aborts the commit. --- lib/dialects/abstract/query-generator.js | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/lib/dialects/abstract/query-generator.js b/lib/dialects/abstract/query-generator.js index c9793a45693e..fb0a46c974a8 100644 --- a/lib/dialects/abstract/query-generator.js +++ b/lib/dialects/abstract/query-generator.js @@ -1062,12 +1062,13 @@ class QueryGenerator { /** * Generates an SQL query that extract JSON property of given path. * - * @param {string} column The JSON column - * @param {string|Array} [path] The path to extract (optional) - * @returns {string} The generated sql query + * @param {string} column The JSON column + * @param {string|Array} [path] The path to extract (optional) + * @param {boolean} [isJson] The value is JSON use alt symbols (optional) + * @returns {string} The generated sql query * @private */ - jsonPathExtractionQuery(column, path) { + jsonPathExtractionQuery(column, path, isJson) { let paths = _.toPath(path); let pathStr; const quotedColumn = this.isIdentifierQuoted(column) @@ -1102,8 +1103,9 @@ class QueryGenerator { return `json_unquote(json_extract(${quotedColumn},${pathStr}))`; case 'postgres': + const join = isJson ? '#>' : '#>>'; pathStr = this.escape(`{${paths.join(',')}}`); - return `(${quotedColumn}#>>${pathStr})`; + return `(${quotedColumn}${join}${pathStr})`; default: throw new Error(`Unsupported ${this.dialect} for JSON operations`); @@ -2459,11 +2461,19 @@ class QueryGenerator { path[path.length - 1] = tmp[0]; } - const pathKey = this.jsonPathExtractionQuery(baseKey, path); + let pathKey = this.jsonPathExtractionQuery(baseKey, path); if (_.isPlainObject(item)) { Utils.getOperators(item).forEach(op => { const value = this._toJSONValue(item[op]); + let isJson = false; + try { + JSON.stringify(value); + isJson = true; + } catch (e) { + // failed to parse, is not json so isJson remains false + } + pathKey = this.jsonPathExtractionQuery(baseKey, path, isJson); items.push(this.whereItemQuery(this._castKey(pathKey, value, cast), { [op]: value })); }); _.forOwn(item, (value, itemProp) => { From 3219e49d2cf3fcb67e5d2b1f6e1801f67e94686f Mon Sep 17 00:00:00 2001 From: Drethic Date: Thu, 16 Dec 2021 14:25:37 -0500 Subject: [PATCH 2/6] fix(abstract): updated unit tests Added check if value is string and operator is contains Updated unit tests with new expected outputs --- lib/dialects/abstract/query-generator.js | 12 +++++++----- test/unit/sql/where.test.js | 4 ++-- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/lib/dialects/abstract/query-generator.js b/lib/dialects/abstract/query-generator.js index fb0a46c974a8..5594dcf9011a 100644 --- a/lib/dialects/abstract/query-generator.js +++ b/lib/dialects/abstract/query-generator.js @@ -2467,11 +2467,13 @@ class QueryGenerator { Utils.getOperators(item).forEach(op => { const value = this._toJSONValue(item[op]); let isJson = false; - try { - JSON.stringify(value); - isJson = true; - } catch (e) { - // failed to parse, is not json so isJson remains false + if (typeof value === 'string' && op === Op.contains) { + try { + JSON.stringify(value); + isJson = true; + } catch (e) { + // failed to parse, is not json so isJson remains false + } } pathKey = this.jsonPathExtractionQuery(baseKey, path, isJson); items.push(this.whereItemQuery(this._castKey(pathKey, value, cast), { [op]: value })); diff --git a/test/unit/sql/where.test.js b/test/unit/sql/where.test.js index 7739395f2d8e..da3429695f13 100644 --- a/test/unit/sql/where.test.js +++ b/test/unit/sql/where.test.js @@ -1263,11 +1263,11 @@ describe(Support.getTestDialectTeaser('SQL'), () => { current.where(current.fn('lower', current.col('name')), null)], { default: '(SUM([hours]) > 0 AND lower([name]) IS NULL)' }); - + testsql(current.where(current.col('hours'), Op.between, [0, 5]), { default: '[hours] BETWEEN 0 AND 5' }); - + testsql(current.where(current.col('hours'), Op.notBetween, [0, 5]), { default: '[hours] NOT BETWEEN 0 AND 5' }); From 06b220fba5733920c1c3c8dc38a12919b83871f1 Mon Sep 17 00:00:00 2001 From: Drethic Date: Tue, 21 Dec 2021 12:58:01 -0500 Subject: [PATCH 3/6] fix(abstract): add query generator unit tests Added tests to validate jsonPathExtractionQuery works with and without isJSON --- .../dialects/abstract/query-generator.test.js | 43 ++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/test/unit/dialects/abstract/query-generator.test.js b/test/unit/dialects/abstract/query-generator.test.js index 25cd79391316..46a8ba798216 100644 --- a/test/unit/dialects/abstract/query-generator.test.js +++ b/test/unit/dialects/abstract/query-generator.test.js @@ -3,7 +3,8 @@ const chai = require('chai'), expect = chai.expect, Op = require('../../../../lib/operators'), - getAbstractQueryGenerator = require('../../support').getAbstractQueryGenerator; + Support = require('../../support'), + getAbstractQueryGenerator = Support.getAbstractQueryGenerator; describe('QueryGenerator', () => { describe('whereItemQuery', () => { @@ -113,5 +114,45 @@ describe('QueryGenerator', () => { expect(() => QG.format(value)).to.throw(Error); }); }); + + describe('jsonPathExtractionQuery', () => { + const expectQueryGenerator = (query, assertions) => { + const expectation = assertions[Support.sequelize.dialect.name]; + if (!expectation) { + throw new Error(`Undefined expectation for "${Support.sequelize.dialect.name}"!`); + } + expect(query).to.equal(expectation); + }; + + it('Should handle isJson parameter true', function() { + const QG = getAbstractQueryGenerator(this.sequelize); + expectQueryGenerator(QG.jsonPathExtractionQuery('profile', 'id', true), { + postgres: '(profile#>\'{id}\')', + sqlite: 'json_extract(profile,\'$.id\')', + mariadb: 'json_unquote(json_extract(profile,\'$.id\'))', + mysql: "json_unquote(json_extract(profile,'$.\\\"id\\\"'))" + }); + }); + + it('Should use default handling if isJson is false', function() { + const QG = getAbstractQueryGenerator(this.sequelize); + expectQueryGenerator(QG.jsonPathExtractionQuery('profile', 'id', false), { + postgres: '(profile#>>\'{id}\')', + sqlite: 'json_extract(profile,\'$.id\')', + mariadb: 'json_unquote(json_extract(profile,\'$.id\'))', + mysql: "json_unquote(json_extract(profile,'$.\\\"id\\\"'))" + }); + }); + + it('Should use default handling if isJson is not passed', function() { + const QG = getAbstractQueryGenerator(this.sequelize); + expectQueryGenerator(QG.jsonPathExtractionQuery('profile', 'id'), { + postgres: '(profile#>>\'{id}\')', + sqlite: 'json_extract(profile,\'$.id\')', + mariadb: 'json_unquote(json_extract(profile,\'$.id\'))', + mysql: "json_unquote(json_extract(profile,'$.\\\"id\\\"'))" + }); + }); + }); }); From 3968e4cefa9d3991e2be5bb7919f729482d42a0a Mon Sep 17 00:00:00 2001 From: Drethic Date: Tue, 21 Dec 2021 15:02:46 -0500 Subject: [PATCH 4/6] fix(abstract): Fix merge error Removed extra bracket from merge conflict --- test/unit/dialects/abstract/query-generator.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/unit/dialects/abstract/query-generator.test.js b/test/unit/dialects/abstract/query-generator.test.js index ab3fec476726..88103f872bf5 100644 --- a/test/unit/dialects/abstract/query-generator.test.js +++ b/test/unit/dialects/abstract/query-generator.test.js @@ -174,7 +174,6 @@ describe('QueryGenerator', () => { }); }); }); -}); describe('queryIdentifier', () => { it('should throw an error if call base quoteIdentifier', function() { From 20db473dfb7bce985c5af479865c9bb68bdacb04 Mon Sep 17 00:00:00 2001 From: Drethic Date: Tue, 21 Dec 2021 15:37:19 -0500 Subject: [PATCH 5/6] fix(abstract): Patch failing tests Updated query generator tests to handle all supported DB types --- .../dialects/abstract/query-generator.test.js | 41 +++++++++++-------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/test/unit/dialects/abstract/query-generator.test.js b/test/unit/dialects/abstract/query-generator.test.js index 88103f872bf5..d59755fa001c 100644 --- a/test/unit/dialects/abstract/query-generator.test.js +++ b/test/unit/dialects/abstract/query-generator.test.js @@ -141,36 +141,45 @@ describe('QueryGenerator', () => { if (!expectation) { throw new Error(`Undefined expectation for "${Support.sequelize.dialect.name}"!`); } - expect(query).to.equal(expectation); + return expectation(query); }; it('Should handle isJson parameter true', function() { const QG = getAbstractQueryGenerator(this.sequelize); - expectQueryGenerator(QG.jsonPathExtractionQuery('profile', 'id', true), { - postgres: '(profile#>\'{id}\')', - sqlite: 'json_extract(profile,\'$.id\')', - mariadb: 'json_unquote(json_extract(profile,\'$.id\'))', - mysql: "json_unquote(json_extract(profile,'$.\\\"id\\\"'))" + expectQueryGenerator(() => QG.jsonPathExtractionQuery('profile', 'id', true), { + postgres: query => expect(query()).to.equal('(profile#>\'{id}\')'), + sqlite: query => expect(query()).to.equal('json_extract(profile,\'$.id\')'), + mariadb: query => expect(query()).to.equal('json_unquote(json_extract(profile,\'$.id\'))'), + mysql: query => expect(query()).to.equal("json_unquote(json_extract(profile,'$.\\\"id\\\"'))"), + mssql: query => expect(query).to.throw(Error), + snowflake: query => expect(query).to.throw(Error), + db2: query => expect(query).to.throw(Error) }); }); it('Should use default handling if isJson is false', function() { const QG = getAbstractQueryGenerator(this.sequelize); - expectQueryGenerator(QG.jsonPathExtractionQuery('profile', 'id', false), { - postgres: '(profile#>>\'{id}\')', - sqlite: 'json_extract(profile,\'$.id\')', - mariadb: 'json_unquote(json_extract(profile,\'$.id\'))', - mysql: "json_unquote(json_extract(profile,'$.\\\"id\\\"'))" + expectQueryGenerator(() => QG.jsonPathExtractionQuery('profile', 'id', false), { + postgres: query => expect(query()).to.equal('(profile#>>\'{id}\')'), + sqlite: query => expect(query()).to.equal('json_extract(profile,\'$.id\')'), + mariadb: query => expect(query()).to.equal('json_unquote(json_extract(profile,\'$.id\'))'), + mysql: query => expect(query()).to.equal("json_unquote(json_extract(profile,'$.\\\"id\\\"'))"), + mssql: query => expect(query).to.throw(Error), + snowflake: query => expect(query).to.throw(Error), + db2: query => expect(query).to.throw(Error) }); }); it('Should use default handling if isJson is not passed', function() { const QG = getAbstractQueryGenerator(this.sequelize); - expectQueryGenerator(QG.jsonPathExtractionQuery('profile', 'id'), { - postgres: '(profile#>>\'{id}\')', - sqlite: 'json_extract(profile,\'$.id\')', - mariadb: 'json_unquote(json_extract(profile,\'$.id\'))', - mysql: "json_unquote(json_extract(profile,'$.\\\"id\\\"'))" + expectQueryGenerator(() => QG.jsonPathExtractionQuery('profile', 'id'), { + postgres: query => expect(query()).to.equal('(profile#>>\'{id}\')'), + sqlite: query => expect(query()).to.equal('json_extract(profile,\'$.id\')'), + mariadb: query => expect(query()).to.equal('json_unquote(json_extract(profile,\'$.id\'))'), + mysql: query => expect(query()).to.equal("json_unquote(json_extract(profile,'$.\\\"id\\\"'))"), + mssql: query => expect(query).to.throw(Error), + snowflake: query => expect(query).to.throw(Error), + db2: query => expect(query).to.throw(Error) }); }); }); From bc18d092bb15c90de2ef6974a047893b60cd5c68 Mon Sep 17 00:00:00 2001 From: Sascha Depold Date: Wed, 22 Dec 2021 12:34:02 +0100 Subject: [PATCH 6/6] fix(tests): update test name --- test/unit/dialects/abstract/query-generator.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/unit/dialects/abstract/query-generator.test.js b/test/unit/dialects/abstract/query-generator.test.js index d59755fa001c..3ed56dd17ba1 100644 --- a/test/unit/dialects/abstract/query-generator.test.js +++ b/test/unit/dialects/abstract/query-generator.test.js @@ -144,7 +144,7 @@ describe('QueryGenerator', () => { return expectation(query); }; - it('Should handle isJson parameter true', function() { + it('should handle isJson parameter true', function() { const QG = getAbstractQueryGenerator(this.sequelize); expectQueryGenerator(() => QG.jsonPathExtractionQuery('profile', 'id', true), { postgres: query => expect(query()).to.equal('(profile#>\'{id}\')'), @@ -157,7 +157,7 @@ describe('QueryGenerator', () => { }); }); - it('Should use default handling if isJson is false', function() { + it('should use default handling if isJson is false', function() { const QG = getAbstractQueryGenerator(this.sequelize); expectQueryGenerator(() => QG.jsonPathExtractionQuery('profile', 'id', false), { postgres: query => expect(query()).to.equal('(profile#>>\'{id}\')'),