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

add deleteMany method #247

Closed
wants to merge 1 commit into from
Closed

add deleteMany method #247

wants to merge 1 commit into from

Conversation

enniel
Copy link
Contributor

@enniel enniel commented Nov 29, 2017

No description provided.

@coveralls
Copy link

coveralls commented Nov 29, 2017

Coverage Status

Coverage increased (+0.02%) to 96.725% when pulling c3a10cf on enniel:delete-many-method into baa5f64 on adonisjs:develop.

@thetutlage
Copy link
Member

Couple of issues.

  1. Your title says deleteMany though it has changes within find and findBy methods.
  2. Also there is no point of having transactions for the fetch queries.

@enniel
Copy link
Contributor Author

enniel commented Nov 29, 2017

@thetutlage don't worked without transactions for the fetch queries

@enniel
Copy link
Contributor Author

enniel commented Nov 29, 2017

I tried to run deleteMany without transactions for find method, but got timeout error in tests:

  static deleteMany (ids, trx) {
     if (ids instanceof Array === false) {
       throw GE
         .InvalidArgumentException
         .invalidParameter(`${this.name}.deleteMany expects an array of ids`, ids)
     }
     return Promise.all(ids.map((id) => this.find(id))).then(instances => {
       return Promise.all(instances.map((instance) => instance.delete(trx)))
     })
   }

@thetutlage
Copy link
Member

Coz you are finding inside that method. Also if you have an array of id, you should do a bulk delete, over deleting one row at a time.

user.query().whereIn('id', ids).delete()

There is no need for deleteMany

@enniel enniel closed this Nov 30, 2017
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

Successfully merging this pull request may close these issues.

None yet

3 participants