#10 - transaction support via knex

[eldridge]

Hi! I've added support for passing the knex transaction object through to knex as jmdobry described in this comment on issue #10. It appears to work pretty well.

I added co to the development dependencies because I couldn't figure out how to reconcile co-mocha's use of generators and knex's use of the transaction callback. I'm new to modern JavaScript -- particularly node.js and ES6 -- and I don't quite feel as though I know when best to use promises versus generators versus try/catch, so there may be a better way to implement my tests.

Thank you for js-data and everyone's assistance on Gitter. Fantastic community.

[eldridge]

Sigh. I should have checked to be sure I was in sync with master before I submitted the PR.

[jmdobry]

@techniq Can probably speak better than me on whether the co stuff is necessary for the transaction callback.

[techniq]

@mechanicalpulse this looks great.

I've only recently began using generators/yielded Promises so I'm not expert (and use them as a stop gap until async/await lands), but I saw an open issue on with knex on using transactions with co and yield. I'm not sure if you came across this already as your tests are using the recommended approach by tgriesser (var result = yield knex.transaction(co.wrap(function *(trx){ /* use trx */}))

Some feedback on your tests

• I had hoped coMocha exported an instance of co as well (to get to co.wrap) but it doesn't appear to, so I'm fine with adding co as a devDependency.
• You have var co = require('co'); inside of each tests. Instead, just add it to the top of each file, or better yet, add global.co = require('co') to mocha.start.js so it's available to all tests
• Regarding when to use generators/yield vs then vs try/catch, my rule of thumb is:
  • Never use .then()/.catch() on a promise but instead always yield the promise within a generator function (as long as your using something like co/co-mocha/koa/etc). By yield promises, you don't create closures as you do with then functions so you don't have to expose variables outside of the closures.
  • Only use try/catch (in the tests) if you require an exception to be thrown, as if you wrap things unnecessarily, a failed assert will be caught by the try and the code within catch will run. For example, in the DSSqlAdapter#create + transaction test, you have...

describe('DSSqlAdapter#create + transaction', function () {
  it('commit should persist created user in a sql db', function* () {
    var id;
    var co = require('co');

    yield adapter.query.transaction(co.wrap(function * (trx) {
      var createUser = yield adapter.create(User, {name: 'Jane'}, {transaction: trx});
      id = createUser.id;
      assert.equal(createUser.name, 'Jane');
      assert.isDefined(createUser.id);

      var findUser = yield adapter.find(User, createUser.id, {transaction: trx});
      assert.equal(findUser.name, 'Jane');
      assert.isDefined(findUser.id);
      assert.equalObjects(findUser, {id: id, name: 'Jane', age: null, profileId: null});

      return findUser;
    })).then(
      function (user) {
        assert.isObject(user, 'transaction returned user object');
        assert.equal(user.name, 'Jane');
        assert.isDefined(user.id);
        assert.equalObjects(user, {id: id, name: 'Jane', age: null, profileId: null});
      },
      function (err) {
        throw new Error('transaction threw exception!');
      }
    );

    try {
      var findUser2 = yield adapter.find(User, id);
      assert.isObject(findUser2, 'user committed to database');
    } catch(err) {
      throw new Error('transaction failed to commit!');
    }
  });

... if assert.equal(createUser.name, 'Jane'); failed (say name was truncated to Jan by the database column), the test would fail, but return transaction threw an exception instead of AssertionError: expected 'Jan' to equal 'Jan'e which makes tracking down the actual problem more difficult. Instead, I would structure the test as such (I removed a lot of duplicate asserts):

  it('commit should persist created user in a sql db', function* () {
    var id;

    var findUser = yield adapter.query.transaction(co.wrap(function * (trx) {
      var createUser = yield adapter.create(User, {name: 'Jane'}, {transaction: trx});
      id = createUser.id;
      assert.equal(createUser.name, 'Jane');
      assert.isDefined(createUser.id);

      return adapter.find(User, createUser.id, {transaction: trx});
    }));

    assert.isObject(findUser, 'user committed to database');
    assert.equal(findUser.name, 'Jane');
    assert.isDefined(findUser.id);
    assert.equalObjects(findUser, {id: id, name: 'Jane', age: null, profileId: null});
  });

I believe I'm still testing everything that needs to be tested in this case (let me know if I'm wrong) but keeps things nice and concise.

If you could make those changes to the tests, I think it looks good and I'll merge and cut a release.

[eldridge]

Regarding the assertions being swallowed up in the thenable -- I appended the fulfillment and rejection handlers in a single then() call instead of using a then().catch(), which should allow the assertions to fail as expected. Unless I'm misunderstanding.

I will move the assertions outside, though, and avoid using then/catch. It does certainly make the tests more straightforward.

It may also be best for the adapter.find() call to be done outside of the transaction callback, since it could potentially fetch stale data:

         1531 Query BEGIN
         1531 Query insert into `user` (`name`) values ('Jane')
         1531 Query select * from `user` where `id` = '556'
         1531 Query select * from `user` where `id` = '556'
         1531 Query COMMIT

versus

         1535 Query BEGIN
         1535 Query insert into `user` (`name`) values ('Jane')
         1535 Query select * from `user` where `id` = '558'
         1535 Query COMMIT
         1535 Query select * from `user` where `id` = '558'

[techniq]

If I changed say line 13 in create_trx.spec.js to assert.equal(findUser.name, 'Jan');, I get:

  1) DSSqlAdapter#create + transaction commit should persist created user in a sql db:
     Error: transaction threw exception!
      at test/create_trx.spec.js:26:15

I'm not an expert, but I'm guessing the failed assert is causing an exception within the generator function that is hitting the "catch" handler.

---

I think it makes since to move the find outside of the transaction. I wasn't sure if it was inside the transaction to test something but agree it should be moved out if not.

[techniq]

Just saw your updates. Everything looks good to me. Thanks!

I'll push a release out. We should update the docs with these changes if you get a chance.

[techniq]

@mechanicalpulse released 0.11.4

[eldridge]

Ah, sorry -- I was looking at the assert.equal(user.name, 'Jane') inside the fulfillment handler, not the assert.equal(createUser.name, 'Jane') in the transaction. I do have to throw an exception inside the transaction for the rollback to occur (I tried just calling trx.rollback() instead of throwing an exception, but the transaction implicitly calls trx.commit() when the function returns, which results in an exception of its own since the transaction is no longer valid), so I think it makes sense to keep the number of nonessential operations inside of the transaction to a minimum in order to decrease the likelihood of an assertion or other unexpected exception being swallowed up and causing confusion.

Thanks for the feedback and quick turnaround. Looking forward to using it!

[jmdobry]

Thanks @mechanicalpulse ! Just a note, future pull requests should be squashed into one commit. Thanks!

[eldridge]

Will do.