-
-
Notifications
You must be signed in to change notification settings - Fork 752
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
feat(databases): Add support for fast bulk operations #2945
base: dove
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for feathers-dove ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
if (Array.isArray(data)) { | ||
return Promise.all(data.map((current) => this._create(current, params))) | ||
} | ||
const payload = Array.isArray(_data) ? _data : [_data] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To skip the extra processing for large arrays, we can add another check for params.bulk
before running .map
:
const payload = Array.isArray(_data) ? _data : [_data]
const results = (params.bulk ? [] : payload).map((value) => {
const id = (value as any)[this.id] || this._uId++
const current = _.extend({}, value, { [this.id]: id })
return _select((this.store[id] = current), params, this.id)
})
return params.bulk ? [] : Array.isArray(_data) ? results : results[0]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we also need to address the scenario where we create a single record and also pass params.bulk
:
const david = await service.create(
{
name: 'David',
age: 3,
created: true
},
{ bulk: true }
)
We don't want to return an empty array for the above scenario since that breaks the request-to-response plurality mapping of the service interface. So we probably ought to throw an error:
if (!Array.isArray(_data) && params.bulk) {
throw new BadRequest()
}
@@ -57,6 +57,10 @@ export abstract class AdapterBase< | |||
* @returns Wether or not multiple updates are allowed. | |||
*/ | |||
allowsMulti(method: string, params: ServiceParams = {} as ServiceParams) { | |||
if (params.bulk) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can update allowMulti
to accept context.id
and have a centralized place to throw an error whenever params.bulk === true
and
- For create:
data
is an object - For patch and remove:
context.id
is null
Lets also solve the merge conflicts |
Any ETA on this? Running to issues where this would help. |
This is more than a year old, can I make the requested changes? |
This pull request adds the ability to the built in database adapters to perform fast bulk operations for
create
,patch
andremove
by passingparams.bulk = true
. Bulk operations will use the fastest database call available and - when successful - always return no data (an empty array) and not send real time events.Closes feathersjs-ecosystem/feathers-mongodb#219