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

Support passing mongoose specific params to queries. #70

Closed
ekryski opened this issue Mar 10, 2016 · 19 comments
Closed

Support passing mongoose specific params to queries. #70

ekryski opened this issue Mar 10, 2016 · 19 comments
Assignees
Milestone

Comments

@ekryski
Copy link
Member

ekryski commented Mar 10, 2016

Similar to this PR feathersjs-ecosystem/feathers-sequelize#27

@beeplin
Copy link

beeplin commented Mar 24, 2016

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
Copy link
Member

daffl commented Mar 27, 2016

@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
Copy link

beeplin commented Mar 27, 2016

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
Copy link
Member

+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
Copy link
Member

daffl commented Mar 27, 2016

@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
Copy link

beeplin commented Mar 28, 2016

oops missed that. Smart design! Thanks~

@daffl
Copy link
Member

daffl commented Jun 17, 2016

Also closed via #87

@daffl daffl closed this as completed Jun 17, 2016
@daffl daffl removed the Backlog label Jun 17, 2016
@beeplin
Copy link

beeplin commented May 6, 2017

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 #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
Copy link
Contributor

eddyystop commented May 6, 2017

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
Copy link

beeplin commented May 7, 2017

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
Copy link
Contributor

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

@beeplin
Copy link

beeplin commented May 7, 2017

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
Copy link
Member

daffl commented May 9, 2017

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

@beeplin
Copy link

beeplin commented May 10, 2017

@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
Copy link

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
Copy link
Member

daffl commented Jun 12, 2017

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
Copy link

sagannotcarl commented Jun 12, 2017

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
Copy link
Member

daffl commented Jun 12, 2017

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

@sagannotcarl
Copy link

So to summarize (for mongoose this time):

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

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

No branches or pull requests

6 participants