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

Executing services methods using transaction, implementation of issue #91 #113

Merged

Conversation

pnakibar
Copy link
Contributor

My solution to the issue #91 .
Pretty much a naive implementation due to not knowing the internals of knex.

Easy to use, simply import the newly created hooks and add them to the hooks of the service you want to use it (usually in global hooks) and then the feathers-knex deal with using the transaction.

Usage:

// Application hooks that run for every service
import { transactionHooks as transaction } from 'feathers-knex';

const logger = require('./hooks/logger');

module.exports = {
  before: {
    all: [transaction.start({ dbServiceName: 'knexClient' })],
    find: [],
    get: [],
    create: [],
    update: [],
    patch: [],
    remove: []
  },

  after: {
    all: [logger(), transaction.end()],
    find: [],
    get: [],
    create: [],
    update: [],
    patch: [],
    remove: []
  },

  error: {
    all: [transaction.rollback(), logger()],
    find: [],
    get: [],
    create: [],
    update: [],
    patch: [],
    remove: []
  }
};

Looking forward to some feedback on the code!

@pnakibar pnakibar changed the title Using transactions, implementation of issue #91 Executing services methods using transaction, implementation of issue #91 Aug 23, 2017
@RobIsHere
Copy link

Great! I'm looking on your solution because I need the same functionality. Put a nested json to a rest service and store it in diverse tables using a all-or-nothing transaction.

One small quesition: Is it possible to use hook.service instead of hook.app.get(dbServiceName) in transaction start?
(Docs here suggest it could work: https://docs.feathersjs.com/api/hooks.html#hook-objects)

This would remove the need for the service name parameter - which is likely to be forgotten, when you rename a service during refactoring later on.

Copy link
Member

@daffl daffl left a comment

Choose a reason for hiding this comment

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

This is great, thank you for doing this! I made a couple of comments/questions.

Ideally, we'd also want to test this functionality somehow (e.g. having two services, one called in a hook with the transaction, throwing an error and making sure both are rolled back). It looks like SQLite supports transactions so you should be able to just run tests via npm test.

src/index.js Outdated

const debug = require('debug')('feathers-knex');
const debugTransaction = require('debug')('feathers-knex-transaction');
Copy link
Member

Choose a reason for hiding this comment

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

We usually only use one debug per module.

src/index.js Outdated
db (params = {}) {
if (params.transaction) {
const { trx, id } = params.transaction;
debugTransaction('ran %s with transaction %s', this.table, id);
Copy link
Member

Choose a reason for hiding this comment

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

I'd change this to just debug

src/index.js Outdated
@@ -169,6 +176,7 @@ class Service {
}

find (params) {
debug('method find of service %s called', this.table);
Copy link
Member

Choose a reason for hiding this comment

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

Should be removed for consistency (we might want to add more debug statements to all database adapters but that would be a different issue).

@@ -0,0 +1,42 @@
const debug = require('debug')('feathers-knex-transaction');
Copy link
Member

@daffl daffl Aug 23, 2017

Choose a reason for hiding this comment

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

We may want to rename this file to just hooks and call the methods transactionStart, transactionEnd and transactionRollback and export it as init.hooks instead of https://github.com/feathersjs/feathers-knex/pull/113/files#diff-1fdf421c05c1140f6d71444ea2b27638R339

Copy link
Member

@daffl daffl Aug 23, 2017

Choose a reason for hiding this comment

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

Alternatively we could also export it as export default { transaction: { start, end, rollback } } and use it as hooks.transaction.start etc.

debug('started transaction system with %s service', dbServiceName);
return (hook) =>
new Promise(resolve =>
hook.app.get(dbServiceName).transaction(trx => {
Copy link
Member

Choose a reason for hiding this comment

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

Can you try if hook.service.Model works here? I think it should (because it is the same thing really).

@pnakibar
Copy link
Contributor Author

@daffl should I remove the debug from the transaction-hooks.js file (now called hooks.js)?

@daffl
Copy link
Member

daffl commented Aug 24, 2017

@pnakibar Sorry, I meant one debug per file (in the hooks case probably called debug('feathers-knex/hooks')).

@pnakibar
Copy link
Contributor Author

It's alright!
The module are a bit redundant in the javascript IMO anyways ;)

Just submitted a new PR with the requested changes.

@daffl
Copy link
Member

daffl commented Aug 24, 2017

Awesome, looks great! Any chance to add some tests or would you like me to try and add those?

@pnakibar
Copy link
Contributor Author

The new app.hooks.js:

// Application hooks that run for every service
import * as feathersKnex from 'feathers-knex';

const { hooks: { transaction } } = feathersKnex;

const logger = require('./hooks/logger');

module.exports = {
  before: {
    all: [transaction.start({ dbServiceName: 'knexClient' })],
    find: [],
    get: [],
    create: [],
    update: [],
    patch: [],
    remove: []
  },

  after: {
    all: [logger(), transaction.end()],
    find: [],
    get: [],
    create: [],
    update: [],
    patch: [],
    remove: []
  },

  error: {
    all: [transaction.rollback(), logger()],
    find: [],
    get: [],
    create: [],
    update: [],
    patch: [],
    remove: []
  }
};

@pnakibar
Copy link
Contributor Author

pnakibar commented Aug 24, 2017

@daffl if you could, it would be awesome!
I struggled a bit to understand the test suit...

@daffl
Copy link
Member

daffl commented Aug 24, 2017

Also, we have to update https://docs.feathersjs.com/api/databases/knexjs.html with a section for transaction support once this is released.

@pnakibar
Copy link
Contributor Author

Also, the options object to the start hook could be better. I will try to improve it once I write the docs!

@daffl
Copy link
Member

daffl commented Aug 24, 2017

I don't think the option is necessary. Did you try hook.service.Model instead of hook.app.get(dbServiceName) in https://github.com/feathersjs/feathers-knex/pull/113/files#diff-95b96dc34a5bf5b035b35940dd2d2d60R8?

@pnakibar
Copy link
Contributor Author

Will try it, my concern was with using two database adapters.

@pnakibar
Copy link
Contributor Author

@daffl tried it. Was a bit unsuccessful, since not all my services have the service.Model, and some of them interact with a database.

@RobIsHere
Copy link

Thank you all for your great work! This is going on so fast, it's amazing!

Maybe you could allow the developer to pass his own Model. Or use the default, if he doesn't.
Would something like this work?


const start = (options) => {
  const Model = options.Model || hook.service.Model; // changed here

  debug('started transaction system'); // changed here
  return (hook) =>
    new Promise(resolve =>
      Model.transaction(trx => {  // changed here
        const id = Date.now();
        hook.params.transaction = {
          trx,
          id
        };
        debug('started a new transaction %s', id);
        return resolve(hook);
      })
    );
};

The parameter name "Model" is already used for passing the db instance at knex db service init. Also if you just need some db instance, exposing only this instead of a whole service could decouple things.

Just thinking about this...

@daffl
Copy link
Member

daffl commented Aug 24, 2017

The Model for Knex is basically just the main Knex client connection instance (see here).

I can't think of a case where this would change throughout a transaction (and I don't think transactions across different databases are possible at the Knex level).

@pnakibar
Copy link
Contributor Author

@daffl is it still ok to merge the pull request?

@daffl daffl merged commit c8b4481 into feathersjs-ecosystem:master Sep 2, 2017
@daffl
Copy link
Member

daffl commented Sep 2, 2017

Sorry, for some reason I dropped the ball on this. Merged. Will add some tests and let you know when it's released.

@daffl
Copy link
Member

daffl commented Sep 3, 2017

Released as v2.8.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants