This PR adds support for .transaction() #814

mdierolf · 2018-07-25T22:42:05Z

New pull request for the functionality in this pull request:
#811

I had to create a new PR and close the old one, as github will not allow me to change the head branch once a PR is filed

This adds support for PromiseConnection.transaction() to MySQL2

…ransactions now work on Pools and Connections

sidorares · 2018-07-26T00:23:53Z

thoughts re new api @sushantdhiman @gajus @mscdex @dougwilson ?
In general I try to resist adding new surface api here but it looks quite useful for end user

https://github.com/CloudQuote/node-mysql2/blob/b3073f992512459eb06c053e75d49dfdb42f0f69/README.md#using-managed-transaction

mscdex · 2018-07-26T01:47:16Z

I don't have an opinion as I don't use Promises.

mdierolf · 2018-07-26T02:49:45Z

One thing to note:

The proposed .transaction() function includes options that the current .beginTransaction() function does not currently accept.

IMO, if .transaction() is going to accept options, then .beginTransaction() should probably do so as well.

sushantdhiman

API looks good, I have a suggestion to add ISOLATION LEVELS as they are useful concept when working with transactions.

sushantdhiman · 2018-07-26T05:17:50Z

promise.js

+    return userPromise(self);
+  });
+
+  if (options.autoCommit === false) {


You meant autoCommit : true?

Actually I meant !== true, as I don't want truthy objects to enable autocommit, only the exact value autoCommit: true

sushantdhiman · 2018-07-26T05:17:58Z

promise.js

+  if (options.autoCommit !== true) {
+    promiseChain = promiseChain.then(function() {
+      return self.query(
+        "START TRANSACTION" +


You might also want to support isolation levels https://dev.mysql.com/doc/refman/8.0/en/innodb-transaction-isolation-levels.html

A constant type can be exported directly by require('mysql2').ISOLATION_LEVELS

sushantdhiman · 2018-07-26T05:21:15Z

promise.js

@@ -214,6 +214,47 @@ PromiseConnection.prototype.changeUser = function(options) {
    });
  });
 };
+PromiseConnection.prototype.transaction = function(options,userPromise) {


/nitpick userPromise is not a promise, its a callback that will return a promise, name should reflect that

Nitpick accepted

gajus · 2018-07-26T15:06:53Z

I have been using an interchangeable API without any issues for a while now (https://github.com/gajus/slonik#slonik-query-methods-transaction).

…tionLevel: as one of the supported parameters

gajus · 2018-07-26T17:16:15Z

I would argue that beginTransaction() should be removed altogether in favour of transaction().

sidorares · 2018-07-27T00:11:47Z

I would argue that beginTransaction() should be removed altogether in favour of transaction().

I don't think it's an option, we have to account for mysql2 users who currently use beginTransaction and mysqljs/mysql users whu might occasionaly switch between libs. If we decide to needs to be coordinated between both and go through long deprecation cycle. Much easier for promise wrapper though

sushantdhiman · 2018-07-30T07:00:20Z

Looks good, will need some integration tests and it should be good to go

dougwilson · 2018-07-30T12:04:04Z

Looking at the user docs for the API I think it is a really neat use for promises.

As for this vs beginTransaction, I guess the only comment I would have is that the new API does not support callbacks at all while the old one does. If the old is removed that would be the only consideration: should callbacks just be removed from the entire driver surface?

sidorares · 2018-07-30T12:17:19Z

@dougwilson you mean remove .beginTransaction() from both promise wrapper and standard api? This is harder as I think 'standard' .beginTransaction() is used actively

dougwilson · 2018-07-30T12:19:21Z

Sorry, I meant that if the idea is after landing this new API that beginTransaction would end up deprecated and removed, then there wouldn't be a transaction callback API any longer.

mdierolf · 2018-07-30T16:08:21Z

I'd recommend leaving beginTransaction() for compatibility with mysqljs, and adding the following behavior:
-Calling beginTransaction() will mark the Connection as having an open transaction
-Calling commit() or rollback() will remove this mark from the Connection
-When a connection is released to the Pool, if it is marked as having an open transaction, rollback() will be called
-Remove the rollback-on-failure logic from PromisePool.transaction() and rely on the pool to automatically rollback on release

This model will allow you to have managed transactions when using the callback API, and managed transactions + automatic connection release when using the Promise API

…se the promise to reject

mdierolf · 2018-11-18T18:31:36Z

Any thoughts on this PR? I've been using it in production for a while, and updated it yesterday to be compatible with the new class-style code that is now in promise.js, which seems to work fine

I'd like it if we could make a decision one way or another; if it's not something that should be included in the main tree i'd rather convert it into a monkey patch in a separate repository so I can maintain it without having to merge changes into my fork

Also, any feedback as far as automatically rolling back open transactions when a connection is returned to the pool? If some bad code throws an exception, and then returns a connection to the pool that has an already open transaction, it seems like that would be very bad situation.

SimonSchick · 2018-11-21T00:38:31Z

promise.js

+      )
+      .catch( err => 
+        this.rollback()
+        .catch(() => null)


This should probably emit an error or even override the previous error, it certainly shouldn't be swallowed.

SimonSchick · 2018-11-21T00:39:23Z

lib/connection.js

+
+    const startTransactionSQL = 
+      "START TRANSACTION" +
+      ( options.consistentSnapshot === true ? " WITH CONSISTENT SNAPSHOT," : "" ) +


Inconsistent quotes, apparently lint isn't enforcing this.

SimonSchick · 2018-11-21T00:41:03Z

promise.js

+        con.release();
+        return res;
+      })
+      .catch(err => {


Nit, you probably want to merge the then and catch into one then(ok, err) chain instead, otherwise the catch handler would potentially catch .release() errors.

henricavalcante · 2019-02-07T17:15:22Z

I'm really looking forward to be able to use transactions on node-mysql2, if there is anything I can help here I'll be glad to do.

mdierolf · 2019-07-11T19:36:16Z

So we've been using this for the past year, it helps a little bit in simplifying our code, but I am thinking of re-designing this, as it doesn't really solve the problem we envisioned.

We have a collection of related problems in the code we have which uses node-mysql2:

We need a way to run one or more queries on the same connection - sometimes we use this API but other times we concatenate the queries and run them with "multipleStatements" option, which is kind of clunky to use since we have to put all our SQL into one long string.
We are typically constructing WHERE strings and replacement values in a rather clunky fashion using two arrays we push into, which gets especially weird when using multipleStatements since the replacements do not correspond to a particular statements, so it is easy to pass values to the wrong statement if you do not construct the placeholders correctly.
We need a more automated way of retrying failed queries (either because we were handed a dead connection from the pool, because the connection died in the middle of the sequence of queries, or because the transaction rolled back due to conflict)
We need a way of routing read vs write queries to different SQL interface for scaling.

So to fix these 4 issues, i'm thinking of making a new module which implements a "managed" query interface that could run on top of the mysql or mysql2 driver - which would allow you to define a chain of queries, possibly with intermediate code run between them to do calculations, which would then run them either in a transaction or raw.

It could also flag the individual queries as read vs write so they can be routed to one or more pools, and would handle failures more intelligently - for example you might choose to retry queries twice then fail hard.

The idea being to add on a couple key convenience features that ORM systems implement while still writing raw SQL queries.

As such, i'm not sure if it makes a ton of sense to merge in one particular method of working with transactions into the MySQL driver - the API in the driver should probably be unopinitated (and allow you to do everything) - or opinionated but optional (like what I am thinking of)

Mark Dierolf added 5 commits July 24, 2018 18:12

added .withConnection(asyncFunction) to PromisePool

cd0447b

wrapped the promise passed to .withConnection() with Promise.resolve()

0c921d2

removed .withConnection(), added .transaction() instead

90b96ec

const variable needed to be declared as var

9b80926

Moved .transaction() code to PromiseConnection from PromisePool, so t…

b3073f9

…ransactions now work on Pools and Connections

sushantdhiman reviewed Jul 26, 2018

View reviewed changes

Resolved several coding style issues

33347a1

Moved the transaction options to .beginTransaction(), and added isola…

f0c9e0d

…tionLevel: as one of the supported parameters

sidorares mentioned this pull request Jul 30, 2018

Release planning information? #820

Closed

Mark Dierolf added 2 commits August 7, 2018 21:29

Fixed a big, where an error thrown within transaction() would not cau…

85cd056

…se the promise to reject

Merge https://github.com/sidorares/node-mysql2 into dottransaction

88d49b6

SimonSchick reviewed Nov 21, 2018

View reviewed changes

Merge remote-tracking branch 'sidorares/master' into dottransaction

5c373ee

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

This PR adds support for .transaction() #814

This PR adds support for .transaction() #814

mdierolf commented Jul 25, 2018

sidorares commented Jul 26, 2018

mscdex commented Jul 26, 2018

mdierolf commented Jul 26, 2018

sushantdhiman left a comment

sushantdhiman Jul 26, 2018

mdierolf Jul 26, 2018

sushantdhiman Jul 26, 2018

sushantdhiman Jul 26, 2018

mdierolf Jul 26, 2018

gajus commented Jul 26, 2018

gajus commented Jul 26, 2018

sidorares commented Jul 27, 2018

sushantdhiman commented Jul 30, 2018

dougwilson commented Jul 30, 2018

sidorares commented Jul 30, 2018

dougwilson commented Jul 30, 2018

mdierolf commented Jul 30, 2018 •

edited

Loading

mdierolf commented Nov 18, 2018 •

edited

Loading

SimonSchick Nov 21, 2018

SimonSchick Nov 21, 2018

SimonSchick Nov 21, 2018

henricavalcante commented Feb 7, 2019

mdierolf commented Jul 11, 2019

This PR adds support for .transaction() #814

Are you sure you want to change the base?

This PR adds support for .transaction() #814

Conversation

mdierolf commented Jul 25, 2018

sidorares commented Jul 26, 2018

mscdex commented Jul 26, 2018

mdierolf commented Jul 26, 2018

sushantdhiman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gajus commented Jul 26, 2018

gajus commented Jul 26, 2018

sidorares commented Jul 27, 2018

sushantdhiman commented Jul 30, 2018

dougwilson commented Jul 30, 2018

sidorares commented Jul 30, 2018

dougwilson commented Jul 30, 2018

mdierolf commented Jul 30, 2018 • edited Loading

mdierolf commented Nov 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henricavalcante commented Feb 7, 2019

mdierolf commented Jul 11, 2019

mdierolf commented Jul 30, 2018 •

edited

Loading

mdierolf commented Nov 18, 2018 •

edited

Loading