AntonNiklasson / eslint-plugin-knex

Lint rule(s) for Knex.js
9 stars 8 forks source link

Add a single transaction rule #12

Closed GradedJestRisk closed 1 year ago

GradedJestRisk commented 3 years ago

:unicorn: Problem

Knex implements explicit transactions, but allow to mix

Such a code would not be implemented purposefully, and may end up exhausting the connection pool

//  Take a connection A from the pool
//  Initiate a transaction on connection A

knex.transaction((trx) => {

   //  Take a connection B from the pool
   //  Initiate a transaction on connection B

  // Bug here, the code should be trx.update()
  knex.update();

   //  Validate transaction in connection B 
   //  Release connection B to the pool

}) 
//  Validate transaction in connection A 
//  Release connection A to the pool

Such an issue has been raised in knex repo

:robot: Solution

Implement a rule that does not allow mixing transactions

:rainbow: Remarks

Such a behavior occurred in production in this repository yesterday, blocking hundred of thousand of users

FYI, the original fix is here

AntonNiklasson commented 3 years ago

Thanks for reporting this! I'm not sure static code analysis is the best approach to solving this, but perhaps we could take a stab at it. Are you open to work on this?

A good name for this rule might be no-mixed-transactions.

GradedJestRisk commented 3 years ago

Well, in the code fix I mentioned, static code analysis wouldn't do it.

However, such a rule would be useful in most cases and no-mixed-transactions sound a good name, as we don't talk about nested transactions.

I would love to work on this, but keep in mind:

I had a first look on a rule implementation and saw there's some documentation also.

What do you recommend ? Start implementing (test, then code) and read docs when stucked, or read docs first ?

AntonNiklasson commented 3 years ago

Hey, sorry for not getting back to you. Did you get started on this?

Don't worry about this being your first time. It's not as hard as it might seem. A great tip is to use ASTExplorer (configured with espree and ESLint V4) to explore and experiment with the AST.

Let me know if I can help. If you don't have time, just let me know 😊

GradedJestRisk commented 3 years ago

I didn't start yet, but with this feedback I can go on !

GradedJestRisk commented 1 year ago

Two years went by, I'm better choosing an easier rule !