Added hooks to support mongo-db transaction

[jaiyashree]

Summary

To directly add api-request level hooks and service level hooks to implement mongo-db-transaction

• Is this PR dependent on PRs in other repos? No

Other Information

The create and remove methods of service.js were modified to pass the params.mongoose to the mongoose dependency to carry the database session and to commit or rollback at the end of api-request or the service call.
Note: This is left optional to ensure the codes already written without this param still works fine.

[daffl]

This looks really interesting, thank you! For new features like that we do need some tests though. A good start would be to show how the example would work and then go from there.

[DaddyWarbucks]

I think that the skipPath argument should be optional. Right now, if you do not pass skipPath the function will error. And if you don't want to skip any paths you must pass an empty array. That is not ideal IMHO. This seems like it was written to be used in an app.hooks scenario instead of a service.hooks scenario. Feathers already has a mechanism for determining which services/paths a hook should be run, which is specifying the hook per service. Unless I am missing something?
I would suggest making skipPath optional. Something like

if (!skipPath || skipPath.indexOf(context.path) === -1) {

This would allow the hook to be used on a per service basis, instead of specifying at app.hooks and skipping paths, but also still allow it to be used that way.
That is the only "critical" issue I see here. If you guys don't mind, I would be happy to amend a few other things that I think could be improved.

Great work!

[jaiyashree]

I think that the skipPath argument should be optional. Right now, if you do not pass skipPath the function will error. And if you don't want to skip any paths you must pass an empty array. That is not ideal IMHO. This seems like it was written to be used in an app.hooks scenario instead of a service.hooks scenario. Feathers already has a mechanism for determining which services/paths a hook should be run, which is specifying the hook per service. Unless I am missing something?
I would suggest making skipPath optional. Something like

if (!skipPath || skipPath.indexOf(context.path) === -1) {

This would allow the hook to be used on a per service basis, instead of specifying at app.hooks and skipping paths, but also still allow it to be used that way.
That is the only "critical" issue I see here. If you guys don't mind, I would be happy to amend a few other things that I think could be improved.

Great work!

Thanks for reviewing. I've modified the skipPath param to hold an empty array if not assigned a value from calling statement. And the app.hooks.js is just a sample and this can be used in any service hooks too. I'm proceeding with the required changes and will amend it asap.

[jaiyashree]

This looks really interesting, thank you! For new features like that we do need some tests though. A good start would be to show how the example would work and then go from there.

Hi. I've added test cases as working-sample and updated README.md for prerequisite. Please review for additional changes to be added.

[jaiyashree]

Hi @daffl,
To enable replica set, please follow this link (https://linode.com/docs/databases/mongodb/create-a-mongodb-replica-set/#start-replication-and-add-members).

[daffl]

This is really tricky in Travis CI. Looks like the best shot is the run-rs package. If it works this would also allow local testing without needing a MongoDB server running.

[jaiyashree]

This is really tricky in Travis CI. Looks like the best shot is the run-rs package. If it works this would also allow local testing without needing a MongoDB server running.

Thanks daffl 👍 I will also try to figure a way out .

[jaiyashree]

Hi @daffl ,
Thanks for giving idea on how to use run-rs in background using '&', it worked 👍 . The problem was that the test cases started to run even before the run-rs is connected. A time wait was enough to run it.
Also, I've updated the node version support to v7.6.0 from v6. This is because async-await is supported from >=v7.6.0.
Note: The node version support for feathers-generator-plus is >= v8 already.

Awaiting your suggestion/review.

[daffl]

Dropping supported Node versions would be a breaking change which planned for the next major release. We'd either have to wait merging this until then or have a promise based version for now.

[jaiyashree]

Hi @daffl
I'll change them to the node 6 standard then by using promise.

[daffl]

Sounds good, thanks!

[daffl]

Mind you, I am planning to make a major release with some other updates in the next couple of weeks that would drop Node 6 and use async/await.

[daffl]

Ok, I just merged the updates I mentioned. If you merge your changes with that version we can get it released 😄

[jaiyashree]

Sure @daffl 👍

[jaiyashree]

@daffl awaiting your review 👍

[daffl]

Even when you are in a session you could still create array data right? Why not just pass createOptions into the model.create(data) that we already have in https://github.com/feathersjs-ecosystem/feathers-mongoose/pull/276/files#diff-6ce8873d3951d1d3483af360fa1f4a93R155

[jaiyashree]

Hi @daffl ... I've passed it as a option to create model...
Note: When a user try creating a single record 'service.create(obj, mongooseOptions)' we need to internally convert it into 'service.create([obj], mongooseOptions)' and them return result[0] because the 'mongoose' dependency does not accept options for a single object creation as it supports a spread of object.

[daffl]

Sweet, made some minor tweaks and released as v7.1.0, thank you!