eslint-plugin-knex icon indicating copy to clipboard operation
eslint-plugin-knex copied to clipboard

Add a single transaction rule

Open GradedJestRisk opened this issue 3 years ago • 4 comments

:unicorn: Problem

Knex implements explicit transactions, but allow to mix

  • a query in an explicit transaction
  • a query in the default transaction

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

GradedJestRisk avatar Mar 26 '21 16:03 GradedJestRisk

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.

AntonNiklasson avatar Apr 01 '21 10:04 AntonNiklasson

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:

  • it would be my first open source contribution;
  • the last time I did parsing and AST was in CS 15 years ago.

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 ?

GradedJestRisk avatar Apr 12 '21 06:04 GradedJestRisk

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 😊

AntonNiklasson avatar Apr 23 '21 17:04 AntonNiklasson

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

GradedJestRisk avatar Apr 24 '21 10:04 GradedJestRisk

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

GradedJestRisk avatar Sep 26 '23 11:09 GradedJestRisk