daffl · Jun 10, 2019 · Jun 10, 2019
diff --git 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
@@ -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
@@ -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;