Skip to content
This repository has been archived by the owner on Mar 15, 2023. It is now read-only.

Add support for passing options (or mongodb) to all methods. #189

Conversation

rudolph9
Copy link
Contributor

@rudolph9 rudolph9 commented Nov 13, 2020

Previously only methods that allowed the multi option supported
additional passed option. That change adds support for all other
methods, ultimately calling e.g. find, findAndModify

This indirectly address #127 as a MongoDB session can be via options (or mongodb). Allowing the user to do something like the following:

import { ObjectID } from 'mongodb'

export default async app => {
  app.use('/fooBarService', {
    async create(data) {
      // assumes you have access to the mongoClient via your app state
      let session = app.mongoClient.startSession()
      try {
        await session.withTransaction(async () => {
            let fooID = new ObjectID()
            let barID = new ObjectID()
            app.service('fooService').create(
              { 
                ...data,
                _id: fooID,
                bar: barID,
              },
              { mongodb: { session } },
            )
            app.service('barService').create(
              {
                ...data,
                _id: barID
                foo: fooID
              },
              { mongodb: { session } },
            )
        })
      } finally {
        await session.endSession()
      }
    }
  })
}

Previously only methods that allowed the `multi` option supported
additional passed option. That change adds support for all other
methods, ultimately calling e.g. `find`, `findAndModify`
@rudolph9
Copy link
Contributor Author

Please note, the linter flags a line of code unchanged by this pull request:

diff --git a/lib/index.js b/lib/index.js
index 1d2b990..8f7e917 100644
--- a/lib/index.js
+++ b/lib/index.js
@@ -226,7 +226,7 @@ class Service extends AdapterService {
     const remapModifier = this._remapModifiers(this._normalizeId(id, data));
 
     const idParams = Object.assign({}, params, {
-      paginate: false,
+      paginate: false
     });
 
     const ids = this._findOrGet(id, idParams)

@daedalus28
Copy link

This looks great and would be a big help when doing transactions. What are the next steps to get this merged @daffl?

@rudolph9 rudolph9 changed the title Add support for passing optoins (or mongodb) to all methods. Add support for passing options (or mongodb) to all methods. Nov 18, 2020
@arlukin
Copy link
Contributor

arlukin commented Nov 21, 2020

Great, I was about to push a pull request with similar code.

One thing that I did differently was to not pass options to estimatedDocumentCount and countDocuments. They don't have any session parameter. But all monodb operations have slightly different options. Have you tried to see what happens if for example countDocuments will get options.session? Will it just nicely ignore it?

Some links to documentation.
http://mongodb.github.io/node-mongodb-native/3.6/api/Collection.html#estimatedDocumentCount
http://mongodb.github.io/node-mongodb-native/3.6/api/Collection.html#countDocuments

@DaddyWarbucks
Copy link
Contributor

This looks cool! I am not a maintainer, but I do contribute often to the DB adapters. I would suggest modifying your code such that it does not change the filterQuery method. That method is on almost all of the adapters and has the same function signature and return values in all of them. I would move the code to pull the options off params into its own method. I also suggest removing the params.options and just using params.mongodb

@daffl
Copy link
Member

daffl commented Nov 21, 2020

Yes, this makes sense, thank you for the for the pull request @rudolph9. This features needs to be added to the documentation in the Readme, other than that it looks good to me.

@rudolph9
Copy link
Contributor Author

Great, I was about to push a pull request with similar code.

One thing that I did differently was to not pass options to estimatedDocumentCount and countDocuments. They don't have any session parameter. But all monodb operations have slightly different options. Have you tried to see what happens if for example countDocuments will get options.session? Will it just nicely ignore it?

Some links to documentation.
http://mongodb.github.io/node-mongodb-native/3.6/api/Collection.html#estimatedDocumentCount
http://mongodb.github.io/node-mongodb-native/3.6/api/Collection.html#countDocuments

Good thing to point out. It appears to ignore the session option on both.

Documents how to utilized transaction and provides
an example.

js syntax, and fixed example code
@rudolph9
Copy link
Contributor Author

@daffl Added documentation to the README.md. Please let me know if there are any other changes you would like to see.

@daffl
Copy link
Member

daffl commented Nov 24, 2020

Looks great. The only other question I had was if there is a reason to have two ways for passing it (via params.options and params.mongodb). Ideally there should only be one way to use a feature (unless for backwards compatibility reasons) and I'd find params.mongodb more clear.

@rudolph9
Copy link
Contributor Author

rudolph9 commented Nov 24, 2020

@daffl

Looks great. The only other question I had was if there is a reason to have two ways for passing it (via params.options and params.mongodb). Ideally there should only be one way to use a feature (unless for backwards compatibility reasons) and I'd find params.mongodb more clear.

I agree, params.mongodb is more clear. The reason I did also did options was because the existing _multiOptions helper method was already doing so. The _options helper I wrote is mirror that.

https://github.com/feathersjs-ecosystem/feathers-mongodb/pull/189/files#diff-92bbac9a308cd5fcf9db165841f2d90ce981baddcb2b1e26cfff170929af3bd1R42

I think we want _options and _multiOptions to be consistent and also not make any breaking changes but obviously this isn't ideal because we're supporting both options and mongodb fields.

In any case, I defer to you to decide how to move forward?

@daffl daffl merged commit 6296078 into feathersjs-ecosystem:master Nov 24, 2020
@daffl
Copy link
Member

daffl commented Nov 24, 2020

I think that makes sense. Released as v6.3.0, thank you again!

@rudolph9 rudolph9 deleted the feature/add-support-for-sessions branch November 25, 2020 15:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants