verful / adonis-permissions

Easily manage user roles and permissions on AdonisJS applications
MIT License
47 stars 2 forks source link

Use current models transaction client #37

Open evoactivity opened 7 months ago

evoactivity commented 7 months ago

I am running into problems where .assignRole is creating a new transaction and using up my transaction pool when used in a loop (think importing users from a CSV).

Patching the package so it uses the current models transaction fixes my issue.

public async assignRole(
  this: HasRolesModelContract,
  ...roles: Array<RoleContract | string>
): Promise<void> {
  const roleModels = await Promise.all(
    roles.map(async (r) =>
      typeof r === 'string' ? await Role.firstOrCreate({ name: r }, { name: r }, { client: this.modelTrx }) : r
    )
  )

  await this.related('roles').attach(roleModels.map((r) => r.id))
}

I guess this should be done for syncRoles and revokeRole as well.

arthur-er commented 7 months ago

We should patch it to do creation of strings in one go using fetchOrCreateMany then merging the array.

PR welcome

mr-feek commented 7 months ago

We should patch it to do creation of strings in one go using fetchOrCreateMany then merging the array.

FWIW, I view that as two separate issues.

Issue 1: It's currently not possible to assign roles inside of an ongoing transaction.

Issue 2: Assigning multiple roles results in querying the roles table multiple times.

arthur-er commented 7 months ago

Agreed