From b59442b77bd8810d3e2edf9094cbcdcab9fd6f03 Mon Sep 17 00:00:00 2001 From: Thiyagu55 Date: Mon, 29 Aug 2022 16:05:07 +0530 Subject: [PATCH 1/4] ; bug fix --- .../packages/sqlcommenter-knex/index.js | 24 ++++++++++++------- .../sqlcommenter-knex/test/comment.test.js | 13 ++++++++-- 2 files changed, 27 insertions(+), 10 deletions(-) diff --git a/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-knex/index.js b/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-knex/index.js index 7735ae7a..8aa687fe 100644 --- a/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-knex/index.js +++ b/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-knex/index.js @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -const {hasComment} = require('./util'); +const { hasComment } = require('./util'); const provider = require('./provider'); const hook = require('./hooks'); @@ -33,7 +33,7 @@ const defaultFields = { * TraceProvider: Should be either 'OpenCensus' or 'OpenTelemetry', indicating which library to collect trace data from. * @return {void} */ -exports.wrapMainKnex = (Knex, include={}, options={}) => { +exports.wrapMainKnex = (Knex, include = {}, options = {}) => { /* c8 ignore next 2 */ if (Knex.___alreadySQLCommenterWrapped___) @@ -46,7 +46,7 @@ exports.wrapMainKnex = (Knex, include={}, options={}) => { // Please don't change this prototype from an explicit function // to use arrow functions lest we'll get bugs with not resolving "this". - Knex.Client.prototype.query = function(conn, obj) { + Knex.Client.prototype.query = function (conn, obj) { // If Knex.VERSION() is available, that means they are using a version of knex.js < 0.16.1 // because as per https://github.com/tgriesser/knex/blob/master/CHANGELOG.md#0161---28-nov-2018 @@ -74,7 +74,7 @@ exports.wrapMainKnex = (Knex, include={}, options={}) => { // Add trace context to comments, depending on the current provider. provider.attachComments(options.TraceProvider, comments); - const filtering = typeof include === 'object' && include && Object.keys(include).length > 0; + const filtering = typeof include === 'object' && include && Object.keys(include).length > 0; // Filter out keys whose values are undefined or aren't to be included by default. const keys = Object.keys(comments).filter((key) => { /* c8 ignore next 6 */ @@ -94,11 +94,19 @@ exports.wrapMainKnex = (Knex, include={}, options={}) => { const uri_encoded_value = encodeURIComponent(comments[key]); return `${uri_encoded_key}='${uri_encoded_value}'`; }).join(','); - + + if (sqlStmt.slice(-1) === ';') { + var trimmedSqlStmt = sqlStmt.slice(0, -1); + commentedSQLStatement = `${trimmedSqlStmt} /*${commentStr}*/;` + } + else { + commentedSQLStatement = `${sqlStmt} /*${commentStr}*/` + } + if (typeof obj === 'string') { - obj = {sql: `${sqlStmt} /*${commentStr}*/`}; + obj = { sql: commentedSQLStatement }; } else { - obj.sql = `${sqlStmt} /*${commentStr}*/`; + obj.sql = commentedSQLStatement; } return query.apply(this, [conn, obj]); @@ -141,7 +149,7 @@ const getKnexVersion = (Knex) => { * TraceProvider: Should be either 'OpenCensus' or 'OpenTelemetry', indicating which library to collect trace data from. * @return {Function} A middleware that is compatible with the express framework. */ -exports.wrapMainKnexAsMiddleware = (Knex, include=null, options) => { +exports.wrapMainKnexAsMiddleware = (Knex, include = null, options) => { exports.wrapMainKnex(Knex, include, options); diff --git a/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-knex/test/comment.test.js b/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-knex/test/comment.test.js index a3ac3aa4..5e8b6dff 100644 --- a/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-knex/test/comment.test.js +++ b/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-knex/test/comment.test.js @@ -94,6 +94,15 @@ describe("Comments for Knex", () => { done(); }); + it("should append ; after comment", (done) => { + const want = "SELECT * from foo /*db_driver='knex%3Afake%3A0.0.1'*/;"; + fakeKnex.Client.prototype.query(null, { sql: 'SELECT * from foo;' }).then(({ sql }) => { + expect(sql).equals(want); + }); + done(); + }); + + it("chaining and repeated calls should NOT indefinitely chain SQL", (done) => { const want = "SELECT * from foo /*db_driver='knex%3Afake%3A0.0.1'*/"; @@ -154,10 +163,10 @@ describe("With OpenTelemetry tracing", () => { it('Starting an OpenTelemetry trace should produce `traceparent`', (done) => { const rootSpan = tracer.startSpan('rootSpan'); context.with(trace.setSpan(context.active(), rootSpan), async () => { - const obj = { sql: 'SELECT * FROM foo' }; + const obj = { sql: 'SELECT * FROM foo;' }; fakeKnex.Client.prototype.query(null, obj).then((got) => { const augmentedSQL = got.sql; - const wantSQL = `SELECT * FROM foo /*db_driver='knex%3Afake%3A0.0.1',traceparent='00-${rootSpan.spanContext().traceId}-${rootSpan.spanContext().spanId}-01'*/`; + const wantSQL = `SELECT * FROM foo /*db_driver='knex%3Afake%3A0.0.1',traceparent='00-${rootSpan.spanContext().traceId}-${rootSpan.spanContext().spanId}-01'*/;`; console.log(augmentedSQL); expect(augmentedSQL).equals(wantSQL); rootSpan.end(); From a869b7e20f7ea2c187272858e3e70fa6701ba3c0 Mon Sep 17 00:00:00 2001 From: Thiyagu55 Date: Tue, 30 Aug 2022 20:13:06 +0530 Subject: [PATCH 2/4] nodejs-sequelize ; bug fix --- .../packages/sqlcommenter-knex/index.js | 1 + .../sqlcommenter-knex/test/comment.test.js | 2 +- .../packages/sqlcommenter-sequelize/index.js | 32 ++++++++++++------- .../test/comment.test.js | 12 +++++++ 4 files changed, 35 insertions(+), 12 deletions(-) diff --git a/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-knex/index.js b/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-knex/index.js index 8aa687fe..9d51ad73 100644 --- a/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-knex/index.js +++ b/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-knex/index.js @@ -95,6 +95,7 @@ exports.wrapMainKnex = (Knex, include = {}, options = {}) => { return `${uri_encoded_key}='${uri_encoded_value}'`; }).join(','); + sqlStmt = sqlStmt.trim(); if (sqlStmt.slice(-1) === ';') { var trimmedSqlStmt = sqlStmt.slice(0, -1); commentedSQLStatement = `${trimmedSqlStmt} /*${commentStr}*/;` diff --git a/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-knex/test/comment.test.js b/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-knex/test/comment.test.js index 5e8b6dff..f22e8662 100644 --- a/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-knex/test/comment.test.js +++ b/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-knex/test/comment.test.js @@ -94,7 +94,7 @@ describe("Comments for Knex", () => { done(); }); - it("should append ; after comment", (done) => { + it("should add ; after the comment ", (done) => { const want = "SELECT * from foo /*db_driver='knex%3Afake%3A0.0.1'*/;"; fakeKnex.Client.prototype.query(null, { sql: 'SELECT * from foo;' }).then(({ sql }) => { expect(sql).equals(want); diff --git a/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-sequelize/index.js b/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-sequelize/index.js index bb683763..8f45df76 100644 --- a/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-sequelize/index.js +++ b/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-sequelize/index.js @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -const {hasComment} = require('./util'); +const { hasComment } = require('./util'); const provider = require('./provider'); const hook = require('./hooks'); @@ -33,7 +33,7 @@ const defaultFields = { * TraceProvider: Should be either 'OpenCensus' or 'OpenTelemetry', indicating which library to collect trace data from. * @return {void} */ -exports.wrapSequelize = (sequelize, include={}, options={}) => { +exports.wrapSequelize = (sequelize, include = {}, options = {}) => { /* c8 ignore next 2 */ if (sequelize.___alreadySQLCommenterWrapped___) @@ -43,7 +43,7 @@ exports.wrapSequelize = (sequelize, include={}, options={}) => { // Please don't change this prototype from an explicit function // to use arrow functions lest we'll get bugs with not resolving "this". - sequelize.dialect.Query.prototype.run = function(sql, sql_options) { + sequelize.dialect.Query.prototype.run = function (sql, sql_options) { // If a comment already exists, do not insert a new one. // See internal issue #20. @@ -54,7 +54,7 @@ exports.wrapSequelize = (sequelize, include={}, options={}) => { client_timezone: this.sequelize.options.timezone, db_driver: `sequelize:${sequelizeVersion}` }; - + if (sequelize.__middleware__) { const context = hook.getContext(); if (context && context.req) { @@ -64,9 +64,9 @@ exports.wrapSequelize = (sequelize, include={}, options={}) => { // Add trace context to comments, depending on the provider. provider.attachComments(options.TraceProvider, comments); - + // Filter out keys whose values are undefined or aren't to be included by default. - const filtering = typeof include === 'object' && include && Object.keys(include).length > 0; + const filtering = typeof include === 'object' && include && Object.keys(include).length > 0; const keys = Object.keys(comments).filter((key) => { /* c8 ignore next 6 */ if (!filtering) @@ -85,10 +85,20 @@ exports.wrapSequelize = (sequelize, include={}, options={}) => { const uri_encoded_value = encodeURIComponent(comments[key]); return `${uri_encoded_key}='${uri_encoded_value}'`; }).join(','); - - if (commentStr && commentStr.length > 0) - sql = `${sql} /*${commentStr}*/`; - + + sql = sql.trim() + if (commentStr && commentStr.length > 0) { + if (sql.slice(-1) === ';') { + var trimmedSql = sql.slice(0, -1); + sql = `${trimmedSql} /*${commentStr}*/;`; + + } + else { + sql = `${sql} /*${commentStr}*/`; + } + + } + return run.apply(this, [sql, sql_options]); } @@ -119,7 +129,7 @@ const sequelizeVersion = resolveSequelizeVersion(); * TraceProvider: Should be either 'OpenCensus' or 'OpenTelemetry', indicating which library to collect trace data from. * @return {Function} A middleware that is compatible with the express framework. */ -exports.wrapSequelizeAsMiddleware = (sequelize, include=null, options) => { +exports.wrapSequelizeAsMiddleware = (sequelize, include = null, options) => { exports.wrapSequelize(sequelize, include, options); diff --git a/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-sequelize/test/comment.test.js b/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-sequelize/test/comment.test.js index ff11d5e1..9bb3a906 100644 --- a/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-sequelize/test/comment.test.js +++ b/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-sequelize/test/comment.test.js @@ -77,6 +77,18 @@ describe("Comments for Sequelize", () => { done(); }); + it("should add ; after the comment ", (done) => { + + const want = `SELECT * FROM foo /*client_timezone='%2B00%3A00',db_driver='sequelize%3A${seq_version}'*/;`; + const query = 'SELECT * FROM foo; '; + + fakeSequelize.dialect.Query.prototype.run(query).then((sql) => { + expect(sql).equals(want); + }); + + done(); + }); + it("should NOT affix comments to statements with existing comments", (done) => { const q = [ From 27df2a754f868826726c5b9b5a4c2e7dc9e4e470 Mon Sep 17 00:00:00 2001 From: Thiyagu55 Date: Tue, 30 Aug 2022 20:58:21 +0530 Subject: [PATCH 3/4] workflow bug fix --- .github/workflows/nodejs-knex-tests.yaml | 4 ++-- .github/workflows/nodejs-sequelize-tests.yaml | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/nodejs-knex-tests.yaml b/.github/workflows/nodejs-knex-tests.yaml index e45d13b5..ec6789e6 100644 --- a/.github/workflows/nodejs-knex-tests.yaml +++ b/.github/workflows/nodejs-knex-tests.yaml @@ -5,10 +5,10 @@ on: branches: - master paths: - - nodejs/sqlcommenter-nodejs/packages/sqlcommenter-knex + - nodejs/sqlcommenter-nodejs/packages/sqlcommenter-knex/** pull_request: paths: - - nodejs/sqlcommenter-nodejs/packages/sqlcommenter-knex + - nodejs/sqlcommenter-nodejs/packages/sqlcommenter-knex/** jobs: unittests: runs-on: ubuntu-latest diff --git a/.github/workflows/nodejs-sequelize-tests.yaml b/.github/workflows/nodejs-sequelize-tests.yaml index 9080edff..3d4629b0 100644 --- a/.github/workflows/nodejs-sequelize-tests.yaml +++ b/.github/workflows/nodejs-sequelize-tests.yaml @@ -5,10 +5,10 @@ on: branches: - master paths: - - nodejs/sqlcommenter-nodejs/packages/sqlcommenter-sequelize + - nodejs/sqlcommenter-nodejs/packages/sqlcommenter-sequelize/** pull_request: paths: - - nodejs/sqlcommenter-nodejs/packages/sqlcommenter-sequelize + - nodejs/sqlcommenter-nodejs/packages/sqlcommenter-sequelize/** jobs: unittests: runs-on: ubuntu-latest From 0f8de4d3ee8fe62e1c26d3129237530857a7905a Mon Sep 17 00:00:00 2001 From: Thiyagu55 Date: Tue, 30 Aug 2022 21:07:28 +0530 Subject: [PATCH 4/4] trimming sqlstatement --- .../packages/sqlcommenter-knex/index.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-knex/index.js b/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-knex/index.js index 9d51ad73..f8484b93 100644 --- a/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-knex/index.js +++ b/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-knex/index.js @@ -95,13 +95,12 @@ exports.wrapMainKnex = (Knex, include = {}, options = {}) => { return `${uri_encoded_key}='${uri_encoded_value}'`; }).join(','); - sqlStmt = sqlStmt.trim(); + var trimmedSqlStmt = sqlStmt.trim(); if (sqlStmt.slice(-1) === ';') { - var trimmedSqlStmt = sqlStmt.slice(0, -1); - commentedSQLStatement = `${trimmedSqlStmt} /*${commentStr}*/;` + commentedSQLStatement = `${trimmedSqlStmt.slice(0, -1)} /*${commentStr}*/;` } else { - commentedSQLStatement = `${sqlStmt} /*${commentStr}*/` + commentedSQLStatement = `${trimmedSqlStmt} /*${commentStr}*/` } if (typeof obj === 'string') {