feathersjs-ecosystem / feathers-mongoose

Easily create a Mongoose Service for Feathersjs.
MIT License
189 stars 96 forks source link

Support passing mongoose specific params to queries. #70

Closed ekryski closed 8 years ago

ekryski commented 8 years ago

Similar to this PR https://github.com/feathersjs/feathers-sequelize/pull/27

beeplin commented 8 years ago

I tested and found basically all mongo query/update operators like $push, $and, $elemMatch... are already available. Amazingly the only operator that's not working is $eq, the most simple one.

daffl commented 8 years ago

@beeplin $eq is supported through our querying mechanism by just passing the object. Most other MongoDB query operators should work (but only with Mongoose and MongoDB) but if they are not part of our querying spec they will probably not work with other database adapters.

This issue will address passing options (like multi: true) to a Mongoose query.

beeplin commented 8 years ago

Thanks for the explanation. I understand that mongo's finding options -- sort, limit, skip and fields selection are already supported in all adapters. For mongo's other two writing options, upsert and multi, since feathers' update\patch\remove only allows id as the first parameter, I don't think multi is necessary, but it would be nice to have an $upsert option.

marshallswain commented 8 years ago

+1 for $upsert support. I was looking for a way to use it last month. While the new Wired Tiger engine in MongoDB does make for really amazing, ridiculous performance charts, if I can save a single query to the database, it will potentially have a big pay off... once the app reaches 40 thousand requests per second. 😃

daffl commented 8 years ago

@beeplin Feathers database adapters do support updating multiple resources if you set the id to null (see http://docs.feathersjs.com/services/readme.html#service-methods), e.g. app.service('todos').patch(null, { complete: true }, { query: { complete: false } }) will set all incomplete todos to be completed. That's why it sets the multi option internally here.

beeplin commented 8 years ago

oops missed that. Smart design! Thanks~

daffl commented 8 years ago

Also closed via #87

beeplin commented 7 years ago

Hi @marshallswain I know it has been long ago, but did you figure out how to use upsert in feathers-mongodb or feathers-mongoose? Recently I am trying to reproduce the behavior of upsert in feathers-mongodb by following @daffl 's suggestion in https://github.com/feathersjs/feathers-mongoose/issues/121, but I found there are some edge cases that can hardly be handled in this way. Maybe it's time to design a special upsert key in params?

eddyystop commented 7 years ago

Silly thought for upsert. Is there any value to do an optimistic update first, then a create should the update fail? What would the performance difference be compared to get then update or create?

beeplin commented 7 years ago

I am using the update-then-create-if-failed method. It can save a get in most times, and what's more important is that, theoretically, the get-then-update-or-create method may possibly fail in some edge cases. Here is an example:

  1. get id='aaa', and fails, which means the id 'aaa' does not exist
  2. create id='aaa'
  3. but between 1 and 2 someone else create a new doc with id='aaa', so the create in 2 fails since the id is duplicate.

So at least the get-then-update-or-create method should be improved like get-then-update-or-create-then-update-if-creation-fails.

However, that's still problematical coz it may return wrong results if two users are trying to 'upsert' the same absent id -- we cannot guarantee the final result is determined by the 'upsert' called later since there are so many async writings to db and it is hard to maintain the proper sequence.

That's why I think an atomic operator like 'upsert' is necessary in this case.

eddyystop commented 7 years ago

I would think only the DB itself could provide an atomic upsert.

beeplin commented 7 years ago

Yes, and mongoDB does have this atomic upsert already -- just pass {upsert: true} as the third parameter when doing update.

The problem here is the feathers-mongodb/feathers-mongoose adapter does not have any interface to call this upsert (maybe for compatibility among different dbs).

daffl commented 7 years ago

Setting params.mongodb or params.mongoose to { upsert: true } doesn't work?

beeplin commented 7 years ago

@daffl yeah I just saw this line https://github.com/feathersjs/feathers-mongodb/blob/master/src/index.js#L36. Sorry that I didn't check earlier.

sagannotcarl commented 7 years ago

I was having a hard time putting all these notes and docs together so in case anyone else is in the same boat here is the line from above with the upsert included.

app.service('todos').patch(null, { complete: true }, { query: { complete: false }, options: { upsert: true } });

daffl commented 7 years ago

Are you sure this is working? params.options does not seem to be used anywhere (unless you are using a pretty old version). It should be params.mongoose which is documented here.

sagannotcarl commented 7 years ago

Oh sorry @daffl, I meant to post this in the mongodb version of this issue! (Too many tabs open!)

This would be the proper way to use upsert for mongodb, right?

daffl commented 7 years ago

It should be params.mongodb (although params.options is still supported for backwards compatibility reasons).

sagannotcarl commented 7 years ago

So to summarize (for mongoose this time):

app.service('todos').patch(null, { complete: true }, { query: { complete: false }, mongoose: { upsert: true } });