bwgjoseph / mongoose-vs-ottoman

feature comparison between mongoose and ottoman
0 stars 1 forks source link

support for query params for all operations #85

Closed bwgjoseph closed 3 years ago

bwgjoseph commented 3 years ago

Hi,

Currently, querying using id and query is not supported. In short, when using *byId method, it only take in account the id and not the additional options provided.

// this is not supported now
.findById('12345', { name: 'hello' });
// the above basically means AND
// so it translate to
// find all doc that matches { name: 'hello', id: '12345' }
.find({ name: 'hello', id: '12345' })

Similarly, for all other byId methods

// this is not supported now
.updateById('12345', { name: 'hello' }, { isBulk: true });
// the above basically means AND
// so it translate to
// all docs that matches { isBulk: true, id: '12345' } should update name to 'hello'
.updateMany({ isBulk: true, id: '12345' }, { name: 'hello' })

P.S: As for all operations, to be more specific, it should be the following

findById
updateById
replaceById
removeById

I hope I did not miss out any

AV25242 commented 3 years ago

@bwgjoseph I dont see how beneficial this feature is. for instance findById as the name suggest is used to find documents by Id and for extended needs you always have the find method. its more confusing than complementing.

bwgjoseph commented 3 years ago

I thought it was standard practice across ODM/ORM as I was developing for the feathers-database-adapter, there were test cases specifically test for this sort of query

See https://docs.feathersjs.com/api/databases/common.html

See https://github.com/feathersjs/feathers/blob/a43e7da22b6b981a96d1321736ea9a0cb924fb4f/packages/adapter-tests/src/methods.ts#L212-L225

And since it's coming from Database common API, and all feathers-adapter should have it, and that's why I thought this was something is that a norm. But I understand why that would be confusing as well.

AV25242 commented 3 years ago

Yeah this will be very confusing if we allow selection by other fields in a ById function. I think better to close this one.

bwgjoseph commented 3 years ago

I think that's fine at the moment. Will revisit again if I have any strong use-case to support it