Relation methods are not using `options.client` when wrapping in `managedTransaction`

[adamcikado]

Package version

21.6.1

Describe the bug

If you provide transaction when calling any of the relation methods like updateOrCreate it won't use it when calling managedTransaction, which results in creating another connection with another transaction.

class Post extends BaseModel {
  @column({ isPrimary: true })
  declare id: number

  @column()
  declare userId: number

  @column()
  declare title: string
}

class User extends BaseModel {
  @column({ isPrimary: true })
  declare id: number

  @column()
  declare username: string

  @hasMany(() => Post)
  declare posts: HasMany<typeof Post>
}

const user = new User()
user.username = 'virk'
await user.save()

const trx = await db.connection().transaction()
await user.related('posts').updateOrCreate({}, { title: 'updateOrCreate' }, { client: trx })
await trx.commit()

Run the command with DEBUG=knex:* and notice that 2 transactions are created.

Reproduction repo

No response

[thetutlage]

Can't we simply set the transaction on the user and then let all the relationships automatically infer it?

const user = new User()
user.username = 'virk'
await user.save()

user.$trx = await db.connection().transaction()
await user.related('posts').updateOrCreate({}, { title: 'updateOrCreate' }, { client: trx })
await user.$trx.commit()

[adamcikado]

@thetutlage yes, that case is working right now. But when you provide custom client without setting the transaction on the parent, it doesn't work, the transaction is not being used.

[thetutlage]

Is there any use-case where you would like to assign the transaction manually? I think passing it down automatically via relationships is more cleaner/less error prone

[adamcikado]

@thetutlage

1. Currently it's allowed to pass custom client via the options.client, but it's not working, so it either has to be fixed or removed.
2. We use this a lot in following way:

await db.transaction(async (trx) => {
  const notificationRecord = await notifiable.related('notifications').create(
    {
      ...notifiable.notificationsRelatedData(),
      type: notification.type,
      data: notification.data,
    },
    { client: trx },
  )

  ...
})

Which is more convenient to use than:

await db.transaction(async (trx) => {
  notifiable.$trx = trx
  const notificationRecord = await notifiable.related('notifications').create(
    {
      ...notifiable.notificationsRelatedData(),
      type: notification.type,
      data: notification.data,
    }
  )

  ...
})

[thetutlage]

Currently it's allowed to pass custom client via the options.client, but it's not working, so it either has to be fixed or removed.

I think, I will be in the favor of removing the client option from the types. I think it was a mistake.

The reason I am uncomfortable is because passing the transaction manually through the chain of nested relationship seems tedious, when the same could be achieved by scoping the top-level model object to a transaction.

[adamcikado]

@thetutlage Got it, I understand that. However #1096 is not a breaking change and it follows the same convention as other methods on BaseModel like firstOrCreate. So it could be merged and later removed as breaking change in the new release, if you prefer to do so.

[enixsoft]

@thetutlage
We have encountered Knex Timeout problems because of this bug before.

It would be much appreciated if you could merge this.