From 895274afda387942f570562b2ef66918188a51a2 Mon Sep 17 00:00:00 2001 From: Igor Savin Date: Thu, 29 Nov 2018 01:34:48 +0100 Subject: [PATCH 1/5] Add failing test --- package.json | 2 +- test/unit/migrate/Migrator.js | 40 ++++++++++++++++++- .../processed-migrations/simple_migration.js | 9 +++++ 3 files changed, 49 insertions(+), 2 deletions(-) create mode 100644 test/unit/migrate/processed-migrations/simple_migration.js diff --git a/package.json b/package.json index d97d962cb3..928ff3f15e 100644 --- a/package.json +++ b/package.json @@ -62,7 +62,7 @@ "prettier": "^1.15.2", "rimraf": "^2.6.2", "sinon": "^7.1.1", - "sinon-chai": "^3.2.0", + "sinon-chai": "^3.3.0", "source-map-support": "^0.5.9", "sqlite3": "^4.0.4", "tap-spec": "^5.0.0", diff --git a/test/unit/migrate/Migrator.js b/test/unit/migrate/Migrator.js index e887919b36..7db13e32b3 100644 --- a/test/unit/migrate/Migrator.js +++ b/test/unit/migrate/Migrator.js @@ -13,12 +13,18 @@ describe('Migrator', () => { knex = Knex({ ...sqliteConfig, migrationSource, - postProcessResponse: () => { + postProcessResponse: (a, b, c) => { throw new Error('Response was processed'); }, }); }); + after(() => { + return knex.migrate.rollback({ + directory: 'test/unit/migrate/migrations', + }); + }); + it('latest', (done) => { expect(() => { knex.migrate @@ -31,4 +37,36 @@ describe('Migrator', () => { }).not.to.throw(); }); }); + + describe('uses postProcessResponse for migrations', (done) => { + let migrationSource; + let knex; + before(() => { + migrationSource = new FsMigrations( + 'test/unit/migrate/processed-migrations/' + ); + }); + + after(() => { + return knex.migrate.rollback({ + directory: 'test/unit/migrate/processed-migrations', + }); + }); + + it('latest', (done) => { + knex = Knex({ + ...sqliteConfig, + migrationSource, + postProcessResponse: () => { + done(); + }, + }); + + expect(() => { + knex.migrate.latest({ + directory: 'test/unit/migrate/processed-migrations', + }); + }).not.to.throw(); + }); + }); }); diff --git a/test/unit/migrate/processed-migrations/simple_migration.js b/test/unit/migrate/processed-migrations/simple_migration.js new file mode 100644 index 0000000000..a92144533f --- /dev/null +++ b/test/unit/migrate/processed-migrations/simple_migration.js @@ -0,0 +1,9 @@ +exports.up = (knex) => { + return knex.schema.createTable('old_users', (table) => { + table.string('name'); + }); +}; + +exports.down = (knex) => { + return knex.schema.dropTable('rules'); +}; From fd7a07a8f5aa1ede74aa97c329272ed84560140c Mon Sep 17 00:00:00 2001 From: Igor Savin Date: Thu, 29 Nov 2018 01:36:17 +0100 Subject: [PATCH 2/5] Remove unneeded params --- test/unit/migrate/Migrator.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/migrate/Migrator.js b/test/unit/migrate/Migrator.js index 7db13e32b3..b4a8877eba 100644 --- a/test/unit/migrate/Migrator.js +++ b/test/unit/migrate/Migrator.js @@ -13,7 +13,7 @@ describe('Migrator', () => { knex = Knex({ ...sqliteConfig, migrationSource, - postProcessResponse: (a, b, c) => { + postProcessResponse: () => { throw new Error('Response was processed'); }, }); From 2216646ef5875b4fa0a07882ff0b081251d0309f Mon Sep 17 00:00:00 2001 From: Igor Savin Date: Thu, 29 Nov 2018 23:44:40 +0100 Subject: [PATCH 3/5] Switch between processing being enabled and disabled --- package.json | 2 +- src/dialects/oracledb/query/compiler.js | 5 +- src/migrate/Migrator.js | 49 ++++++++++++------- src/util/make-knex.js | 22 +++++++++ test/unit/migrate/Migrator.js | 14 +++--- .../processed-migrations/simple_migration.js | 2 +- 6 files changed, 63 insertions(+), 31 deletions(-) diff --git a/package.json b/package.json index 928ff3f15e..6b6662aaaf 100644 --- a/package.json +++ b/package.json @@ -57,7 +57,7 @@ "mysql": "^2.16.0", "mysql2": "^1.6.4", "nyc": "^13.1.0", - "pg": "^7.6.1", + "pg": "^7.7.1", "pg-query-stream": "^1.1.2", "prettier": "^1.15.2", "rimraf": "^2.6.2", diff --git a/src/dialects/oracledb/query/compiler.js b/src/dialects/oracledb/query/compiler.js index 6ec1b41974..d1c790b6f5 100644 --- a/src/dialects/oracledb/query/compiler.js +++ b/src/dialects/oracledb/query/compiler.js @@ -296,10 +296,7 @@ _.assign(Oracledb_Compiler.prototype, { let returningClause = ''; let intoClause = ''; - if ( - _.isEmpty(updates) && - typeof this.single.update !== 'function' - ) { + if (_.isEmpty(updates) && typeof this.single.update !== 'function') { return ''; } diff --git a/src/migrate/Migrator.js b/src/migrate/Migrator.js index 2c9e4ba14b..b6d61b9edf 100644 --- a/src/migrate/Migrator.js +++ b/src/migrate/Migrator.js @@ -48,11 +48,14 @@ const CONFIG_DEFAULT = Object.freeze({ export default class Migrator { constructor(knex) { // Clone knex instance and remove post-processing that is unnecessary for internal queries from a cloned config - this.knex = isFunction(knex) - ? knex.withUserParams(knex.userParams) - : Object.assign({}, knex); - this.knex.client.config.wrapIdentifier = null; - this.knex.client.config.postProcessResponse = null; + if (isFunction(knex)) { + this.knex = knex.withUserParams({ + ...knex.userParams, + }); + this.knex.disableProcessing(); + } else { + this.knex = Object.assign({}, knex); + } this.config = getMergedConfig(this.knex.client.config.migrations); this.generator = new MigrationGenerator(this.knex.client.config.migrations); @@ -346,15 +349,25 @@ export default class Migrator { !trx && this._useTransaction(migrationContent, disableTransactions) ) { - return this._transaction(migrationContent, direction, name); + this.knex.enableProcessing(); + return this._transaction( + this.knex, + migrationContent, + direction, + name + ); } - return warnPromise( - this.knex, + + trxOrKnex.enableProcessing(); + return checkPromise( + this.knex.client.logger, migrationContent[direction](trxOrKnex, Promise), name ); }) .then(() => { + trxOrKnex.disableProcessing(); + this.knex.disableProcessing(); log.push(name); if (direction === 'up') { return trxOrKnex.into(getTableName(tableName, schemaName)).insert({ @@ -375,10 +388,10 @@ export default class Migrator { return current.thenReturn([batchNo, log]); } - _transaction(migrationContent, direction, name) { - return this.knex.transaction((trx) => { - return warnPromise( - this.knex, + _transaction(knex, migrationContent, direction, name) { + return knex.transaction((trx) => { + return checkPromise( + knex.client.logger, migrationContent[direction](trx, Promise), name, () => { @@ -447,10 +460,12 @@ function getNewMigrations(migrationSource, all, completed) { }); } -function warnPromise(knex, value, name, fn) { - if (!value || typeof value.then !== 'function') { - knex.client.logger.warn(`migration ${name} did not return a promise`); - if (fn && typeof fn === 'function') fn(); +function checkPromise(logger, migrationPromise, name, commitFn) { + if (!migrationPromise || typeof migrationPromise.then !== 'function') { + logger.warn(`migration ${name} did not return a promise`); + if (commitFn) { + commitFn(); + } } - return value; + return migrationPromise; } diff --git a/src/util/make-knex.js b/src/util/make-knex.js index 86ba9bcd01..9294ea87fa 100644 --- a/src/util/make-knex.js +++ b/src/util/make-knex.js @@ -54,6 +54,26 @@ function initContext(knexFn) { return this.client.ref(ref); }, + disableProcessing() { + if (this.userParams.isProcessingDisabled) { + return; + } + this.userParams.wrapIdentifier = this.client.config.wrapIdentifier; + this.userParams.postProcessResponse = this.client.config.postProcessResponse; + this.client.config.wrapIdentifier = null; + this.client.config.postProcessResponse = null; + this.userParams.isProcessingDisabled = true; + }, + + enableProcessing() { + if (!this.userParams.isProcessingDisabled) { + return; + } + this.client.config.wrapIdentifier = this.userParams.wrapIdentifier; + this.client.config.postProcessResponse = this.userParams.postProcessResponse; + this.userParams.isProcessingDisabled = false; + }, + withUserParams(params) { const knexClone = shallowCloneFunction(knexFn); // We need to include getters in our clone if (this.client) { @@ -103,6 +123,8 @@ function redefineProperties(knex, client) { knex.ref = context.ref; knex.withUserParams = context.withUserParams; knex.queryBuilder = context.queryBuilder; + knex.disableProcessing = context.disableProcessing; + knex.enableProcessing = context.enableProcessing; }, configurable: true, }, diff --git a/test/unit/migrate/Migrator.js b/test/unit/migrate/Migrator.js index b4a8877eba..abaecaa13a 100644 --- a/test/unit/migrate/Migrator.js +++ b/test/unit/migrate/Migrator.js @@ -8,10 +8,11 @@ describe('Migrator', () => { describe('does not use postProcessResponse for internal queries', (done) => { let migrationSource; let knex; - before(() => { + beforeEach(() => { migrationSource = new FsMigrations('test/unit/migrate/migrations/'); knex = Knex({ ...sqliteConfig, + connection: ':memory:', migrationSource, postProcessResponse: () => { throw new Error('Response was processed'); @@ -19,10 +20,8 @@ describe('Migrator', () => { }); }); - after(() => { - return knex.migrate.rollback({ - directory: 'test/unit/migrate/migrations', - }); + afterEach(() => { + knex.destroy(); }); it('latest', (done) => { @@ -48,14 +47,13 @@ describe('Migrator', () => { }); after(() => { - return knex.migrate.rollback({ - directory: 'test/unit/migrate/processed-migrations', - }); + knex.destroy(); }); it('latest', (done) => { knex = Knex({ ...sqliteConfig, + connection: ':memory:', migrationSource, postProcessResponse: () => { done(); diff --git a/test/unit/migrate/processed-migrations/simple_migration.js b/test/unit/migrate/processed-migrations/simple_migration.js index a92144533f..8fefd10f10 100644 --- a/test/unit/migrate/processed-migrations/simple_migration.js +++ b/test/unit/migrate/processed-migrations/simple_migration.js @@ -5,5 +5,5 @@ exports.up = (knex) => { }; exports.down = (knex) => { - return knex.schema.dropTable('rules'); + return knex.schema.dropTable('old_users'); }; From 565c537ff398f44d1e859261e507f697bdb4150d Mon Sep 17 00:00:00 2001 From: Igor Savin Date: Fri, 30 Nov 2018 00:42:14 +0100 Subject: [PATCH 4/5] Make tests slightly more consistent --- test/unit/migrate/Migrator.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/unit/migrate/Migrator.js b/test/unit/migrate/Migrator.js index abaecaa13a..ed83718c69 100644 --- a/test/unit/migrate/Migrator.js +++ b/test/unit/migrate/Migrator.js @@ -40,13 +40,13 @@ describe('Migrator', () => { describe('uses postProcessResponse for migrations', (done) => { let migrationSource; let knex; - before(() => { + beforeEach(() => { migrationSource = new FsMigrations( 'test/unit/migrate/processed-migrations/' ); }); - after(() => { + afterEach(() => { knex.destroy(); }); From 05d306bfcd3ddc070d8d9642602f35be7f119266 Mon Sep 17 00:00:00 2001 From: Igor Savin Date: Mon, 3 Dec 2018 23:24:48 +0100 Subject: [PATCH 5/5] Improvements after code review --- package.json | 8 ++++---- src/util/make-knex.js | 4 ++++ test/unit/migrate/Migrator.js | 12 ++++++++---- 3 files changed, 16 insertions(+), 8 deletions(-) diff --git a/package.json b/package.json index 4e0771972a..5ecc1dc0fe 100644 --- a/package.json +++ b/package.json @@ -32,9 +32,9 @@ ] }, "devDependencies": { - "@babel/cli": "^7.1.5", - "@babel/core": "^7.1.6", - "@babel/preset-env": "^7.1.6", + "@babel/cli": "^7.2.0", + "@babel/core": "^7.2.0", + "@babel/preset-env": "^7.2.0", "@types/node": "*", "JSONStream": "^1.3.5", "async": "^2.6.1", @@ -59,7 +59,7 @@ "nyc": "^13.1.0", "pg": "^7.7.1", "pg-query-stream": "^1.1.2", - "prettier": "^1.15.2", + "prettier": "^1.15.3", "rimraf": "^2.6.2", "sinon": "^7.1.1", "sinon-chai": "^3.3.0", diff --git a/src/util/make-knex.js b/src/util/make-knex.js index 9294ea87fa..8ef3cc426e 100644 --- a/src/util/make-knex.js +++ b/src/util/make-knex.js @@ -54,6 +54,8 @@ function initContext(knexFn) { return this.client.ref(ref); }, + // Do not document this as public API until naming and API is improved for general consumption + // This method exists to disable processing of internal queries in migrations disableProcessing() { if (this.userParams.isProcessingDisabled) { return; @@ -65,6 +67,8 @@ function initContext(knexFn) { this.userParams.isProcessingDisabled = true; }, + // Do not document this as public API until naming and API is improved for general consumption + // This method exists to enable execution of non-internal queries with consistent identifier naming in migrations enableProcessing() { if (!this.userParams.isProcessingDisabled) { return; diff --git a/test/unit/migrate/Migrator.js b/test/unit/migrate/Migrator.js index ed83718c69..bf942e3a9d 100644 --- a/test/unit/migrate/Migrator.js +++ b/test/unit/migrate/Migrator.js @@ -51,20 +51,24 @@ describe('Migrator', () => { }); it('latest', (done) => { + let wasPostProcessed = false; knex = Knex({ ...sqliteConfig, connection: ':memory:', migrationSource, postProcessResponse: () => { - done(); + wasPostProcessed = true; }, }); - expect(() => { - knex.migrate.latest({ + knex.migrate + .latest({ directory: 'test/unit/migrate/processed-migrations', + }) + .then(() => { + expect(wasPostProcessed).to.equal(true); + done(); }); - }).not.to.throw(); }); }); });