Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply postprocessing in migrations #2934

Merged
merged 6 commits into from
Dec 3, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,12 @@
"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",
"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",
Expand Down
5 changes: 1 addition & 4 deletions src/dialects/oracledb/query/compiler.js
Original file line number Diff line number Diff line change
Expand Up @@ -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') {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prettier picked this one up for some reason

return '';
}

Expand Down
49 changes: 32 additions & 17 deletions src/migrate/Migrator.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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({
Expand All @@ -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,
() => {
Expand Down Expand Up @@ -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;
}
22 changes: 22 additions & 0 deletions src/util/make-knex.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,26 @@ function initContext(knexFn) {
return this.client.ref(ref);
},

disableProcessing() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to add some comment that these should not be added documentation to be a part to knex public API before we have designed better API names and parameters for this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And also to mention why these exist (that it was needed for migrations to be able to run those internal queries, with consistent identifier naming).

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) {
Expand Down Expand Up @@ -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,
},
Expand Down
38 changes: 37 additions & 1 deletion test/unit/migrate/Migrator.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,22 @@ 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');
},
});
});

afterEach(() => {
knex.destroy();
});

it('latest', (done) => {
expect(() => {
knex.migrate
Expand All @@ -31,4 +36,35 @@ 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(() => {
knex.destroy();
});

it('latest', (done) => {
knex = Knex({
...sqliteConfig,
connection: ':memory:',
migrationSource,
postProcessResponse: () => {
done();
Copy link
Member

@elhigu elhigu Dec 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test actually calls done, before execution of migrations has ended. Because that is separate inmemory database, it probably won't cause any problems. Except test might have been ended already before the expectation below has finished execution.

},
});

expect(() => {
knex.migrate.latest({
directory: 'test/unit/migrate/processed-migrations',
});
}).not.to.throw();
});
});
});
9 changes: 9 additions & 0 deletions test/unit/migrate/processed-migrations/simple_migration.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
exports.up = (knex) => {
return knex.schema.createTable('old_users', (table) => {
table.string('name');
});
};

exports.down = (knex) => {
return knex.schema.dropTable('old_users');
};