juanluispaz / ts-sql-query

Type-safe SQL query builder like QueryDSL or JOOQ in Java or Linq in .Net for TypeScript with MariaDB, MySql, Oracle, PostgreSql, Sqlite and SqlServer support.
https://ts-sql-query.readthedocs.io/
MIT License
291 stars 19 forks source link

nested transactions #104

Closed kir4ik closed 1 year ago

kir4ik commented 1 year ago

Hi 👋, I've encountered such a problem - each nested transaction sends a request to start a transaction and commit or rollback. This leads to the fact that when I open a transaction and update data through different models, which can also open transactions within themselves, each internal transaction is completed with a commit and the data is saved, further errors will no longer completely roll back all changes. And it turns out a top-level transaction is no longer a transaction, but only holds the current connection.

Therefore, I do not understand why the transactionLevel variable is needed in the MySqlQueryRunner.ts file, since nested transactions still send requests to start a transaction and rollback.

Here is a simplified example of what it might look like:

const {MySqlConnection} = require('ts-sql-query/connections/MySqlConnection');
const {MySqlPoolQueryRunner} = require('ts-sql-query/queryRunners/MySqlPoolQueryRunner');
const {Table} = require('ts-sql-query/Table');
const createMysqlPoolWithMetadata = require('ui-common/lib/mysql/createMysqlPoolWithMetadata').default;

const config = {
  // ...
};

const mysqlPool = createMysqlPoolWithMetadata(config);

class Connection extends MySqlConnection {
  allowEmptyString = true;
}

const createMysqlConnection = () => new Connection(new MySqlPoolQueryRunner(mysqlPool, 'mySql'));

class OSTypeTable extends Table {
  id = this.column('id', 'string');
  name = this.column('name', 'string');
  parentId = this.optionalColumn('parent_id', 'string');
  order = this.optionalColumnWithDefaultValue('order', 'double');

  constructor() {
    super('os_type');
  }
}

const tOSType = new OSTypeTable();

main()
  .catch(console.error)
  .finally(async () => {});

async function main() {
  const connection = createMysqlConnection();

  let ref;
  await connection.transaction(async () => {
    await connection.queryRunner.execute(async (connection, transaction) => {
      const currentConnection = (transaction || connection);
      ref = currentConnection;
    });

    await connection.transaction(async () => {
      await connection.selectFrom(tOSType).select({
        id: tOSType.id,
        name: tOSType.name,
        parentId: tOSType.parentId,
        order: tOSType.order
      }).executeSelectMany();

      throw new Error('Error 1');
    }).catch(console.error);

    await connection.transaction(async () => {
      await connection.selectFrom(tOSType).select({
        id: tOSType.id,
        name: tOSType.name,
        parentId: tOSType.parentId,
        order: tOSType.order
      }).executeSelectMany();
    })

    await connection.transaction(async () => {
      await connection.selectFrom(tOSType).select({
        id: tOSType.id,
        name: tOSType.name,
        parentId: tOSType.parentId,
        order: tOSType.order
      }).executeSelectMany();

      throw new Error('Error 3')
    })
  }).finally(() => {
    console.log(ref.toJSON());
  });
}

In the end I get a list of sql queries

[
  "START TRANSACTION",
  "START TRANSACTION",
  "select id as id, `name` as `name`, parent_id as parentId, `order` as `order` from os_type",
  "ROLLBACK",
  "START TRANSACTION",
  "select id as id, `name` as `name`, parent_id as parentId, `order` as `order` from os_type",
  "COMMIT",
  "START TRANSACTION",
  "select id as id, `name` as `name`, parent_id as parentId, `order` as `order` from os_type",
  "ROLLBACK",
  "ROLLBACK"
]

Maybe when you try to start a transaction inside a transaction, you need to throw an error? And/Or can make it so that an open transaction is transferred to transaction cb and then work with it, transactions by types should not have methods to manage it For example:

connection.transaction(async (transaction) => {...})
kir4ik commented 1 year ago

Perhaps it will be enough that requests for transaction management are not duplicated. We do not seem to catch errors in such requests

juanluispaz commented 1 year ago

Hi,

Nested transaction is a database-specific feature that you can use with a specific connector (PorgreSQL with Pg support it).

In the case of MySql, it doesn't support nested transactions, but it doesn't throw an error. Unexpectedly, it does a horrible behaviour where when you try to open the inner transaction, the outer one is committed ( see more details here ) causing the behaviour your describe in this issue (it is MySql intended behaviour).

I believe ts-sql-query should throw an error warning you nested transaction is not available in MySql or MariaDB (it is done in that way in other Query Runners).

Not sure why you are trying to open a transaction inside another transaction; maybe the transaction scope in your application is too blurry. If that is the case, maybe you will be interested in having a function that opens a transaction only if there is no one:

class DBConnection extends MySqlConnection<'DBConnection'> {
    joinOrBeginTransaction<T>(fn: () => Promise<T>): Promise<T> {
        if (this.isTransactionActive()) {
            return fn()
        } else {
            return this.transaction(fn)
        }
    }
}

Another possibility is that what you are trying to do is to use save points, but, reading your message, it doesn't sound like that.

Let me know your comments on this.

juanluispaz commented 1 year ago

Released ts-sql-query 1.54.0. In this version, an Error will be thrown if you try to open a nested transaction in MySql or MariaDB to avoid the database silently closing the previous one.

Let me know your feedback.

kir4ik commented 1 year ago

Yes, we had such a problem due to the fact that different models were launched inside a transaction and could also launch transactions, and we did not expect that this would lead to duplication of transaction requests. I thought that nested calls are ignored due to transactionLevel.

Using the joinOrBeginTransaction method looks good. Throwing the error should help with the detection of such problems.

Thank you.