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 diff --git a/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-knex/index.js b/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-knex/index.js index 7735ae7a..f8484b93 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(','); - + + var trimmedSqlStmt = sqlStmt.trim(); + if (sqlStmt.slice(-1) === ';') { + commentedSQLStatement = `${trimmedSqlStmt.slice(0, -1)} /*${commentStr}*/;` + } + else { + commentedSQLStatement = `${trimmedSqlStmt} /*${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..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,6 +94,15 @@ describe("Comments for Knex", () => { 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); + }); + 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(); 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 = [