From 28f7ee36865871fd6ce4a85ccc3a482be9cc4769 Mon Sep 17 00:00:00 2001 From: Dmitry Maganov Date: Mon, 10 Jun 2019 04:57:20 +0300 Subject: [PATCH] Hooks: proper nesting with savepoints, customisation, fix tests --- lib/hooks.js | 98 +++++++++++++---------- test/index.test.js | 109 ++++++++++++++++++++------ test/service-method-overrides.test.js | 2 +- 3 files changed, 146 insertions(+), 63 deletions(-) diff --git a/lib/hooks.js b/lib/hooks.js index a4e47ae..c17c1c5 100644 --- a/lib/hooks.js +++ b/lib/hooks.js @@ -1,67 +1,85 @@ const debug = require('debug')('feathers-knex-transaction'); -const RollbackReason = function (error) { - this.error = error; +const ROLLBACK = { rollback: true }; + +const getKnex = (hook) => { + const knex = hook.service.Model; + + return knex && typeof knex.transaction === 'function' ? knex : undefined; }; -const start = () => { +const start = (options = {}) => { + options = Object.assign({ getKnex }, options); + return hook => { - if (!hook.service.Model || typeof hook.service.Model.transaction !== 'function') { - return Promise.resolve(hook); - } + const { transaction } = hook.params; + const knex = transaction ? transaction.trx : options.getKnex(hook); - if (hook.params.transaction) { - hook.params.transaction.count += 1; - return Promise.resolve(hook); + if (!knex) { + return; } return new Promise((resolve, reject) => { - const id = Date.now(); - - debug('started a new transaction %s', id); - hook.service.Model - .transaction(trx => resolve({ - id, - trx, - count: 0 - })) - .catch(err => reject(err)); - }).then(trx => { - hook.params.transaction = trx; - return hook; + const transaction = {}; + + transaction.starting = true; + transaction.promise = knex.transaction(trx => { + transaction.trx = trx; + transaction.id = Date.now(); + + hook.params.transaction = transaction; + + debug('started a new transaction %s', transaction.id); + + resolve(); + }).catch((error) => { + if (transaction.starting) { + reject(error); + } else if (error !== ROLLBACK) { + throw error; + } + }); }); }; }; const end = () => { return hook => { - if (hook.params.transaction) { - const { trx, id, count } = hook.params.transaction; + const { transaction } = hook.params; - if (count > 0) { - hook.params.transaction.count -= 1; - return Promise.resolve(hook); - } + if (!transaction) { + return; + } - hook.params.transaction = undefined; + const { trx, id, promise } = transaction; - return trx.commit() - .then(() => debug('finished transaction %s with success', id)) - .then(() => hook); - } - return hook; + hook.params.transaction = undefined; + transaction.starting = false; + + return trx.commit() + .then(() => promise) + .then(() => debug('ended transaction %s', id)) + .then(() => hook); }; }; const rollback = () => { return hook => { - if (hook.params.transaction) { - const { trx, id } = hook.params.transaction; - return trx.rollback(new RollbackReason(hook.error)) - .then(() => debug('rolling back transaction %s', id)) - .then(() => hook); + const { transaction } = hook.params; + + if (!transaction) { + return; } - return hook; + + const { trx, id, promise } = transaction; + + hook.params.transaction = undefined; + transaction.starting = false; + + return trx.rollback(ROLLBACK) + .then(() => promise) + .then(() => debug('rolled back transaction %s', id)) + .then(() => hook); }; }; diff --git a/test/index.test.js b/test/index.test.js index d3b239c..bbd88bb 100644 --- a/test/index.test.js +++ b/test/index.test.js @@ -387,39 +387,104 @@ describe('Feathers Knex Service', () => { }); describe('hooks', () => { - const people2 = service({ - Model: db, - name: 'people2', - events: [ 'testing' ] + afterEach(async () => { + await db('people').truncate(); }); - const app2 = feathers() - .hooks({ + it('does reject on problem with commit', async () => { + const app = feathers(); + + app.hooks({ before: transaction.start(), after: [ (context) => { - let client = context.params.transaction.trx.client; - let query = client.query; + const client = context.params.transaction.trx.client; + const query = client.query; + client.query = (conn, sql) => { - if (sql === 'COMMIT;') sql = 'COMMITA;'; - return query.call(client, conn, sql); + let result = query.call(client, conn, sql); + + if (sql === 'COMMIT;') { + result = result.then(() => { + throw new TypeError('Deliberate'); + }); + } + + return result; }; }, - transaction.end() + transaction.end(), ], - error: transaction.rollback() - }) - .use('/people', people2); + error: transaction.rollback(), + }); - it('does fail on unsuccessful commit', async () => { - const message = 'Should never get here'; + app.use('/people', people); - try { - await app2.service('/people').create({ name: 'Foo' }); - throw new Error(message); - } catch (error) { - expect(error.message !== message); - } + await expect(app.service('/people').create({ name: 'Foo' })) + .to.eventually.be.rejectedWith(TypeError, 'Deliberate'); + }); + + it('does commit, rollback, nesting', async () => { + const app = feathers(); + + app.hooks({ + before: transaction.start({ getKnex: () => db }), + after: transaction.end(), + error: transaction.rollback(), + }); + + app.use('/people', people); + + app.use('/test', { create: async (data, params) => { + await app.service('/people').create({ name: 'Foo' }, { ...params }); + + if (data.throw) { + throw new TypeError('Deliberate'); + } + } } ); + + await expect(app.service('/test').create({ throw: true })) + .to.eventually.be.rejectedWith(TypeError, 'Deliberate'); + + expect(await app.service('/people').find()).to.have.length(0); + + await expect(app.service('/test').create({})) + .to.eventually.be.fulfilled; + + expect(await app.service('/people').find()).to.have.length(1); + }); + + it('does use savepoints for nested calls', async () => { + const app = feathers(); + + app.hooks({ + before: transaction.start({ getKnex: () => db }), + after: transaction.end(), + error: transaction.rollback(), + }); + + app.use('/people', people); + + app.use('/success', { create: async (data, params) => { + await app.service('/people').create({ name: 'Success' }, { ...params }); + }}); + + app.use('/fail', { create: async (data, params) => { + await app.service('/people').create({ name: 'Fail' }, { ...params }); + throw new TypeError('Deliberate'); + }}); + + app.use('/test', { create: async (data, params) => { + await app.service('/success').create({}, { ...params }); + await app.service('/fail').create({}, { ...params }).catch(() => {}); + }}); + + await app.service('/test').create({}); + + const created = await await app.service('/people').find(); + + expect(created).to.have.length(1); + expect(created[0]).to.have.property('name', 'Success'); }); }); diff --git a/test/service-method-overrides.test.js b/test/service-method-overrides.test.js index 21489a7..1b3a781 100644 --- a/test/service-method-overrides.test.js +++ b/test/service-method-overrides.test.js @@ -67,7 +67,7 @@ function attachSchema () { return db.schema.raw(`attach database '${schemaName}.sqlite' as ${schemaName}`); } -describe.only('Feathers Knex Overridden Method With Self-Join', () => { +describe('Feathers Knex Overridden Method With Self-Join', () => { let ancestor; let animal;